netdev - Re: [PATCH bpf-next 2/2] selftests/bpf: Adding test for arg dereference in extension trace

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, 11 Sep 2020 15:02:32 +0200
From:   Jiri Olsa <jolsa@...hat.com>
To:     Andrii Nakryiko <andrii.nakryiko@...il.com>
Cc:     Jiri Olsa <jolsa@...nel.org>, Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        Networking <netdev@...r.kernel.org>, bpf <bpf@...r.kernel.org>,
        Martin KaFai Lau <kafai@...com>,
        Song Liu <songliubraving@...com>, Yonghong Song <yhs@...com>,
        Andrii Nakryiko <andriin@...com>,
        John Fastabend <john.fastabend@...il.com>,
        KP Singh <kpsingh@...omium.org>,
        Jesper Dangaard Brouer <brouer@...hat.com>,
        Eelco Chaudron <echaudro@...hat.com>,
        Toke Høiland-Jørgensen <toke@...hat.com>
Subject: Re: [PATCH bpf-next 2/2] selftests/bpf: Adding test for arg
 dereference in extension trace

On Thu, Sep 10, 2020 at 03:34:26PM -0700, Andrii Nakryiko wrote:

SNIP

> > +
> > +void test_trace_ext(void)
> > +{
> > +       struct test_trace_ext_tracing *skel_trace = NULL;
> > +       struct test_trace_ext_tracing__bss *bss_trace;
> > +       const char *file = "./test_pkt_md_access.o";
> > +       struct test_trace_ext *skel_ext = NULL;
> > +       struct test_trace_ext__bss *bss_ext;
> > +       int err, prog_fd, ext_fd;
> > +       struct bpf_object *obj;
> > +       char buf[100];
> > +       __u32 retval;
> > +       __u64 len;
> > +
> > +       err = bpf_prog_load(file, BPF_PROG_TYPE_SCHED_CLS, &obj, &prog_fd);
> > +       if (CHECK_FAIL(err))
> > +               return;
> 
> We should avoid using bpf_prog_load() for new code. Can you please
> just skeleton instead? Or at least bpf_object__open_file()?

ok

> 
> > +
> > +       DECLARE_LIBBPF_OPTS(bpf_object_open_opts, opts,
> > +                           .attach_prog_fd = prog_fd,
> > +       );
> 
> DECLARE_LIBBPF_OPTS does declare a variable, so should be together
> with all the other variables above, otherwise some overly strict C89
> mode compiler will start complaining. You can assign
> `opts.attach_prog_fd = prog_fd;` outside of declaration. But I also
> don't think you need this one. Having .attach_prog_fd in open_opts is
> not great, because it's a per-program setting specified at bpf_object
> level. Would bpf_program__set_attach_target() work here?

right, I'll try it, it should be enough

SNIP

> > +
> > +cleanup:
> > +       test_trace_ext__destroy(skel_ext);
> > +       bpf_object__close(obj);
> > +}
> > diff --git a/tools/testing/selftests/bpf/progs/test_trace_ext.c b/tools/testing/selftests/bpf/progs/test_trace_ext.c
> > new file mode 100644
> > index 000000000000..a6318f6b52ee
> > --- /dev/null
> > +++ b/tools/testing/selftests/bpf/progs/test_trace_ext.c
> > @@ -0,0 +1,18 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +// Copyright (c) 2019 Facebook
> > +#include <linux/bpf.h>
> > +#include <stdbool.h>
> > +#include <bpf/bpf_helpers.h>
> > +#include <bpf/bpf_endian.h>
> > +#include <bpf/bpf_tracing.h>
> > +
> > +volatile __u64 ext_called = 0;
> 
> nit: no need for volatile, global variables are not going anywhere;
> same below in two places

ok, thanks

jirka