netdev - Re: [PATCH v4 bpf 05/11] i40e: handle multi-buffer packets that are shrunk by xdp prog

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: <20240120113541.GA110624@kernel.org>
Date: Sat, 20 Jan 2024 11:35:41 +0000
From: Simon Horman <horms@...nel.org>
To: Maciej Fijalkowski <maciej.fijalkowski@...el.com>
Cc: bpf@...r.kernel.org, ast@...nel.org, daniel@...earbox.net,
	andrii@...nel.org, netdev@...r.kernel.org,
	magnus.karlsson@...el.com, bjorn@...nel.org, echaudro@...hat.com,
	lorenzo@...nel.org, martin.lau@...ux.dev,
	tirthendu.sarkar@...el.com, john.fastabend@...il.com
Subject: Re: [PATCH v4 bpf 05/11] i40e: handle multi-buffer packets that are
 shrunk by xdp prog

On Sat, Jan 20, 2024 at 12:30:31AM +0100, Maciej Fijalkowski wrote:
> From: Tirthendu Sarkar <tirthendu.sarkar@...el.com>
> 
> XDP programs can shrink packets by calling the bpf_xdp_adjust_tail()
> helper function. For multi-buffer packets this may lead to reduction of
> frag count stored in skb_shared_info area of the xdp_buff struct. This
> results in issues with the current handling of XDP_PASS and XDP_DROP
> cases.
> 
> For XDP_PASS, currently skb is being built using frag count of
> xdp_buffer before it was processed by XDP prog and thus will result in
> an inconsistent skb when frag count gets reduced by XDP prog. To fix
> this, get correct frag count while building the skb instead of using
> pre-obtained frag count.
> 
> For XDP_DROP, current page recycling logic will not reuse the page but
> instead will adjust the pagecnt_bias so that the page can be freed. This
> again results in inconsistent behavior as the page count has already
> been changed by the helper while freeing the frag(s) as part of
> shrinking the packet. To fix this, only adjust pagecnt_bias for buffers
> that are stillpart of the packet post-xdp prog run.
> 
> Fixes: e213ced19bef ("i40e: add support for XDP multi-buffer Rx")
> Reported-by: Maciej Fijalkowski <maciej.fijalkowski@...el.com>
> Tested-by: Maciej Fijalkowski <maciej.fijalkowski@...el.com>
> Reviewed-by: Maciej Fijalkowski <maciej.fijalkowski@...el.com>
> Signed-off-by: Tirthendu Sarkar <tirthendu.sarkar@...el.com>

...

> @@ -2129,20 +2130,20 @@ static void i40e_process_rx_buffs(struct i40e_ring *rx_ring, int xdp_res,
>   * i40e_construct_skb - Allocate skb and populate it
>   * @rx_ring: rx descriptor ring to transact packets on
>   * @xdp: xdp_buff pointing to the data
> - * @nr_frags: number of buffers for the packet
>   *
>   * This function allocates an skb.  It then populates it with the page
>   * data from the current receive descriptor, taking care to set up the
>   * skb correctly.
>   */
>  static struct sk_buff *i40e_construct_skb(struct i40e_ring *rx_ring,
> -					  struct xdp_buff *xdp,
> -					  u32 nr_frags)
> +					  struct xdp_buff *xdp)
>  {
>  	unsigned int size = xdp->data_end - xdp->data;
>  	struct i40e_rx_buffer *rx_buffer;
> +	struct skb_shared_info *sinfo;
>  	unsigned int headlen;
>  	struct sk_buff *skb;
> +	u32 nr_frags;
>  
>  	/* prefetch first cache line of first page */
>  	net_prefetch(xdp->data);
> @@ -2180,6 +2181,10 @@ static struct sk_buff *i40e_construct_skb(struct i40e_ring *rx_ring,
>  	memcpy(__skb_put(skb, headlen), xdp->data,
>  	       ALIGN(headlen, sizeof(long)));
>  
> +	if (unlikely(xdp_buff_has_frags(xdp))) {
> +		sinfo = xdp_get_shared_info_from_buff(xdp);
> +		nr_frags = sinfo->nr_frags;
> +	}
>  	rx_buffer = i40e_rx_bi(rx_ring, rx_ring->next_to_clean);
>  	/* update all of the pointers */
>  	size -= headlen;

Hi Maciej,

Above, nr_frags is initialised only if xdp_buff_has_frags(xdp) is true.
The code immediately following this hunk is:

	if (size) {
		if (unlikely(nr_frags >= MAX_SKB_FRAGS)) {
			...

Can it be the case that nr_frags is used uninitialised here?

Flagged by Smatch.

...