linux-kernel - Re: [RFC net 1/2] netdev-genl: Hold rcu_read_lock in napi

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <ZzQFeivicJPnxzzx@LQ3V64L9R2>
Date: Tue, 12 Nov 2024 17:48:42 -0800
From: Joe Damato <jdamato@...tly.com>
To: Jakub Kicinski <kuba@...nel.org>
Cc: netdev@...r.kernel.org, pabeni@...hat.com, edumazet@...gle.com,
	amritha.nambiar@...el.com, sridhar.samudrala@...el.com,
	mkarsten@...terloo.ca, stable@...r.kernel.org,
	"David S. Miller" <davem@...emloft.net>,
	Simon Horman <horms@...nel.org>,
	Mina Almasry <almasrymina@...gle.com>,
	open list <linux-kernel@...r.kernel.org>
Subject: Re: [RFC net 1/2] netdev-genl: Hold rcu_read_lock in napi_get

On Tue, Nov 12, 2024 at 05:28:40PM -0800, Jakub Kicinski wrote:
> On Tue, 12 Nov 2024 18:13:58 +0000 Joe Damato wrote:
> > +/* must be called under rcu_read_lock(), because napi_by_id requires it */
> > +static struct napi_struct *__do_napi_by_id(unsigned int napi_id,
> > +					   struct genl_info *info, int *err)
> > +{
> > +	struct napi_struct *napi;
> > +
> > +	napi = napi_by_id(napi_id);
> > +	if (napi) {
> > +		*err = 0;
> > +	} else {
> > +		NL_SET_BAD_ATTR(info->extack, info->attrs[NETDEV_A_NAPI_ID]);
> > +		*err = -ENOENT;
> > +	}
> > +
> > +	return napi;
> > +}
> 
> Thanks for the quick follow up! I vote we don't factor this out.
> I don't see what it buys us, TBH, normally we factor out code
> to avoid having to unlock before return, but this code doesn't
> have extra returns...
> 
> Just slap an rcu_read_lock / unlock around and that's it?

Sure sounds good.

Sorry for the noob question: should I break it up into two patches
with one CCing stable and the other not like I did for this RFC?

Patch 1 definitely "feels" like a fixes + CC stable
Patch 2 could be either net-next or a net + "fixes" without stable?

> Feel free to repost soon.

Will do, just lmk on the above so I can submit it the correct way.

Thanks for the quick feedback.