| 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: <bc297803-68e6-4f59-a32d-490398b8e590@linux.dev>
Date: Tue, 16 Sep 2025 15:59:22 -0700
From: Martin KaFai Lau <martin.lau@...ux.dev>
To: Amery Hung <ameryhung@...il.com>
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 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.
> + 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)".
> + 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