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@xxxxxxxxx>
> >
> > 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@xxxxxxxxx>
> > Tested-by: Maciej Fijalkowski <maciej.fijalkowski@xxxxxxxxx>
> > Reviewed-by: Maciej Fijalkowski <maciej.fijalkowski@xxxxxxxxx>
> > Signed-off-by: Tirthendu Sarkar <tirthendu.sarkar@xxxxxxxxx>
>
> ...
>
> > @@ -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.
>
> ...
