netdev - Re: [PATCH net-next v3 05/18] xdp: Use nested-BH locking for system_page

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: <20250502150705.1sewZ77B@linutronix.de>
Date: Fri, 2 May 2025 17:07:05 +0200
From: Sebastian Andrzej Siewior <bigeasy@...utronix.de>
To: Toke Høiland-Jørgensen <toke@...hat.com>
Cc: netdev@...r.kernel.org, linux-rt-devel@...ts.linux.dev,
	"David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	Simon Horman <horms@...nel.org>,
	Thomas Gleixner <tglx@...utronix.de>,
	Andrew Lunn <andrew+netdev@...n.ch>,
	Alexei Starovoitov <ast@...nel.org>,
	Daniel Borkmann <daniel@...earbox.net>,
	Jesper Dangaard Brouer <hawk@...nel.org>,
	John Fastabend <john.fastabend@...il.com>
Subject: Re: [PATCH net-next v3 05/18] xdp: Use nested-BH locking for
 system_page_pool

On 2025-05-02 16:33:10 [+0200], Toke Høiland-Jørgensen wrote:
> 
> > @@ -751,16 +751,13 @@ struct sk_buff *xdp_build_skb_from_zc(struct xdp_buff *xdp)
> >  	local_lock_nested_bh(&system_page_pool.bh_lock);
> >  	pp = this_cpu_read(system_page_pool.pool);
> >  	data = page_pool_dev_alloc_va(pp, &truesize);
> > -	if (unlikely(!data)) {
> > -		local_unlock_nested_bh(&system_page_pool.bh_lock);
> > -		return NULL;
> > -	}
> > +	if (unlikely(!data))
> > +		goto out;
> >  
> >  	skb = napi_build_skb(data, truesize);
> >  	if (unlikely(!skb)) {
> >  		page_pool_free_va(pp, data, true);
> > -		local_unlock_nested_bh(&system_page_pool.bh_lock);
> > -		return NULL;
> > +		goto out;
> >  	}
> >  
> >  	skb_mark_for_recycle(skb);
> > @@ -778,15 +775,16 @@ struct sk_buff *xdp_build_skb_from_zc(struct xdp_buff *xdp)
> >  
> >  	if (unlikely(xdp_buff_has_frags(xdp)) &&
> >  	    unlikely(!xdp_copy_frags_from_zc(skb, xdp, pp))) {
> > -		local_unlock_nested_bh(&system_page_pool.bh_lock);
> >  		napi_consume_skb(skb, true);
> > -		return NULL;
> > +		skb = NULL;
> >  	}
> > +
> > +out:
> >  	local_unlock_nested_bh(&system_page_pool.bh_lock);
> > -
> > -	xsk_buff_free(xdp);
> > -
> > -	skb->protocol = eth_type_trans(skb, rxq->dev);
> > +	if (skb) {
> > +		xsk_buff_free(xdp);
> > +		skb->protocol = eth_type_trans(skb, rxq->dev);
> > +	}
> 
> I had in mind moving the out: label (and the unlock) below the
> skb->protocol assignment, which would save the if(skb) check; any reason
> we can't call xsk_buff_free() while holding the lock?

We could do that, I wasn't entirely sure about xsk_buff_free(). It is
just larger scope but nothing else so far.

I've been staring at xsk_buff_free() and the counterparts such as
xsk_buff_alloc_batch() and I didn't really figure out what is protecting
the list. Do we rely on the fact that this is used once per-NAPI
instance within RX-NAPI and never somewhere else?

> -Toke

Sebastian