linux-kernel - Re: [PATCH net] net/sched: fix netdevice reference leaks in attach_one_default

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20220825085906.35704ce5@kernel.org>
Date:   Thu, 25 Aug 2022 08:59:06 -0700
From:   Jakub Kicinski <kuba@...nel.org>
To:     "wanghai (M)" <wanghai38@...wei.com>
Cc:     <jhs@...atatu.com>, <xiyou.wangcong@...il.com>, <jiri@...nulli.us>,
        <davem@...emloft.net>, <edumazet@...gle.com>, <pabeni@...hat.com>,
        <brouer@...hat.com>, <netdev@...r.kernel.org>,
        <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH net] net/sched: fix netdevice reference leaks in
 attach_one_default_qdisc()

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?