Message ID | 1503634348.2499.98.camel@edumazet-glaptop3.roam.corp.google.com |
---|---|
State | Accepted, archived |
Delegated to: | David Miller |
Headers | show |
From: Eric Dumazet <eric.dumazet@gmail.com> Date: Thu, 24 Aug 2017 21:12:28 -0700 > From: Eric Dumazet <edumazet@google.com> > > syzkaller reported a refcount_t warning [1] > > Issue here is that noop_qdisc refcnt was never really considered as > a true refcount, since qdisc_destroy() found TCQ_F_BUILTIN set : > > if (qdisc->flags & TCQ_F_BUILTIN || > !refcount_dec_and_test(&qdisc->refcnt))) > return; > > Meaning that all atomic_inc() we did on noop_qdisc.refcnt were not > really needed, but harmless until refcount_t came. > > To fix this problem, we simply need to not increment noop_qdisc.refcnt, > since we never decrement it. > > [1] > refcount_t: increment on 0; use-after-free. ... > Fixes: 7b9364050246 ("net, sched: convert Qdisc.refcnt from atomic_t to refcount_t") > Signed-off-by: Eric Dumazet <edumazet@google.com> > Reported-by: Dmitry Vyukov <dvyukov@google.com> > Cc: Reshetova, Elena <elena.reshetova@intel.com> Applied.
diff --git a/include/net/sch_generic.h b/include/net/sch_generic.h index 67f815e5d52517390226bc3531b1ea7b5f1020bc..c1109cdbbfa6afb9aff0d6033aef7b615630ffc1 100644 --- a/include/net/sch_generic.h +++ b/include/net/sch_generic.h @@ -101,6 +101,13 @@ struct Qdisc { spinlock_t busylock ____cacheline_aligned_in_smp; }; +static inline void qdisc_refcount_inc(struct Qdisc *qdisc) +{ + if (qdisc->flags & TCQ_F_BUILTIN) + return; + refcount_inc(&qdisc->refcnt); +} + static inline bool qdisc_is_running(const struct Qdisc *qdisc) { return (raw_read_seqcount(&qdisc->running) & 1) ? true : false; diff --git a/net/sched/sch_api.c b/net/sched/sch_api.c index a3fa144b864871088209386fd573bded1886432f..4fb5a3222d0d324167f079f755be14eb028b4a50 100644 --- a/net/sched/sch_api.c +++ b/net/sched/sch_api.c @@ -836,7 +836,7 @@ static int qdisc_graft(struct net_device *dev, struct Qdisc *parent, old = dev_graft_qdisc(dev_queue, new); if (new && i > 0) - refcount_inc(&new->refcnt); + qdisc_refcount_inc(new); if (!ingress) qdisc_destroy(old); @@ -847,7 +847,7 @@ static int qdisc_graft(struct net_device *dev, struct Qdisc *parent, notify_and_destroy(net, skb, n, classid, dev->qdisc, new); if (new && !new->ops->attach) - refcount_inc(&new->refcnt); + qdisc_refcount_inc(new); dev->qdisc = new ? : &noop_qdisc; if (new && new->ops->attach) @@ -1256,7 +1256,7 @@ static int tc_modify_qdisc(struct sk_buff *skb, struct nlmsghdr *n, if (q == p || (p && check_loop(q, p, 0))) return -ELOOP; - refcount_inc(&q->refcnt); + qdisc_refcount_inc(q); goto graft; } else { if (!q) diff --git a/net/sched/sch_generic.c b/net/sched/sch_generic.c index 57ba406f1437323a4ba172d52f14a8b687b86708..4ba6da5fb2546c35ad48fe1f3632df8ca9957b34 100644 --- a/net/sched/sch_generic.c +++ b/net/sched/sch_generic.c @@ -785,7 +785,7 @@ static void attach_default_qdiscs(struct net_device *dev) dev->priv_flags & IFF_NO_QUEUE) { netdev_for_each_tx_queue(dev, attach_one_default_qdisc, NULL); dev->qdisc = txq->qdisc_sleeping; - refcount_inc(&dev->qdisc->refcnt); + qdisc_refcount_inc(dev->qdisc); } else { qdisc = qdisc_create_dflt(txq, &mq_qdisc_ops, TC_H_ROOT); if (qdisc) {