Re: [PATCH bpf-next v6 1/2] bpf: Add bpf_task_under_cgroup() kfunc

[Yonghong Song]

On 5/5/23 12:18 AM, Feng Zhou wrote:
> 在 2023/5/5 14:58, Hao Luo 写道:
> > On Thu, May 4, 2023 at 11:08 PM Feng zhou <zhoufeng.zf@xxxxxxxxxxxxx> 
> > wrote:
> > >
> > <...>
> > > ---
> > >   kernel/bpf/helpers.c | 20 ++++++++++++++++++++
> > >   1 file changed, 20 insertions(+)
> > >
> > > diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
> > > index bb6b4637ebf2..453cbd312366 100644
> > > --- a/kernel/bpf/helpers.c
> > > +++ b/kernel/bpf/helpers.c
> > > @@ -2149,6 +2149,25 @@ __bpf_kfunc struct cgroup 
> > > *bpf_cgroup_from_id(u64 cgid)
> > >                  return NULL;
> > >          return cgrp;
> > >   }
> > > +
> > > +/**
> > > + * bpf_task_under_cgroup - wrap task_under_cgroup_hierarchy() as a 
> > > kfunc, test
> > > + * task's membership of cgroup ancestry.
> > > + * @task: the task to be tested
> > > + * @ancestor: possible ancestor of @task's cgroup
> > > + *
> > > + * Tests whether @task's default cgroup hierarchy is a descendant of 
> > > @ancestor.
> > > + * It follows all the same rules as cgroup_is_descendant, and only 
> > > applies
> > > + * to the default hierarchy.
> > > + */
> > > +__bpf_kfunc long bpf_task_under_cgroup(struct task_struct *task,
> > > +                                      struct cgroup *ancestor)
> > > +{
> > > +       if (unlikely(!ancestor || !task))
> > > +               return -EINVAL;
> > > +
> > > +       return task_under_cgroup_hierarchy(task, ancestor);
> > > +}
> > >   #endif /* CONFIG_CGROUPS */
> >
> > I wonder in what situation a null 'task' or 'ancestor' can be passed.
> > Please call out in the comment that the returned value can be a
> > negative error, so that writing if(bpf_task_under_cgroup()) may cause
> > surprising results.
> >
> > Hao
>
> Hmm, you are right. As kfunc, the NULL value of the parameter is judged, 
> and bpf verify will prompt the developer to add it. There is really no 
> need to add this part of the judgment. See other people's opinions.

Thanks for pointing out Hou.

Currently, bpf_task_under_cgroup() is marked as KF_RCU.

Per documentation:
2.4.7 KF_RCU flag
-----------------

The KF_RCU flag is a weaker version of KF_TRUSTED_ARGS. The kfuncs 
marked with
KF_RCU expect either PTR_TRUSTED or MEM_RCU arguments. The verifier 
guarantees
that the objects are valid and there is no use-after-free. The pointers 
are not
NULL, but the object's refcount could have reached zero. The kfuncs need to
consider doing refcnt != 0 check, especially when returning a KF_ACQUIRE
pointer. Note as well that a KF_ACQUIRE kfunc that is KF_RCU should very 
likely
also be KF_RET_NULL.


The pointer cannot be NULL, so the following line of code can be removed:
>>> +       if (unlikely(!ancestor || !task))
>>> +               return -EINVAL;

I think we do not need to check refcnt != 0 case since ancestor and
task won't go away.

In the example of second patch, both arguments are TRUSTED arguments
which is stronger than RCU, so the test itself is okay.
I am considering whether we should enforce arguments of the kfunc
to be KF_TRUSTED_ARGS, but I think esp. in some cases, cgroup
might be RCU protected e.g., task->cgroup->dfl_cgrp. So leaving argument
requirement as KF_RCU should be better.

>