diff mbox series

[bpf-next,v2,10/15] bpf: Track spilled unbounded scalars

Message ID 20240108205209.838365-11-maxtram95@gmail.com (mailing list archive)
State Accepted
Commit 53ac20c9e0dd3f7d1872671f7471be8d3fc3a361
Delegated to: BPF
Headers show
Series Improvements for tracking scalars in the BPF verifier | expand

Checks

Context Check Description
bpf/vmtest-bpf-next-PR success PR summary
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 fail Errors and warnings before: 1093 this patch: 1094
netdev/cc_maintainers success CCed 0 of 0 maintainers
netdev/build_clang fail Errors and warnings before: 1109 this patch: 1111
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 fail Errors and warnings before: 1120 this patch: 1121
netdev/checkpatch warning WARNING: line length of 84 exceeds 80 columns WARNING: line length of 92 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-6 success Logs for aarch64-gcc / test (test_maps, false, 360) / test_maps 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-16 success Logs for x86_64-llvm-17 / build / build for x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-14 success Logs for x86_64-gcc / build / build for x86_64 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-19 success Logs for x86_64-gcc / build / build for x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-20 success Logs for x86_64-gcc / build-release
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-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-42 success Logs for x86_64-llvm-18 / veristat
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-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-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-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-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-22 success Logs for x86_64-gcc / test (test_progs, false, 360) / test_progs on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-23 success 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-31 success 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 success 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-38 success 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 success 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-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-0 success Logs for Lint
bpf/vmtest-bpf-next-VM_Test-3 success Logs for Validate matrix.py
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-5 success Logs for aarch64-gcc / build-release
bpf/vmtest-bpf-next-VM_Test-7 success Logs for s390x-gcc / build-release
bpf/vmtest-bpf-next-VM_Test-8 success Logs for set-matrix
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-13 success Logs for set-matrix
bpf/vmtest-bpf-next-VM_Test-15 success Logs for x86_64-gcc / build-release

Commit Message

Maxim Mikityanskiy Jan. 8, 2024, 8:52 p.m. UTC 
From: Maxim Mikityanskiy <maxim@isovalent.com>

Support the pattern where an unbounded scalar is spilled to the stack,
then boundary checks are performed on the src register, after which the
stack frame slot is refilled into a register.

Before this commit, the verifier didn't treat the src register and the
stack slot as related if the src register was an unbounded scalar. The
register state wasn't copied, the id wasn't preserved, and the stack
slot was marked as STACK_MISC. Subsequent boundary checks on the src
register wouldn't result in updating the boundaries of the spilled
variable on the stack.

After this commit, the verifier will preserve the bond between src and
dst even if src is unbounded, which permits to do boundary checks on src
and refill dst later, still remembering its boundaries. Such a pattern
is sometimes generated by clang when compiling complex long functions.

One test is adjusted to reflect the fact that an untracked register is
marked as precise at an earlier stage, and one more test is adjusted to
reflect that now unbounded scalars are tracked.

Signed-off-by: Maxim Mikityanskiy <maxim@isovalent.com>
Acked-by: Eduard Zingerman <eddyz87@gmail.com>
---
 kernel/bpf/verifier.c                                   | 7 +------
 tools/testing/selftests/bpf/progs/verifier_spill_fill.c | 6 +++---
 tools/testing/selftests/bpf/verifier/precise.c          | 6 +++---
 3 files changed, 7 insertions(+), 12 deletions(-)

Comments

Alexei Starovoitov Jan. 12, 2024, 7:10 p.m. UTC | #1 
On Mon, Jan 8, 2024 at 12:53 PM Maxim Mikityanskiy <maxtram95@gmail.com> wrote:
>
> From: Maxim Mikityanskiy <maxim@isovalent.com>
>
> Support the pattern where an unbounded scalar is spilled to the stack,
> then boundary checks are performed on the src register, after which the
> stack frame slot is refilled into a register.
>
> Before this commit, the verifier didn't treat the src register and the
> stack slot as related if the src register was an unbounded scalar. The
> register state wasn't copied, the id wasn't preserved, and the stack
> slot was marked as STACK_MISC. Subsequent boundary checks on the src
> register wouldn't result in updating the boundaries of the spilled
> variable on the stack.
>
> After this commit, the verifier will preserve the bond between src and
> dst even if src is unbounded, which permits to do boundary checks on src
> and refill dst later, still remembering its boundaries. Such a pattern
> is sometimes generated by clang when compiling complex long functions.
>
> One test is adjusted to reflect the fact that an untracked register is
> marked as precise at an earlier stage, and one more test is adjusted to
> reflect that now unbounded scalars are tracked.
>
> Signed-off-by: Maxim Mikityanskiy <maxim@isovalent.com>
> Acked-by: Eduard Zingerman <eddyz87@gmail.com>
> ---
>  kernel/bpf/verifier.c                                   | 7 +------
>  tools/testing/selftests/bpf/progs/verifier_spill_fill.c | 6 +++---
>  tools/testing/selftests/bpf/verifier/precise.c          | 6 +++---
>  3 files changed, 7 insertions(+), 12 deletions(-)
>
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 055fa8096a08..e7fff5f5aa1d 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -4389,11 +4389,6 @@ static bool __is_scalar_unbounded(struct bpf_reg_state *reg)
>                reg->u32_min_value == 0 && reg->u32_max_value == U32_MAX;
>  }
>
> -static bool register_is_bounded(struct bpf_reg_state *reg)
> -{
> -       return reg->type == SCALAR_VALUE && !__is_scalar_unbounded(reg);
> -}
> -
>  static bool __is_pointer_value(bool allow_ptr_leaks,
>                                const struct bpf_reg_state *reg)
>  {
> @@ -4504,7 +4499,7 @@ static int check_stack_write_fixed_off(struct bpf_verifier_env *env,
>                 return err;
>
>         mark_stack_slot_scratched(env, spi);
> -       if (reg && !(off % BPF_REG_SIZE) && register_is_bounded(reg) && env->bpf_capable) {
> +       if (reg && !(off % BPF_REG_SIZE) && reg->type == SCALAR_VALUE && env->bpf_capable) {
>                 bool reg_value_fits;
>
>                 reg_value_fits = get_reg_width(reg) <= BITS_PER_BYTE * size;
> diff --git a/tools/testing/selftests/bpf/progs/verifier_spill_fill.c b/tools/testing/selftests/bpf/progs/verifier_spill_fill.c
> index b05aab925ee5..57eb70e100a3 100644
> --- a/tools/testing/selftests/bpf/progs/verifier_spill_fill.c
> +++ b/tools/testing/selftests/bpf/progs/verifier_spill_fill.c
> @@ -452,9 +452,9 @@ l0_%=:      r1 >>= 16;                                      \
>  SEC("raw_tp")
>  __log_level(2)
>  __success
> -__msg("fp-8=0m??mmmm")
> -__msg("fp-16=00mm??mm")
> -__msg("fp-24=00mm???m")
> +__msg("fp-8=0m??scalar()")
> +__msg("fp-16=00mm??scalar()")
> +__msg("fp-24=00mm???scalar()")
>  __naked void spill_subregs_preserve_stack_zero(void)
>  {
>         asm volatile (
> diff --git a/tools/testing/selftests/bpf/verifier/precise.c b/tools/testing/selftests/bpf/verifier/precise.c
> index 8a2ff81d8350..0a9293a57211 100644
> --- a/tools/testing/selftests/bpf/verifier/precise.c
> +++ b/tools/testing/selftests/bpf/verifier/precise.c
> @@ -183,10 +183,10 @@
>         .prog_type = BPF_PROG_TYPE_XDP,
>         .flags = BPF_F_TEST_STATE_FREQ,
>         .errstr = "mark_precise: frame0: last_idx 7 first_idx 7\
> -       mark_precise: frame0: parent state regs=r4 stack=:\
> +       mark_precise: frame0: parent state regs=r4 stack=-8:\
>         mark_precise: frame0: last_idx 6 first_idx 4\
> -       mark_precise: frame0: regs=r4 stack= before 6: (b7) r0 = -1\
> -       mark_precise: frame0: regs=r4 stack= before 5: (79) r4 = *(u64 *)(r10 -8)\
> +       mark_precise: frame0: regs=r4 stack=-8 before 6: (b7) r0 = -1\
> +       mark_precise: frame0: regs=r4 stack=-8 before 5: (79) r4 = *(u64 *)(r10 -8)\
>         mark_precise: frame0: regs= stack=-8 before 4: (7b) *(u64 *)(r3 -8) = r0\
>         mark_precise: frame0: parent state regs=r0 stack=:\
>         mark_precise: frame0: last_idx 3 first_idx 3\

Yesterday I've applied patches 1 through 11 to bpf-next.
Then Yonghong found that removal of register_is_bounded()
in this patch 10 makes __is_scalar_unbounded() unused which
breaks build.
So I dropped patches 10 and 11.

Today we found out that test_verifier is broken with patches 1 through 9.
Turned out that this hunk for verifier/precise.c in patch 10 should have been
in patch 8.
I manually took it and force pushed bpf-next again.
Please test bisectability of the series more carefully in the future.

As far as register_is_bounded() issue.
Maybe order patch 14 that uses __is_scalar_unbounded() first and
then add this patch 10 ?
Other ideas?
Maxim Mikityanskiy Jan. 12, 2024, 8:44 p.m. UTC | #2 
On Fri, 12 Jan 2024 at 11:10:57 -0800, Alexei Starovoitov wrote:
> On Mon, Jan 8, 2024 at 12:53 PM Maxim Mikityanskiy <maxtram95@gmail.com> wrote:
> >
> > From: Maxim Mikityanskiy <maxim@isovalent.com>
> >
> > Support the pattern where an unbounded scalar is spilled to the stack,
> > then boundary checks are performed on the src register, after which the
> > stack frame slot is refilled into a register.
> >
> > Before this commit, the verifier didn't treat the src register and the
> > stack slot as related if the src register was an unbounded scalar. The
> > register state wasn't copied, the id wasn't preserved, and the stack
> > slot was marked as STACK_MISC. Subsequent boundary checks on the src
> > register wouldn't result in updating the boundaries of the spilled
> > variable on the stack.
> >
> > After this commit, the verifier will preserve the bond between src and
> > dst even if src is unbounded, which permits to do boundary checks on src
> > and refill dst later, still remembering its boundaries. Such a pattern
> > is sometimes generated by clang when compiling complex long functions.
> >
> > One test is adjusted to reflect the fact that an untracked register is
> > marked as precise at an earlier stage, and one more test is adjusted to
> > reflect that now unbounded scalars are tracked.
> >
> > Signed-off-by: Maxim Mikityanskiy <maxim@isovalent.com>
> > Acked-by: Eduard Zingerman <eddyz87@gmail.com>
> > ---
> >  kernel/bpf/verifier.c                                   | 7 +------
> >  tools/testing/selftests/bpf/progs/verifier_spill_fill.c | 6 +++---
> >  tools/testing/selftests/bpf/verifier/precise.c          | 6 +++---
> >  3 files changed, 7 insertions(+), 12 deletions(-)
> >
> > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> > index 055fa8096a08..e7fff5f5aa1d 100644
> > --- a/kernel/bpf/verifier.c
> > +++ b/kernel/bpf/verifier.c
> > @@ -4389,11 +4389,6 @@ static bool __is_scalar_unbounded(struct bpf_reg_state *reg)
> >                reg->u32_min_value == 0 && reg->u32_max_value == U32_MAX;
> >  }
> >
> > -static bool register_is_bounded(struct bpf_reg_state *reg)
> > -{
> > -       return reg->type == SCALAR_VALUE && !__is_scalar_unbounded(reg);
> > -}
> > -
> >  static bool __is_pointer_value(bool allow_ptr_leaks,
> >                                const struct bpf_reg_state *reg)
> >  {
> > @@ -4504,7 +4499,7 @@ static int check_stack_write_fixed_off(struct bpf_verifier_env *env,
> >                 return err;
> >
> >         mark_stack_slot_scratched(env, spi);
> > -       if (reg && !(off % BPF_REG_SIZE) && register_is_bounded(reg) && env->bpf_capable) {
> > +       if (reg && !(off % BPF_REG_SIZE) && reg->type == SCALAR_VALUE && env->bpf_capable) {
> >                 bool reg_value_fits;
> >
> >                 reg_value_fits = get_reg_width(reg) <= BITS_PER_BYTE * size;
> > diff --git a/tools/testing/selftests/bpf/progs/verifier_spill_fill.c b/tools/testing/selftests/bpf/progs/verifier_spill_fill.c
> > index b05aab925ee5..57eb70e100a3 100644
> > --- a/tools/testing/selftests/bpf/progs/verifier_spill_fill.c
> > +++ b/tools/testing/selftests/bpf/progs/verifier_spill_fill.c
> > @@ -452,9 +452,9 @@ l0_%=:      r1 >>= 16;                                      \
> >  SEC("raw_tp")
> >  __log_level(2)
> >  __success
> > -__msg("fp-8=0m??mmmm")
> > -__msg("fp-16=00mm??mm")
> > -__msg("fp-24=00mm???m")
> > +__msg("fp-8=0m??scalar()")
> > +__msg("fp-16=00mm??scalar()")
> > +__msg("fp-24=00mm???scalar()")
> >  __naked void spill_subregs_preserve_stack_zero(void)
> >  {
> >         asm volatile (
> > diff --git a/tools/testing/selftests/bpf/verifier/precise.c b/tools/testing/selftests/bpf/verifier/precise.c
> > index 8a2ff81d8350..0a9293a57211 100644
> > --- a/tools/testing/selftests/bpf/verifier/precise.c
> > +++ b/tools/testing/selftests/bpf/verifier/precise.c
> > @@ -183,10 +183,10 @@
> >         .prog_type = BPF_PROG_TYPE_XDP,
> >         .flags = BPF_F_TEST_STATE_FREQ,
> >         .errstr = "mark_precise: frame0: last_idx 7 first_idx 7\
> > -       mark_precise: frame0: parent state regs=r4 stack=:\
> > +       mark_precise: frame0: parent state regs=r4 stack=-8:\
> >         mark_precise: frame0: last_idx 6 first_idx 4\
> > -       mark_precise: frame0: regs=r4 stack= before 6: (b7) r0 = -1\
> > -       mark_precise: frame0: regs=r4 stack= before 5: (79) r4 = *(u64 *)(r10 -8)\
> > +       mark_precise: frame0: regs=r4 stack=-8 before 6: (b7) r0 = -1\
> > +       mark_precise: frame0: regs=r4 stack=-8 before 5: (79) r4 = *(u64 *)(r10 -8)\
> >         mark_precise: frame0: regs= stack=-8 before 4: (7b) *(u64 *)(r3 -8) = r0\
> >         mark_precise: frame0: parent state regs=r0 stack=:\
> >         mark_precise: frame0: last_idx 3 first_idx 3\
> 
> Yesterday I've applied patches 1 through 11 to bpf-next.
> Then Yonghong found that removal of register_is_bounded()
> in this patch 10 makes __is_scalar_unbounded() unused which
> breaks build.
> So I dropped patches 10 and 11.
> 
> Today we found out that test_verifier is broken with patches 1 through 9.
> Turned out that this hunk for verifier/precise.c in patch 10 should have been
> in patch 8.
> I manually took it and force pushed bpf-next again.
> Please test bisectability of the series more carefully in the future.

So sorry I caused this trouble! I indeed mistakenly attributed this hunk
to a wrong patch, must have been more careful =/

> As far as register_is_bounded() issue.
> Maybe order patch 14 that uses __is_scalar_unbounded() first and
> then add this patch 10 ?
> Other ideas?

As for the unused function warning, I thought it wasn't a big deal if I
start using the function again later in the same series. Couldn't
anticipate how it turns out. The idea of having patch 14 in the end of
the series was to show the performance numbers. I'll reorder it before
patch 10, so that we avoid that warning. Sorry again for this mess.
Alexei Starovoitov Jan. 12, 2024, 8:50 p.m. UTC | #3 
On Fri, Jan 12, 2024 at 12:44 PM Maxim Mikityanskiy <maxtram95@gmail.com> wrote:
>
> On Fri, 12 Jan 2024 at 11:10:57 -0800, Alexei Starovoitov wrote:
> > On Mon, Jan 8, 2024 at 12:53 PM Maxim Mikityanskiy <maxtram95@gmail.com> wrote:
> > >
> > > From: Maxim Mikityanskiy <maxim@isovalent.com>
> > >
> > > Support the pattern where an unbounded scalar is spilled to the stack,
> > > then boundary checks are performed on the src register, after which the
> > > stack frame slot is refilled into a register.
> > >
> > > Before this commit, the verifier didn't treat the src register and the
> > > stack slot as related if the src register was an unbounded scalar. The
> > > register state wasn't copied, the id wasn't preserved, and the stack
> > > slot was marked as STACK_MISC. Subsequent boundary checks on the src
> > > register wouldn't result in updating the boundaries of the spilled
> > > variable on the stack.
> > >
> > > After this commit, the verifier will preserve the bond between src and
> > > dst even if src is unbounded, which permits to do boundary checks on src
> > > and refill dst later, still remembering its boundaries. Such a pattern
> > > is sometimes generated by clang when compiling complex long functions.
> > >
> > > One test is adjusted to reflect the fact that an untracked register is
> > > marked as precise at an earlier stage, and one more test is adjusted to
> > > reflect that now unbounded scalars are tracked.
> > >
> > > Signed-off-by: Maxim Mikityanskiy <maxim@isovalent.com>
> > > Acked-by: Eduard Zingerman <eddyz87@gmail.com>
> > > ---
> > >  kernel/bpf/verifier.c                                   | 7 +------
> > >  tools/testing/selftests/bpf/progs/verifier_spill_fill.c | 6 +++---
> > >  tools/testing/selftests/bpf/verifier/precise.c          | 6 +++---
> > >  3 files changed, 7 insertions(+), 12 deletions(-)
> > >
> > > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> > > index 055fa8096a08..e7fff5f5aa1d 100644
> > > --- a/kernel/bpf/verifier.c
> > > +++ b/kernel/bpf/verifier.c
> > > @@ -4389,11 +4389,6 @@ static bool __is_scalar_unbounded(struct bpf_reg_state *reg)
> > >                reg->u32_min_value == 0 && reg->u32_max_value == U32_MAX;
> > >  }
> > >
> > > -static bool register_is_bounded(struct bpf_reg_state *reg)
> > > -{
> > > -       return reg->type == SCALAR_VALUE && !__is_scalar_unbounded(reg);
> > > -}
> > > -
> > >  static bool __is_pointer_value(bool allow_ptr_leaks,
> > >                                const struct bpf_reg_state *reg)
> > >  {
> > > @@ -4504,7 +4499,7 @@ static int check_stack_write_fixed_off(struct bpf_verifier_env *env,
> > >                 return err;
> > >
> > >         mark_stack_slot_scratched(env, spi);
> > > -       if (reg && !(off % BPF_REG_SIZE) && register_is_bounded(reg) && env->bpf_capable) {
> > > +       if (reg && !(off % BPF_REG_SIZE) && reg->type == SCALAR_VALUE && env->bpf_capable) {
> > >                 bool reg_value_fits;
> > >
> > >                 reg_value_fits = get_reg_width(reg) <= BITS_PER_BYTE * size;
> > > diff --git a/tools/testing/selftests/bpf/progs/verifier_spill_fill.c b/tools/testing/selftests/bpf/progs/verifier_spill_fill.c
> > > index b05aab925ee5..57eb70e100a3 100644
> > > --- a/tools/testing/selftests/bpf/progs/verifier_spill_fill.c
> > > +++ b/tools/testing/selftests/bpf/progs/verifier_spill_fill.c
> > > @@ -452,9 +452,9 @@ l0_%=:      r1 >>= 16;                                      \
> > >  SEC("raw_tp")
> > >  __log_level(2)
> > >  __success
> > > -__msg("fp-8=0m??mmmm")
> > > -__msg("fp-16=00mm??mm")
> > > -__msg("fp-24=00mm???m")
> > > +__msg("fp-8=0m??scalar()")
> > > +__msg("fp-16=00mm??scalar()")
> > > +__msg("fp-24=00mm???scalar()")
> > >  __naked void spill_subregs_preserve_stack_zero(void)
> > >  {
> > >         asm volatile (
> > > diff --git a/tools/testing/selftests/bpf/verifier/precise.c b/tools/testing/selftests/bpf/verifier/precise.c
> > > index 8a2ff81d8350..0a9293a57211 100644
> > > --- a/tools/testing/selftests/bpf/verifier/precise.c
> > > +++ b/tools/testing/selftests/bpf/verifier/precise.c
> > > @@ -183,10 +183,10 @@
> > >         .prog_type = BPF_PROG_TYPE_XDP,
> > >         .flags = BPF_F_TEST_STATE_FREQ,
> > >         .errstr = "mark_precise: frame0: last_idx 7 first_idx 7\
> > > -       mark_precise: frame0: parent state regs=r4 stack=:\
> > > +       mark_precise: frame0: parent state regs=r4 stack=-8:\
> > >         mark_precise: frame0: last_idx 6 first_idx 4\
> > > -       mark_precise: frame0: regs=r4 stack= before 6: (b7) r0 = -1\
> > > -       mark_precise: frame0: regs=r4 stack= before 5: (79) r4 = *(u64 *)(r10 -8)\
> > > +       mark_precise: frame0: regs=r4 stack=-8 before 6: (b7) r0 = -1\
> > > +       mark_precise: frame0: regs=r4 stack=-8 before 5: (79) r4 = *(u64 *)(r10 -8)\
> > >         mark_precise: frame0: regs= stack=-8 before 4: (7b) *(u64 *)(r3 -8) = r0\
> > >         mark_precise: frame0: parent state regs=r0 stack=:\
> > >         mark_precise: frame0: last_idx 3 first_idx 3\
> >
> > Yesterday I've applied patches 1 through 11 to bpf-next.
> > Then Yonghong found that removal of register_is_bounded()
> > in this patch 10 makes __is_scalar_unbounded() unused which
> > breaks build.
> > So I dropped patches 10 and 11.
> >
> > Today we found out that test_verifier is broken with patches 1 through 9.
> > Turned out that this hunk for verifier/precise.c in patch 10 should have been
> > in patch 8.
> > I manually took it and force pushed bpf-next again.
> > Please test bisectability of the series more carefully in the future.
>
> So sorry I caused this trouble! I indeed mistakenly attributed this hunk
> to a wrong patch, must have been more careful =/
>
> > As far as register_is_bounded() issue.
> > Maybe order patch 14 that uses __is_scalar_unbounded() first and
> > then add this patch 10 ?
> > Other ideas?
>
> As for the unused function warning, I thought it wasn't a big deal if I
> start using the function again later in the same series. Couldn't
> anticipate how it turns out. The idea of having patch 14 in the end of
> the series was to show the performance numbers. I'll reorder it before
> patch 10, so that we avoid that warning. Sorry again for this mess.

Makes sense to me and no worries.
No one would have noticed if the whole series were applied.
Looking forward to v3.
diff mbox series

Patch

diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 055fa8096a08..e7fff5f5aa1d 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -4389,11 +4389,6 @@  static bool __is_scalar_unbounded(struct bpf_reg_state *reg)
 	       reg->u32_min_value == 0 && reg->u32_max_value == U32_MAX;
 }
 
-static bool register_is_bounded(struct bpf_reg_state *reg)
-{
-	return reg->type == SCALAR_VALUE && !__is_scalar_unbounded(reg);
-}
-
 static bool __is_pointer_value(bool allow_ptr_leaks,
 			       const struct bpf_reg_state *reg)
 {
@@ -4504,7 +4499,7 @@  static int check_stack_write_fixed_off(struct bpf_verifier_env *env,
 		return err;
 
 	mark_stack_slot_scratched(env, spi);
-	if (reg && !(off % BPF_REG_SIZE) && register_is_bounded(reg) && env->bpf_capable) {
+	if (reg && !(off % BPF_REG_SIZE) && reg->type == SCALAR_VALUE && env->bpf_capable) {
 		bool reg_value_fits;
 
 		reg_value_fits = get_reg_width(reg) <= BITS_PER_BYTE * size;
diff --git a/tools/testing/selftests/bpf/progs/verifier_spill_fill.c b/tools/testing/selftests/bpf/progs/verifier_spill_fill.c
index b05aab925ee5..57eb70e100a3 100644
--- a/tools/testing/selftests/bpf/progs/verifier_spill_fill.c
+++ b/tools/testing/selftests/bpf/progs/verifier_spill_fill.c
@@ -452,9 +452,9 @@  l0_%=:	r1 >>= 16;					\
 SEC("raw_tp")
 __log_level(2)
 __success
-__msg("fp-8=0m??mmmm")
-__msg("fp-16=00mm??mm")
-__msg("fp-24=00mm???m")
+__msg("fp-8=0m??scalar()")
+__msg("fp-16=00mm??scalar()")
+__msg("fp-24=00mm???scalar()")
 __naked void spill_subregs_preserve_stack_zero(void)
 {
 	asm volatile (
diff --git a/tools/testing/selftests/bpf/verifier/precise.c b/tools/testing/selftests/bpf/verifier/precise.c
index 8a2ff81d8350..0a9293a57211 100644
--- a/tools/testing/selftests/bpf/verifier/precise.c
+++ b/tools/testing/selftests/bpf/verifier/precise.c
@@ -183,10 +183,10 @@ 
 	.prog_type = BPF_PROG_TYPE_XDP,
 	.flags = BPF_F_TEST_STATE_FREQ,
 	.errstr = "mark_precise: frame0: last_idx 7 first_idx 7\
-	mark_precise: frame0: parent state regs=r4 stack=:\
+	mark_precise: frame0: parent state regs=r4 stack=-8:\
 	mark_precise: frame0: last_idx 6 first_idx 4\
-	mark_precise: frame0: regs=r4 stack= before 6: (b7) r0 = -1\
-	mark_precise: frame0: regs=r4 stack= before 5: (79) r4 = *(u64 *)(r10 -8)\
+	mark_precise: frame0: regs=r4 stack=-8 before 6: (b7) r0 = -1\
+	mark_precise: frame0: regs=r4 stack=-8 before 5: (79) r4 = *(u64 *)(r10 -8)\
 	mark_precise: frame0: regs= stack=-8 before 4: (7b) *(u64 *)(r3 -8) = r0\
 	mark_precise: frame0: parent state regs=r0 stack=:\
 	mark_precise: frame0: last_idx 3 first_idx 3\