Message-ID: <Y0WJ7FNlYIsWbTjP@maniforge.dhcp.thefacebook.com>
Date:   Tue, 11 Oct 2022 10:21:16 -0500
From:   David Vernet <void@...ifault.com>
To:     Kumar Kartikeya Dwivedi <memxor@...il.com>
Cc:     ast@...nel.org, daniel@...earbox.net, andrii@...nel.org,
        martin.lau@...ux.dev, kernel-team@...com, bpf@...r.kernel.org,
        linux-kernel@...r.kernel.org, yhs@...com, song@...nel.org,
        john.fastabend@...il.com, kpsingh@...nel.org, sdf@...gle.com,
        haoluo@...gle.com, jolsa@...nel.org, tj@...nel.org
Subject: Re: [PATCH v2] selftests/bpf: Update map_kptr examples to reflect
 real use-cases

On Tue, Oct 11, 2022 at 08:10:26AM +0530, Kumar Kartikeya Dwivedi wrote:

[...]

> > > > > 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.
> >
> > Right, a warning would probably show up if we did a release that caused
> > an underflow, but it would not for an acquire after the refcount dropped
> > to 0.
> >
> 
> It should, see REFCOUNT_ADD_UAF in include/linux/refcount.h.

Ahh, right you are, fair enough and thanks for hand holding and pointing
me directly at the relevant code. I now agree that the warns on the
destroyed field are just redundant.

> > > 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.
> >
> > Ok, let me update the tests to do two things then:
> >
> > 1. Add a new test kfunc called bpf_kfunc_call_test_alloc() which returns
> >    a dynamically allocated instance of a prog_test_ref_kfunc *. This is
> >    similar in intention to bpf_xdp_ct_alloc().
> > 2. Update bpf_kfunc_call_test_acquire() and
> >    bpf_kfunc_call_test_release() to take a trusted pointer to that
> >    allocated prog_test_ref_kfunc *.
> 
> This should work, but you would have to go through a lot of tests,
> sadly I assumed it is global in a lot of places to make testing easier
> (e.g. observing count after releasing by doing p->next, which gave me
> a PTR_TO_BTF_ID that is preserved after release).
> Some other way would have to be found to do the same thing.

Hmmm, ok, let me spend a bit of time trying to make all of this dynamic.
If it becomes clear that it's too much of a PITA, I might just drop the
patch; especially considering that your __rcu kptr work will address
what I was really initially trying to fix (which is that the kptr_get
pattern used in the test would likely be racy for a real kfunc). Or if
we want to, we could keep what it has now, but I could just update
delayed_destroy_test_ref_struct() to do nothing other than
WARN_ON_ONCE() and remove the extra warns for
prog_test_struct.destroyed. We can make a final call when I send out v3.

> > 3. Keep the other changes which e.g. use RCU in
> >    bpf_kfunc_call_test_kptr_get() to synchronize on getting the kptr.
> >    Once the __rcu kptr variant is landed we can get rid of
> >    bpf_kfunc_call_test_kptr_get() and make bpf_kfunc_call_test_acquire()
> >    require an __rcu pointer.
> >
> 
> In the case of RCU I don't plan on passing the rcu pointer to acquire
> functions, but rather kptr_get stops taking pointer to pointer. I.e.
> in your point 3, kptr_get does what you want _acquire to do. It takes
> an rcu protected pointer to an object and safely increments its count.

Oh, ok. So the idea is that the acquire function takes a normal trusted
pointer, and kptr_get takes an RCU protected kptr (so it would still
have to do refcount_inc_not_zero()). Makes sense.

> > Continuing on point (3) above, we should _probably_ also have an example
> > for an object that isn't RCU-protected? I imagine that we don't want to
> > get rid of KF_KPTR_GET entirely once we have __rcu pointers because some
> > kptr_get() implementations may synchronize in other ways, such as with
> > spinlocks or whatever. Let's leave this until after __rcu lands though.
> >
> 
> I think it's unlikely kptr_get can work without it, spinlocks may be
> required internally (e.g. to inspect the object key/generation in
> SLAB_TYPESAFE_BY_RCU case without races) but that goes on top of RCU
> protection. But yes, it depends, maybe it will work for some special
> cases. Though I don't think it's worth adding an example for the
> uncommon case.

Yeah, let's leave it off for now until we have a concrete use case in
the kernel that we want to mirror in a testcase.

Powered by blists - more mailing lists