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

Message-ID: <87frhn57i3.fsf@toke.dk>
Date: Fri, 02 May 2025 17:59:00 +0200
From: Toke Høiland-Jørgensen <toke@...hat.com>
To: Sebastian Andrzej Siewior <bigeasy@...utronix.de>
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

Sebastian Andrzej Siewior <bigeasy@...utronix.de> writes:

> 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?

Yeah, I believe so. The commit adding the API[0] mentions this being
"single core (single producer/consumer)".

-Toke

[0] 2b43470add8c ("xsk: Introduce AF_XDP buffer allocation API")