Re: [PATCH v2] selftests/bpf: Update map_kptr examples to reflect real use-cases

[Kumar Kartikeya Dwivedi]

On Mon, 3 Oct 2022 at 15:52, David Vernet <void@xxxxxxxxxxxxx> wrote:
>
> On Mon, Oct 03, 2022 at 01:35:49AM +0200, Kumar Kartikeya Dwivedi wrote:
> > On Sun, 2 Oct 2022 at 19:10, David Vernet <void@xxxxxxxxxxxxx> wrote:
> > >
> > > In the map_kptr selftest, the bpf_kfunc_call_test_kptr_get() kfunc is used
> > > to verify and illustrate a typical use case of kptrs wherein an additional
> > > reference is taken on a referenced kptr that is already stored in a map.
> > > This would be useful for programs that, for example, want to pass the
> > > referenced kptr to a kfunc without removing it from the map.
> > >
> > > Unfortunately, the implementation of bpf_kfunc_call_test_kptr_get() isn't
> > > representative of a real kfunc that needs to guard against possible
> > > refcounting races by BPF program callers. bpf_kfunc_call_test_kptr_get()
> > > does a READ_ONCE() on the struct prog_test_ref_kfunc **pp passed by the
> > > user and then calls refcount_inc() if the pointer is nonzero, but this
> > > can race with another callback in the same program that removes the kptr
> > > from the map  and frees it:
> > >
> > > 1. A BPF program with a referenced kptr in a map passes the kptr to
> > >    bpf_kfunc_call_test_kptr_get() as:
> > >
> > >    p = bpf_kfunc_call_test_kptr_get(&v->ref_ptr, 0, 0);
> > >
> > >    from CPU 0.
> > >
> > > 2. bpf_kfunc_call_test_kptr_get() does READ_ONCE(), and sees that the
> > >    struct prog_test_ref_kfunc **pp contains a non-NULL pointer.
> > >
> > > 3. Another BPF handler on CPU 1 then invokes bpf_kptr_xchg() to remove
> > >    the kptr from the map, and frees it with a call to
> > >    bpf_kfunc_call_test_release(). This drops the final refcount on the
> > >    kptr.
> > >
> > > 4. CPU 0 then issues refcount_inc() on the kptr with refcount 0, causing
> > >    a use-after-free.
> > >
> > > In the map_kptr selftest, this doesn't cause a use-after-free because
> > > the structure being refcounted is statically allocated, and the
> > > refcounts aren't actually used to control the object lifecycle. In a
> > > kfunc supporting a real use case, the refcount going to 0 would likely
> > > cause the object to be freed, as it does for e.g. struct task_struct.
> > >
> > > A more realistic use-case would use something like RCU in the kfunc
> > > handler to ensure that the kptr object can be safely accessed, and then
> > > issuing a refcount_inc_not_zero() to acquire a refcount on the object.
> > > This patch updates the map_kptr selftest to do this.
> > >
> > > Signed-off-by: David Vernet <void@xxxxxxxxxxxxx>
> > > ---
> >
> > In my defense, I did note all this in the commit adding support for
> > kptr_get, so it seemed overkill to do it for a static struct.
> > But it's probably not a bad idea to have a real example, given it's a
> > necessity that such a helper requires reclamation of the object
> > through RCU, and people probably won't go and read the commit message.
>
> No defense needed, though I of course agree that it's important to have
> a real example here. I decided to add it because I got confused about
> how it was safe to use refcount_inc() instead of refcount_inc_not_zero()
> in bpf_kfunc_call_test_kptr_get().
>
> > However, some questions below...
> >
> > >  net/bpf/test_run.c                            | 31 ++++++++++++++++---
> > >  .../selftests/bpf/prog_tests/map_kptr.c       |  4 +--
> > >  .../testing/selftests/bpf/verifier/map_kptr.c |  4 +--
> > >  3 files changed, 31 insertions(+), 8 deletions(-)
> > >
> > > diff --git a/net/bpf/test_run.c b/net/bpf/test_run.c
> > > index 13d578ce2a09..3fe9495abcbe 100644
> > > --- a/net/bpf/test_run.c
> > > +++ b/net/bpf/test_run.c
> > > @@ -565,6 +565,8 @@ struct prog_test_ref_kfunc {
> > >         int b;
> > >         struct prog_test_member memb;
> > >         struct prog_test_ref_kfunc *next;
> > > +       struct rcu_head rcu;
> > > +       atomic_t destroyed;
> > >         refcount_t cnt;
> > >  };
> > >
> > > @@ -572,12 +574,14 @@ static struct prog_test_ref_kfunc prog_test_struct = {
> > >         .a = 42,
> > >         .b = 108,
> > >         .next = &prog_test_struct,
> > > +       .destroyed = ATOMIC_INIT(0),
> > >         .cnt = REFCOUNT_INIT(1),
> > >  };
> > >
> > >  noinline struct prog_test_ref_kfunc *
> > >  bpf_kfunc_call_test_acquire(unsigned long *scalar_ptr)
> > >  {
> > > +       WARN_ON_ONCE(atomic_read(&prog_test_struct.destroyed));
> > >         refcount_inc(&prog_test_struct.cnt);
> > >         return &prog_test_struct;
> > >  }
> > > @@ -589,12 +593,22 @@ bpf_kfunc_call_memb_acquire(void)
> > >         return NULL;
> > >  }
> > >
> > > +static void delayed_destroy_test_ref_struct(struct rcu_head *rhp)
> > > +{
> > > +       struct prog_test_ref_kfunc *p = container_of(rhp, struct prog_test_ref_kfunc, rcu);
> > > +
> > > +       WARN_ON_ONCE(refcount_read(&p->cnt) > 0);
> > > +       atomic_set(&p->destroyed, true);
> > > +}
> > > +
> > >  noinline void bpf_kfunc_call_test_release(struct prog_test_ref_kfunc *p)
> > >  {
> > >         if (!p)
> > >                 return;
> > >
> > > -       refcount_dec(&p->cnt);
> > > +       WARN_ON_ONCE(atomic_read(&p->destroyed));
> > > +       if (refcount_dec_and_test(&p->cnt))
> > > +               call_rcu(&p->rcu, delayed_destroy_test_ref_struct);
> >
> > I wonder whether this is ever called, I haven't really given this
> > patch a shot, but I don't see how the refcount can ever drop back to
> > zero. It's initialized as 1, and then only acquired after that, so
> > pairing all releases should still preserve refcount as 1.
>
> Yeah, the call_rcu() path is never hit. If we wanted to update the test
> so that this codepath was actually exercised, I think we'd need to add
> another kfunc that returned a reference to a dynamically allocated
> object rather than using the global, static one. I'm happy to do that if
> we think it's useful. The downside to adding more of these test kfuncs
> is that they actually do add a small bit of runtime overhead to the
> kernel because they're unconditionally registered in the __init function
> for test_run.c.
>

But that only happens once, right? It still happens, so I don't see
what changes.

> > Also, even if you made it work, wouldn't you have the warning once you
> > run more selftests using prog_test_run, if you just set the  destroyed
> > bit on each test run?
>
> If we want to update the test to have the refcount drop to 0, we would
> probably have to instead use dynamically allocated objects. At that
> point, we'd probably just crash instead of seeing a warning if we
> accidentally let a caller invoke acquire or release after the object had
> been destroyed. Maybe the better thing to do here is to just warn
> unconditionally in the destructor rather than setting a flag? What we
> really want to ensure is that the final refcount that's "owned" by the
> main kernel is never dropped.

I think the refcount_t API already warns if underflow happens.

To be clear, I don't mind if you want to improve this, it's certainly
a mess right now. Tests can't even run in parallel easily because it's
global. Testing like an actually allocated object might be a good way
to simulate a real scenario. And I totally agree that having a real
example is useful to people who want to add support for more kptrs.

Maybe some comments around the code would also be helpful on what the
expectations are. We should make it easy for people to hook into this
stuff. Nobody reads documentation, people usually only look at
existing examples.