linux-kernel - Re: [PATCH bpf-next v6 1/2] bpf: Add bpf_task_under

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 5 May 2023 11:43:49 -0700
From:   Yonghong Song <yhs@...a.com>
To:     Feng Zhou <zhoufeng.zf@...edance.com>, Hao Luo <haoluo@...gle.com>
Cc:     martin.lau@...ux.dev, ast@...nel.org, daniel@...earbox.net,
        andrii@...nel.org, song@...nel.org, yhs@...com,
        john.fastabend@...il.com, kpsingh@...nel.org, sdf@...gle.com,
        jolsa@...nel.org, davem@...emloft.net, edumazet@...gle.com,
        kuba@...nel.org, pabeni@...hat.com, mykolal@...com,
        shuah@...nel.org, bpf@...r.kernel.org,
        linux-kernel@...r.kernel.org, netdev@...r.kernel.org,
        linux-kselftest@...r.kernel.org, yangzhenze@...edance.com,
        wangdongdong.6@...edance.com
Subject: Re: [PATCH bpf-next v6 1/2] bpf: Add bpf_task_under_cgroup() kfunc



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@...edance.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.

> 
>