diff mbox series

[bpf-next,01/15] selftests/bpf: Fix the u64_offset_to_skb_data test

Message ID 20231220214013.3327288-2-maxtram95@gmail.com (mailing list archive)
State New, archived
Delegated to: BPF
Headers show
Series Improvements for tracking scalars in the BPF verifier | expand

Checks

Context Check Description
netdev/series_format success Posting correctly formatted
netdev/tree_selection success Clearly marked for bpf-next
netdev/ynl success SINGLE THREAD; Generated files up to date; no warnings/errors; no diff in generated;
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 8 this patch: 8
netdev/cc_maintainers success CCed 17 of 17 maintainers
netdev/build_clang success Errors and warnings before: 8 this patch: 8
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 8 this patch: 8
netdev/checkpatch fail ERROR: "(foo*)" should be "(foo *)" WARNING: line length of 96 exceeds 80 columns
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0
bpf/vmtest-bpf-next-VM_Test-5 success Logs for aarch64-gcc / build-release
bpf/vmtest-bpf-next-VM_Test-4 success Logs for aarch64-gcc / build / build for aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-10 success Logs for aarch64-gcc / veristat
bpf/vmtest-bpf-next-VM_Test-12 success Logs for s390x-gcc / build-release
bpf/vmtest-bpf-next-VM_Test-6 success Logs for aarch64-gcc / test (test_maps, false, 360) / test_maps on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-7 fail Logs for aarch64-gcc / test (test_progs, false, 360) / test_progs on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-8 fail Logs for aarch64-gcc / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-9 success Logs for aarch64-gcc / test (test_verifier, false, 360) / test_verifier on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-11 success Logs for s390x-gcc / build / build for s390x with gcc
bpf/vmtest-bpf-next-VM_Test-17 success Logs for s390x-gcc / veristat
bpf/vmtest-bpf-next-VM_Test-18 success Logs for set-matrix
bpf/vmtest-bpf-next-VM_Test-20 success Logs for x86_64-gcc / build-release
bpf/vmtest-bpf-next-VM_Test-19 success Logs for x86_64-gcc / build / build for x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-28 success Logs for x86_64-llvm-17 / build / build for x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-34 success Logs for x86_64-llvm-17 / veristat
bpf/vmtest-bpf-next-VM_Test-35 success Logs for x86_64-llvm-18 / build / build for x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-42 success Logs for x86_64-llvm-18 / veristat
bpf/vmtest-bpf-next-VM_Test-36 success Logs for x86_64-llvm-18 / build-release / build for x86_64 with llvm-18 and -O2 optimization
bpf/vmtest-bpf-next-VM_Test-29 success Logs for x86_64-llvm-17 / build-release / build for x86_64 with llvm-17 and -O2 optimization
bpf/vmtest-bpf-next-VM_Test-22 fail Logs for x86_64-gcc / test (test_progs, false, 360) / test_progs on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-23 fail Logs for x86_64-gcc / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-24 success Logs for x86_64-gcc / test (test_progs_no_alu32_parallel, true, 30) / test_progs_no_alu32_parallel on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-25 success Logs for x86_64-gcc / test (test_progs_parallel, true, 30) / test_progs_parallel on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-26 success Logs for x86_64-gcc / test (test_verifier, false, 360) / test_verifier on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-27 success Logs for x86_64-gcc / veristat / veristat on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-41 success Logs for x86_64-llvm-18 / test (test_verifier, false, 360) / test_verifier on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-21 success Logs for x86_64-gcc / test (test_maps, false, 360) / test_maps on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-30 success Logs for x86_64-llvm-17 / test (test_maps, false, 360) / test_maps on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-31 fail Logs for x86_64-llvm-17 / test (test_progs, false, 360) / test_progs on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-32 fail Logs for x86_64-llvm-17 / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-33 success Logs for x86_64-llvm-17 / test (test_verifier, false, 360) / test_verifier on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-37 success Logs for x86_64-llvm-18 / test (test_maps, false, 360) / test_maps on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-38 fail Logs for x86_64-llvm-18 / test (test_progs, false, 360) / test_progs on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-39 fail Logs for x86_64-llvm-18 / test (test_progs_cpuv4, false, 360) / test_progs_cpuv4 on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-40 fail Logs for x86_64-llvm-18 / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-14 fail Logs for s390x-gcc / test (test_progs, false, 360) / test_progs on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-16 success Logs for s390x-gcc / test (test_verifier, false, 360) / test_verifier on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-15 fail Logs for s390x-gcc / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on s390x with gcc
bpf/vmtest-bpf-next-PR fail PR summary
bpf/vmtest-bpf-next-VM_Test-13 success Logs for s390x-gcc / test (test_maps, false, 360) / test_maps on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-0 success Logs for Lint
bpf/vmtest-bpf-next-VM_Test-1 success Logs for ShellCheck
bpf/vmtest-bpf-next-VM_Test-2 success Logs for Unittests
bpf/vmtest-bpf-next-VM_Test-3 success Logs for Validate matrix.py

Commit Message

Maxim Mikityanskiy Dec. 20, 2023, 9:39 p.m. UTC 
From: Maxim Mikityanskiy <maxim@isovalent.com>

The u64_offset_to_skb_data test is supposed to make a 64-bit fill, but
instead makes a 16-bit one. Fix the test according to its intention. The
16-bit fill is covered by u16_offset_to_skb_data.

Signed-off-by: Maxim Mikityanskiy <maxim@isovalent.com>
---
 tools/testing/selftests/bpf/progs/verifier_spill_fill.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

Shung-Hsi Yu Dec. 26, 2023, 9:52 a.m. UTC | #1 
On Wed, Dec 20, 2023 at 11:39:59PM +0200, Maxim Mikityanskiy wrote:
> From: Maxim Mikityanskiy <maxim@isovalent.com>
> 
> The u64_offset_to_skb_data test is supposed to make a 64-bit fill, but
> instead makes a 16-bit one. Fix the test according to its intention. The
> 16-bit fill is covered by u16_offset_to_skb_data.

Cover letter mentioned

  Patch 1 (Maxim): Fix for an existing test, it will matter later in the
  series.

However no subsequent patch touch upon u64_offset_to_skb_data(). Was the
followup missing from this series?

> Signed-off-by: Maxim Mikityanskiy <maxim@isovalent.com>
> [...]
>  SEC("tc")
>  __description("Spill u32 const scalars.  Refill as u64.  Offset to skb->data")
> -__failure __msg("invalid access to packet")
> +__failure __msg("math between pkt pointer and register with unbounded min value is not allowed")
>  __naked void u64_offset_to_skb_data(void)
>  {
>  	asm volatile ("					\
> @@ -253,7 +253,7 @@ __naked void u64_offset_to_skb_data(void)
>  	w7 = 20;					\
>  	*(u32*)(r10 - 4) = r6;				\
>  	*(u32*)(r10 - 8) = r7;				\
> -	r4 = *(u16*)(r10 - 8);				\
> +	r4 = *(u64*)(r10 - 8);				\
>  	r0 = r2;					\
>  	/* r0 += r4 R0=pkt R2=pkt R3=pkt_end R4=umax=65535 */\
>  	r0 += r4;					\
Maxim Mikityanskiy Dec. 26, 2023, 10:38 a.m. UTC | #2 
On Tue, 26 Dec 2023 at 17:52:56 +0800, Shung-Hsi Yu wrote:
> On Wed, Dec 20, 2023 at 11:39:59PM +0200, Maxim Mikityanskiy wrote:
> > From: Maxim Mikityanskiy <maxim@isovalent.com>
> > 
> > The u64_offset_to_skb_data test is supposed to make a 64-bit fill, but
> > instead makes a 16-bit one. Fix the test according to its intention. The
> > 16-bit fill is covered by u16_offset_to_skb_data.
> 
> Cover letter mentioned
> 
>   Patch 1 (Maxim): Fix for an existing test, it will matter later in the
>   series.
> 
> However no subsequent patch touch upon u64_offset_to_skb_data(). Was the
> followup missing from this series?

Thanks for your vigilance, but it's actually correct, sorry for not
making it clear enough. In patch 11 ("bpf: Preserve boundaries and track
scalars on narrowing fill") I modify u16_offset_to_skb_data, because it
becomes a valid pattern after that change. If I didn't change and fix
u64_offset_to_skb_data here, I'd need to modify it in patch 11 as well
(that's what I meant when I said "it will matter later in the series",
it's indeed subtle and implicit, now that I look at it), because it
would also start passing, however, that's not what we want, because:

1. Both tests would essentially test the same thing: a 16-bit fill after
a 32-bit spill.

2. The description of u64_offset_to_skb_data clearly says: "Refill as
u64". It's a typo in the code, u16->u64 makes sense, because we spill
two u32s and fill them as a single u64.

So, this patch essentially prevents wrong changes in a further patch.

> > Signed-off-by: Maxim Mikityanskiy <maxim@isovalent.com>
> > [...]
> >  SEC("tc")
> >  __description("Spill u32 const scalars.  Refill as u64.  Offset to skb->data")
> > -__failure __msg("invalid access to packet")
> > +__failure __msg("math between pkt pointer and register with unbounded min value is not allowed")
> >  __naked void u64_offset_to_skb_data(void)
> >  {
> >  	asm volatile ("					\
> > @@ -253,7 +253,7 @@ __naked void u64_offset_to_skb_data(void)
> >  	w7 = 20;					\
> >  	*(u32*)(r10 - 4) = r6;				\
> >  	*(u32*)(r10 - 8) = r7;				\
> > -	r4 = *(u16*)(r10 - 8);				\
> > +	r4 = *(u64*)(r10 - 8);				\
> >  	r0 = r2;					\
> >  	/* r0 += r4 R0=pkt R2=pkt R3=pkt_end R4=umax=65535 */\
> >  	r0 += r4;					\
Shung-Hsi Yu Dec. 26, 2023, 1:22 p.m. UTC | #3 
On Tue, Dec 26, 2023 at 12:38:06PM +0200, Maxim Mikityanskiy wrote:
> On Tue, 26 Dec 2023 at 17:52:56 +0800, Shung-Hsi Yu wrote:
> > On Wed, Dec 20, 2023 at 11:39:59PM +0200, Maxim Mikityanskiy wrote:
> > > From: Maxim Mikityanskiy <maxim@isovalent.com>
> > > 
> > > The u64_offset_to_skb_data test is supposed to make a 64-bit fill, but
> > > instead makes a 16-bit one. Fix the test according to its intention. The
> > > 16-bit fill is covered by u16_offset_to_skb_data.
> > 
> > Cover letter mentioned
> > 
> >   Patch 1 (Maxim): Fix for an existing test, it will matter later in the
> >   series.
> > 
> > However no subsequent patch touch upon u64_offset_to_skb_data(). Was the
> > followup missing from this series?
> 
> Thanks for your vigilance, but it's actually correct, sorry for not
> making it clear enough. In patch 11 ("bpf: Preserve boundaries and track
> scalars on narrowing fill") I modify u16_offset_to_skb_data, because it
> becomes a valid pattern after that change. If I didn't change and fix
> u64_offset_to_skb_data here, I'd need to modify it in patch 11 as well
> (that's what I meant when I said "it will matter later in the series",
> it's indeed subtle and implicit, now that I look at it), because it
> would also start passing, however, that's not what we want, because:
> 
> 1. Both tests would essentially test the same thing: a 16-bit fill after
> a 32-bit spill.
> 
> 2. The description of u64_offset_to_skb_data clearly says: "Refill as
> u64". It's a typo in the code, u16->u64 makes sense, because we spill
> two u32s and fill them as a single u64.
> 
> So, this patch essentially prevents wrong changes in a further patch.

Thank for the thorough explanation. Now I can see and agree that the
u16->u64 change should be made. Digging back a big, the change also
aligns with what's said in commit 0be2516f865f5 ("selftests/bpf: Tests
for state pruning with u32 spill/fill") that introduced the check:

  ... checks that a filled u64 register is marked unknown if the
  register spilled in the same slack slot was less than 8B.

Side note: the r4 value in comment is still "R4=umax=65535", that
probably should be updated as well now that r4 is unbounded.

> [...]
> > > -	r4 = *(u16*)(r10 - 8);				\
> > > +	r4 = *(u64*)(r10 - 8);				\
> > >  	r0 = r2;					\
> > >  	/* r0 += r4 R0=pkt R2=pkt R3=pkt_end R4=umax=65535 */\
> > >  	r0 += r4;					\
Eduard Zingerman Jan. 4, 2024, 2:22 a.m. UTC | #4 
On Wed, 2023-12-20 at 23:39 +0200, Maxim Mikityanskiy wrote:
> > From: Maxim Mikityanskiy <maxim@isovalent.com>
> > 
> > The u64_offset_to_skb_data test is supposed to make a 64-bit fill, but
> > instead makes a 16-bit one. Fix the test according to its intention. The
> > 16-bit fill is covered by u16_offset_to_skb_data.
> > 
> > Signed-off-by: Maxim Mikityanskiy <maxim@isovalent.com>
> > ---

Acked-by: Eduard Zingerman <eddyz87@gmail.com>

(Sorry, I messed up batch-sending acks first time,
 accidental HTML emails had been rejected by mailing lists.
 Resending with reduced To/CC to avoid too much spam)
diff mbox series

Patch

diff --git a/tools/testing/selftests/bpf/progs/verifier_spill_fill.c b/tools/testing/selftests/bpf/progs/verifier_spill_fill.c
index 39fe3372e0e0..84eccab36582 100644
--- a/tools/testing/selftests/bpf/progs/verifier_spill_fill.c
+++ b/tools/testing/selftests/bpf/progs/verifier_spill_fill.c
@@ -243,7 +243,7 @@  l0_%=:	r0 = 0;						\
 
 SEC("tc")
 __description("Spill u32 const scalars.  Refill as u64.  Offset to skb->data")
-__failure __msg("invalid access to packet")
+__failure __msg("math between pkt pointer and register with unbounded min value is not allowed")
 __naked void u64_offset_to_skb_data(void)
 {
 	asm volatile ("					\
@@ -253,7 +253,7 @@  __naked void u64_offset_to_skb_data(void)
 	w7 = 20;					\
 	*(u32*)(r10 - 4) = r6;				\
 	*(u32*)(r10 - 8) = r7;				\
-	r4 = *(u16*)(r10 - 8);				\
+	r4 = *(u64*)(r10 - 8);				\
 	r0 = r2;					\
 	/* r0 += r4 R0=pkt R2=pkt R3=pkt_end R4=umax=65535 */\
 	r0 += r4;					\