netdev - Re: [PATCH net-next v2 6/8] netdev: depend on netdev->lock for xdp features

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: <20250411121938.0afae1b3@kernel.org>
Date: Fri, 11 Apr 2025 12:19:38 -0700
From: Jakub Kicinski <kuba@...nel.org>
To: Stanislav Fomichev <stfomichev@...il.com>
Cc: sdf@...ichev.me, Kuniyuki Iwashima <kuniyu@...zon.com>,
 andrew+netdev@...n.ch, davem@...emloft.net, edumazet@...gle.com,
 horms@...nel.org, hramamurthy@...gle.com, jdamato@...tly.com,
 netdev@...r.kernel.org, pabeni@...hat.com
Subject: Re: [PATCH net-next v2 6/8] netdev: depend on netdev->lock for xdp
 features

On Fri, 11 Apr 2025 10:41:58 -0700 Stanislav Fomichev wrote:
> > Ugh, REGISTER is ops locked we'd need conditional locking here.
> > 
> > Stanislav, I can make the REGISTERED notifier fully locked, right?
> > I suspect any new object we add that's protected by the instance
> > lock will want to lock the dev.  
> 
> Are you suggesting to do s/netdev_lock_ops/netdev_lock/ around
> call_netdevice_notifiers in register_netdevice?

Aha

> We can try, the biggest concern, as usual, are the stacking devices
> (with an extra lock), but casually grepping for NETDEV_REGISTER
> doesn't bring up anything suspicious.
> 
> But if you're gonna do conditional locking for NETDEV_UNREGISTER, any
> reason not to play it safe and add conditional locking to
> NETDEV_REGISTER in netdev_genl_netdevice_event?

Just trying to think what will lead to fewer problems down the line.

Let's me think it thru. So we have this situation:
 - device A - getting registered
 - device L - listens / has the notifier
with upper devs our concern is usually that taking the A's lock
around the notifier forces A -> L lock ordering (between A's instance
lock and whatever lock of L, can be either instance or some other).

If A is an arbitrary device then L has to already be ready to handle
its REGISTER callback under A's instance lock, because A may be ops
locked. So as you say for generic bonding/team changing lock type
is a noop.

If A is a specific device that L is looking for - changing the lock type
around REGISTER may impact L. /me goes to look at the code
Ugh there is a bunch of drivers that wait for a specific device to
register and then take a lock. Let me play it safe then...