On Mon, Aug 25, 2025 at 12:39:12PM -0700, Amery Hung wrote:

xdp programs can change the layout of an xdp_buff through
bpf_xdp_adjust_tail(), bpf_xdp_adjust_head(). Therefore, the driver
cannot assume the size of the linear data area nor fragments. Fix the
bug in mlx5e driver by generating skb according to xdp_buff layout.

Good find! Thanks for tackling this Amery.

Currently, when handling multi-buf xdp, the mlx5e driver assumes the
layout of an xdp_buff to be unchanged. That is, the linear data area
continues to be empty and the fragments remains the same.

This is true only for striding rq xdp. Legacy rq xdp puts the header
in the linear part.

This may
cause the driver to generate erroneous skb or triggering a kernel
warning. When an xdp program added linear data through
bpf_xdp_adjust_head() the linear data will be ignored as
mlx5e_build_linear_skb() builds an skb with empty linear data and then
pull data from fragments to fill the linear data area. When an xdp
program has shrunk the nonlinear data through bpf_xdp_adjust_tail(),
the delta passed to __pskb_pull_tail() may exceed the actual nonlinear
data size and trigger the BUG_ON in it.

To fix the issue, first build the skb with linear data area matching
the xdp_buff. Then, call __pskb_pull_tail() to fill the linear data for
up to MLX5E_RX_MAX_HEAD bytes. In addition, recalculate nr_frags and
truesize after xdp program runs.

The ordering here seems misleading. AFAIU recalculating nr_frags happens
first.

Is this trying to skip counting the frags for the linear part? If yes,
don't understand the reasoning. If not, I don't follow the code.

AFAIU frags have to be counted for the linear part + sinfo->nr_frags.
Frags could be less after xdp program execution, but the linear part is
still there.

-			for (pwi = head_wi; pwi < wi; pwi++)
+			for (i = 0; i < sinfo->nr_frags; i++, pwi++)
 				pwi->frag_page->frags++;

Why not:

	pwi = head_wi;
	for (int i = 0; i < (sinfo->nr_frags + 1); i++, pwi++)
		pwi->frag_page->frags++;

 		}
 		return NULL; /* page/packet was consumed by XDP */
 	}
 
+	nr_frags = sinfo->nr_frags;

This makes sense. You are using this in xdp_update_skb_shared_info()
below.

+	pwi = head_wi + 1;
+
+	if (prog) {

You could do here: if (unlikely(sinfo->nr_frags != nr_frags).

+		truesize = sinfo->nr_frags * frag_info->frag_stride;
+

Ack. Recalculating truesize.

+		while (pwi->frag_page->netmem != sinfo->frags[0].netmem && pwi < wi)
+			pwi++;

Why is this needed here?

+	}

Why not pull the pwi assignmet to head_wi + 1 up from the for scope and use i
with i < nr_frags condition?

Why do you need this?

-				for (pfp = head_page; pfp < frag_page; pfp++)
-					pfp->frags++;
+				for (i = 0; i < sinfo->nr_frags; i++)
+					pagep->frags++;

This looks good here but with pfp = head_page. head_page should point to the first
frag. The linear part is in wi->linear_page.

This makes sense.

I am not sure that this is accurate. The last fragment might be smaller
than page size. It should be aligned to BIT(rq->mpwqe.log_stride_sz).

 
 			/* sinfo->nr_frags is reset by build_skb, calculate again. */
-			xdp_update_skb_shared_info(skb, frag_page - head_page,
+			xdp_update_skb_shared_info(skb, nr_frags,
 						   sinfo->xdp_frags_size, truesize,
 						   xdp_buff_is_frag_pfmemalloc(
 							&mxbuf->xdp));
 
-			pagep = head_page;
-			do
+			while (pagep->netmem != sinfo->frags[0].netmem && pagep < frag_page)
+				pagep++;
+
+			for (i = 0; i < nr_frags; i++, pagep++)
 				pagep->frags++;
-			while (++pagep < frag_page);
+
+			headlen = min_t(u16, MLX5E_RX_MAX_HEAD - len, sinfo->xdp_frags_size);
+			__pskb_pull_tail(skb, headlen);
 		}
-		__pskb_pull_tail(skb, headlen);

What happens when there are no more frags? (bpf_xdp_frags_shrink_tail()
shrinked them out). Is that at all possible?

In general, I think the code would be nicer if it would do a rewind of
the end pointer based on the diff between the old and new nr_frags.

Thanks,
Dragos