linux-kernel - Re: [RFC/RFT PATCH net-next v3 1/2] net: Add ndo_write_rx

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: <20251106141259.60e48f1e@kernel.org>
Date: Thu, 6 Nov 2025 14:12:59 -0800
From: Jakub Kicinski <kuba@...nel.org>
To: I Viswanath <viswanathiyyappan@...il.com>
Cc: davem@...emloft.net, edumazet@...gle.com, pabeni@...hat.com,
 horms@...nel.org, sdf@...ichev.me, kuniyu@...gle.com, ahmed.zaki@...el.com,
 aleksander.lobakin@...el.com, jacob.e.keller@...el.com,
 netdev@...r.kernel.org, linux-kernel@...r.kernel.org,
 skhan@...uxfoundation.org, linux-kernel-mentees@...ts.linux.dev,
 david.hunter.linux@...il.com, khalid@...nel.org
Subject: Re: [RFC/RFT PATCH net-next v3 1/2] net: Add ndo_write_rx_config
 and helper structs and functions:

On Thu, 6 Nov 2025 21:38:59 +0530 I Viswanath wrote:
> On Wed, 5 Nov 2025 at 06:16, Jakub Kicinski <kuba@...nel.org> wrote:
> > I wouldn't use atomic flags. IIRC ndo_set_rx_mode is called under
> > netif_addr_lock_bh(), so we can reuse that lock, have update_config()
> > assume ownership of the pending config and update it directly.
> > And read_config() (which IIUC runs from a wq) can take that lock
> > briefly, and swap which config is pending.  
> 
> How does this look?
> 
> It's possible for the actual work of set_rx_mode to be in a work item
> so we need to validate that dev->addr_list_lock is held in update_config()
> 
> // These variables will be part of dev->netif_rx_config_ctx in the final code
> bool pending_cfg_ready = false;
> struct netif_rx_config *ready, *pending;
> 
> void update_config()
> {
>     WARN_ONCE(!spin_is_locked(&dev->addr_list_lock),
>     "netif_update_rx_config() called without netif_addr_lock_bh()\n");
> 
>     int rc = netif_prepare_rx_config(&pending);
>     if (rc)
>         return;
> 
>     pending_cfg_ready = true;
> }
> 
> void read_config()
> {
>     // We could introduce a new lock for this but
>     // reusing the addr lock works well enough
>     netif_addr_lock_bh();
> 
>     // There's no point continuing if the pending config
>     // is not ready
>     if(!pending_cfg_ready) {
>        netif_addr_unlock_bh();
>        return;
>     }
> 
>     swap(ready, pending);
>     pending_cfg_ready = false;
> 
>     netif_addr_unlock_bh();
> 
>     do_io(ready);
> }

Yes, I think this flow looks good.

> On the topic of virtio_net:
> 
> set_rx_mode in virtio_net schedules and does the actual work in a work
> item, so would
> the correct justification here be moving I/O out of the rtnl lock?

Avoiding rtnl_lock is not a goal right now. We should still take
rtnl_lock around read_config() in case the driver assumes rtnl_lock
is held around all its config paths.

The objective is just to simplify the driver. Avoid the state
management and work scheduling in every driver that needs to the config
async. IOW we just want to give drivers an ndo_set_rx_mode that can
sleep.