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
| ||
|
Message-ID: <aBzYAzPtf_TlhT0n@mini-arch> Date: Thu, 8 May 2025 09:12:51 -0700 From: Stanislav Fomichev <stfomichev@...il.com> To: Cosmin Ratiu <cratiu@...dia.com> Cc: netdev@...r.kernel.org, "David S . Miller" <davem@...emloft.net>, Jakub Kicinski <kuba@...nel.org>, Eric Dumazet <edumazet@...gle.com>, Paolo Abeni <pabeni@...hat.com>, Simon Horman <horms@...nel.org>, Stanislav Fomichev <sdf@...ichev.me>, Saeed Mahameed <saeedm@...dia.com>, Tariq Toukan <tariqt@...dia.com>, Dragos Tatulea <dtatulea@...dia.com> Subject: Re: [PATCH net v2] net: Lock lower level devices when updating features 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@...dia.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).
Powered by blists - more mailing lists