On Tue, Oct 04, 2022 at 12:22:08AM +0200, Kumar Kartikeya Dwivedi wrote: > > Thanks for providing additional context, Kumar. So what do we want to do > > for this patch set? IMO it doesn't seem useful to restrict > > bpf_kfunc_acquire() to only be callable by non-sleepable programs if our > > goal is to avoid crashes for nested task structs. We could easily > > accidentally crash if e.g. those pointers are NULL, or someone is doing > > something weird like stashing some extra flag bits in unused portions of > > the pointer which are masked out when it's actually dereferenced > > regardless of whether we're in RCU. Trusting ctx loads sounds like the > > right approach, barring some of the challenges you pointed out such as > > dealing with fexit paths after free where the object may not be valid > > anymore. > > > > In general, it seems like we should maybe decide on what our policy > > should be for kfuncs until we can wire up whatever we need to properly > > trust ctx. > > Well, we could add it now and work towards closing the gaps after > this, especially if bpf_task_acquire is really only useful in > sleepable programs where it works on the tracing args. A lot of other > kfuncs need these fixes as well, so it's a general problem and not > specific to this set. I am not very familiar with your exact use case. > Hopefully when it is fixed this particular case won't really break, if > you only use the tracepoint argument. I'm also interested in using this with struct_ops, not just tracing. I think that struct_ops should be totally fine though, and easier to reason about than tracing as we just have to make sure that a few specific callbacks are always passed a valid, referenced task, rather than e.g. worrying about fexit on __put_task_struct(). I'm fine with adding this now and working towards closing the gaps later, though I'd like to hear what Martin, Alexei, and the rest of the BPF maintainers think. I think Martin asked if there was any preliminary work you'd already done that we could try to tie into this patch set, and I'm similarly curious. > It is true that waiting for all the fixes will unnecessarily stall > this, it is not clear how each of the issues will be addressed either. > > Later its use can be made conditional in sleepable programs for > trusted and rcu tagged pointers under appropriate RCU read lock. I > will try to prioritize sending it out so that we resolve this soon. Much appreciated!
