| 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
| ||
|
Date: Mon, 1 Aug 2022 15:55:43 -0700
From: Hao Luo <haoluo@...gle.com>
To: Andrii Nakryiko <andrii.nakryiko@...il.com>
Cc: linux-kernel@...r.kernel.org, bpf@...r.kernel.org,
cgroups@...r.kernel.org, netdev@...r.kernel.org,
Alexei Starovoitov <ast@...nel.org>,
Andrii Nakryiko <andrii@...nel.org>,
Daniel Borkmann <daniel@...earbox.net>,
Martin KaFai Lau <kafai@...com>,
Song Liu <songliubraving@...com>, Yonghong Song <yhs@...com>,
Tejun Heo <tj@...nel.org>, Zefan Li <lizefan.x@...edance.com>,
KP Singh <kpsingh@...nel.org>,
Johannes Weiner <hannes@...xchg.org>,
Michal Hocko <mhocko@...nel.org>,
Benjamin Tissoires <benjamin.tissoires@...hat.com>,
John Fastabend <john.fastabend@...il.com>,
Michal Koutny <mkoutny@...e.com>,
Roman Gushchin <roman.gushchin@...ux.dev>,
David Rientjes <rientjes@...gle.com>,
Stanislav Fomichev <sdf@...gle.com>,
Shakeel Butt <shakeelb@...gle.com>,
Yosry Ahmed <yosryahmed@...gle.com>
Subject: Re: [PATCH bpf-next v6 5/8] selftests/bpf: Test cgroup_iter.
On Mon, Aug 1, 2022 at 2:51 PM Andrii Nakryiko
<andrii.nakryiko@...il.com> wrote:
>
> On Mon, Aug 1, 2022 at 10:54 AM Hao Luo <haoluo@...gle.com> wrote:
> >
> > Add a selftest for cgroup_iter. The selftest creates a mini cgroup tree
> > of the following structure:
> >
> > ROOT (working cgroup)
> > |
> > PARENT
> > / \
> > CHILD1 CHILD2
> >
> > and tests the following scenarios:
> >
> > - invalid cgroup fd.
> > - pre-order walk over descendants from PARENT.
> > - post-order walk over descendants from PARENT.
> > - walk of ancestors from PARENT.
> > - early termination.
> >
> > Acked-by: Yonghong Song <yhs@...com>
> > Signed-off-by: Hao Luo <haoluo@...gle.com>
> > ---
> > .../selftests/bpf/prog_tests/cgroup_iter.c | 193 ++++++++++++++++++
> > tools/testing/selftests/bpf/progs/bpf_iter.h | 7 +
> > .../testing/selftests/bpf/progs/cgroup_iter.c | 39 ++++
> > 3 files changed, 239 insertions(+)
> > create mode 100644 tools/testing/selftests/bpf/prog_tests/cgroup_iter.c
> > create mode 100644 tools/testing/selftests/bpf/progs/cgroup_iter.c
> >
> > diff --git a/tools/testing/selftests/bpf/prog_tests/cgroup_iter.c b/tools/testing/selftests/bpf/prog_tests/cgroup_iter.c
> > new file mode 100644
> > index 000000000000..5dc843a3f507
> > --- /dev/null
> > +++ b/tools/testing/selftests/bpf/prog_tests/cgroup_iter.c
> > @@ -0,0 +1,193 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/* Copyright (c) 2022 Google */
> > +
> > +#include <test_progs.h>
> > +#include <bpf/libbpf.h>
> > +#include <bpf/btf.h>
> > +#include "cgroup_iter.skel.h"
> > +#include "cgroup_helpers.h"
> > +
> > +#define ROOT 0
> > +#define PARENT 1
> > +#define CHILD1 2
> > +#define CHILD2 3
> > +#define NUM_CGROUPS 4
> > +
> > +#define PROLOGUE "prologue\n"
> > +#define EPILOGUE "epilogue\n"
> > +
> > +#define format_expected_output1(cg_id1) \
> > + snprintf(expected_output, sizeof(expected_output), \
> > + PROLOGUE "%8llu\n" EPILOGUE, (cg_id1))
> > +
> > +#define format_expected_output2(cg_id1, cg_id2) \
> > + snprintf(expected_output, sizeof(expected_output), \
> > + PROLOGUE "%8llu\n%8llu\n" EPILOGUE, \
> > + (cg_id1), (cg_id2))
> > +
> > +#define format_expected_output3(cg_id1, cg_id2, cg_id3) \
> > + snprintf(expected_output, sizeof(expected_output), \
> > + PROLOGUE "%8llu\n%8llu\n%8llu\n" EPILOGUE, \
> > + (cg_id1), (cg_id2), (cg_id3))
> > +
>
> you use format_expected_output{1,2} just once and
> format_expected_output3 twice. Is it worth defining macros for that?
>
If not, we'd see this snprintf and format all over the place. It looks
worse than the current one I think, prefer leave as-is.
> > +const char *cg_path[] = {
> > + "/", "/parent", "/parent/child1", "/parent/child2"
> > +};
> > +
> > +static int cg_fd[] = {-1, -1, -1, -1};
> > +static unsigned long long cg_id[] = {0, 0, 0, 0};
> > +static char expected_output[64];
> > +
> > +int setup_cgroups(void)
> > +{
> > + int fd, i = 0;
> > +
> > + for (i = 0; i < NUM_CGROUPS; i++) {
> > + fd = create_and_get_cgroup(cg_path[i]);
> > + if (fd < 0)
> > + return fd;
> > +
> > + cg_fd[i] = fd;
> > + cg_id[i] = get_cgroup_id(cg_path[i]);
> > + }
> > + return 0;
> > +}
> > +
> > +void cleanup_cgroups(void)
>
> some more statics to cover (same for setup_cgroups)
>
Oops. Will fix.
> > +{
> > + int i;
> > +
> > + for (i = 0; i < NUM_CGROUPS; i++)
> > + close(cg_fd[i]);
> > +}
> > +
> > +static void read_from_cgroup_iter(struct bpf_program *prog, int cgroup_fd,
> > + int order, const char *testname)
> > +{
> > + DECLARE_LIBBPF_OPTS(bpf_iter_attach_opts, opts);
> > + union bpf_iter_link_info linfo;
> > + struct bpf_link *link;
> > + int len, iter_fd;
> > + static char buf[64];
> > +
> > + memset(&linfo, 0, sizeof(linfo));
> > + linfo.cgroup.cgroup_fd = cgroup_fd;
> > + linfo.cgroup.traversal_order = order;
> > + opts.link_info = &linfo;
> > + opts.link_info_len = sizeof(linfo);
> > +
> > + link = bpf_program__attach_iter(prog, &opts);
> > + if (!ASSERT_OK_PTR(link, "attach_iter"))
> > + return;
> > +
> > + iter_fd = bpf_iter_create(bpf_link__fd(link));
> > + if (iter_fd < 0)
> > + goto free_link;
> > +
> > + memset(buf, 0, sizeof(buf));
> > + while ((len = read(iter_fd, buf, sizeof(buf))) > 0)
> > + ;
>
> this is broken, in general, you are overriding buffer content with
> each call to len
>
> I think you intended to advance buf after each read() call (and reduce
> remaining available buf size)?
>
Ah. My bad. Copied from bpf_iter but didn't realize that in the
bpf_iter case, it didn't care about the content read from buffer. Will
fix.
> > +
> > + ASSERT_STREQ(buf, expected_output, testname);
> > +
> > + /* read() after iter finishes should be ok. */
> > + if (len == 0)
> > + ASSERT_OK(read(iter_fd, buf, sizeof(buf)), "second_read");
> > +
> > + close(iter_fd);
> > +free_link:
> > + bpf_link__destroy(link);
> > +}
> > +
> > +/* Invalid cgroup. */
> > +static void test_invalid_cgroup(struct cgroup_iter *skel)
> > +{
> > + DECLARE_LIBBPF_OPTS(bpf_iter_attach_opts, opts);
> > + union bpf_iter_link_info linfo;
> > + struct bpf_link *link;
> > +
> > + memset(&linfo, 0, sizeof(linfo));
> > + linfo.cgroup.cgroup_fd = (__u32)-1;
> > + opts.link_info = &linfo;
> > + opts.link_info_len = sizeof(linfo);
> > +
> > + link = bpf_program__attach_iter(skel->progs.cgroup_id_printer, &opts);
> > + if (!ASSERT_ERR_PTR(link, "attach_iter"))
> > + bpf_link__destroy(link);
>
> nit: you can call bpf_link__destroy() even if link is NULL or IS_ERR
>
Ack. Still need to ASSERT on 'link' though, so the saving is probably
just an indentation. Anyway, will change.
> > +}
> > +
>
> [...]
>
> > diff --git a/tools/testing/selftests/bpf/progs/cgroup_iter.c b/tools/testing/selftests/bpf/progs/cgroup_iter.c
> > new file mode 100644
> > index 000000000000..2a34d146d6df
> > --- /dev/null
> > +++ b/tools/testing/selftests/bpf/progs/cgroup_iter.c
> > @@ -0,0 +1,39 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/* Copyright (c) 2022 Google */
> > +
> > +#include "bpf_iter.h"
> > +#include <bpf/bpf_helpers.h>
> > +#include <bpf/bpf_tracing.h>
> > +
> > +char _license[] SEC("license") = "GPL";
> > +volatile int terminate_early = 0;
> > +volatile u64 terminal_cgroup = 0;
> > +
>
> nit: you shouldn't need volatile for non-const global variables. Did
> you see any problems without volatile?
>
Nah. I don't know about that and see there are other tests that have
this pattern. Will fix.
> > +static inline u64 cgroup_id(struct cgroup *cgrp)
> > +{
> > + return cgrp->kn->id;
> > +}
> > +
>
> [...]
Powered by blists - more mailing lists