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

[Jakub Kicinski <kuba@kernel.org>]

On Wed, 15 Jan 2020 00:26:22 +0900, Taehee Yoo wrote:
> On Sun, 12 Jan 2020 at 23:19, Jakub Kicinski wrote:
> Hi Jakub,
> Thank you for your review!

Thank you for fixing these tricky bugs! :)

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

My concern is that process A opens a sysfs file (eg. numvfs) then
process B deletes the device, but process A still has a file descriptor
(and device reference) so it may be able to write/read the numvfs file
even though nsim_bus_dev was already freed. 

I may very well be wrong, and something else may be preventing this
condition. It's just a bit strange to see release free an internal
sub-structure, while the main structure is freed immediately..

> > >       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.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);

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

Hm, we can't use the same lock for the bus ops and port ops.
But the port ops already take port lock, do we really need 
another lock there?

Also does nsim_bus_exit() really need to iterate over devices to remove
them? Does core not do it for us?