linux-kernel - Re: [PATCH] selftests/bpf: simplify cgroup_hierarchical

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]

Message-ID: <CAEf4BzaH7xgoDfKstCmQzVY5HJpE8Hn8WFfyUU7PH64QpQcwsg@mail.gmail.com>
Date:   Fri, 9 Sep 2022 17:49:49 -0700
From:   Andrii Nakryiko <andrii.nakryiko@...il.com>
To:     Yosry Ahmed <yosryahmed@...gle.com>
Cc:     Hao Luo <haoluo@...gle.com>, Andrii Nakryiko <andrii@...nel.org>,
        Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        Martin KaFai Lau <martin.lau@...ux.dev>,
        Mykola Lysenko <mykolal@...com>, 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>, bpf <bpf@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] selftests/bpf: simplify cgroup_hierarchical_stats selftest

On Tue, Sep 6, 2022 at 2:35 PM Yosry Ahmed <yosryahmed@...gle.com> wrote:
>
> On Mon, Aug 29, 2022 at 6:50 PM Yosry Ahmed <yosryahmed@...gle.com> wrote:
> >
> > On Mon, Aug 29, 2022 at 6:42 PM Hao Luo <haoluo@...gle.com> wrote:
> > >
> > > On Mon, Aug 29, 2022 at 6:07 PM Yosry Ahmed <yosryahmed@...gle.com> wrote:
> > > >
> > > > On Mon, Aug 29, 2022 at 3:15 PM Yosry Ahmed <yosryahmed@...gle.com> wrote:
> > > > >
> > > > > On Mon, Aug 29, 2022 at 1:08 PM Hao Luo <haoluo@...gle.com> wrote:
> > > > > >
> > > > > > On Fri, Aug 26, 2022 at 4:06 PM Yosry Ahmed <yosryahmed@...gle.com> wrote:
> > > > > > >
> > > [...]
> > > > > > >
> > > > > > > -SEC("tp_btf/mm_vmscan_memcg_reclaim_begin")
> > > > > > > -int BPF_PROG(vmscan_start, int order, gfp_t gfp_flags)
> > > > > > > +SEC("fentry/cgroup_attach_task")
> > > > > >
> > > > > > Can we select an attachpoint that is more stable? It seems
> > > > > > 'cgroup_attach_task' is an internal helper function in cgroup, and its
> > > > > > signature can change. I'd prefer using those commonly used tracepoints
> > > > > > and EXPORT'ed functions. IMHO their interfaces are more stable.
> > > > > >
> > > > >
> > > > > Will try to find a more stable attach point. Thanks!
> > > >
> > > > Hey Hao,
> > > >
> > > > I couldn't find any suitable stable attach points under kernel/cgroup.
> > > > Most tracepoints are created using TRACE_CGROUP_PATH which only
> > > > invokes the tracepoint if the trace event is enabled, which I assume
> > > > is not something we can rely on. Otherwise, there is only
> > >
> > > Can we explicitly enable the cgroup_attach_task event, just for this
> > > test? If it's not easy, I am fine with using fentry.
> >
> > I see a couple of tests that read from /sys/kernel/debug/tracing, but
> > they are mostly reading event ids, I don't see any tests enabling or
> > disabling a tracing event, so I am not sure if that's an accepted
> > pattern. Also I am not sure if we can rely on tracefs being in that
> > path. Andrii, is this considered acceptable?
> >
>
> Anyone with thoughts here? Is it acceptable to explicitly enable a
> trace event in a BPF selftest to attach to a tracepoint that is only
> invoked if the trace event is enabled (e.g. cgroup_attach_task) ?
> Otherwise the test program would attach to the fentry of an internal
> function, which is more vulnerable to being changed and breaking the
> test (until someone updates the test with the new signature).
>

IMO it's fine to use fentry. If something changes about signature,
we'll detect it soon enough and adjust selftests.

Messing with global tracefs in selftests is less desirable. It will
also potentially force tests to be sequential.

> > >
> > > > trace_cgroup_setup_root() and trace_cgroup_destroy_root() which are
> > > > irrelevant here. A lot of EXPORT'ed functions are not called in the
> > > > kernel, or cannot be invoked from userspace (the test) in a
> > > > straightforward way. Even if they did, future changes to such code
> > > > paths can also change in the future, so I don't think there is really
> > > > a way to guarantee that future changes don't break the test.
> > > >
> > > > Let me know what you think.
> > > >