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.
...
