Re: [PATCH] bpf: Fix mix-up of 4096 and page size.

[Saket Kumar Bhaskar <skb99@xxxxxxxxxxxxx>]

On Tue, Jan 28, 2025 at 04:03:11PM +0100, Alexander Lobakin wrote:
> From: Alexei Starovoitov <alexei.starovoitov@xxxxxxxxx>
> Date: Thu, 23 Jan 2025 21:14:04 -0800
> 
> > On Wed, Jan 22, 2025 at 10:38 AM Saket Kumar Bhaskar
> > <skb99@xxxxxxxxxxxxx> 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.
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.

Thanks,
Saket