[PATCH] net: Revert "net_sched: no need to free qdisc in RCU callback"
flat view
STALE3224d
From: John Fastabend <john.fastabend@gmail.com>
Date: 2017-12-20 20:09:35
Subsystem:
networking [general], tc subsystem, the rest · Maintainers:
"David S. Miller", Eric Dumazet, Jakub Kicinski, Paolo Abeni, Jamal Hadi Salim, Jiri Pirko, Linus Torvalds
RCU grace period is needed for lockless qdiscs added in the commit
c5ad119fb6c09 ("net: sched: pfifo_fast use skb_array").
It is needed now that qdiscs may be lockless otherwise we risk
free'ing a qdisc that is still in use from datapath. Additionally,
push list cleanup into RCU callback. Otherwise we risk the datapath
adding skbs during removal.
Fixes: c5ad119fb6c09 ("net: sched: pfifo_fast use skb_array")
Signed-off-by: John Fastabend <john.fastabend@gmail.com>
---
include/net/sch_generic.h | 1 +
net/sched/sch_generic.c | 50 ++++++++++++++++++++++++---------------------
2 files changed, 28 insertions(+), 23 deletions(-)
diff --git a/include/net/sch_generic.h b/include/net/sch_generic.h
index bc6b25f..a65306b 100644
--- a/include/net/sch_generic.h
+++ b/include/net/sch_generic.h@@ -97,6 +97,7 @@ struct Qdisc { unsigned long state; struct Qdisc *next_sched; struct sk_buff_head skb_bad_txq; + struct rcu_head rcu_head; int padded; refcount_t refcnt;
diff --git a/net/sched/sch_generic.c b/net/sched/sch_generic.c
index 876fab2..ab497ef 100644
--- a/net/sched/sch_generic.c
+++ b/net/sched/sch_generic.c@@ -873,31 +873,15 @@ void qdisc_reset(struct Qdisc *qdisc) } EXPORT_SYMBOL(qdisc_reset); -static void qdisc_free(struct Qdisc *qdisc) +static void qdisc_rcu_free(struct rcu_head *head) { - if (qdisc_is_percpu_stats(qdisc)) { - free_percpu(qdisc->cpu_bstats); - free_percpu(qdisc->cpu_qstats); - } - - kfree((char *) qdisc - qdisc->padded); -} - -void qdisc_destroy(struct Qdisc *qdisc) -{ - const struct Qdisc_ops *ops = qdisc->ops; + struct Qdisc *qdisc = container_of(head, struct Qdisc, rcu_head); + const struct Qdisc_ops *ops = qdisc->ops; struct sk_buff *skb, *tmp; - if (qdisc->flags & TCQ_F_BUILTIN || - !refcount_dec_and_test(&qdisc->refcnt)) - return; - -#ifdef CONFIG_NET_SCHED - qdisc_hash_del(qdisc); - - qdisc_put_stab(rtnl_dereference(qdisc->stab)); -#endif - gen_kill_estimator(&qdisc->rate_est); + /* At this point no outstanding references to this Qdisc should + * exist in the datapath so its safe to clean up skb lists, etc. + */ if (ops->reset) ops->reset(qdisc); if (ops->destroy)
@@ -916,7 +900,27 @@ void qdisc_destroy(struct Qdisc *qdisc) kfree_skb_list(skb); } - qdisc_free(qdisc); + if (qdisc_is_percpu_stats(qdisc)) { + free_percpu(qdisc->cpu_bstats); + free_percpu(qdisc->cpu_qstats); + } + + kfree((char *) qdisc - qdisc->padded); +} + +void qdisc_destroy(struct Qdisc *qdisc) +{ + if (qdisc->flags & TCQ_F_BUILTIN || + !refcount_dec_and_test(&qdisc->refcnt)) + return; + +#ifdef CONFIG_NET_SCHED + qdisc_hash_del(qdisc); + + qdisc_put_stab(rtnl_dereference(qdisc->stab)); +#endif + gen_kill_estimator(&qdisc->rate_est); + call_rcu(&qdisc->rcu_head, qdisc_rcu_free); } EXPORT_SYMBOL(qdisc_destroy);