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] [day] [month] [year] [list]
Date:   Fri, 5 May 2023 11:48:38 -0700
From:   Alexei Starovoitov <alexei.starovoitov@...il.com>
To:     Yonghong Song <yhs@...a.com>
Cc:     Feng Zhou <zhoufeng.zf@...edance.com>, Hao Luo <haoluo@...gle.com>,
        Martin KaFai Lau <martin.lau@...ux.dev>,
        Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        Andrii Nakryiko <andrii@...nel.org>,
        Song Liu <song@...nel.org>, Yonghong Song <yhs@...com>,
        John Fastabend <john.fastabend@...il.com>,
        KP Singh <kpsingh@...nel.org>,
        Stanislav Fomichev <sdf@...gle.com>,
        Jiri Olsa <jolsa@...nel.org>,
        "David S. Miller" <davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>,
        Mykola Lysenko <mykolal@...com>, Shuah Khan <shuah@...nel.org>,
        bpf <bpf@...r.kernel.org>, LKML <linux-kernel@...r.kernel.org>,
        Network Development <netdev@...r.kernel.org>,
        "open list:KERNEL SELFTEST FRAMEWORK" 
        <linux-kselftest@...r.kernel.org>, yangzhenze@...edance.com,
        Dongdong Wang <wangdongdong.6@...edance.com>
Subject: Re: [PATCH bpf-next v6 1/2] bpf: Add bpf_task_under_cgroup() kfunc

On Fri, May 5, 2023 at 11:44 AM Yonghong Song <yhs@...a.com> wrote:
>
>
>
> 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;

Right. With KF_RCU the verifier guarantees != NULL.
Let's get rid of this check.
This will make the return value clean.

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

correct.

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

+1

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ