Message-ID: <CANn89iJt72meYXVex-K4JyQ-tR+J4bgHFR5PbcN7EYRGfcL5Tw@mail.gmail.com>
Date: Wed, 26 Mar 2025 23:10:04 +0100
From: Eric Dumazet <edumazet@...gle.com>
To: Kuniyuki Iwashima <kuniyu@...zon.com>
Cc: ychemla@...dia.com, davem@...emloft.net, horms@...nel.org, kuba@...nel.org, 
	kuni1840@...il.com, netdev@...r.kernel.org, pabeni@...hat.com
Subject: Re: [PATCH v5 net 2/3] net: Fix dev_net(dev) race in unregister_netdevice_notifier_dev_net().

On Wed, Mar 26, 2025 at 10:55 PM Kuniyuki Iwashima <kuniyu@...zon.com> wrote:
>
> From: Yael Chemla <ychemla@...dia.com>
> Date: Wed, 26 Mar 2025 15:46:40 +0200
> > On 17/02/2025 21:11, Kuniyuki Iwashima wrote:
> > > After the cited commit, dev_net(dev) is fetched before holding RTNL
> > > and passed to __unregister_netdevice_notifier_net().
> > >
> > > However, dev_net(dev) might be different after holding RTNL.
> > >
> > > In the reported case [0], while removing a VF device, its netns was
> > > being dismantled and the VF was moved to init_net.
> > >
> > > So the following sequence is basically illegal when dev was fetched
> > > without lookup:
> > >
> > >   net = dev_net(dev);
> > >   rtnl_net_lock(net);
> > >
> > > Let's use a new helper rtnl_net_dev_lock() to fix the race.
> > >
> > > It fetches dev_net_rcu(dev), bumps its net->passive, and checks if
> > > dev_net_rcu(dev) is changed after rtnl_net_lock().
> > >
> > > [0]:
> > > BUG: KASAN: slab-use-after-free in notifier_call_chain (kernel/notifier.c:75 (discriminator 2))
> > > Read of size 8 at addr ffff88810cefb4c8 by task test-bridge-lag/21127
> > > Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS rel-1.13.0-0-gf21b5a4aeb02-prebuilt.qemu.org 04/01/2014
> > > Call Trace:
> > >  <TASK>
> > >  dump_stack_lvl (lib/dump_stack.c:123)
> > >  print_report (mm/kasan/report.c:379 mm/kasan/report.c:489)
> > >  kasan_report (mm/kasan/report.c:604)
> > >  notifier_call_chain (kernel/notifier.c:75 (discriminator 2))
> > >  call_netdevice_notifiers_info (net/core/dev.c:2011)
> > >  unregister_netdevice_many_notify (net/core/dev.c:11551)
> > >  unregister_netdevice_queue (net/core/dev.c:11487)
> > >  unregister_netdev (net/core/dev.c:11635)
> > >  mlx5e_remove (drivers/net/ethernet/mellanox/mlx5/core/en_main.c:6552 drivers/net/ethernet/mellanox/mlx5/core/en_main.c:6579) mlx5_core
> > >  auxiliary_bus_remove (drivers/base/auxiliary.c:230)
> > >  device_release_driver_internal (drivers/base/dd.c:1275 drivers/base/dd.c:1296)
> > >  bus_remove_device (./include/linux/kobject.h:193 drivers/base/base.h:73 drivers/base/bus.c:583)
> > >  device_del (drivers/base/power/power.h:142 drivers/base/core.c:3855)
> > >  mlx5_rescan_drivers_locked (./include/linux/auxiliary_bus.h:241 drivers/net/ethernet/mellanox/mlx5/core/dev.c:333 drivers/net/ethernet/mellanox/mlx5/core/dev.c:535 drivers/net/ethernet/mellanox/mlx5/core/dev.c:549) mlx5_core
> > >  mlx5_unregister_device (drivers/net/ethernet/mellanox/mlx5/core/dev.c:468) mlx5_core
> > >  mlx5_uninit_one (./include/linux/instrumented.h:68 ./include/asm-generic/bitops/instrumented-non-atomic.h:141 drivers/net/ethernet/mellanox/mlx5/core/main.c:1563) mlx5_core
> > >  remove_one (drivers/net/ethernet/mellanox/mlx5/core/main.c:965 drivers/net/ethernet/mellanox/mlx5/core/main.c:2019) mlx5_core
> > >  pci_device_remove (./include/linux/pm_runtime.h:129 drivers/pci/pci-driver.c:475)
> > >  device_release_driver_internal (drivers/base/dd.c:1275 drivers/base/dd.c:1296)
> > >  unbind_store (drivers/base/bus.c:245)
> > >  kernfs_fop_write_iter (fs/kernfs/file.c:338)
> > >  vfs_write (fs/read_write.c:587 (discriminator 1) fs/read_write.c:679 (discriminator 1))
> > >  ksys_write (fs/read_write.c:732)
> > >  do_syscall_64 (arch/x86/entry/common.c:52 (discriminator 1) arch/x86/entry/common.c:83 (discriminator 1))
> > >  entry_SYSCALL_64_after_hwframe (arch/x86/entry/entry_64.S:130)
> > > RIP: 0033:0x7f6a4d5018b7
> > >
> > > Fixes: 7fb1073300a2 ("net: Hold rtnl_net_lock() in (un)?register_netdevice_notifier_dev_net().")
> > > Reported-by: Yael Chemla <ychemla@...dia.com>
> > > Closes: https://lore.kernel.org/netdev/146eabfe-123c-4970-901e-e961b4c09bc3@nvidia.com/
> > > Signed-off-by: Kuniyuki Iwashima <kuniyu@...zon.com>
> > > Reviewed-by: Eric Dumazet <edumazet@...gle.com>
> > > ---
> > > v5:
> > >   * Use do-while loop
> > >
> > > v4:
> > >   * Fix build failure when !CONFIG_NET_NS
> > >   * Use net_passive_dec()
> > >
> > > v3:
> > >   * Bump net->passive instead of maybe_get_net()
> > >   * Remove msleep(1) loop
> > >   * Use rcu_access_pointer() instead of rcu_read_lock().
> > >
> > > v2:
> > >   * Use dev_net_rcu().
> > >   * Use msleep(1) instead of cond_resched() after maybe_get_net()
> > >   * Remove cond_resched() after net_eq() check
> > >
> > > v1: https://lore.kernel.org/netdev/20250130232435.43622-2-kuniyu@amazon.com/
> > > ---
> > >  net/core/dev.c | 48 ++++++++++++++++++++++++++++++++++++++++++++----
> > >  1 file changed, 44 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/net/core/dev.c b/net/core/dev.c
> > > index b91658e8aedb..19e268568282 100644
> > > --- a/net/core/dev.c
> > > +++ b/net/core/dev.c
> > > @@ -2070,6 +2070,42 @@ static void __move_netdevice_notifier_net(struct net *src_net,
> > >     __register_netdevice_notifier_net(dst_net, nb, true);
> > >  }
> > >
> > > +static void rtnl_net_dev_lock(struct net_device *dev)
> > > +{
> > > +   bool again;
> > > +
> > > +   do {
> > > +           struct net *net;
> > > +
> > > +           again = false;
> > > +
> > > +           /* netns might be being dismantled. */
> > > +           rcu_read_lock();
> > > +           net = dev_net_rcu(dev);
> > > +           net_passive_inc(net);
> >
> > Hi Kuniyuki,
> > It seems we are still encountering the previously reorted issue,
> > even when running with your latest fix. However, the problem has become
> > less frequent, now requiring multiple test iterations to reproduce.
>
> Thanks for reporting!
>
>
> >
> > 1) we identified the following warnings (each accompanied by a call
> > trace; only one is detailed below, though others are similar):
> >
> > refcount_t: addition on 0; use-after-free.
> > WARNING: CPU: 6 PID: 1105 at lib/refcount.c:25 refcount_warn_saturate
> > (/usr/work/linux/lib/refcount.c:25 (discriminator 1))
> >
> > and also
> >
> > refcount_t: underflow; use-after-free.
> > WARNING: CPU: 6 PID: 1105 at lib/refcount.c:28 refcount_warn_saturate
> > (/usr/work/linux/lib/refcount.c:28 (discriminator 1))
> >
> >
> > 2) test scenario:
> > sets up a network topology of two VFs on different eSwitch, performs
> > ping tests between them, verifies traffic rules offloading, and cleans
> > up the environment afterward.
> >
> > 3) the warning is triggered upon reaching the
> > unregister_netdevice_notifier_dev_net when both net->ns.count and
> > net->passive reference counts are zero.
>
> It looks unlikely but I missed there is still a race window.
>
> If dev_net_rcu() is called between synchronize_net() and dev_net_set()
> in netif_change_net_namespace(), there might be no synchronize_rcu()
> called after that and net_passive_dec() could be called in cleanup_net()
> earlier than net_passive_inc() ... ?
>
> Could you test this ?
>
> ---8<---
> diff --git a/include/net/net_namespace.h b/include/net/net_namespace.h
> index bd57d8fb54f1..c275e95c83ab 100644
> --- a/include/net/net_namespace.h
> +++ b/include/net/net_namespace.h
> @@ -337,9 +337,9 @@ static inline void net_passive_dec(struct net *net)
>  }
>  #endif
>
> -static inline void net_passive_inc(struct net *net)
> +static inline bool net_passive_inc(struct net *net)
>  {
> -       refcount_inc(&net->passive);
> +       return refcount_inc_not_zero(&net->passive);

Hmm

We want the two oher net_passive_inc() callers to have refcount safety.

Please add a new net_passive_inc_not_zero()

Powered by blists - more mailing lists