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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20240206072114.244738a9@kernel.org>
Date: Tue, 6 Feb 2024 07:21:14 -0800
From: Jakub Kicinski <kuba@...nel.org>
To: Shinas Rasheed <srasheed@...vell.com>
Cc: "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
 "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>, Haseeb Gani
 <hgani@...vell.com>, Vimlesh Kumar <vimleshk@...vell.com>, Sathesh B Edara
 <sedara@...vell.com>, "egallen@...hat.com" <egallen@...hat.com>,
 "mschmidt@...hat.com" <mschmidt@...hat.com>, "pabeni@...hat.com"
 <pabeni@...hat.com>, "horms@...nel.org" <horms@...nel.org>,
 "wizhao@...hat.com" <wizhao@...hat.com>, "kheib@...hat.com"
 <kheib@...hat.com>, "konguyen@...hat.com" <konguyen@...hat.com>, "David S.
 Miller" <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>, Jonathan
 Corbet <corbet@....net>, Veerasenareddy Burru <vburru@...vell.com>,
 Satananda Burla <sburla@...vell.com>, Shannon Nelson
 <shannon.nelson@....com>, Tony Nguyen <anthony.l.nguyen@...el.com>, Joshua
 Hay <joshua.a.hay@...el.com>, Rahul Rameshbabu <rrameshbabu@...dia.com>,
 Brett Creeley <brett.creeley@....com>, Andrew Lunn <andrew@...n.ch>, Jacob
 Keller <jacob.e.keller@...el.com>
Subject: Re: [EXT] Re: [PATCH net-next v5 1/8] octeon_ep_vf: Add driver
 framework and device initialization

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.