Re: [EXT] Re: [PATCH net-next v5 1/8] octeon_ep_vf: Add driver framework and device initialization

[Jakub Kicinski]

On Tue, 6 Feb 2024 07:42:12 +0000 Shinas Rasheed wrote:
> > > We do cancel_work_sync in octep_vf_remove function.  
> > 
> > But the device is still registered, so the timeout can happen after you
> > cancel but before you unregister.  
> 
> There is rtnl_lock inside octep_vf_tx_timeout_task (the work task
> function), which can protect from unregister_netdev, for such cases
> (code snippet for quick reference below):
> 
> static void octep_vf_tx_timeout_task(struct work_struct *work)
> {
>         struct octep_vf_device *oct = container_of(work, struct octep_vf_device, tx_timeout_task);
>         struct net_device *netdev = oct->netdev;
> 
>         rtnl_lock();
>         if (netif_running(netdev)) {
>                 octep_vf_stop(netdev);
>                 octep_vf_open(netdev);
>         }
>         rtnl_unlock();
> }
> 
> I hope this takes care of it? Please let me know if my thought
> process feels wrong. Thanks!

The problem I see is that if the queue is somehow overloaded or delayed
there can be a race where we cancel, then timeout happens, work doesn't
run for a while, and we free the netdev. So what I'm suggesting is -
hold a reference on the netdevice. That way you can be sure it doesn't
get freed and you can depend on the rtnl+netif_running() doing its job.