netdev - Re: [PATCH net-next] net: sched: calls synchronize

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <7e2e96b2-27a6-4f1d-90da-546f7c26ed0c@redhat.com>
Date: Tue, 14 Jan 2025 10:17:24 +0100
From: Paolo Abeni <pabeni@...hat.com>
To: Eric Dumazet <edumazet@...gle.com>, Cong Wang <xiyou.wangcong@...il.com>
Cc: "David S . Miller" <davem@...emloft.net>, Jakub Kicinski
 <kuba@...nel.org>, Simon Horman <horms@...nel.org>,
 Jamal Hadi Salim <jhs@...atatu.com>, Jiri Pirko <jiri@...nulli.us>,
 netdev@...r.kernel.org, eric.dumazet@...il.com
Subject: Re: [PATCH net-next] net: sched: calls synchronize_net() only when
 needed

On 1/13/25 5:08 PM, Eric Dumazet wrote:
> On Fri, Jan 10, 2025 at 6:49 AM Eric Dumazet <edumazet@...gle.com> wrote:
>>
>> On Fri, Jan 10, 2025 at 6:33 AM Cong Wang <xiyou.wangcong@...il.com> wrote:
>>>
>>> On Thu, Jan 09, 2025 at 05:18:50PM +0000, Eric Dumazet wrote:
>>>> dev_deactivate_many() role is to remove the qdiscs
>>>> of a network device.
>>>>
>>>> When/if a qdisc is dismantled, an rcu grace period
>>>> is needed to make sure all outstanding qdisc enqueue
>>>> are done before we proceed with a qdisc reset.
>>>>
>>>> Most virtual devices do not have a qdisc (if we exclude
>>>> noqueue ones).
>>>
>>> Such as? To me, most virtual devices use noqueue:
>>>
>>> $ git grep IFF_NO_QUEUE -- drivers/net/
>>> drivers/net/amt.c:      dev->priv_flags         |= IFF_NO_QUEUE;
>>> drivers/net/bareudp.c:  dev->priv_flags |= IFF_NO_QUEUE;
>>> drivers/net/bonding/bond_main.c:        bond_dev->priv_flags |= IFF_BONDING | IFF_UNICAST_FLT | IFF_NO_QUEUE;
>>> drivers/net/caif/caif_serial.c: dev->priv_flags |= IFF_NO_QUEUE;
>>> drivers/net/dummy.c:    dev->priv_flags |= IFF_LIVE_ADDR_CHANGE | IFF_NO_QUEUE;
>>> drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c:        dev->priv_flags |= IFF_NO_QUEUE;
>>> drivers/net/ethernet/netronome/nfp/nfp_net_repr.c:      netdev->priv_flags |= IFF_NO_QUEUE | IFF_DISABLE_NETPOLL;
>>> drivers/net/geneve.c:   dev->priv_flags |= IFF_LIVE_ADDR_CHANGE | IFF_NO_QUEUE;
>>> drivers/net/gtp.c:      dev->priv_flags |= IFF_NO_QUEUE;
>>> drivers/net/ipvlan/ipvlan_main.c:       dev->priv_flags |= IFF_UNICAST_FLT | IFF_NO_QUEUE;
>>> drivers/net/ipvlan/ipvtap.c:    dev->priv_flags &= ~IFF_NO_QUEUE;
>>> drivers/net/loopback.c: dev->priv_flags         |= IFF_LIVE_ADDR_CHANGE | IFF_NO_QUEUE;
>>> drivers/net/macsec.c:   dev->priv_flags |= IFF_NO_QUEUE;
>>> drivers/net/macvlan.c:  dev->priv_flags |= IFF_NO_QUEUE;
>>> drivers/net/net_failover.c:     failover_dev->priv_flags |= IFF_UNICAST_FLT | IFF_NO_QUEUE;
>>> drivers/net/netdevsim/netdev.c:                    IFF_NO_QUEUE;
>>> drivers/net/netkit.c:   dev->priv_flags |= IFF_NO_QUEUE;
>>> drivers/net/nlmon.c:    dev->priv_flags |= IFF_NO_QUEUE;
>>> drivers/net/pfcp.c:     dev->priv_flags |= IFF_NO_QUEUE;
>>> drivers/net/team/team_core.c:   dev->priv_flags |= IFF_NO_QUEUE;
>>> drivers/net/veth.c:     dev->priv_flags |= IFF_NO_QUEUE;
>>> drivers/net/vrf.c:      dev->priv_flags |= IFF_NO_QUEUE;
>>> drivers/net/vsockmon.c: dev->priv_flags |= IFF_NO_QUEUE;
>>> drivers/net/vxlan/vxlan_core.c: dev->priv_flags |= IFF_NO_QUEUE;
>>> drivers/net/wan/hdlc_fr.c:      dev->priv_flags |= IFF_NO_QUEUE;
>>> drivers/net/wireguard/device.c: dev->priv_flags |= IFF_NO_QUEUE;
>>> drivers/net/wireless/virtual/mac80211_hwsim.c:  dev->priv_flags |= IFF_NO_QUEUE;
>>>
>>>
>>> And noqueue_qdisc_ops sets ->enqueue to noop_enqueue():
>>>
>>> struct Qdisc_ops noqueue_qdisc_ops __read_mostly = {
>>>         .id             =       "noqueue",
>>>         .priv_size      =       0,
>>>         .init           =       noqueue_init,
>>>         .enqueue        =       noop_enqueue,
>>>         .dequeue        =       noop_dequeue,
>>>         .peek           =       noop_dequeue,
>>>         .owner          =       THIS_MODULE,
>>> };
>>
>> Sure, but please a look at :
>>
>> static int noqueue_init(struct Qdisc *qdisc, struct nlattr *opt,
>> struct netlink_ext_ack *extack)
>> {
>>         /* register_qdisc() assigns a default of noop_enqueue if unset,
>>         * but __dev_queue_xmit() treats noqueue only as such
>>         * if this is NULL - so clear it here. */
>>         qdisc->enqueue = NULL;
>>         return 0;
>> }
> 
> How can we proceed on this patch ?
> 
> I can remove the "(if we exclude noqueue ones)" part if this is confusing.

I personally interpret the lack of reply from Cong to your previous
message as agreement/understanding. I'll drop the blamed sentence from
the commit message when applying the patch.

BTW, in the long run I think it would be nice to remove the noqueue_init
hack - AFAICS no in-tree kernel has NULL enqueue/dequeue/peek CB, so the
related checks in register_qdisc() could possibly be dropped.

/P