netdev - Re: [PATCH v3 bpf 2/4] xsk: fix usage of multi-buffer BPF helpers for ZC XDP

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: <7d2cd6c4-0d65-4a65-beb1-2dd995ac9b2f@linux.dev>
Date: Wed, 3 Jan 2024 14:53:20 -0800
From: Martin KaFai Lau <martin.lau@...ux.dev>
To: Maciej Fijalkowski <maciej.fijalkowski@...el.com>
Cc: netdev@...r.kernel.org, magnus.karlsson@...el.com, bjorn@...nel.org,
 echaudro@...hat.com, lorenzo@...nel.org, tirthendu.sarkar@...el.com,
 bpf@...r.kernel.org, ast@...nel.org, daniel@...earbox.net, andrii@...nel.org
Subject: Re: [PATCH v3 bpf 2/4] xsk: fix usage of multi-buffer BPF helpers for
 ZC XDP

On 1/3/24 4:04 AM, Maciej Fijalkowski wrote:
> On Tue, Jan 02, 2024 at 02:58:00PM -0800, Martin KaFai Lau wrote:
>> On 12/21/23 5:26 AM, Maciej Fijalkowski wrote:
>>> This comes from __xdp_return() call with xdp_buff argument passed as
>>> NULL which is supposed to be consumed by xsk_buff_free() call.
>>>
>>> To address this properly, in ZC case, a node that represents the frag
>>> being removed has to be pulled out of xskb_list. Introduce
>>> appriopriate xsk helpers to do such node operation and use them
>>> accordingly within bpf_xdp_adjust_tail().
>>
>> [ ... ]
>>
>>> +static inline struct xdp_buff *xsk_buff_get_tail(struct xdp_buff *first)
>>> +{
>>> +	struct xdp_buff_xsk *xskb = container_of(first, struct xdp_buff_xsk, xdp);
>>> +	struct xdp_buff_xsk *frag;
>>> +
>>> +	frag = list_last_entry(&xskb->pool->xskb_list, struct xdp_buff_xsk,
>>> +			       xskb_list_node);
>>> +	return &frag->xdp;
>>> +}
>>> +
>>
>> [ ... ]
>>
>>> +static void __shrink_data(struct xdp_buff *xdp, struct xdp_mem_info *mem_info,
>>> +			  skb_frag_t *frag, int shrink)
>>> +{
>>> +	if (mem_info->type == MEM_TYPE_XSK_BUFF_POOL) {
>>> +		struct xdp_buff *tail = xsk_buff_get_tail(xdp);
>>> +
>>> +		if (tail)
>>> +			tail->data_end -= shrink;
>>> +	}
>>> +	skb_frag_size_sub(frag, shrink);
>>> +}
>>> +
>>> +static bool shrink_data(struct xdp_buff *xdp, skb_frag_t *frag, int shrink)
>>> +{
>>> +	struct xdp_mem_info *mem_info = &xdp->rxq->mem;
>>> +
>>> +	if (skb_frag_size(frag) == shrink) {
>>> +		struct page *page = skb_frag_page(frag);
>>> +		struct xdp_buff *zc_frag = NULL;
>>> +
>>> +		if (mem_info->type == MEM_TYPE_XSK_BUFF_POOL) {
>>> +			zc_frag = xsk_buff_get_tail(xdp);
>>> +
>>> +			if (zc_frag) {
>>
>> Based on the xsk_buff_get_tail(), would zc_frag ever be NULL?
> 
> Hey Martin thanks for taking a look, I had to do this in order to satisfy
> !CONFIG_XDP_SOCKETS builds :/

There is compilation/checker warning if it does not check for NULL?

hmm... but it still should not reach here in the runtime and call 
xsk_buff_get_tail() in the !CONFIG_XDP_SOCKETS build. Can the NULL test on the 
get_tail() return value be removed? The above "mem_info->type == 
MEM_TYPE_XSK_BUFF_POOL" should have avoided the get_tail() call for the 
!CONFIG_XDP_SOCKETS build. Otherwise, it could be passing NULL to the 
__xdp_return() and hit the same bug again. The NULL check here is pretty hard to 
reason logically.

> 
>>
>>> +				xdp_buff_clear_frags_flag(zc_frag);
>>> +				xsk_buff_del_tail(zc_frag);
>>> +			}
>>> +		}
>>> +
>>> +		__xdp_return(page_address(page), mem_info, false, zc_frag);
>>
>> and iiuc, this patch is fixing a bug when zc_frag is NULL and
>> MEM_TYPE_XSK_BUFF_POOL.
> 
> Generally I don't see the need for xdp_return_buff() (which calls in the
> end __xdp_return() being discussed) to handle MEM_TYPE_XSK_BUFF_POOL, this
> could be refactored later and then probably this fix would look different,
> but this is out of the scope now.
> 
>>
>>> +		return true;
>>> +	}
>>> +	__shrink_data(xdp, mem_info, frag, shrink);
>>> +	return false;
>>> +}
>>> +
>>
>>
>