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.
