Re: [RFC bpf-next v1 3/7] bpf: Support pulling non-linear xdp data

[Amery Hung <ameryhung@xxxxxxxxx>]

On Mon, Aug 25, 2025 at 3:39 PM Jakub Kicinski <kuba@xxxxxxxxxx> wrote:
>
> On Mon, 25 Aug 2025 12:39:14 -0700 Amery Hung wrote:
> > +__bpf_kfunc int bpf_xdp_pull_data(struct xdp_md *x, u32 len, u64 flags)
> > +{
> > +     struct xdp_buff *xdp = (struct xdp_buff *)x;
> > +     struct skb_shared_info *sinfo = xdp_get_shared_info_from_buff(xdp);
> > +     void *data_end, *data_hard_end = xdp_data_hard_end(xdp);
> > +     int i, delta, buff_len, n_frags_free = 0, len_free = 0;
> > +
> > +     buff_len = xdp_get_buff_len(xdp);
> > +
> > +     if (unlikely(len > buff_len))
> > +             return -EINVAL;
> > +
> > +     if (!len)
> > +             len = xdp_get_buff_len(xdp);
> > +
> > +     data_end = xdp->data + len;
> > +     delta = data_end - xdp->data_end;
> > +
> > +     if (delta <= 0)
> > +             return 0;
> > +
> > +     if (unlikely(data_end > data_hard_end))
> > +             return -EINVAL;
>
> Is this safe against pointers wrapping on 32b systems?
>

You are right. This may be a problem.

> Maybe it's better to do:
>
>          if (unlikely(data_hard_end - xdp->data_end < delta))
>
> ?

But delta may be negative if the pointer wraps around and then the
function will still continue. How about adding data_end < xdp->data
check and reorganizing the checks like this?

        buff_len = xdp_get_buff_len(xdp);

        /* cannot pull more than the packet size */
        if (unlikely(len > buff_len))
                return -EINVAL;

        len = len ?: buff_len;

        data_end = xdp->data + len;

        /* pointer wraps around */
        if (unlikely(data_end < xdp->data))
                return -EINVAL;

        /* cannot pull without enough tailroom in the linear area */
        if (unlikely(data_end > data_hard_end))
                return -EINVAL;

        /* len bytes of data already in the linear area */
        delta = data_end - xdp->data_end;
        if (delta <= 0)
                return 0;

>
> > +     for (i = 0; i < sinfo->nr_frags && delta; i++) {
> > +             skb_frag_t *frag = &sinfo->frags[i];
> > +             u32 shrink = min_t(u32, delta, skb_frag_size(frag));
> > +
> > +             memcpy(xdp->data_end + len_free, skb_frag_address(frag), shrink);
> > +
> > +             len_free += shrink;
> > +             delta -= shrink;
> > +             if (bpf_xdp_shrink_data(xdp, frag, shrink, false))
> > +                     n_frags_free++;
>
> possibly
>
>                 else
>                         break;
>
> and then you don't have to check delta in the for loop condition?
>

I will drop the delta check and add the else branch. I will also make
the bpf_xdp_shrink_data() refactor in patch 2 consistent with this.

> > +     }
> > +
> > +     for (i = 0; i < sinfo->nr_frags - n_frags_free; i++) {
> > +             memcpy(&sinfo->frags[i], &sinfo->frags[i + n_frags_free],
> > +                    sizeof(skb_frag_t));
>
> This feels like it'd really want to be a memmove(), no?
>

Right. Thanks for the suggestion!


> > +     }
> > +
> > +     sinfo->nr_frags -= n_frags_free;
> > +     sinfo->xdp_frags_size -= len_free;
> > +     xdp->data_end = data_end;
> > +
> > +     if (unlikely(!sinfo->nr_frags))
> > +             xdp_buff_clear_frags_flag(xdp);
> > +
> > +     return 0;
> > +}