From 8d50b53d66a8a6ae41bafbdcabe401467803f33a Mon Sep 17 00:00:00 2001 From: "David S. Miller" Date: Wed, 30 Jul 2008 02:37:46 -0700 Subject: pkt_sched: Fix OOPS on ingress qdisc add. Bug report from Steven Jan Springl: Issuing the following command causes a kernel oops: tc qdisc add dev eth0 handle ffff: ingress The problem mostly stems from all of the special case handling of ingress qdiscs. So, to fix this, do the grafting operation the same way we do for TX qdiscs. Which means that dev_activate() and dev_deactivate() now do the "qdisc_sleeping <--> qdisc" transitions on dev->rx_queue too. Future simplifications are possible now, mainly because it is impossible for dev_queue->{qdisc,qdisc_sleeping} to be NULL. There are NULL checks all over to handle the ingress qdisc special case that used to exist before this commit. Signed-off-by: David S. Miller --- net/sched/sch_api.c | 57 +++++++++++++++-------------------------------------- 1 file changed, 16 insertions(+), 41 deletions(-) (limited to 'net/sched/sch_api.c') diff --git a/net/sched/sch_api.c b/net/sched/sch_api.c index b0601642e227..4840aff47256 100644 --- a/net/sched/sch_api.c +++ b/net/sched/sch_api.c @@ -572,44 +572,21 @@ static u32 qdisc_alloc_handle(struct net_device *dev) static struct Qdisc *dev_graft_qdisc(struct netdev_queue *dev_queue, struct Qdisc *qdisc) { + struct Qdisc *oqdisc = dev_queue->qdisc_sleeping; spinlock_t *root_lock; - struct Qdisc *oqdisc; - int ingress; - - ingress = 0; - if (qdisc && qdisc->flags&TCQ_F_INGRESS) - ingress = 1; - - if (ingress) { - oqdisc = dev_queue->qdisc; - } else { - oqdisc = dev_queue->qdisc_sleeping; - } root_lock = qdisc_root_lock(oqdisc); spin_lock_bh(root_lock); - if (ingress) { - /* Prune old scheduler */ - if (oqdisc && atomic_read(&oqdisc->refcnt) <= 1) { - /* delete */ - qdisc_reset(oqdisc); - dev_queue->qdisc = NULL; - } else { /* new */ - dev_queue->qdisc = qdisc; - } + /* Prune old scheduler */ + if (oqdisc && atomic_read(&oqdisc->refcnt) <= 1) + qdisc_reset(oqdisc); - } else { - /* Prune old scheduler */ - if (oqdisc && atomic_read(&oqdisc->refcnt) <= 1) - qdisc_reset(oqdisc); - - /* ... and graft new one */ - if (qdisc == NULL) - qdisc = &noop_qdisc; - dev_queue->qdisc_sleeping = qdisc; - dev_queue->qdisc = &noop_qdisc; - } + /* ... and graft new one */ + if (qdisc == NULL) + qdisc = &noop_qdisc; + dev_queue->qdisc_sleeping = qdisc; + dev_queue->qdisc = &noop_qdisc; spin_unlock_bh(root_lock); @@ -678,7 +655,8 @@ static int qdisc_graft(struct net_device *dev, struct Qdisc *parent, ingress = 0; num_q = dev->num_tx_queues; - if (q && q->flags & TCQ_F_INGRESS) { + if ((q && q->flags & TCQ_F_INGRESS) || + (new && new->flags & TCQ_F_INGRESS)) { num_q = 1; ingress = 1; } @@ -692,13 +670,10 @@ static int qdisc_graft(struct net_device *dev, struct Qdisc *parent, if (!ingress) dev_queue = netdev_get_tx_queue(dev, i); - if (ingress) { - old = dev_graft_qdisc(dev_queue, q); - } else { - old = dev_graft_qdisc(dev_queue, new); - if (new && i > 0) - atomic_inc(&new->refcnt); - } + old = dev_graft_qdisc(dev_queue, new); + if (new && i > 0) + atomic_inc(&new->refcnt); + notify_and_destroy(skb, n, classid, old, new); } @@ -817,7 +792,7 @@ qdisc_create(struct net_device *dev, struct netdev_queue *dev_queue, goto err_out3; } } - if (parent) + if (parent && !(sch->flags & TCQ_F_INGRESS)) list_add_tail(&sch->list, &dev_queue->qdisc->list); return sch; -- cgit v1.2.3 From ee7af8264dafa0c8c76a8dc596803966c2e29ebc Mon Sep 17 00:00:00 2001 From: "David S. Miller" Date: Wed, 6 Aug 2008 23:35:59 -0700 Subject: pkt_sched: Fix "parent is root" test in qdisc_create(). As noticed by Stephen Hemminger, the root qdisc is denoted by TC_H_ROOT, not zero. Signed-off-by: David S. Miller --- net/sched/sch_api.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'net/sched/sch_api.c') diff --git a/net/sched/sch_api.c b/net/sched/sch_api.c index 4840aff47256..83b23b55ce36 100644 --- a/net/sched/sch_api.c +++ b/net/sched/sch_api.c @@ -792,7 +792,7 @@ qdisc_create(struct net_device *dev, struct netdev_queue *dev_queue, goto err_out3; } } - if (parent && !(sch->flags & TCQ_F_INGRESS)) + if ((parent != TC_H_ROOT) && !(sch->flags & TCQ_F_INGRESS)) list_add_tail(&sch->list, &dev_queue->qdisc->list); return sch; -- cgit v1.2.3 From 827ebd6410005b05b3c930ef6a116666c6986886 Mon Sep 17 00:00:00 2001 From: "David S. Miller" Date: Thu, 7 Aug 2008 20:26:40 -0700 Subject: pkt_sched: Fix qdisc config when link is down. Bug reported by Stephen Hemminger. We need to fetch the root from ->qdisc_sleeping not ->qdisc. Signed-off-by: David S. Miller --- net/sched/sch_api.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) (limited to 'net/sched/sch_api.c') diff --git a/net/sched/sch_api.c b/net/sched/sch_api.c index 83b23b55ce36..ba1d121f3127 100644 --- a/net/sched/sch_api.c +++ b/net/sched/sch_api.c @@ -189,7 +189,7 @@ struct Qdisc *qdisc_lookup(struct net_device *dev, u32 handle) for (i = 0; i < dev->num_tx_queues; i++) { struct netdev_queue *txq = netdev_get_tx_queue(dev, i); - struct Qdisc *q, *txq_root = txq->qdisc; + struct Qdisc *q, *txq_root = txq->qdisc_sleeping; if (!(txq_root->flags & TCQ_F_BUILTIN) && txq_root->handle == handle) @@ -793,7 +793,7 @@ qdisc_create(struct net_device *dev, struct netdev_queue *dev_queue, } } if ((parent != TC_H_ROOT) && !(sch->flags & TCQ_F_INGRESS)) - list_add_tail(&sch->list, &dev_queue->qdisc->list); + list_add_tail(&sch->list, &dev_queue->qdisc_sleeping->list); return sch; } @@ -1236,11 +1236,11 @@ static int tc_dump_qdisc(struct sk_buff *skb, struct netlink_callback *cb) q_idx = 0; dev_queue = netdev_get_tx_queue(dev, 0); - if (tc_dump_qdisc_root(dev_queue->qdisc, skb, cb, &q_idx, s_q_idx) < 0) + if (tc_dump_qdisc_root(dev_queue->qdisc_sleeping, skb, cb, &q_idx, s_q_idx) < 0) goto done; dev_queue = &dev->rx_queue; - if (tc_dump_qdisc_root(dev_queue->qdisc, skb, cb, &q_idx, s_q_idx) < 0) + if (tc_dump_qdisc_root(dev_queue->qdisc_sleeping, skb, cb, &q_idx, s_q_idx) < 0) goto done; cont: -- cgit v1.2.3 From 8123b421e8ed944671d7241323ed3198cccb4041 Mon Sep 17 00:00:00 2001 From: "David S. Miller" Date: Fri, 8 Aug 2008 23:23:39 -0700 Subject: pkt_sched: Fix ingress deletion and filter attachment. Based upon bug reports by Stephen Hemminger. We still had some cases using ->qdisc instead of ->qdisc_sleeping. Also, qdisc_lookup() should return ingress qdiscs. Signed-off-by: David S. Miller --- net/sched/sch_api.c | 36 +++++++++++++++++++++++------------- 1 file changed, 23 insertions(+), 13 deletions(-) (limited to 'net/sched/sch_api.c') diff --git a/net/sched/sch_api.c b/net/sched/sch_api.c index ba1d121f3127..bbf149dd7818 100644 --- a/net/sched/sch_api.c +++ b/net/sched/sch_api.c @@ -183,6 +183,21 @@ EXPORT_SYMBOL(unregister_qdisc); (root qdisc, all its children, children of children etc.) */ +struct Qdisc *qdisc_match_from_root(struct Qdisc *root, u32 handle) +{ + struct Qdisc *q; + + if (!(root->flags & TCQ_F_BUILTIN) && + root->handle == handle) + return root; + + list_for_each_entry(q, &root->list, list) { + if (q->handle == handle) + return q; + } + return NULL; +} + struct Qdisc *qdisc_lookup(struct net_device *dev, u32 handle) { unsigned int i; @@ -191,16 +206,11 @@ struct Qdisc *qdisc_lookup(struct net_device *dev, u32 handle) struct netdev_queue *txq = netdev_get_tx_queue(dev, i); struct Qdisc *q, *txq_root = txq->qdisc_sleeping; - if (!(txq_root->flags & TCQ_F_BUILTIN) && - txq_root->handle == handle) - return txq_root; - - list_for_each_entry(q, &txq_root->list, list) { - if (q->handle == handle) - return q; - } + q = qdisc_match_from_root(txq_root, handle); + if (q) + return q; } - return NULL; + return qdisc_match_from_root(dev->rx_queue.qdisc_sleeping, handle); } static struct Qdisc *qdisc_leaf(struct Qdisc *p, u32 classid) @@ -908,7 +918,7 @@ static int tc_get_qdisc(struct sk_buff *skb, struct nlmsghdr *n, void *arg) return -ENOENT; q = qdisc_leaf(p, clid); } else { /* ingress */ - q = dev->rx_queue.qdisc; + q = dev->rx_queue.qdisc_sleeping; } } else { struct netdev_queue *dev_queue; @@ -978,7 +988,7 @@ replay: return -ENOENT; q = qdisc_leaf(p, clid); } else { /*ingress */ - q = dev->rx_queue.qdisc; + q = dev->rx_queue.qdisc_sleeping; } } else { struct netdev_queue *dev_queue; @@ -1529,11 +1539,11 @@ static int tc_dump_tclass(struct sk_buff *skb, struct netlink_callback *cb) t = 0; dev_queue = netdev_get_tx_queue(dev, 0); - if (tc_dump_tclass_root(dev_queue->qdisc, skb, tcm, cb, &t, s_t) < 0) + if (tc_dump_tclass_root(dev_queue->qdisc_sleeping, skb, tcm, cb, &t, s_t) < 0) goto done; dev_queue = &dev->rx_queue; - if (tc_dump_tclass_root(dev_queue->qdisc, skb, tcm, cb, &t, s_t) < 0) + if (tc_dump_tclass_root(dev_queue->qdisc_sleeping, skb, tcm, cb, &t, s_t) < 0) goto done; done: -- cgit v1.2.3 From 1cfa26661a85549063e369e2b40275eeaa7b923c Mon Sep 17 00:00:00 2001 From: Jarek Poplawski Date: Mon, 11 Aug 2008 18:11:06 -0700 Subject: pkt_sched: Add BH protection for qdisc_stab_lock. Since qdisc_stab_lock is used in qdisc_put_stab(), which is called in BH context from __qdisc_destroy() RCU callback, softirq safe locking is needed. Signed-off-by: Jarek Poplawski Signed-off-by: David S. Miller --- net/sched/sch_api.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) (limited to 'net/sched/sch_api.c') diff --git a/net/sched/sch_api.c b/net/sched/sch_api.c index bbf149dd7818..c25465e5607a 100644 --- a/net/sched/sch_api.c +++ b/net/sched/sch_api.c @@ -331,7 +331,7 @@ static struct qdisc_size_table *qdisc_get_stab(struct nlattr *opt) if (!s || tsize != s->tsize || (!tab && tsize > 0)) return ERR_PTR(-EINVAL); - spin_lock(&qdisc_stab_lock); + spin_lock_bh(&qdisc_stab_lock); list_for_each_entry(stab, &qdisc_stab_list, list) { if (memcmp(&stab->szopts, s, sizeof(*s))) @@ -339,11 +339,11 @@ static struct qdisc_size_table *qdisc_get_stab(struct nlattr *opt) if (tsize > 0 && memcmp(stab->data, tab, tsize * sizeof(u16))) continue; stab->refcnt++; - spin_unlock(&qdisc_stab_lock); + spin_unlock_bh(&qdisc_stab_lock); return stab; } - spin_unlock(&qdisc_stab_lock); + spin_unlock_bh(&qdisc_stab_lock); stab = kmalloc(sizeof(*stab) + tsize * sizeof(u16), GFP_KERNEL); if (!stab) @@ -354,9 +354,9 @@ static struct qdisc_size_table *qdisc_get_stab(struct nlattr *opt) if (tsize > 0) memcpy(stab->data, tab, tsize * sizeof(u16)); - spin_lock(&qdisc_stab_lock); + spin_lock_bh(&qdisc_stab_lock); list_add_tail(&stab->list, &qdisc_stab_list); - spin_unlock(&qdisc_stab_lock); + spin_unlock_bh(&qdisc_stab_lock); return stab; } @@ -366,14 +366,14 @@ void qdisc_put_stab(struct qdisc_size_table *tab) if (!tab) return; - spin_lock(&qdisc_stab_lock); + spin_lock_bh(&qdisc_stab_lock); if (--tab->refcnt == 0) { list_del(&tab->list); kfree(tab); } - spin_unlock(&qdisc_stab_lock); + spin_unlock_bh(&qdisc_stab_lock); } EXPORT_SYMBOL(qdisc_put_stab); -- cgit v1.2.3