Re: [PATCH net 1/5] netdevsim: fix a race condition in netdevsim operations

[Jakub Kicinski <jakub.kicinski@netronome.com>]

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.

> Signed-off-by: Taehee Yoo <ap420073@gmail.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?

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

>  	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?

>  	ret = kstrtouint(buf, 0, &num_vfs);
> -	if (ret)
> +	if (ret) {
> +		mutex_unlock(&nsim_bus_dev_ops_lock);
>  		return ret;
> +	}
>  
>  	rtnl_lock();
>  	if (nsim_bus_dev->num_vfs == num_vfs)