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

[Jakub Kicinski <kuba@kernel.org>]

On Fri, 24 Jan 2020 14:16:03 +0900, Taehee Yoo wrote:
> On Wed, 15 Jan 2020 at 23:16, Jakub Kicinski <kuba@kernel.org> wrote:
> > 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..
> 
> I didn't think about ordering of resource release routine.
> So I took a look at the release routine.
> 
> del_device_store()
>     nsim_bus_dev_del()
>         nsim_bus_dev_del()
>             kobject_put()
>                 device_release()
>                     nsim_bus_dev_release()
>                         kfree(nsim_bus_dev->vconfigs)
>     kfree(nsim_bus_dev)
> 
> Before freeing nsim_bus_dev, all resources are freed in the
> device_unregister(). So, I think it's safe.

I see

> > > > >       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?
> >  
> 
> A synchronize routine is needed.
> new_port() and del_port() operations access many device resources.
> It could be used even before resources are allocated or initialized.
> So, new_port() and del_port() should be allowed to use after resources
> are initialized. But sriov_numvfs() doesn't use uninitialized resource
> so it doesn't make any problem.

Oh - because the device can be registered, but that doesn't
mean it's probed yet..

> If a simple flag variable is used, we can avoid using a trylock.
> The flag is set after resources are initialized.
> So if new_port() and del_port() check the flag, it doesn't access
> uninitialized resources.
>
> I would like to try to avoid using trylock.

Sounds good!
 
> > Also does nsim_bus_exit() really need to iterate over devices to remove
> > them? Does core not do it for us?  
> 
> I couldn't find the logic, which remove devices.
> So I think it's needed.

OK, thanks for checking!