On Wed, 4 Dec 2024 at 21:40, Alexei Starovoitov
<alexei.starovoitov@xxxxxxxxx> wrote:
>
> On Wed, Dec 4, 2024 at 12:22 PM Kumar Kartikeya Dwivedi
> <memxor@xxxxxxxxx> wrote:
> >
> > On Wed, 4 Dec 2024 at 21:19, Kumar Kartikeya Dwivedi <memxor@xxxxxxxxx> wrote:
> > >
> > > On Wed, 4 Dec 2024 at 21:12, Eduard Zingerman <eddyz87@xxxxxxxxx> wrote:
> > > >
> > > > On Tue, 2024-12-03 at 18:41 -0800, Kumar Kartikeya Dwivedi wrote:
> > > >
> > > > [...]
> > > >
> > > > > +/* r2 with offset is checked, which marks r1 with off=0 as non-NULL */
> > > > > +SEC("tp_btf/bpf_testmod_test_raw_tp_null")
> > > > > +__failure
> > > > > +__msg("3: (07) r2 += 8 ; R2_w=trusted_ptr_or_null_sk_buff(id=1,off=8)")
> > > > > +__msg("4: (15) if r2 == 0x0 goto pc+2 ; R2_w=trusted_ptr_or_null_sk_buff(id=2,off=8)")
> > > > > +__msg("5: (bf) r1 = r1 ; R1_w=trusted_ptr_sk_buff()")
> > > >
> > > > This looks like a bug.
> > > > 'r1 != 0' does not follow from 'r2 == r1 + 8 and r2 != 0'.
> > > >
> > >
> > > Hmm, yes, it's broken.
> > > I am realizing where we do it now will walk r1 first and we'll not see
> > > r2 off != 0 until after we mark it already.
> > > I guess we need to do the check sooner outside this function in
> > > mark_ptr_or_null_regs.
> > > There we have the register being operated on, so if off != 0 we don't
> > > walk all regs in state.
> >
> > What this will do in both cases::
> > First, avoid walking states when off != 0, and reset id.
> > If off == 0, go inside mark_ptr_or_null_reg and walk all regs, and
> > remove marks for those with off != 0.
>
> That's getting intrusive.
> How about we reset id=0 in adjust_ptr_min_max_vals()
> right after we suppressed "null-check it first" message for raw_tp-s.
>
> That will address the issue as well, right?
Yes (minor detail, it needs to be reset to a new id, otherwise we have
warn on maybe_null set but !reg->id, but the idea is the same).
Let's see what Eduard thinks and then I can give it a go.
