Re: [PATCH v5 3/3] bpf/selftests: Add selftests for new task kfuncs

[David Vernet]

On Thu, Oct 20, 2022 at 11:49:03AM +0530, Kumar Kartikeya Dwivedi wrote:
> On Wed, Oct 19, 2022 at 11:09:05PM IST, David Vernet wrote:
> > > On Sat, 15 Oct 2022 at 01:45, David Vernet <void@xxxxxxxxxxxxx> wrote:
> > > >
> > > > A previous change added a series of kfuncs for storing struct
> > > > task_struct objects as referenced kptrs. This patch adds a new
> > > > task_kfunc test suite for validating their expected behavior.
> > > >
> > > > Signed-off-by: David Vernet <void@xxxxxxxxxxxxx>
> > > > ---
> > > > [...]
> > > > +
> > > > +SEC("tp_btf/task_newtask")
> > > > +int BPF_PROG(task_kfunc_acquire_trusted_nested, struct task_struct *task, u64 clone_flags)
> > > > +{
> > > > +       struct task_struct *acquired;
> > > > +
> > > > +       if (!is_test_kfunc_task())
> > > > +               return 0;
> > > > +
> > > > +       /* Can't invoke bpf_task_acquire() on a trusted pointer at a nonzero offset. */
> > > > +       acquired = bpf_task_acquire(task->last_wakee);
> > >
> > > The comment is incorrect, that would be &task->last_wakee instead,
> > > this is PTR_TO_BTF_ID | PTR_NESTED.
> >
> > Well, it's a nonzero offset from task. But yes, to your point, it's a
> > misleading comment because the offset is 0 in the verifier. I'll
> 
> The load insn has a non-zero offset, but not the destination reg.

Yeah, this is what I meant by hand-wavily saying "0 in the verifier". I
agree that the comment is incorrect as stated, I'll fix it in the next
revision per your suggestion.

> What you did was:
> r1 = rX + offsetof(task_struct, last_wakee); // r1 == rX + off
> r1 = *(r1); // r1 == PTR_TO_BTF_ID of task_struct, off = 0
> 
> Embedded structs are different,
> &file->f_path means non-zero offset into PTR_TO_BTF_ID of struct file
> 
> > rephrase this to reflect that it's a nested pointer (or a walked
> > pointer, whatever nomenclature we end up going with).
> >
> > > > +       if (!acquired)
> > > > +               return 0;
> > > > +       bpf_task_release(acquired);
> > > > +
> > > > +       return 0;
> > > > +}
> > > > +
> > > > [...]
> > > > +
> > > > +static int test_acquire_release(struct task_struct *task)
> > > > +{
> > > > +       struct task_struct *acquired;
> > > > +
> > > > +       acquired = bpf_task_acquire(task);
> > >
> > > Unfortunately a side effect of this change is that now since
> > > PTR_TO_BTF_ID without ref_obj_id is considered trusted, the bpf_ct_*
> > > functions would begin working with tp_btf args. That probably needs
> > > be fixed so that they reject them (ideally with a failing test case to
> > > make sure it doesn't resurface), probably with a new suffix __ref/or
> > > __owned as added here [0].
> > >
> > > Alexei, since you've suggested avoiding adding that suffix, do you see
> > > any other way out here?
> > > It's questionable whether bpf_ct_set_timeout/status should work for CT
> > > not owned by the BPF program.
> > >
> > >   [0]: https://lore.kernel.org/bpf/dfb859a6b76a9234baa194e795ae89cb7ca5694b.1662383493.git.lorenzo@kerne
> >
> > Ah, yeah, it makes sense that some kfuncs really should only ever be
> > passed an object if the program owns a reference on it. Specifically for
> > e.g. bpf_ct_set_timeout/status() as you point out, which should only be
> > passed a struct nf_conn__init that was allocated by bpf_skb_ct_alloc().
> >
> > It'd be nice if we could just add another flag like KF_REFERENCED_ARGS
> > or KF_OWNED_ARGS, which would allow a subset of arguments affored by
> > KF_TRUSTED_ARGS, only those with ref_obj_id > 0. That approach wouldn't
> > allow the flexibility of having per-argument specifications as your
> > proposal to use __ref or __owned suffixes on the names, but that already
> > applies to KF_TRUSTED_ARGS as well.
> >
> > Personally I'm in agreement with Alexei that it's not a user friendly
> > API to use suffixes in the name like this. If we want to allow kfunc
> > authors to have per-argument specifiers, using compiler attributes
> > and/or some kind of tagging is probably the way to do it?
> 
> Sadly GCC doesn't support BTF tags. So this is the next best tagging approach.
> 
> There was also another horrendous proposal from me to add flags to each argument:
> https://gist.github.com/kkdwivedi/7839cc9e4f002acc3e15350b1b86c88c#file-kfunc-arg-patch-L137

I feel like I must be missing something given that you said this was
horrendous, but I actually don't hate this. This is pretty similar to
what we do for helpers anyways, no? I certainly prefer it over the
suffix naming approach. IMO the problem with that is it kind of requires
users to dive into the verifier to understand how to implement kfuncs.
That also reminds me that in the next revision, I'll also update the
documentation for KF_TRUSTED_ARGS (and KF_OWNED_ARGS) to reflect the new
state of things.

> > My proposal for now is to add a new KF_OWNED_ARGS flag, and to very
> > clearly document exactly what that and KF_TRUSTED_ARGS implies for
> > kfuncs. Later on, we could explore solutions for having per-arg
> > specifiers. What do you and Alexei think?
> 
> Based on your proposal above:
> 
> For KF_OWNED_ARGS, any PTR_TO_BTF_ID should have non-zero ref_obj_id, for
> KF_TRUSTED_ARGS there will be no such condition. Otherwise they will be the
> same. Then switch all CT helpers to KF_OWNED_ARGS.

Exactly

> This should work fine.

Great, I'll get to work on this. Unless Alexei or anyone else objects?