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

[Maciej Fijalkowski <maciej.fijalkowski@intel.com>]

On Sat, Jan 20, 2024 at 11:35:41AM +0000, Simon Horman wrote:
> 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.

Argh. Picked the old version of patch from Tirtha. Will resend with a
correct one.

> 
> ...