Date:   Wed, 15 Jan 2020 00:26:22 +0900
From:   Taehee Yoo <ap420073@...il.com>
To:     Jakub Kicinski <jakub.kicinski@...ronome.com>
Cc:     David Miller <davem@...emloft.net>,
        Netdev <netdev@...r.kernel.org>, Jiri Pirko <jiri@...nulli.us>
Subject: Re: [PATCH net 1/5] netdevsim: fix a race condition in netdevsim operations

On Sun, 12 Jan 2020 at 23:19, Jakub Kicinski
<jakub.kicinski@...ronome.com> wrote:
>

Hi Jakub,
Thank you for your review!

> On Sat, 11 Jan 2020 16:36:55 +0000, Taehee Yoo wrote:
> > netdevsim basic operations are called with sysfs.
> >
> > Create netdevsim device:
> > echo "1 1" > /sys/bus/netdevsim/new_device
> > Delete netdevsim device:
> > echo 1 > /sys/bus/netdevsim/del_device
> > Create netdevsim port:
> > echo 4 > /sys/devices/netdevsim1/new_port
> > Delete netdevsim port:
> > echo 4 > /sys/devices/netdevsim1/del_port
> > Set sriov number:
> > echo 4 > /sys/devices/netdevsim1/sriov_numvfs
> >
> > These operations use the same resource so they should acquire a lock for
> > the whole resource not only for a list.
> >
> > Test commands:
> >     #SHELL1
> >     modprobe netdevsim
> >     while :
> >     do
> >         echo "1 1" > /sys/bus/netdevsim/new_device
> >         echo "1 1" > /sys/bus/netdevsim/del_device
> >     done
> >
> >     #SHELL2
> >     while :
> >     do
> >         echo 1 > /sys/devices/netdevsim1/new_port
> >         echo 1 > /sys/devices/netdevsim1/del_port
> >     done
> >
> > Splat looks like:
> > [  151.623634][ T1165] kasan: CONFIG_KASAN_INLINE enabled
> > [  151.626503][ T1165] kasan: GPF could be caused by NULL-ptr deref or user memory access
>
>
> > In this patch, __init and __exit function also acquire a lock.
> > operations could be called while __init and __exit functions are
> > processing. If so, uninitialized or freed resources could be used.
> > So, __init() and __exit function also need lock.
> >
> > Fixes: 83c9e13aa39a ("netdevsim: add software driver for testing offloads")
>
> I don't think that's the correct Fixes tag, the first version of the
> driver did not use the sysfs interface.
>

I checked up the Fixes tag. you're right, this Fixes tag is not correct.
I will fix this.

> > Signed-off-by: Taehee Yoo <ap420073@...il.com>
>
> > --- a/drivers/net/netdevsim/bus.c
> > +++ b/drivers/net/netdevsim/bus.c
> > @@ -16,7 +16,8 @@
> >
> >  static DEFINE_IDA(nsim_bus_dev_ids);
> >  static LIST_HEAD(nsim_bus_dev_list);
> > -static DEFINE_MUTEX(nsim_bus_dev_list_lock);
> > +/* mutex lock for netdevsim operations */
> > +DEFINE_MUTEX(nsim_bus_dev_ops_lock);
> >
> >  static struct nsim_bus_dev *to_nsim_bus_dev(struct device *dev)
> >  {
> > @@ -51,9 +52,14 @@ nsim_bus_dev_numvfs_store(struct device *dev, struct device_attribute *attr,
>
> Could the vf handling use the device lock like PCI does?
>
> But actually, we free VF info from the release function, which IIUC is
> called after all references to the device are gone. So this should be
> fine, no?
>

I have tested this patch without locking in the
nsim_bus_dev_numvfs_store(). It worked well.
The freeing and allocating routines are protected by RTNL.
As you said, the ->release() routine, which frees vconfig is protected by
reference count. So, It's safe.
I will drop this at a v2 patch.

> Perhaps the entire bus dev structure should be freed from release?
>

I tested this like this.

@@ -146,6 +161,8 @@ static void nsim_bus_dev_release(struct device *dev)
        struct nsim_bus_dev *nsim_bus_dev = to_nsim_bus_dev(dev);

        nsim_bus_dev_vfs_disable(nsim_bus_dev);
+       ida_free(&nsim_bus_dev_ids, nsim_bus_dev->dev.id);
+       kfree(nsim_bus_dev);
 }
@@ -300,8 +320,6 @@ nsim_bus_dev_new(unsigned int id, unsigned int port_count)
 static void nsim_bus_dev_del(struct nsim_bus_dev *nsim_bus_dev)
 {
        device_unregister(&nsim_bus_dev->dev);
-       ida_free(&nsim_bus_dev_ids, nsim_bus_dev->dev.id);
-       kfree(nsim_bus_dev);
 }

It works well. but I'm not sure this is needed.

> >       unsigned int num_vfs;
> >       int ret;
> >
> > +     if (!mutex_trylock(&nsim_bus_dev_ops_lock))
> > +             return -EBUSY;
>
> Why the trylocks? Are you trying to catch the races between unregister
> and other ops?
>

The reason is to avoid deadlock.
If we use mutex_lock() instead of mutex_trylock(), the below message
will be printed and actual deadlock also appeared.

[  426.879562][  T805] WARNING: possible circular locking dependency detected
[  426.880477][  T805] 5.5.0-rc5+ #280 Not tainted
[  426.881080][  T805] ------------------------------------------------------
[  426.882035][  T805] bash/805 is trying to acquire lock:
[  426.882826][  T805] ffffffffc03b7830 (nsim_bus_dev_ops_lock){+.+.},
at: del_port_store+0x68/0xe0 [netdevsim]
[  426.884159][  T805]
[  426.884159][  T805] but task is already holding lock:
[  426.885061][  T805] ffff88805edb54b0 (kn->count#170){++++}, at:
kernfs_fop_write+0x1f2/0x410
[  426.886130][  T805]
[  426.886130][  T805] which lock already depends on the new lock.
[  426.886130][  T805]
[  426.887492][  T805]
[  426.887492][  T805] the existing dependency chain (in reverse order) is:
[  426.888606][  T805]
[  426.888606][  T805] -> #1 (kn->count#170){++++}:
[  426.889749][  T805]        __kernfs_remove+0x612/0x7f0
[  426.890392][  T805]        kernfs_remove_by_name_ns+0x40/0x80
[  426.891342][  T805]        remove_files.isra.1+0x6c/0x170
[  426.892359][  T805]        sysfs_remove_group+0x9b/0x170
[  426.893414][  T805]        sysfs_remove_groups+0x4f/0x90
[  426.894405][  T805]        device_remove_attrs+0xc9/0x130
[  426.895198][  T805]        device_del+0x413/0xc10
[  426.895798][  T805]        device_unregister+0x16/0xa0
[  426.896457][  T805]        del_device_store+0x288/0x3c0 [netdevsim]
[  426.897250][  T805]        kernfs_fop_write+0x276/0x410
[  426.897915][  T805]        vfs_write+0x197/0x4a0
[  426.898506][  T805]        ksys_write+0x141/0x1d0
[  426.899111][  T805]        do_syscall_64+0x99/0x4f0
[  426.899738][  T805]        entry_SYSCALL_64_after_hwframe+0x49/0xbe
[  426.900557][  T805]
[  426.900557][  T805] -> #0 (nsim_bus_dev_ops_lock){+.+.}:
[  426.901425][  T805]        __lock_acquire+0x2d8d/0x3de0
[  426.902023][  T805]        lock_acquire+0x164/0x3b0
[  426.902586][  T805]        __mutex_lock+0x14d/0x14b0
[  426.903166][  T805]        del_port_store+0x68/0xe0 [netdevsim]
[  426.903840][  T805]        kernfs_fop_write+0x276/0x410
[  426.904430][  T805]        vfs_write+0x197/0x4a0
[  426.904952][  T805]        ksys_write+0x141/0x1d0
[  426.905481][  T805]        do_syscall_64+0x99/0x4f0
[  426.906032][  T805]        entry_SYSCALL_64_after_hwframe+0x49/0xbe
[  426.906745][  T805]
[  426.906745][  T805] other info that might help us debug this:
[  426.906745][  T805]
[  426.907883][  T805]  Possible unsafe locking scenario:
[  426.907883][  T805]
[  426.908715][  T805]        CPU0                    CPU1
[  426.909312][  T805]        ----                    ----
[  426.909902][  T805]   lock(kn->count#170);
[  426.910372][  T805]
lock(nsim_bus_dev_ops_lock);
[  426.911277][  T805]                                lock(kn->count#170);
[  426.912032][  T805]   lock(nsim_bus_dev_ops_lock);
[  426.912581][  T805]
[  426.912581][  T805]  *** DEADLOCK ***
[  426.912581][  T805]
[  426.913495][  T805] 3 locks held by bash/805:
[  426.913998][  T805]  #0: ffff88806547c478 (sb_writers#5){.+.+}, at:
vfs_write+0x3bb/0x4a0
[  426.914944][  T805]  #1: ffff88805ff55c90 (&of->mutex){+.+.}, at:
kernfs_fop_write+0x1cf/0x410
[  426.915928][  T805]  #2: ffff88805edb54b0 (kn->count#170){++++},
at: kernfs_fop_write+0x1f2/0x410
[  426.916957][  T805]
[  426.916957][  T805] stack backtrace:
[  426.917614][  T805] CPU: 1 PID: 805 Comm: bash Not tainted 5.5.0-rc5+ #280
[  426.918398][  T805] Hardware name: innotek GmbH
VirtualBox/VirtualBox, BIOS VirtualBox 12/01/2006
[  426.919408][  T805] Call Trace:
[  426.919804][  T805]  dump_stack+0x96/0xdb
[  426.920263][  T805]  check_noncircular+0x371/0x450
[  426.920813][  T805]  ? print_circular_bug.isra.36+0x310/0x310
[  426.921475][  T805]  ? lock_acquire+0x164/0x3b0
[  426.921993][  T805]  ? hlock_class+0x130/0x130
[  426.922502][  T805]  ? __lock_acquire+0x2d8d/0x3de0
[  426.923158][  T805]  __lock_acquire+0x2d8d/0x3de0
[  426.923692][  T805]  ? register_lock_class+0x14d0/0x14d0
[  426.924282][  T805]  lock_acquire+0x164/0x3b0
[  426.924784][  T805]  ? del_port_store+0x68/0xe0 [netdevsim]
[  426.925415][  T805]  __mutex_lock+0x14d/0x14b0
[  426.925925][  T805]  ? del_port_store+0x68/0xe0 [netdevsim]
[  426.926551][  T805]  ? __lock_acquire+0xdfe/0x3de0
[  426.927108][  T805]  ? del_port_store+0x68/0xe0 [netdevsim]
[  426.927740][  T805]  ? mutex_lock_io_nested+0x1380/0x1380
[  426.928349][  T805]  ? register_lock_class+0x14d0/0x14d0
[  426.928947][  T805]  ? check_chain_key+0x236/0x5d0
[  426.929490][  T805]  ? kernfs_fop_write+0x1cf/0x410
[  426.930042][  T805]  ? lock_acquire+0x164/0x3b0
[  426.930558][  T805]  ? kernfs_fop_write+0x1f2/0x410
[  426.931123][  T805]  ? del_port_store+0x68/0xe0 [netdevsim]
[  426.931761][  T805]  del_port_store+0x68/0xe0 [netdevsim]
[  426.932389][  T805]  ? nsim_bus_dev_release+0x100/0x100 [netdevsim]
[  426.933097][  T805]  ? sysfs_file_ops+0x112/0x160
[  426.933630][  T805]  ? sysfs_kf_write+0x3b/0x180
[  426.934152][  T805]  ? sysfs_file_ops+0x160/0x160
[  426.934683][  T805]  kernfs_fop_write+0x276/0x410
[  426.935226][  T805]  ? __sb_start_write+0x215/0x2e0
[  426.935780][  T805]  vfs_write+0x197/0x4a0

Locking ordering of {new/del}_device() and {new/del}_port is different.
So, a deadlock could occur.

Thank you,
Taehee Yoo

Powered by blists - more mailing lists