From 5d944c640b4ae5f37c537acf491c2f0eb89fa0d6 Mon Sep 17 00:00:00 2001 From: Eric Dumazet Date: Wed, 31 Mar 2010 07:06:04 +0000 Subject: [PATCH] gen_estimator: deadlock fix One of my test machine got a deadlock during "tc" sessions, adding/deleting classes & filters, using traffic estimators. After some analysis, I believe we have a potential use after free case in est_timer() : spin_lock(e->stats_lock); << HERE >> read_lock(&est_lock); if (e->bstats == NULL) << TEST >> goto skip; Test is done a bit late, because after estimator is killed, and before rcu grace period elapsed, we might already have freed/reuse memory where e->stats_locks points to (some qdisc->q.lock) A possible fix is to respect a rcu grace period at Qdisc dismantle time. On 64bit, sizeof(struct Qdisc) is exactly 192 bytes. Adding 16 bytes to it (for struct rcu_head) is a problem because it might change performance, given QDISC_ALIGNTO is 32 bytes. This is why I also change QDISC_ALIGNTO to 64 bytes, to satisfy most current alignment requirements. Signed-off-by: Eric Dumazet Signed-off-by: David S. Miller --- include/net/pkt_sched.h | 2 +- include/net/sch_generic.h | 1 + net/sched/sch_generic.c | 15 +++++++++++++-- 3 files changed, 15 insertions(+), 3 deletions(-) diff --git a/include/net/pkt_sched.h b/include/net/pkt_sched.h index b6cdc33b39c1..9d4d87cc970e 100644 --- a/include/net/pkt_sched.h +++ b/include/net/pkt_sched.h @@ -12,7 +12,7 @@ struct qdisc_walker { int (*fn)(struct Qdisc *, unsigned long cl, struct qdisc_walker *); }; -#define QDISC_ALIGNTO 32 +#define QDISC_ALIGNTO 64 #define QDISC_ALIGN(len) (((len) + QDISC_ALIGNTO-1) & ~(QDISC_ALIGNTO-1)) static inline void *qdisc_priv(struct Qdisc *q) diff --git a/include/net/sch_generic.h b/include/net/sch_generic.h index 67dc08eaaa45..03ca5d826757 100644 --- a/include/net/sch_generic.h +++ b/include/net/sch_generic.h @@ -73,6 +73,7 @@ struct Qdisc { struct sk_buff_head q; struct gnet_stats_basic_packed bstats; struct gnet_stats_queue qstats; + struct rcu_head rcu_head; }; struct Qdisc_class_ops { diff --git a/net/sched/sch_generic.c b/net/sched/sch_generic.c index 5173c1e1b19c..17513252e83f 100644 --- a/net/sched/sch_generic.c +++ b/net/sched/sch_generic.c @@ -528,7 +528,7 @@ struct Qdisc *qdisc_alloc(struct netdev_queue *dev_queue, unsigned int size; int err = -ENOBUFS; - /* ensure that the Qdisc and the private data are 32-byte aligned */ + /* ensure that the Qdisc and the private data are 64-byte aligned */ size = QDISC_ALIGN(sizeof(*sch)); size += ops->priv_size + (QDISC_ALIGNTO - 1); @@ -590,6 +590,13 @@ void qdisc_reset(struct Qdisc *qdisc) } EXPORT_SYMBOL(qdisc_reset); +static void qdisc_rcu_free(struct rcu_head *head) +{ + struct Qdisc *qdisc = container_of(head, struct Qdisc, rcu_head); + + kfree((char *) qdisc - qdisc->padded); +} + void qdisc_destroy(struct Qdisc *qdisc) { const struct Qdisc_ops *ops = qdisc->ops; @@ -613,7 +620,11 @@ void qdisc_destroy(struct Qdisc *qdisc) dev_put(qdisc_dev(qdisc)); kfree_skb(qdisc->gso_skb); - kfree((char *) qdisc - qdisc->padded); + /* + * gen_estimator est_timer() might access qdisc->q.lock, + * wait a RCU grace period before freeing qdisc. + */ + call_rcu(&qdisc->rcu_head, qdisc_rcu_free); } EXPORT_SYMBOL(qdisc_destroy);