Re: [PATCH bpf v1 2/2] selftests/bpf: Add raw_tp tests for PTR_MAYBE_NULL marking

[Eduard Zingerman <eddyz87@gmail.com>]

On Wed, 2024-12-04 at 13:13 -0800, Alexei Starovoitov wrote:
> On Wed, Dec 4, 2024 at 1:08 PM Eduard Zingerman <eddyz87@gmail.com> wrote:
> > 
> > On Wed, 2024-12-04 at 21:48 +0100, Kumar Kartikeya Dwivedi wrote:
> > 
> > [...[
> > 
> > (A) ----.
> >         |
> >         v
> > > > > 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.
> > 
> > (B) ----.
> >         |
> >         v
> > > > 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.
> > 
> > Sorry for delay.
> > 
> > I like what Kumar is proposing in (A) because it could be generalized:
> > there is no real harm in doing 'r2 = r1; r2 += 8; r1 != 0; ...'
> > and what Kumar suggests could be used to lift the "null-check it first ..."
> > restriction.
> 
> I don't see how it can be generalized.
> Also 'avoid walking states when off != 0' is far from simple.
> We call into mark_ptr_or_null_regs() with id == 0 already
> and with reg->off != 0 for RCU and alloc_obj.

I did not try to implement this, so there might be a devil in the details.
The naive approach looks as below.

Suppose we want to allow 'rX += K' when rX is PTR_MAYBE_NULL.
Such operations generate a set of pointers with different .off values
but same .id .
For a regular (non raw_tp) case:
- dereferencing PTR_MAYBE_NULL is disallowed;
- if there is a check 'if rY != 0' and rY.off == 0,
  the non-null status could be propagated to each
  register in a set (and PTR_MAYBE_NULL mark removed);
- if there is a check 'if rY != 0' and rY.off != 0,
  nothing happens, no marks are changed.

For a raw_tp case:
- dereferencing PTR_MAYBE_NULL is allowed (as it is already);
- the mechanics for 'if rY != 0' and rY.off ==/!= 0 can remain the same,
  nothing is wrong with removing PTR_MAYBE_NULL marks from such pointers.

> 'avoid walking with off != 0' doesn't look trivial.
> It would need to be special cased to raw_tp and some other
> conditions.
> I could be missing something.
> 
> Let's see how patches look.
> 
> > However, as far as I understand, the plan is to fix this by generating
> > two entry tracepoint states: one with parameter as null, another with
> > parameter not-null (all combinations for every parameter).
> > If that is the plan, what Alexei suggests in (B) is simpler.