| 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
| ||
|
Message-ID: <CAMB2axMmHzDjpe40Xr64m1iEc=RRTJgQ+O4YQu9krqEYtskxFQ@mail.gmail.com>
Date: Wed, 17 Sep 2025 10:23:40 -0700
From: Amery Hung <ameryhung@...il.com>
To: Martin KaFai Lau <martin.lau@...ux.dev>
Cc: bpf@...r.kernel.org, netdev@...r.kernel.org, alexei.starovoitov@...il.com,
andrii@...nel.org, daniel@...earbox.net, paul.chaignon@...il.com,
kuba@...nel.org, 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 4/6] bpf: Support specifying linear xdp packet
data size for BPF_PROG_TEST_RUN
On Tue, Sep 16, 2025 at 3:59 PM Martin KaFai Lau <martin.lau@...ux.dev> wrote:
>
> On 9/15/25 3:47 PM, Amery Hung wrote:
> > To test bpf_xdp_pull_data(), an xdp packet containing fragments as well
> > as free linear data area after xdp->data_end needs to be created.
> > However, bpf_prog_test_run_xdp() always fills the linear area with
> > data_in before creating fragments, leaving no space to pull data. This
> > patch will allow users to specify the linear data size through
> > ctx->data_end.
> >
> > Currently, ctx_in->data_end must match data_size_in and will not be the
> > final ctx->data_end seen by xdp programs. This is because ctx->data_end
> > is populated according to the xdp_buff passed to test_run. The linear
> > data area available in an xdp_buff, max_data_sz, is alawys filled up
> > before copying data_in into fragments.
> >
> > This patch will allow users to specify the size of data that goes into
> > the linear area. When ctx_in->data_end is different from data_size_in,
> > only ctx_in->data_end bytes of data will be put into the linear area when
> > creating the xdp_buff.
> >
> > While ctx_in->data_end will be allowed to be different from data_size_in,
> > it cannot be larger than the data_size_in as there will be no data to
> > copy from user space. If it is larger than the maximum linear data area
> > size, the layout suggested by the user will not be honored. Data beyond
> > max_data_sz bytes will still be copied into fragments.
> >
> > Finally, since it is possible for a NIC to produce a xdp_buff with empty
> > linear data area, allow it when calling bpf_test_init() from
> > bpf_prog_test_run_xdp() so that we can test XDP kfuncs with such
> > xdp_buff.
> >
> > Signed-off-by: Amery Hung <ameryhung@...il.com>
> > ---
> > net/bpf/test_run.c | 26 ++++++++++++-------
> > .../bpf/prog_tests/xdp_context_test_run.c | 4 +--
> > 2 files changed, 17 insertions(+), 13 deletions(-)
> >
> > diff --git a/net/bpf/test_run.c b/net/bpf/test_run.c
> > index 4a862d605386..558126bbd180 100644
> > --- a/net/bpf/test_run.c
> > +++ b/net/bpf/test_run.c
> > @@ -660,12 +660,15 @@ BTF_ID_FLAGS(func, bpf_kfunc_call_memb_release, KF_RELEASE)
> > BTF_KFUNCS_END(test_sk_check_kfunc_ids)
> >
> > static void *bpf_test_init(const union bpf_attr *kattr, u32 user_size,
> > - u32 size, u32 headroom, u32 tailroom)
> > + u32 size, u32 headroom, u32 tailroom, bool is_xdp)
>
> Understood that the patch has inherited this function. I found it hard to read
> when it is called by xdp but this could be just me. For example, what is passed
> as "size" from the bpf_prog_test_run_xdp(), which ends up being "PAGE_SIZE -
> headroom - tailroom". I am not sure how to fix it. e.g. can we always allocate a
> PAGE_SIZE for non xdp callers also. or may be the xdp should not reuse this
> function. This probably is a fruit of thoughts for later. Not asking to consider
> it in this set.
>
> I think at least the first step is to avoid adding "is_xdp" specific logic here.
>
> > {
> > void __user *data_in = u64_to_user_ptr(kattr->test.data_in);
> > void *data;
> >
> > - if (user_size < ETH_HLEN || user_size > PAGE_SIZE - headroom - tailroom)
> > + if (!is_xdp && user_size < ETH_HLEN)
>
> Move the lower bound check to its caller. test_run_xdp() does not need this
> check. test_run_flow_dissector() and test_run_nf() already have its own check.
> test_run_nf() actually has a different bound. test_run_skb() is the only one
> that needs this check, so it can be explicitly done in there like other callers.
>
Yeah, is _xdp is bad. I will move lower bound checks to callers.
Thanks for pointing this out.
> > + return ERR_PTR(-EINVAL);
> > +
> > + if (user_size > PAGE_SIZE - headroom - tailroom)
> > return ERR_PTR(-EINVAL);
> >
> > size = SKB_DATA_ALIGN(size);
> > @@ -1003,7 +1006,8 @@ int bpf_prog_test_run_skb(struct bpf_prog *prog, const union bpf_attr *kattr,
> >
> > data = bpf_test_init(kattr, kattr->test.data_size_in,
> > size, NET_SKB_PAD + NET_IP_ALIGN,
> > - SKB_DATA_ALIGN(sizeof(struct skb_shared_info)));
> > + SKB_DATA_ALIGN(sizeof(struct skb_shared_info)),
> > + false);
> > if (IS_ERR(data))
> > return PTR_ERR(data);
> >
> > @@ -1207,8 +1211,8 @@ int bpf_prog_test_run_xdp(struct bpf_prog *prog, const union bpf_attr *kattr,
> > {
> > bool do_live = (kattr->test.flags & BPF_F_TEST_XDP_LIVE_FRAMES);
> > u32 tailroom = SKB_DATA_ALIGN(sizeof(struct skb_shared_info));
> > + u32 retval = 0, duration, max_data_sz, data_sz;
> > u32 batch_size = kattr->test.batch_size;
> > - u32 retval = 0, duration, max_data_sz;
> > u32 size = kattr->test.data_size_in;
> > u32 headroom = XDP_PACKET_HEADROOM;
> > u32 repeat = kattr->test.repeat;
> > @@ -1246,7 +1250,7 @@ int bpf_prog_test_run_xdp(struct bpf_prog *prog, const union bpf_attr *kattr,
> >
> > if (ctx) {
> > /* There can't be user provided data before the meta data */
> > - if (ctx->data_meta || ctx->data_end != size ||
> > + if (ctx->data_meta || ctx->data_end > size ||
> > ctx->data > ctx->data_end ||
> > unlikely(xdp_metalen_invalid(ctx->data)) ||
> > (do_live && (kattr->test.data_out || kattr->test.ctx_out)))
> > @@ -1256,14 +1260,15 @@ int bpf_prog_test_run_xdp(struct bpf_prog *prog, const union bpf_attr *kattr,
> > }
> >
> > max_data_sz = PAGE_SIZE - headroom - tailroom;
> > - if (size > max_data_sz) {
> > + data_sz = (ctx && ctx->data_end < max_data_sz) ? ctx->data_end : max_data_sz;
>
> hmm... can the "size" (not data_sz) be directly updated to ctx->data_end in the
> above "if (ctx)".
>
That simplifies things a lot. Will change in the next version.
> > + if (size > data_sz) {
> > /* disallow live data mode for jumbo frames */
> > if (do_live)
> > goto free_ctx;
> > - size = max_data_sz;
> > + size = data_sz;
> > }
> >
> > - data = bpf_test_init(kattr, size, max_data_sz, headroom, tailroom);
> > + data = bpf_test_init(kattr, size, max_data_sz, headroom, tailroom, true);
> > if (IS_ERR(data)) {
> > ret = PTR_ERR(data);
> > goto free_ctx;
> > @@ -1386,7 +1391,7 @@ int bpf_prog_test_run_flow_dissector(struct bpf_prog *prog,
> > if (size < ETH_HLEN)
> > return -EINVAL;
> >
> > - data = bpf_test_init(kattr, kattr->test.data_size_in, size, 0, 0);
> > + data = bpf_test_init(kattr, kattr->test.data_size_in, size, 0, 0, false);
> > if (IS_ERR(data))
> > return PTR_ERR(data);
> >
> > @@ -1659,7 +1664,8 @@ int bpf_prog_test_run_nf(struct bpf_prog *prog,
> >
> > data = bpf_test_init(kattr, kattr->test.data_size_in, size,
> > NET_SKB_PAD + NET_IP_ALIGN,
> > - SKB_DATA_ALIGN(sizeof(struct skb_shared_info)));
> > + SKB_DATA_ALIGN(sizeof(struct skb_shared_info)),
> > + false);
> > if (IS_ERR(data))
> > return PTR_ERR(data);
> >
> > diff --git a/tools/testing/selftests/bpf/prog_tests/xdp_context_test_run.c b/tools/testing/selftests/bpf/prog_tests/xdp_context_test_run.c
> > index 46e0730174ed..178292d1251a 100644
> > --- a/tools/testing/selftests/bpf/prog_tests/xdp_context_test_run.c
> > +++ b/tools/testing/selftests/bpf/prog_tests/xdp_context_test_run.c
> > @@ -97,9 +97,7 @@ void test_xdp_context_test_run(void)
> > /* Meta data must be 255 bytes or smaller */
> > test_xdp_context_error(prog_fd, opts, 0, 256, sizeof(data), 0, 0, 0);
> >
> > - /* Total size of data must match data_end - data_meta */
> > - test_xdp_context_error(prog_fd, opts, 0, sizeof(__u32),
> > - sizeof(data) - 1, 0, 0, 0);
> > + /* Total size of data must be data_end - data_meta or larger */
> > test_xdp_context_error(prog_fd, opts, 0, sizeof(__u32),
> > sizeof(data) + 1, 0, 0, 0);
> >
>
Powered by blists - more mailing lists