netdev - Re: [PATCH iwl-net] ice: add missing napi deletion

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]

Date: Wed, 21 Jun 2023 13:43:33 +0200
From: Maciej Fijalkowski <maciej.fijalkowski@...el.com>
To: Jakub Kicinski <kuba@...nel.org>
CC: <intel-wired-lan@...ts.osuosl.org>, <netdev@...r.kernel.org>,
	<anthony.l.nguyen@...el.com>, <magnus.karlsson@...el.com>,
	<michal.swiatkowski@...el.com>
Subject: Re: [PATCH iwl-net] ice: add missing napi deletion

On Tue, Jun 20, 2023 at 10:49:11AM -0700, Jakub Kicinski wrote:
> On Tue, 20 Jun 2023 19:22:01 +0200 Maciej Fijalkowski wrote:
> > On Tue, Jun 20, 2023 at 09:53:35AM -0700, Jakub Kicinski wrote:
> > > Is there user visible impact? I agree that it's a good habit, but
> > > since unregister cleans up NAPI instances automatically the patch
> > > is not necessarily a fix.  
> > 
> > It's rather free_netdev() not unregistering per se, no? I sent this patch
> > as I found that cited commit didn't delete napis on ice_probe()'s error
> > path - I just saw that as a regression. 
> > 
> > But as you're saying when getting rid of netdev we actually do
> > netif_napi_del() - it seems redundant to do explicit napi delete on remove
> > path as it is supposed do free the netdev. Does it mean that many drivers
> > should be verified against that? Sorta tired so might be missing
> > something, pardon. If not, I'll send a v2 that just removes
> > ice_napi_del().
> 
> I personally prefer to keep track of my resources, so I avoid devm_*
> and delete NAPI instances by hand. It's up to the author and/or
> maintainer of the driver in question.

Hmm I am not a fan of devm either but I didn't mean that in my response at
all.

There are quite a few drivers that do this:

net/core/dev.c:
void free_netdev(struct net_device *dev)
{
	(...)
	list_for_each_entry_safe(p, n, &dev->napi_list, dev_list)
		netif_napi_del(p);
	(...)
}

static inline void netif_napi_del(struct napi_struct *napi)
{
	__netif_napi_del(napi);
	synchronize_net();
}

drivers/net/ethernet/xxxcorp/xxx/xxx_main.c:
static void xxx_remove(struct pci_dev *pdev)
{
	// retrieve net_device and napi_struct ptrs

	netif_napi_del(napi); // redundant, covered below
	(...)
	free_netdev(netdev);
	(...)
}

I believe this is what you were referring to originally and I said that
after a short drivers audit there is a bunch going via flow shown
above...plus my patch was trying to introduce that :)

Although in such case __netif_napi_del() will exit early as
NAPI_STATE_LISTED bit was cleared, if driver holds multiple napi instances
we will be going unnecessarily via synchronize_net() calls.

> 
> My only real ask is to no route this via net and drop the Fixes tag.
> Whether you prefer to keep the patch as is or drop ice_napi_del() --
> up to you.

I'll go through -next and remove ice_napi_del(), given the above
explanation what I meant previously.

>