netdev - Re: [PATCH bpf-next v3 2/6] bpf: Support pulling non-linear xdp data

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CAMB2axODF+XGfe-yrsCCzSO1er6KKBBXCaiEHYGsLBNFZnpOHg@mail.gmail.com>
Date: Wed, 17 Sep 2025 12:37:07 -0700
From: Amery Hung <ameryhung@...il.com>
To: Jakub Kicinski <kuba@...nel.org>
Cc: bpf@...r.kernel.org, netdev@...r.kernel.org, alexei.starovoitov@...il.com, 
	andrii@...nel.org, daniel@...earbox.net, paul.chaignon@...il.com, 
	stfomichev@...il.com, martin.lau@...nel.org, mohsin.bashr@...il.com, 
	noren@...dia.com, dtatulea@...dia.com, saeedm@...dia.com, tariqt@...dia.com, 
	mbloch@...dia.com, maciej.fijalkowski@...el.com, kernel-team@...a.com
Subject: Re: [PATCH bpf-next v3 2/6] bpf: Support pulling non-linear xdp data

On Tue, Sep 16, 2025 at 5:17 PM Jakub Kicinski <kuba@...nel.org> wrote:
>
> On Mon, 15 Sep 2025 15:47:57 -0700 Amery Hung wrote:
> > +/**
> > + * bpf_xdp_pull_data() - Pull in non-linear xdp data.
> > + * @x: &xdp_md associated with the XDP buffer
> > + * @len: length of data to be made directly accessible in the linear part
> > + *
> > + * Pull in non-linear data in case the XDP buffer associated with @x is
>
> looks like there will be a v4, so nit, I'd drop the first non-linear:
>
>         Pull in data in case the XDP buffer associated with @x is
>
> we say linear too many times, makes the doc hard to read
>
> > + * non-linear and not all @len are in the linear data area.
> > + *
> > + * Direct packet access allows reading and writing linear XDP data through
> > + * packet pointers (i.e., &xdp_md->data + offsets). The amount of data which
> > + * ends up in the linear part of the xdp_buff depends on the NIC and its
> > + * configuration. When an eBPF program wants to directly access headers that
>
> s/eBPF/frag-capable XDP/ ?
>

Will change. Thanks for helping improve the comments.

> > + * may be in the non-linear area, call this kfunc to make sure the data is
> > + * available in the linear area. Alternatively, use dynptr or
> > + * bpf_xdp_{load,store}_bytes() to access data without pulling.
> > + *
> > + * This kfunc can also be used with bpf_xdp_adjust_head() to decapsulate
> > + * headers in the non-linear data area.
> > + *
> > + * A call to this kfunc may reduce headroom. If there is not enough tailroom
> > + * in the linear data area, metadata and data will be shifted down.
> > + *
> > + * A call to this kfunc is susceptible to change the buffer geometry.
> > + * Therefore, at load time, all checks on pointers previously done by the
> > + * verifier are invalidated and must be performed again, if the kfunc is used
> > + * in combination with direct packet access.
> > + *
> > + * Return:
> > + * * %0         - success
> > + * * %-EINVAL   - invalid len
> > + */
> > +__bpf_kfunc int bpf_xdp_pull_data(struct xdp_md *x, u32 len)
> > +{
> > +     struct xdp_buff *xdp = (struct xdp_buff *)x;
> > +     int i, delta, shift, headroom, tailroom, n_frags_free = 0, len_free = 0;
> > +     struct skb_shared_info *sinfo = xdp_get_shared_info_from_buff(xdp);
> > +     void *data_hard_end = xdp_data_hard_end(xdp);
> > +     int data_len = xdp->data_end - xdp->data;
> > +     void *start, *new_end = xdp->data + len;
> > +
> > +     if (len <= data_len)
> > +             return 0;
> > +
> > +     if (unlikely(len > xdp_get_buff_len(xdp)))
> > +             return -EINVAL;
> > +
> > +     start = xdp_data_meta_unsupported(xdp) ? xdp->data : xdp->data_meta;
> > +
> > +     headroom = start - xdp->data_hard_start - sizeof(struct xdp_frame);
> > +     tailroom = data_hard_end - xdp->data_end;
> > +
> > +     delta = len - data_len;
> > +     if (unlikely(delta > tailroom + headroom))
> > +             return -EINVAL;
> > +
> > +     shift = delta - tailroom;
> > +     if (shift > 0) {
> > +             memmove(start - shift, start, xdp->data_end - start);
> > +
> > +             xdp->data_meta -= shift;
> > +             xdp->data -= shift;
> > +             xdp->data_end -= shift;
> > +
> > +             new_end = data_hard_end;
> > +     }
> > +
> > +     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++;
> > +     }
> > +
> > +     if (unlikely(n_frags_free)) {
> > +             memmove(sinfo->frags, sinfo->frags + n_frags_free,
> > +                     (sinfo->nr_frags - n_frags_free) * sizeof(skb_frag_t));
> > +
> > +             sinfo->nr_frags -= n_frags_free;
> > +
> > +             if (!sinfo->nr_frags)
> > +                     xdp_buff_clear_frags_flag(xdp);
> > +     }
> > +
> > +     sinfo->xdp_frags_size -= len_free;
> > +     xdp->data_end = new_end;
>
> Not sure I see the benefit of maintaining the new_end, and len_free.
> We could directly adjust
>
>         xdp->data_end += shrink;
>         sinfo->xdp_frags_size -= shrink;
>
> as we copy from the frags. But either way:
>

Great suggestion! I will drop new_end and len_free.

> Reviewed-by: Jakub Kicinski <kuba@...nel.org>
>
> The whole things actually looks pretty clean, I was worried
> the shifting down of the data would add a lot of complexity :)