From: Saket Kumar Bhaskar skb99@linux.ibm.com Date: Tue, 4 Feb 2025 12:27:52 +0530

On Tue, Jan 28, 2025 at 04:03:11PM +0100, Alexander Lobakin wrote:

...

From: Alexei Starovoitov alexei.starovoitov@gmail.com Date: Thu, 23 Jan 2025 21:14:04 -0800

...

On Wed, Jan 22, 2025 at 10:38 AM Saket Kumar Bhaskar skb99@linux.ibm.com wrote:

...

For platforms on powerpc architecture with a default page size greater than 4096, there was an inconsistency in fragment size calculation. This caused the BPF selftest xdp_adjust_tail/xdp_adjust_frags_tail_grow to fail on powerpc.

The issue occurred because the fragment buffer size in bpf_prog_test_run_xdp() was set to 4096, while the actual data size in the fragment within the shared skb was checked against PAGE_SIZE (65536 on powerpc) in min_t, causing it to exceed 4096 and be set accordingly. This discrepancy led to an overflow when bpf_xdp_frags_increase_tail() checked for tailroom, as skb_frag_size(frag) could be greater than rxq->frag_size (when PAGE_SIZE > 4096).

This commit updates the page size references to 4096 to ensure consistency and prevent overflow issues in fragment size calculations.

This isn't right. Please fix the selftest instead.

It's not _that_ easy, I had tried in the past. Anyway, this patch is *not* a good "solution".

If you (Saket) really want to fix this, both test_run and the selftest must be in sync, so you need to (both are arch-dependent): 1) get the correct PAGE_SIZE; 2) calculate the correct tailroom in userspace (which depends on sizeof(shinfo) and SKB_DATA_ALIGN -> SMP_CACHE_BYTES).

...

pw-bot: cr

Thanks, Olek

There is a mixup in kernel b/w 4096 and PAGE_SIZE and all selftest seem to be based on 4096 as the size, so I changed the PAGE_SIZE to 4096, but if we have to use PAGE_SIZE we need this change in kernel.

I know how it is done, I was working on adjacent code, that's why I spoke up and told you what you need to account if you want to fix this properly.

xdp->frame_sz is hard buffer len, usually in range [PAGE_SIZE / 2, PAGE_SIZE], and it includes:

headroom (XDP_PACKET_HEADROOM + some drivers reserve NET_IP_ALIGN) actual data buffer tailroom (SKB_DATA_ALIGN(sizeof(skb_shared_info)))

So to determine the actual data buffer size, you need to:

* know PAGE_SIZE * know headroom * know tailroom

Hardcoding anything from the list will lead to selftest fails.

In place of PAGE_SIZE 4096 was used here:

diff --git a/net/bpf/test_run.c b/net/bpf/test_run.c index 501ec4249..6b7fddfbb 100644 --- a/net/bpf/test_run.c +++ b/net/bpf/test_run.c @@ -1251,7 +1251,7 @@ int bpf_prog_test_run_xdp(struct bpf_prog *prog, const union bpf_attr *kattr, headroom -= ctx->data; }

•   max_data_sz = 4096 - headroom - tailroom;
  

•   max_data_sz = PAGE_SIZE - headroom - tailroom;
    if (size > max_data_sz) {
            /* disallow live data mode for jumbo frames */
            if (do_live)
  
  

Assuming that change in kernel we should also update the selftest to 64K page size for ppc64:

diff --git a/tools/testing/selftests/bpf/prog_tests/xdp_adjust_tail.c b/tools/testing/selftests/bpf/prog_tests/xdp_adjust_tail.c index 53d6ad8c2..037142e21 100644 --- a/tools/testing/selftests/bpf/prog_tests/xdp_adjust_tail.c +++ b/tools/testing/selftests/bpf/prog_tests/xdp_adjust_tail.c @@ -226,7 +226,7 @@ static void test_xdp_adjust_frags_tail_grow(void)

    prog_fd = bpf_program__fd(prog);

•   buf = malloc(16384);
  

•   buf = malloc(262144);
    if (!ASSERT_OK_PTR(buf, "alloc buf 16Kb"))
            goto out;
  
  

@@ -254,12 +254,12 @@ static void test_xdp_adjust_frags_tail_grow(void) ASSERT_EQ(buf[i], 1, "9Kb+10b-untouched");

    /* Test a too large grow */

•   memset(buf, 1, 16384);
  

•   exp_size = 9001;
  

•   memset(buf, 1, 262144);
  

•   exp_size = 132001;
  
    topts.data_in = topts.data_out = buf;
  

•   topts.data_size_in = 9001;
  

•   topts.data_size_out = 16384;
  

•   topts.data_size_in = 132001;
  

•   topts.data_size_out = 262144;
    err = bpf_prog_test_run_opts(prog_fd, &topts);
  
    ASSERT_OK(err, "9Kb+10b");
  
  

diff --git a/tools/testing/selftests/bpf/progs/test_xdp_adjust_tail_grow.c b/tools/testing/selftests/bpf/progs/test_xdp_adjust_tail_grow.c index 81bb38d72..40a0c5469 100644 --- a/tools/testing/selftests/bpf/progs/test_xdp_adjust_tail_grow.c +++ b/tools/testing/selftests/bpf/progs/test_xdp_adjust_tail_grow.c @@ -27,8 +27,8 @@ int _xdp_adjust_tail_grow(struct xdp_md *xdp) offset = 4096 - 256 - tailroom - data_len; } else if (data_len == 9000) { offset = 10;

•   } else if (data_len == 9001) {
  

•           offset = 4096;
  

•   } else if (data_len == 132001) {
  

•           offset = 65536;
    } else {
            return XDP_ABORTED; /* No matching test */
    }
  
  

The above change is intended for feedback. The date_len and other values in the test cases can be adjusted to be based on the page size, rather than being hard-coded, to ensure compatibility with different page sizes.

In the code above I only see one hardcode replaced with another one. Note that PAGE_SIZE == 4096 was hardcoded to be able to run selftests on x86_64 in the first place. If you want to enable them on non-fixed-page-size arches, then I mentioned 2 times already what you need to do.

Thanks, Saket

Thanks, Olek