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

[Kumar Kartikeya Dwivedi]

On Tue, 4 Oct 2022 at 20:40, David Vernet <void@xxxxxxxxxxxxx> wrote:
>
> 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's mostly a few experimental patches in my local tree, so nowhere
close to completion. Ofcourse doing it properly will be a lot of work,
but I will be happy to help with reviews if you want to focus on
pointers loaded from ctx for now and make that part of this set, while
not permitting any other cases. It should not be very difficult to add
just that.

So you can set KF_TRUSTED_ARGS for your kfunc, then make it work for
PTR_TO_BTF_ID where it either has PTR_TRUSTED, ref_obj_id > 0, or
both. Just that PTR_TRUSTED is lost for the destination reg as soon as
btf_struct_access is used to walk pointers (unlike PTR_UNTRUSTED which
is inherited). Note that you don't _set_ PTR_UNTRUSTED here for the
dst_reg.

This should enable your immediate use case, while also being useful
for future work that builds on it. It should also preserve backwards
compatibility with existing stable helpers. You set PTR_TRUSTED when
you access ctx of tracing or struct_ops etc. All this will be handled
in is_valid_access callback/btf_ctx_access and setting the flag in
bpf_insn_access_aux. Unless I missed some detail/corner case it won't
be a lot of code.

If that turns out to be too restrictive/coarse for your use case, we
can just go with this as is for now.

Does that sound good? Any other comments/opinions?