diff mbox series

[stable] net: sch_generic: fix the missing new qdisc assignment bug

Message ID 1604373938-211588-1-git-send-email-linyunsheng@huawei.com (mailing list archive)
State Not Applicable
Delegated to: Netdev Maintainers
Headers show
Series [stable] net: sch_generic: fix the missing new qdisc assignment bug | expand

Commit Message

Yunsheng Lin Nov. 3, 2020, 3:25 a.m. UTC
commit 2fb541c862c9 ("net: sch_generic: aviod concurrent reset and enqueue op for lockless qdisc")

When the above upstream commit is backported to stable kernel,
one assignment is missing, which causes two problems reported
by Joakim and Vishwanath, see [1] and [2].

So add the assignment back to fix it.

1. https://www.spinics.net/lists/netdev/msg693916.html
2. https://www.spinics.net/lists/netdev/msg695131.html

Fixes: 749cc0b0c7f3 ("net: sch_generic: aviod concurrent reset and enqueue op for lockless qdisc")
Signed-off-by: Yunsheng Lin <linyunsheng@huawei.com>
---
 net/sched/sch_generic.c | 3 +++
 1 file changed, 3 insertions(+)

Comments

Vishwanath Pai Nov. 3, 2020, 3:46 p.m. UTC | #1
On 11/2/20 10:25 PM, Yunsheng Lin wrote:
 > commit 2fb541c862c9 ("net: sch_generic: aviod concurrent reset and 
enqueue op for lockless qdisc")
 >
 > When the above upstream commit is backported to stable kernel,
 > one assignment is missing, which causes two problems reported
 > by Joakim and Vishwanath, see [1] and [2].
 >
 > So add the assignment back to fix it.
 >
 > 1. 
https://urldefense.com/v3/__https://www.spinics.net/lists/netdev/msg693916.html__;!!GjvTz_vk!AqzcoNtwXeDu-vDNRKnOiOWYmi4B-2atZZExjZTvpp2jeJ9asOyQBVUtQyBp$
 > 2. 
https://urldefense.com/v3/__https://www.spinics.net/lists/netdev/msg695131.html__;!!GjvTz_vk!AqzcoNtwXeDu-vDNRKnOiOWYmi4B-2atZZExjZTvpp2jeJ9asOyQBQlaitCQ$
 >
 > Fixes: 749cc0b0c7f3 ("net: sch_generic: aviod concurrent reset and 
enqueue op for lockless qdisc")
 > Signed-off-by: Yunsheng Lin <linyunsheng@huawei.com>
 > ---
 >  net/sched/sch_generic.c | 3 +++
 >  1 file changed, 3 insertions(+)
 >
 > diff --git a/net/sched/sch_generic.c b/net/sched/sch_generic.c
 > index 0e275e1..6e6147a 100644
 > --- a/net/sched/sch_generic.c
 > +++ b/net/sched/sch_generic.c
 > @@ -1127,10 +1127,13 @@ static void dev_deactivate_queue(struct 
net_device *dev,
 >                   void *_qdisc_default)
 >  {
 >      struct Qdisc *qdisc = rtnl_dereference(dev_queue->qdisc);
 > +    struct Qdisc *qdisc_default = _qdisc_default;
 >
 >      if (qdisc) {
 >          if (!(qdisc->flags & TCQ_F_BUILTIN))
 >              set_bit(__QDISC_STATE_DEACTIVATED, &qdisc->state);
 > +
 > +        rcu_assign_pointer(dev_queue->qdisc, qdisc_default);
 >      }
 >  }
 >

I have tested the patch on v5.4.71 and it fixes our issues.

Tested-by: Vishwanath Pai <vpai@akamai.com>
Jakub Kicinski Nov. 3, 2020, 7:06 p.m. UTC | #2
On Tue, 3 Nov 2020 11:25:38 +0800 Yunsheng Lin wrote:
> commit 2fb541c862c9 ("net: sch_generic: aviod concurrent reset and enqueue op for lockless qdisc")
> 
> When the above upstream commit is backported to stable kernel,
> one assignment is missing, which causes two problems reported
> by Joakim and Vishwanath, see [1] and [2].
> 
> So add the assignment back to fix it.
> 
> 1. https://www.spinics.net/lists/netdev/msg693916.html
> 2. https://www.spinics.net/lists/netdev/msg695131.html
> 
> Fixes: 749cc0b0c7f3 ("net: sch_generic: aviod concurrent reset and enqueue op for lockless qdisc")
> Signed-off-by: Yunsheng Lin <linyunsheng@huawei.com>

Acked-by: Jakub Kicinski <kuba@kernel.org>
Greg KH Nov. 9, 2020, 12:46 p.m. UTC | #3
On Tue, Nov 03, 2020 at 11:25:38AM +0800, Yunsheng Lin wrote:
> commit 2fb541c862c9 ("net: sch_generic: aviod concurrent reset and enqueue op for lockless qdisc")
> 
> When the above upstream commit is backported to stable kernel,
> one assignment is missing, which causes two problems reported
> by Joakim and Vishwanath, see [1] and [2].
> 
> So add the assignment back to fix it.
> 
> 1. https://www.spinics.net/lists/netdev/msg693916.html
> 2. https://www.spinics.net/lists/netdev/msg695131.html
> 
> Fixes: 749cc0b0c7f3 ("net: sch_generic: aviod concurrent reset and enqueue op for lockless qdisc")
> Signed-off-by: Yunsheng Lin <linyunsheng@huawei.com>
> ---
>  net/sched/sch_generic.c | 3 +++
>  1 file changed, 3 insertions(+)

What kernel tree(s) does this need to be backported to?

thanks,

greg k-h
Yunsheng Lin Nov. 10, 2020, 12:58 a.m. UTC | #4
On 2020/11/9 20:46, Greg KH wrote:
> On Tue, Nov 03, 2020 at 11:25:38AM +0800, Yunsheng Lin wrote:
>> commit 2fb541c862c9 ("net: sch_generic: aviod concurrent reset and enqueue op for lockless qdisc")
>>
>> When the above upstream commit is backported to stable kernel,
>> one assignment is missing, which causes two problems reported
>> by Joakim and Vishwanath, see [1] and [2].
>>
>> So add the assignment back to fix it.
>>
>> 1. https://www.spinics.net/lists/netdev/msg693916.html
>> 2. https://www.spinics.net/lists/netdev/msg695131.html
>>
>> Fixes: 749cc0b0c7f3 ("net: sch_generic: aviod concurrent reset and enqueue op for lockless qdisc")
>> Signed-off-by: Yunsheng Lin <linyunsheng@huawei.com>
>> ---
>>  net/sched/sch_generic.c | 3 +++
>>  1 file changed, 3 insertions(+)
> 
> What kernel tree(s) does this need to be backported to?

4.19.x and 5.4.x

Thanks

> 
> thanks,
> 
> greg k-h
> .
>
Brian Norris Nov. 13, 2020, 3:51 a.m. UTC | #5
On Tue, Nov 03, 2020 at 11:25:38AM +0800, Yunsheng Lin wrote:
> commit 2fb541c862c9 ("net: sch_generic: aviod concurrent reset and enqueue op for lockless qdisc")
> 
> When the above upstream commit is backported to stable kernel,
> one assignment is missing, which causes two problems reported
> by Joakim and Vishwanath, see [1] and [2].
> 
> So add the assignment back to fix it.
> 
> 1. https://www.spinics.net/lists/netdev/msg693916.html
> 2. https://www.spinics.net/lists/netdev/msg695131.html
> 
> Fixes: 749cc0b0c7f3 ("net: sch_generic: aviod concurrent reset and enqueue op for lockless qdisc")
> Signed-off-by: Yunsheng Lin <linyunsheng@huawei.com>

For whatever it's worth, we've seen similar problems (particularly,
ENOBUFS on AF_PACKET sockets) and have tested this fix on 4.19.y (we're
also queueing it up on our 5.4.y branch, but haven't tested it as much):

Tested-by: Brian Norris <briannorris@chromium.org>

Thanks!
Greg KH Nov. 17, 2020, 12:09 p.m. UTC | #6
On Tue, Nov 10, 2020 at 08:58:17AM +0800, Yunsheng Lin wrote:
> On 2020/11/9 20:46, Greg KH wrote:
> > On Tue, Nov 03, 2020 at 11:25:38AM +0800, Yunsheng Lin wrote:
> >> commit 2fb541c862c9 ("net: sch_generic: aviod concurrent reset and enqueue op for lockless qdisc")
> >>
> >> When the above upstream commit is backported to stable kernel,
> >> one assignment is missing, which causes two problems reported
> >> by Joakim and Vishwanath, see [1] and [2].
> >>
> >> So add the assignment back to fix it.
> >>
> >> 1. https://www.spinics.net/lists/netdev/msg693916.html
> >> 2. https://www.spinics.net/lists/netdev/msg695131.html
> >>
> >> Fixes: 749cc0b0c7f3 ("net: sch_generic: aviod concurrent reset and enqueue op for lockless qdisc")
> >> Signed-off-by: Yunsheng Lin <linyunsheng@huawei.com>
> >> ---
> >>  net/sched/sch_generic.c | 3 +++
> >>  1 file changed, 3 insertions(+)
> > 
> > What kernel tree(s) does this need to be backported to?
> 
> 4.19.x and 5.4.x

Now queued up, thanks.

greg k-h
diff mbox series

Patch

diff --git a/net/sched/sch_generic.c b/net/sched/sch_generic.c
index 0e275e1..6e6147a 100644
--- a/net/sched/sch_generic.c
+++ b/net/sched/sch_generic.c
@@ -1127,10 +1127,13 @@  static void dev_deactivate_queue(struct net_device *dev,
 				 void *_qdisc_default)
 {
 	struct Qdisc *qdisc = rtnl_dereference(dev_queue->qdisc);
+	struct Qdisc *qdisc_default = _qdisc_default;
 
 	if (qdisc) {
 		if (!(qdisc->flags & TCQ_F_BUILTIN))
 			set_bit(__QDISC_STATE_DEACTIVATED, &qdisc->state);
+
+		rcu_assign_pointer(dev_queue->qdisc, qdisc_default);
 	}
 }