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
State New
Headers show
Series Improvements for tracking scalars in the BPF verifier | expand

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;					\
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;					\