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.
> Fixes: f52ac7028bec ("net/mlx5e: RX, Add XDP multi-buffer support in Striding RQ")
> Signed-off-by: Amery Hung <ameryhung@xxxxxxxxx>
> ---
> .../net/ethernet/mellanox/mlx5/core/en_rx.c | 59 ++++++++++++++-----
> 1 file changed, 43 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_rx.c b/drivers/net/ethernet/mellanox/mlx5/core/en_rx.c
> index b8c609d91d11..c5173f1ccb4e 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/en_rx.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_rx.c
> @@ -1725,16 +1725,17 @@ mlx5e_skb_from_cqe_nonlinear(struct mlx5e_rq *rq, struct mlx5e_wqe_frag_info *wi
> struct mlx5_cqe64 *cqe, u32 cqe_bcnt)
> {
> struct mlx5e_rq_frag_info *frag_info = &rq->wqe.info.arr[0];
> + struct mlx5e_wqe_frag_info *pwi, *head_wi = wi;
> struct mlx5e_xdp_buff *mxbuf = &rq->mxbuf;
> - struct mlx5e_wqe_frag_info *head_wi = wi;
> u16 rx_headroom = rq->buff.headroom;
> struct mlx5e_frag_page *frag_page;
> struct skb_shared_info *sinfo;
> - u32 frag_consumed_bytes;
> + u32 frag_consumed_bytes, i;
> struct bpf_prog *prog;
> struct sk_buff *skb;
> dma_addr_t addr;
> u32 truesize;
> + u8 nr_frags;
> void *va;
>
> frag_page = wi->frag_page;
> @@ -1775,14 +1776,26 @@ mlx5e_skb_from_cqe_nonlinear(struct mlx5e_rq *rq, struct mlx5e_wqe_frag_info *wi
> prog = rcu_dereference(rq->xdp_prog);
> if (prog && mlx5e_xdp_handle(rq, prog, mxbuf)) {
> if (__test_and_clear_bit(MLX5E_RQ_FLAG_XDP_XMIT, rq->flags)) {
> - struct mlx5e_wqe_frag_info *pwi;
> + pwi = head_wi;
> + while (pwi->frag_page->netmem != sinfo->frags[0].netmem && pwi < wi)
> + pwi++;
>
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?
> + }
> skb = mlx5e_build_linear_skb(
> rq, mxbuf->xdp.data_hard_start, rq->buff.frame0_sz,
> mxbuf->xdp.data - mxbuf->xdp.data_hard_start,
> @@ -1796,12 +1809,12 @@ mlx5e_skb_from_cqe_nonlinear(struct mlx5e_rq *rq, struct mlx5e_wqe_frag_info *wi
>
> if (xdp_buff_has_frags(&mxbuf->xdp)) {
> /* sinfo->nr_frags is reset by build_skb, calculate again. */
> - xdp_update_skb_shared_info(skb, wi - head_wi - 1,
> + xdp_update_skb_shared_info(skb, nr_frags,
> sinfo->xdp_frags_size, truesize,
> xdp_buff_is_frag_pfmemalloc(
> &mxbuf->xdp));
>
> - for (struct mlx5e_wqe_frag_info *pwi = head_wi + 1; pwi < wi; pwi++)
> + for (i = 0; i < nr_frags; i++, pwi++)
> pwi->frag_page->frags++;
Why not pull the pwi assignmet to head_wi + 1 up from the for scope and use i
with i < nr_frags condition?
> }
>
> @@ -2073,12 +2086,18 @@ mlx5e_skb_from_cqe_mpwrq_nonlinear(struct mlx5e_rq *rq, struct mlx5e_mpw_info *w
> }
>
> if (prog) {
> + u8 nr_frags;
> + u32 len, i;
> +
> if (mlx5e_xdp_handle(rq, prog, mxbuf)) {
> if (__test_and_clear_bit(MLX5E_RQ_FLAG_XDP_XMIT, rq->flags)) {
> - struct mlx5e_frag_page *pfp;
> + struct mlx5e_frag_page *pagep = head_page;
> +
> + while (pagep->netmem != sinfo->frags[0].netmem && pagep < frag_page)
> + pagep++;
>
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.
> wi->linear_page.frags++;
> }
> @@ -2087,9 +2106,12 @@ mlx5e_skb_from_cqe_mpwrq_nonlinear(struct mlx5e_rq *rq, struct mlx5e_mpw_info *w
> return NULL; /* page/packet was consumed by XDP */
> }
>
> + len = mxbuf->xdp.data_end - mxbuf->xdp.data;
> + nr_frags = sinfo->nr_frags;
> +
> skb = mlx5e_build_linear_skb(
> rq, mxbuf->xdp.data_hard_start, linear_frame_sz,
> - mxbuf->xdp.data - mxbuf->xdp.data_hard_start, 0,
> + mxbuf->xdp.data - mxbuf->xdp.data_hard_start, len,
> mxbuf->xdp.data - mxbuf->xdp.data_meta);
This makes sense.
> if (unlikely(!skb)) {
> mlx5e_page_release_fragmented(rq->page_pool,
> @@ -2102,20 +2124,25 @@ mlx5e_skb_from_cqe_mpwrq_nonlinear(struct mlx5e_rq *rq, struct mlx5e_mpw_info *w
> mlx5e_page_release_fragmented(rq->page_pool, &wi->linear_page);
>
> if (xdp_buff_has_frags(&mxbuf->xdp)) {
> - struct mlx5e_frag_page *pagep;
> + struct mlx5e_frag_page *pagep = head_page;
> +
> + truesize = nr_frags * PAGE_SIZE;
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
