[PATCH net v2] net: Lock lower level devices when updating features

[PATCH net v2] net: Lock lower level devices when updating features

[Cosmin Ratiu]

__netdev_update_features() expects the netdevice to be ops-locked, but
it gets called recursively on the lower level netdevices to sync their
features, and nothing locks those.

This commit fixes that, with the assumption that it shouldn't be possible
for both higher-level and lover-level netdevices to require the instance
lock, because that would lead to lock dependency warnings.

Without this, playing with higher level (e.g. vxlan) netdevices on top
of netdevices with instance locking enabled can run into issues:

WARNING: CPU: 59 PID: 206496 at ./include/net/netdev_lock.h:17 netif_napi_add_weight_locked+0x753/0xa60
[...]
Call Trace:
 <TASK>
 mlx5e_open_channel+0xc09/0x3740 [mlx5_core]
 mlx5e_open_channels+0x1f0/0x770 [mlx5_core]
 mlx5e_safe_switch_params+0x1b5/0x2e0 [mlx5_core]
 set_feature_lro+0x1c2/0x330 [mlx5_core]
 mlx5e_handle_feature+0xc8/0x140 [mlx5_core]
 mlx5e_set_features+0x233/0x2e0 [mlx5_core]
 __netdev_update_features+0x5be/0x1670
 __netdev_update_features+0x71f/0x1670
 dev_ethtool+0x21c5/0x4aa0
 dev_ioctl+0x438/0xae0
 sock_ioctl+0x2ba/0x690
 __x64_sys_ioctl+0xa78/0x1700
 do_syscall_64+0x6d/0x140
 entry_SYSCALL_64_after_hwframe+0x4b/0x53
 </TASK>

Fixes: 7e4d784f5810 ("net: hold netdev instance lock during rtnetlink operations")
Signed-off-by: Cosmin Ratiu <cratiu@nvidia.com>
---
 net/core/dev.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/net/core/dev.c b/net/core/dev.c
index 1be7cb73a602..4b5df59d6246 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -10454,7 +10454,9 @@ static void netdev_sync_lower_features(struct net_device *upper,
 			netdev_dbg(upper, "Disabling feature %pNF on lower dev %s.\n",
 				   &feature, lower->name);
 			lower->wanted_features &= ~feature;
+			netdev_lock_ops(lower);
 			__netdev_update_features(lower);
+			netdev_unlock_ops(lower);
 
 			if (unlikely(lower->features & feature))
 				netdev_WARN(upper, "failed to disable %pNF on %s!\n",
-- 
2.45.0

Re: [PATCH net v2] net: Lock lower level devices when updating features

[Jakub Kicinski]

On Thu, 8 May 2025 17:54:59 +0300 Cosmin Ratiu wrote:
>  			lower->wanted_features &= ~feature;
> +			netdev_lock_ops(lower);
>  			__netdev_update_features(lower);
> +			netdev_unlock_ops(lower);
>  
>  			if (unlikely(lower->features & feature))

I'd be slightly tempted to try to cover the accesses to members of
lower, because why not:

			netdev_lock_ops(lower);
 			lower->wanted_features &= ~feature;
 			__netdev_update_features(lower);
			still_enabled = lower->features & feature;
			netdev_unlock_ops(lower);

 			if (unlikely(still_enabled))

WDYT?

Re: [PATCH net v2] net: Lock lower level devices when updating features

[Stanislav Fomichev]

On 05/08, Cosmin Ratiu wrote:
> __netdev_update_features() expects the netdevice to be ops-locked, but
> it gets called recursively on the lower level netdevices to sync their
> features, and nothing locks those.
> 
> This commit fixes that, with the assumption that it shouldn't be possible
> for both higher-level and lover-level netdevices to require the instance
> lock, because that would lead to lock dependency warnings.
> 
> Without this, playing with higher level (e.g. vxlan) netdevices on top
> of netdevices with instance locking enabled can run into issues:
> 
> WARNING: CPU: 59 PID: 206496 at ./include/net/netdev_lock.h:17 netif_napi_add_weight_locked+0x753/0xa60
> [...]
> Call Trace:
>  <TASK>
>  mlx5e_open_channel+0xc09/0x3740 [mlx5_core]
>  mlx5e_open_channels+0x1f0/0x770 [mlx5_core]
>  mlx5e_safe_switch_params+0x1b5/0x2e0 [mlx5_core]
>  set_feature_lro+0x1c2/0x330 [mlx5_core]
>  mlx5e_handle_feature+0xc8/0x140 [mlx5_core]
>  mlx5e_set_features+0x233/0x2e0 [mlx5_core]
>  __netdev_update_features+0x5be/0x1670
>  __netdev_update_features+0x71f/0x1670
>  dev_ethtool+0x21c5/0x4aa0
>  dev_ioctl+0x438/0xae0
>  sock_ioctl+0x2ba/0x690
>  __x64_sys_ioctl+0xa78/0x1700
>  do_syscall_64+0x6d/0x140
>  entry_SYSCALL_64_after_hwframe+0x4b/0x53
>  </TASK>
> 
> Fixes: 7e4d784f5810 ("net: hold netdev instance lock during rtnetlink operations")
> Signed-off-by: Cosmin Ratiu <cratiu@nvidia.com>
> ---
>  net/core/dev.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/net/core/dev.c b/net/core/dev.c
> index 1be7cb73a602..4b5df59d6246 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -10454,7 +10454,9 @@ static void netdev_sync_lower_features(struct net_device *upper,
>  			netdev_dbg(upper, "Disabling feature %pNF on lower dev %s.\n",
>  				   &feature, lower->name);
>  			lower->wanted_features &= ~feature;
> +			netdev_lock_ops(lower);
>  			__netdev_update_features(lower);
> +			netdev_unlock_ops(lower);
>  
>  			if (unlikely(lower->features & feature))
>  				netdev_WARN(upper, "failed to disable %pNF on %s!\n",

Any reason not to cover the whole section under the if()? For example,
looking at netdev_features_change, most of its invocations are under the
lock, so keeping the lock around it might help with consistency (and
we can clarify it as such in Documentation/networking/netdevices.rst).
Plus, wanted_features is already sort of ops-protected (looking at
netif_disable_lro+dev_disable_lro).

Re: [PATCH net v2] net: Lock lower level devices when updating features

[Cosmin Ratiu]

On Thu, 2025-05-08 at 09:12 -0700, Stanislav Fomichev wrote:
> > --- a/net/core/dev.c
> > +++ b/net/core/dev.c
> > @@ -10454,7 +10454,9 @@ static void
> > netdev_sync_lower_features(struct net_device *upper,
> >  			netdev_dbg(upper, "Disabling feature %pNF
> > on lower dev %s.\n",
> >  				   &feature, lower->name);
> >  			lower->wanted_features &= ~feature;
> > +			netdev_lock_ops(lower);
> >  			__netdev_update_features(lower);
> > +			netdev_unlock_ops(lower);
> >  
> >  			if (unlikely(lower->features & feature))
> >  				netdev_WARN(upper, "failed to
> > disable %pNF on %s!\n",
> 
> Any reason not to cover the whole section under the if()? For
> example,
> looking at netdev_features_change, most of its invocations are under
> the
> lock, so keeping the lock around it might help with consistency (and
> we can clarify it as such in
> Documentation/networking/netdevices.rst).
> Plus, wanted_features is already sort of ops-protected (looking at
> netif_disable_lro+dev_disable_lro).

The critical section could be extended for the whole if, but there are
a lot of netdev_features_change() calls in many drivers, which I am not
sure are ops protected. So I'd be reluctant to state that
NETDEV_FEAT_CHANGE is ops-protected in
Documentation/networking/netdevices.rst, even though all core
invocations would be made with the ops lock held.

I guess that's v3 coming soon then.

Cosmin.

Re: [PATCH net v2] net: Lock lower level devices when updating features

[Stanislav Fomichev]

On 05/08, Cosmin Ratiu wrote:
> On Thu, 2025-05-08 at 09:12 -0700, Stanislav Fomichev wrote:
> > > --- a/net/core/dev.c
> > > +++ b/net/core/dev.c
> > > @@ -10454,7 +10454,9 @@ static void
> > > netdev_sync_lower_features(struct net_device *upper,
> > >  			netdev_dbg(upper, "Disabling feature %pNF
> > > on lower dev %s.\n",
> > >  				   &feature, lower->name);
> > >  			lower->wanted_features &= ~feature;
> > > +			netdev_lock_ops(lower);
> > >  			__netdev_update_features(lower);
> > > +			netdev_unlock_ops(lower);
> > >  
> > >  			if (unlikely(lower->features & feature))
> > >  				netdev_WARN(upper, "failed to
> > > disable %pNF on %s!\n",
> > 
> > Any reason not to cover the whole section under the if()? For
> > example,
> > looking at netdev_features_change, most of its invocations are under
> > the
> > lock, so keeping the lock around it might help with consistency (and
> > we can clarify it as such in
> > Documentation/networking/netdevices.rst).
> > Plus, wanted_features is already sort of ops-protected (looking at
> > netif_disable_lro+dev_disable_lro).
> 
> The critical section could be extended for the whole if, but there are
> a lot of netdev_features_change() calls in many drivers, which I am not
> sure are ops protected. So I'd be reluctant to state that
> NETDEV_FEAT_CHANGE is ops-protected in
> Documentation/networking/netdevices.rst, even though all core
> invocations would be made with the ops lock held.

Ack, I don't think the calls in drivers/ matter, none of these are
ops-protected drivers, but we can do that separately.