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

[Yonghong Song <yhs@meta.com>]

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@bytedance.com> 
>> 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.

> 
>