Re: [PATCH v2 2/2] bpf/selftests: Add selftests for new task kfuncs

[David Vernet <void@manifault.com>]

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!