[Patch net] net_sched: invoke ->attach() after setting dev->qdisc

[Patch net] net_sched: invoke ->attach() after setting dev->qdisc

[Cong Wang]

For mq qdisc, we add per tx queue qdisc to root qdisc
for display purpose, however, that happens too early,
before the new dev->qdisc is finally set, this causes
q->list points to an old root qdisc which is going to be
freed right before assigning with a new one.

Fix this by moving ->attach() after setting dev->qdisc.

For the record, this fixes the following crash:

 ------------[ cut here ]------------
 WARNING: CPU: 1 PID: 975 at lib/list_debug.c:59 __list_del_entry+0x5a/0x98()
 list_del corruption. prev->next should be ffff8800d1998ae8, but was 6b6b6b6b6b6b6b6b
 CPU: 1 PID: 975 Comm: tc Not tainted 4.1.0-rc4+ #1019
 Hardware name: Bochs Bochs, BIOS Bochs 01/01/2011
  0000000000000009 ffff8800d73fb928 ffffffff81a44e7f 0000000047574756
  ffff8800d73fb978 ffff8800d73fb968 ffffffff810790da ffff8800cfc4cd20
  ffffffff814e725b ffff8800d1998ae8 ffffffff82381250 0000000000000000
 Call Trace:
  [<ffffffff81a44e7f>] dump_stack+0x4c/0x65
  [<ffffffff810790da>] warn_slowpath_common+0x9c/0xb6
  [<ffffffff814e725b>] ? __list_del_entry+0x5a/0x98
  [<ffffffff81079162>] warn_slowpath_fmt+0x46/0x48
  [<ffffffff81820eb0>] ? dev_graft_qdisc+0x5e/0x6a
  [<ffffffff814e725b>] __list_del_entry+0x5a/0x98
  [<ffffffff814e72a7>] list_del+0xe/0x2d
  [<ffffffff81822f05>] qdisc_list_del+0x1e/0x20
  [<ffffffff81820cd1>] qdisc_destroy+0x30/0xd6
  [<ffffffff81822676>] qdisc_graft+0x11d/0x243
  [<ffffffff818233c1>] tc_get_qdisc+0x1a6/0x1d4
  [<ffffffff810b5eaf>] ? mark_lock+0x2e/0x226
  [<ffffffff817ff8f5>] rtnetlink_rcv_msg+0x181/0x194
  [<ffffffff817ff72e>] ? rtnl_lock+0x17/0x19
  [<ffffffff817ff72e>] ? rtnl_lock+0x17/0x19
  [<ffffffff817ff774>] ? __rtnl_unlock+0x17/0x17
  [<ffffffff81855dc6>] netlink_rcv_skb+0x4d/0x93
  [<ffffffff817ff756>] rtnetlink_rcv+0x26/0x2d
  [<ffffffff818544b2>] netlink_unicast+0xcb/0x150
  [<ffffffff81161db9>] ? might_fault+0x59/0xa9
  [<ffffffff81854f78>] netlink_sendmsg+0x4fa/0x51c
  [<ffffffff817d6e09>] sock_sendmsg_nosec+0x12/0x1d
  [<ffffffff817d8967>] sock_sendmsg+0x29/0x2e
  [<ffffffff817d8cf3>] ___sys_sendmsg+0x1b4/0x23a
  [<ffffffff8100a1b8>] ? native_sched_clock+0x35/0x37
  [<ffffffff810a1d83>] ? sched_clock_local+0x12/0x72
  [<ffffffff810a1fd4>] ? sched_clock_cpu+0x9e/0xb7
  [<ffffffff810def2a>] ? current_kernel_time+0xe/0x32
  [<ffffffff810b4bc5>] ? lock_release_holdtime.part.29+0x71/0x7f
  [<ffffffff810ddebf>] ? read_seqcount_begin.constprop.27+0x5f/0x76
  [<ffffffff810b6292>] ? trace_hardirqs_on_caller+0x17d/0x199
  [<ffffffff811b14d5>] ? __fget_light+0x50/0x78
  [<ffffffff817d9808>] __sys_sendmsg+0x42/0x60
  [<ffffffff817d9838>] SyS_sendmsg+0x12/0x1c
  [<ffffffff81a50e97>] system_call_fastpath+0x12/0x6f
 ---[ end trace ef29d3fb28e97ae7 ]---

For long term, we probably need to clean up the qdisc_graft() code
in case it hides other bugs like this.

Fixes: 95dc19299f74 ("pkt_sched: give visibility to mq slave qdiscs")
Cc: Jamal Hadi Salim <jhs@mojatatu.com>
Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
---
 net/sched/sch_api.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/net/sched/sch_api.c b/net/sched/sch_api.c
index ad9eed7..1e1c89e 100644
--- a/net/sched/sch_api.c
+++ b/net/sched/sch_api.c
@@ -815,10 +815,8 @@ static int qdisc_graft(struct net_device *dev, struct Qdisc *parent,
 		if (dev->flags & IFF_UP)
 			dev_deactivate(dev);
 
-		if (new && new->ops->attach) {
-			new->ops->attach(new);
-			num_q = 0;
-		}
+		if (new && new->ops->attach)
+			goto skip;
 
 		for (i = 0; i < num_q; i++) {
 			struct netdev_queue *dev_queue = dev_ingress_queue(dev);
@@ -834,12 +832,16 @@ static int qdisc_graft(struct net_device *dev, struct Qdisc *parent,
 				qdisc_destroy(old);
 		}
 
+skip:
 		if (!ingress) {
 			notify_and_destroy(net, skb, n, classid,
 					   dev->qdisc, new);
 			if (new && !new->ops->attach)
 				atomic_inc(&new->refcnt);
 			dev->qdisc = new ? : &noop_qdisc;
+
+			if (new && new->ops->attach)
+				new->ops->attach(new);
 		} else {
 			notify_and_destroy(net, skb, n, classid, old, new);
 		}
-- 
1.8.3.1

Re: [Patch net] net_sched: invoke ->attach() after setting dev->qdisc

[Eric Dumazet]

On Tue, 2015-05-26 at 16:08 -0700, Cong Wang wrote:
> For mq qdisc, we add per tx queue qdisc to root qdisc
> for display purpose, however, that happens too early,
> before the new dev->qdisc is finally set, this causes
> q->list points to an old root qdisc which is going to be
> freed right before assigning with a new one.
> 
> Fix this by moving ->attach() after setting dev->qdisc.
> 
> For the record, this fixes the following crash:
...

> For long term, we probably need to clean up the qdisc_graft() code
> in case it hides other bugs like this.
> 
> Fixes: 95dc19299f74 ("pkt_sched: give visibility to mq slave qdiscs")
> Cc: Jamal Hadi Salim <jhs@mojatatu.com>
> Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
> ---
>  net/sched/sch_api.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/net/sched/sch_api.c b/net/sched/sch_api.c
> index ad9eed7..1e1c89e 100644
> --- a/net/sched/sch_api.c
> +++ b/net/sched/sch_api.c
> @@ -815,10 +815,8 @@ static int qdisc_graft(struct net_device *dev, struct Qdisc *parent,
>  		if (dev->flags & IFF_UP)
>  			dev_deactivate(dev);
>  
> -		if (new && new->ops->attach) {
> -			new->ops->attach(new);
> -			num_q = 0;
> -		}
> +		if (new && new->ops->attach)
> +			goto skip;
>  
>  		for (i = 0; i < num_q; i++) {
>  			struct netdev_queue *dev_queue = dev_ingress_queue(dev);
> @@ -834,12 +832,16 @@ static int qdisc_graft(struct net_device *dev, struct Qdisc *parent,
>  				qdisc_destroy(old);
>  		}
>  
> +skip:
>  		if (!ingress) {
>  			notify_and_destroy(net, skb, n, classid,
>  					   dev->qdisc, new);
>  			if (new && !new->ops->attach)
>  				atomic_inc(&new->refcnt);
>  			dev->qdisc = new ? : &noop_qdisc;
> +
> +			if (new && new->ops->attach)
> +				new->ops->attach(new);
>  		} else {
>  			notify_and_destroy(net, skb, n, classid, old, new);
>  		}

Good catch !

Please CC author of a buggy patch, always interesting to learn from our
mistakes. ;)

Note that attach() method is called with the 'new' qdisc,
we might pass this parameter up to qdisc_list_add()

Conceptually, setting dev->qdisc before attach() seems a bit awkward,
but given that attach() returns void, we did not planned having an error
path anyway.

No strong feeling here, I think your patch is fine as is.

Acked-by: Eric Dumazet <edumazet@google.com>

Thanks a lot.

Re: [Patch net] net_sched: invoke ->attach() after setting dev->qdisc

[David Miller]

From: Cong Wang <xiyou.wangcong@gmail.com>
Date: Tue, 26 May 2015 16:08:48 -0700

> For mq qdisc, we add per tx queue qdisc to root qdisc
> for display purpose, however, that happens too early,
> before the new dev->qdisc is finally set, this causes
> q->list points to an old root qdisc which is going to be
> freed right before assigning with a new one.
> 
> Fix this by moving ->attach() after setting dev->qdisc.
> 
> For the record, this fixes the following crash:
 ...
> For long term, we probably need to clean up the qdisc_graft() code
> in case it hides other bugs like this.
> 
> Fixes: 95dc19299f74 ("pkt_sched: give visibility to mq slave qdiscs")
> Cc: Jamal Hadi Salim <jhs@mojatatu.com>
> Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>

Applied, and queued up for -stable, thanks!