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

[Simon Horman <horms@kernel.org>]

On Sat, Jan 20, 2024 at 12:30:31AM +0100, Maciej Fijalkowski wrote:
> From: Tirthendu Sarkar <tirthendu.sarkar@intel.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@intel.com>
> Tested-by: Maciej Fijalkowski <maciej.fijalkowski@intel.com>
> Reviewed-by: Maciej Fijalkowski <maciej.fijalkowski@intel.com>
> Signed-off-by: Tirthendu Sarkar <tirthendu.sarkar@intel.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.

...