* [PATCH net] net/sched: fix netdevice reference leaks in attach_one_default_qdisc()
@ 2022-08-17 10:46 Wang Hai
2022-08-18 17:56 ` Jakub Kicinski
0 siblings, 1 reply; 5+ messages in thread
From: Wang Hai @ 2022-08-17 10:46 UTC (permalink / raw)
To: jhs, xiyou.wangcong, jiri, davem, edumazet, kuba, pabeni, brouer
Cc: netdev, linux-kernel
In attach_default_qdiscs(), when attach default qdisc (fq_codel) fails
and fallback to noqueue, if the original attached qdisc is not released
and a new one is directly attached, this will cause netdevice reference
leaks.
The following is the bug log:
veth0: default qdisc (fq_codel) fail, fallback to noqueue
unregister_netdevice: waiting for veth0 to become free. Usage count = 32
leaked reference.
qdisc_alloc+0x12e/0x210
qdisc_create_dflt+0x62/0x140
attach_one_default_qdisc.constprop.41+0x44/0x70
dev_activate+0x128/0x290
__dev_open+0x12a/0x190
__dev_change_flags+0x1a2/0x1f0
dev_change_flags+0x23/0x60
do_setlink+0x332/0x1150
__rtnl_newlink+0x52f/0x8e0
rtnl_newlink+0x43/0x70
rtnetlink_rcv_msg+0x140/0x3b0
netlink_rcv_skb+0x50/0x100
netlink_unicast+0x1bb/0x290
netlink_sendmsg+0x37c/0x4e0
sock_sendmsg+0x5f/0x70
____sys_sendmsg+0x208/0x280
In attach_one_default_qdisc(), release the old one before attaching
a new qdisc to fix this bug.
Fixes: bf6dba76d278 ("net: sched: fallback to qdisc noqueue if default qdisc setup fail")
Signed-off-by: Wang Hai <wanghai38@huawei.com>
---
net/sched/sch_generic.c | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/net/sched/sch_generic.c b/net/sched/sch_generic.c
index d47b9689eba6..87b61ef14497 100644
--- a/net/sched/sch_generic.c
+++ b/net/sched/sch_generic.c
@@ -1140,6 +1140,11 @@ static void attach_one_default_qdisc(struct net_device *dev,
if (!netif_is_multiqueue(dev))
qdisc->flags |= TCQ_F_ONETXQUEUE | TCQ_F_NOPARENT;
+
+ if (dev_queue->qdisc_sleeping &&
+ dev_queue->qdisc_sleeping != &noop_qdisc)
+ qdisc_put(dev_queue->qdisc_sleeping);
+
dev_queue->qdisc_sleeping = qdisc;
}
--
2.17.1
^ permalink raw reply related [flat|nested] 5+ messages in thread* Re: [PATCH net] net/sched: fix netdevice reference leaks in attach_one_default_qdisc()
2022-08-17 10:46 [PATCH net] net/sched: fix netdevice reference leaks in attach_one_default_qdisc() Wang Hai
@ 2022-08-18 17:56 ` Jakub Kicinski
2022-08-19 15:58 ` wanghai (M)
0 siblings, 1 reply; 5+ messages in thread
From: Jakub Kicinski @ 2022-08-18 17:56 UTC (permalink / raw)
To: Wang Hai
Cc: jhs, xiyou.wangcong, jiri, davem, edumazet, pabeni, brouer,
netdev, linux-kernel
On Wed, 17 Aug 2022 18:46:46 +0800 Wang Hai wrote:
> In attach_default_qdiscs(), when attach default qdisc (fq_codel) fails
> and fallback to noqueue, if the original attached qdisc is not released
> and a new one is directly attached, this will cause netdevice reference
> leaks.
Could you provide more details on the failure path? My preference would
be to try to clean up properly there, if possible.
> The following is the bug log:
>
> veth0: default qdisc (fq_codel) fail, fallback to noqueue
> unregister_netdevice: waiting for veth0 to become free. Usage count = 32
> leaked reference.
> qdisc_alloc+0x12e/0x210
> qdisc_create_dflt+0x62/0x140
> attach_one_default_qdisc.constprop.41+0x44/0x70
> dev_activate+0x128/0x290
> __dev_open+0x12a/0x190
> __dev_change_flags+0x1a2/0x1f0
> dev_change_flags+0x23/0x60
> do_setlink+0x332/0x1150
> __rtnl_newlink+0x52f/0x8e0
> rtnl_newlink+0x43/0x70
> rtnetlink_rcv_msg+0x140/0x3b0
> netlink_rcv_skb+0x50/0x100
> netlink_unicast+0x1bb/0x290
> netlink_sendmsg+0x37c/0x4e0
> sock_sendmsg+0x5f/0x70
> ____sys_sendmsg+0x208/0x280
>
> In attach_one_default_qdisc(), release the old one before attaching
> a new qdisc to fix this bug.
>
> Fixes: bf6dba76d278 ("net: sched: fallback to qdisc noqueue if default qdisc setup fail")
> Signed-off-by: Wang Hai <wanghai38@huawei.com>
> ---
> net/sched/sch_generic.c | 5 +++++
> 1 file changed, 5 insertions(+)
>
> diff --git a/net/sched/sch_generic.c b/net/sched/sch_generic.c
> index d47b9689eba6..87b61ef14497 100644
> --- a/net/sched/sch_generic.c
> +++ b/net/sched/sch_generic.c
> @@ -1140,6 +1140,11 @@ static void attach_one_default_qdisc(struct net_device *dev,
>
> if (!netif_is_multiqueue(dev))
> qdisc->flags |= TCQ_F_ONETXQUEUE | TCQ_F_NOPARENT;
> +
> + if (dev_queue->qdisc_sleeping &&
> + dev_queue->qdisc_sleeping != &noop_qdisc)
> + qdisc_put(dev_queue->qdisc_sleeping);
> +
> dev_queue->qdisc_sleeping = qdisc;
> }
>
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH net] net/sched: fix netdevice reference leaks in attach_one_default_qdisc()
2022-08-18 17:56 ` Jakub Kicinski
@ 2022-08-19 15:58 ` wanghai (M)
2022-08-25 12:29 ` wanghai (M)
0 siblings, 1 reply; 5+ messages in thread
From: wanghai (M) @ 2022-08-19 15:58 UTC (permalink / raw)
To: Jakub Kicinski
Cc: jhs, xiyou.wangcong, jiri, davem, edumazet, pabeni, brouer,
netdev, linux-kernel
在 2022/8/19 1:56, Jakub Kicinski 写道:
> On Wed, 17 Aug 2022 18:46:46 +0800 Wang Hai wrote:
>> In attach_default_qdiscs(), when attach default qdisc (fq_codel) fails
>> and fallback to noqueue, if the original attached qdisc is not released
>> and a new one is directly attached, this will cause netdevice reference
>> leaks.
> Could you provide more details on the failure path? My preference would
> be to try to clean up properly there, if possible.
Hi Jakub.
Here are the details of the failure. Do I need to do cleanup under the
failed path?
If a dev has multiple queues and queue 0 fails to attach qdisc
because there is no memory in attach_one_default_qdisc(). Then
dev->qdisc will be noop_qdisc by default. But the other queues
may be able to successfully attach to default qdisc.
In this case, the fallback to noqueue process will be triggered
static void attach_default_qdiscs(struct net_device *dev)
{
...
if (!netif_is_multiqueue(dev) ||
dev->priv_flags & IFF_NO_QUEUE) {
...
netdev_for_each_tx_queue(dev, attach_one_default_qdisc,
NULL); // queue 0 attach failed because -ENOBUFS, but the other queues
attach successfully
qdisc = txq->qdisc_sleeping;
rcu_assign_pointer(dev->qdisc, qdisc); // dev->qdisc =
&noop_qdisc
...
}
...
if (qdisc == &noop_qdisc) {
...
netdev_for_each_tx_queue(dev, attach_one_default_qdisc, NULL);
// Re-attach, but not release the previously created qdisc
...
}
}
>> The following is the bug log:
>>
>> veth0: default qdisc (fq_codel) fail, fallback to noqueue
>> unregister_netdevice: waiting for veth0 to become free. Usage count = 32
>> leaked reference.
>> qdisc_alloc+0x12e/0x210
>> qdisc_create_dflt+0x62/0x140
>> attach_one_default_qdisc.constprop.41+0x44/0x70
>> dev_activate+0x128/0x290
>> __dev_open+0x12a/0x190
>> __dev_change_flags+0x1a2/0x1f0
>> dev_change_flags+0x23/0x60
>> do_setlink+0x332/0x1150
>> __rtnl_newlink+0x52f/0x8e0
>> rtnl_newlink+0x43/0x70
>> rtnetlink_rcv_msg+0x140/0x3b0
>> netlink_rcv_skb+0x50/0x100
>> netlink_unicast+0x1bb/0x290
>> netlink_sendmsg+0x37c/0x4e0
>> sock_sendmsg+0x5f/0x70
>> ____sys_sendmsg+0x208/0x280
>>
>> In attach_one_default_qdisc(), release the old one before attaching
>> a new qdisc to fix this bug.
>>
>> Fixes: bf6dba76d278 ("net: sched: fallback to qdisc noqueue if default qdisc setup fail")
>> Signed-off-by: Wang Hai <wanghai38@huawei.com>
>> ---
>> net/sched/sch_generic.c | 5 +++++
>> 1 file changed, 5 insertions(+)
>>
>> diff --git a/net/sched/sch_generic.c b/net/sched/sch_generic.c
>> index d47b9689eba6..87b61ef14497 100644
>> --- a/net/sched/sch_generic.c
>> +++ b/net/sched/sch_generic.c
>> @@ -1140,6 +1140,11 @@ static void attach_one_default_qdisc(struct net_device *dev,
>>
>> if (!netif_is_multiqueue(dev))
>> qdisc->flags |= TCQ_F_ONETXQUEUE | TCQ_F_NOPARENT;
>> +
>> + if (dev_queue->qdisc_sleeping &&
>> + dev_queue->qdisc_sleeping != &noop_qdisc)
>> + qdisc_put(dev_queue->qdisc_sleeping);
>> +
>> dev_queue->qdisc_sleeping = qdisc;
>> }
>>
> .
--
Wang Hai
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH net] net/sched: fix netdevice reference leaks in attach_one_default_qdisc()
2022-08-19 15:58 ` wanghai (M)
@ 2022-08-25 12:29 ` wanghai (M)
2022-08-25 15:59 ` Jakub Kicinski
0 siblings, 1 reply; 5+ messages in thread
From: wanghai (M) @ 2022-08-25 12:29 UTC (permalink / raw)
To: Jakub Kicinski
Cc: jhs, xiyou.wangcong, jiri, davem, edumazet, pabeni, brouer,
netdev, linux-kernel
在 2022/8/19 23:58, wanghai (M) 写道:
>
> 在 2022/8/19 1:56, Jakub Kicinski 写道:
>> On Wed, 17 Aug 2022 18:46:46 +0800 Wang Hai wrote:
>>> In attach_default_qdiscs(), when attach default qdisc (fq_codel) fails
>>> and fallback to noqueue, if the original attached qdisc is not released
>>> and a new one is directly attached, this will cause netdevice reference
>>> leaks.
>> Could you provide more details on the failure path? My preference would
>> be to try to clean up properly there, if possible.
> Hi Jakub.
>
> Here are the details of the failure. Do I need to do cleanup under the
> failed path?
>
> If a dev has multiple queues and queue 0 fails to attach qdisc
> because there is no memory in attach_one_default_qdisc(). Then
> dev->qdisc will be noop_qdisc by default. But the other queues
> may be able to successfully attach to default qdisc.
>
> In this case, the fallback to noqueue process will be triggered
>
> static void attach_default_qdiscs(struct net_device *dev)
> {
> ...
> if (!netif_is_multiqueue(dev) ||
> dev->priv_flags & IFF_NO_QUEUE) {
> ...
> netdev_for_each_tx_queue(dev, attach_one_default_qdisc,
> NULL); // queue 0 attach failed because -ENOBUFS, but the other queues
> attach successfully
> qdisc = txq->qdisc_sleeping;
> rcu_assign_pointer(dev->qdisc, qdisc); // dev->qdisc =
> &noop_qdisc
> ...
> }
> ...
> if (qdisc == &noop_qdisc) {
> ...
> netdev_for_each_tx_queue(dev, attach_one_default_qdisc, NULL);
> // Re-attach, but not release the previously created qdisc
> ...
> }
> }
>
Hi Jakub.
Do you have any other suggestions for this patch? Any replies would be
appreciated.
>>> The following is the bug log:
>>>
>>> veth0: default qdisc (fq_codel) fail, fallback to noqueue
>>> unregister_netdevice: waiting for veth0 to become free. Usage count
>>> = 32
>>> leaked reference.
>>> qdisc_alloc+0x12e/0x210
>>> qdisc_create_dflt+0x62/0x140
>>> attach_one_default_qdisc.constprop.41+0x44/0x70
>>> dev_activate+0x128/0x290
>>> __dev_open+0x12a/0x190
>>> __dev_change_flags+0x1a2/0x1f0
>>> dev_change_flags+0x23/0x60
>>> do_setlink+0x332/0x1150
>>> __rtnl_newlink+0x52f/0x8e0
>>> rtnl_newlink+0x43/0x70
>>> rtnetlink_rcv_msg+0x140/0x3b0
>>> netlink_rcv_skb+0x50/0x100
>>> netlink_unicast+0x1bb/0x290
>>> netlink_sendmsg+0x37c/0x4e0
>>> sock_sendmsg+0x5f/0x70
>>> ____sys_sendmsg+0x208/0x280
>>>
>>> In attach_one_default_qdisc(), release the old one before attaching
>>> a new qdisc to fix this bug.
>>>
>>> Fixes: bf6dba76d278 ("net: sched: fallback to qdisc noqueue if
>>> default qdisc setup fail")
>>> Signed-off-by: Wang Hai <wanghai38@huawei.com>
>>> ---
>>> net/sched/sch_generic.c | 5 +++++
>>> 1 file changed, 5 insertions(+)
>>>
>>> diff --git a/net/sched/sch_generic.c b/net/sched/sch_generic.c
>>> index d47b9689eba6..87b61ef14497 100644
>>> --- a/net/sched/sch_generic.c
>>> +++ b/net/sched/sch_generic.c
>>> @@ -1140,6 +1140,11 @@ static void attach_one_default_qdisc(struct
>>> net_device *dev,
>>> if (!netif_is_multiqueue(dev))
>>> qdisc->flags |= TCQ_F_ONETXQUEUE | TCQ_F_NOPARENT;
>>> +
>>> + if (dev_queue->qdisc_sleeping &&
>>> + dev_queue->qdisc_sleeping != &noop_qdisc)
>>> + qdisc_put(dev_queue->qdisc_sleeping);
>>> +
>>> dev_queue->qdisc_sleeping = qdisc;
>>> }
>> .
>
--
Wang Hai
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH net] net/sched: fix netdevice reference leaks in attach_one_default_qdisc()
2022-08-25 12:29 ` wanghai (M)
@ 2022-08-25 15:59 ` Jakub Kicinski
0 siblings, 0 replies; 5+ messages in thread
From: Jakub Kicinski @ 2022-08-25 15:59 UTC (permalink / raw)
To: wanghai (M)
Cc: jhs, xiyou.wangcong, jiri, davem, edumazet, pabeni, brouer,
netdev, linux-kernel
On Thu, 25 Aug 2022 20:29:21 +0800 wanghai (M) wrote:
> 在 2022/8/19 23:58, wanghai (M) 写道:
> > Here are the details of the failure. Do I need to do cleanup under the
> > failed path?
> >
> > If a dev has multiple queues and queue 0 fails to attach qdisc
> > because there is no memory in attach_one_default_qdisc(). Then
> > dev->qdisc will be noop_qdisc by default. But the other queues
> > may be able to successfully attach to default qdisc.
> >
> > In this case, the fallback to noqueue process will be triggered
> >
> > static void attach_default_qdiscs(struct net_device *dev)
> > {
> > ...
> > if (!netif_is_multiqueue(dev) ||
> > dev->priv_flags & IFF_NO_QUEUE) {
> > ...
> > netdev_for_each_tx_queue(dev, attach_one_default_qdisc,
> > NULL); // queue 0 attach failed because -ENOBUFS, but the other queues
> > attach successfully
> > qdisc = txq->qdisc_sleeping;
> > rcu_assign_pointer(dev->qdisc, qdisc); // dev->qdisc =
> > &noop_qdisc
> > ...
> > }
> > ...
> > if (qdisc == &noop_qdisc) {
> > ...
> > netdev_for_each_tx_queue(dev, attach_one_default_qdisc, NULL);
> > // Re-attach, but not release the previously created qdisc
> > ...
> > }
> > }
>
> Do you have any other suggestions for this patch? Any replies would be
> appreciated.
Sorry for the silence and thanks for a solid explanation!
Can we do a:
netdev_for_each_tx_queue(dev, shutdown_scheduler_queue, &noop_qdisc);
before trying to re-attach, to clear out any non-noop qdisc that may
have gotten assigned?
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2022-08-25 15:59 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-08-17 10:46 [PATCH net] net/sched: fix netdevice reference leaks in attach_one_default_qdisc() Wang Hai
2022-08-18 17:56 ` Jakub Kicinski
2022-08-19 15:58 ` wanghai (M)
2022-08-25 12:29 ` wanghai (M)
2022-08-25 15:59 ` Jakub Kicinski
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).