linux-kernel - Re: [PATCH v6 bpf-next 4/6] selftests/bpf: add bpf_snprintf

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: <CAEf4BzZxWC2cO9dmZczWWCQgGH6TLLjmDSiO_LrMzSu7Es5ZSw@mail.gmail.com>
Date:   Fri, 25 Sep 2020 10:36:44 -0700
From:   Andrii Nakryiko <andrii.nakryiko@...il.com>
To:     Alexei Starovoitov <alexei.starovoitov@...il.com>
Cc:     Alan Maguire <alan.maguire@...cle.com>,
        Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        Andrii Nakryiko <andriin@...com>, Yonghong Song <yhs@...com>,
        Rasmus Villemoes <linux@...musvillemoes.dk>,
        andriy.shevchenko@...ux.intel.com, Petr Mladek <pmladek@...e.com>,
        Martin Lau <kafai@...com>, Song Liu <songliubraving@...com>,
        john fastabend <john.fastabend@...il.com>,
        KP Singh <kpsingh@...omium.org>, Shuah Khan <shuah@...nel.org>,
        Andrey Ignatov <rdna@...com>, scott.branden@...adcom.com,
        Quentin Monnet <quentin@...valent.com>,
        Carlos Neira <cneirabustos@...il.com>,
        Jakub Sitnicki <jakub@...udflare.com>,
        Ingo Molnar <mingo@...hat.com>,
        Steven Rostedt <rostedt@...dmis.org>,
        bpf <bpf@...r.kernel.org>, Networking <netdev@...r.kernel.org>,
        open list <linux-kernel@...r.kernel.org>,
        "open list:KERNEL SELFTEST FRAMEWORK" 
        <linux-kselftest@...r.kernel.org>,
        Arnaldo Carvalho de Melo <acme@...nel.org>
Subject: Re: [PATCH v6 bpf-next 4/6] selftests/bpf: add bpf_snprintf_btf
 helper tests

On Thu, Sep 24, 2020 at 5:51 PM Alexei Starovoitov
<alexei.starovoitov@...il.com> wrote:
>
> On Wed, Sep 23, 2020 at 06:46:26PM +0100, Alan Maguire wrote:
> > +static int __strncmp(const void *m1, const void *m2, size_t len)
> > +{
> > +     const unsigned char *s1 = m1;
> > +     const unsigned char *s2 = m2;
> > +     int i, delta = 0;
> > +
> > +#pragma clang loop unroll(full)
>
> Shouldn't be needed?
> The verifier supports bounded loops.
>
> > +     for (i = 0; i < len; i++) {
> > +             delta = s1[i] - s2[i];
> > +             if (delta || s1[i] == 0 || s2[i] == 0)
> > +                     break;
> > +     }
> > +     return delta;
> > +}
> > +
> > +/* Use __builtin_btf_type_id to test snprintf_btf by type id instead of name */
> > +#if __has_builtin(__builtin_btf_type_id)
> > +#define TEST_BTF_BY_ID(_str, _typestr, _ptr, _hflags)                        \
> > +     do {                                                            \
> > +             int _expected_ret = ret;                                \
> > +             _ptr.type = 0;                                          \
> > +             _ptr.type_id = __builtin_btf_type_id(_typestr, 0);      \
>
> The test is passing for me, but I don't understand why :)
> __builtin_btf_type_id(, 0); means btf_id of the bpf program.
> While bpf_snprintf_btf() is treating it as btf_id of vmlinux_btf.
> So it really should have been __builtin_btf_type_id(,1);

Better still to use bpf_core_type_id_kernel() macro from bpf_core_read.h.

>
> The following diff works:
> diff --git a/tools/testing/selftests/bpf/progs/netif_receive_skb.c b/tools/testing/selftests/bpf/progs/netif_receive_skb.c
> index b4f96f1f6830..bffa786e3b03 100644
> --- a/tools/testing/selftests/bpf/progs/netif_receive_skb.c
> +++ b/tools/testing/selftests/bpf/progs/netif_receive_skb.c
> @@ -45,7 +45,7 @@ static int __strncmp(const void *m1, const void *m2, size_t len)
>         do {                                                            \
>                 int _expected_ret = ret;                                \
>                 _ptr.type = 0;                                          \
> -               _ptr.type_id = __builtin_btf_type_id(_typestr, 0);      \
> +               _ptr.type_id = __builtin_btf_type_id(_typestr, 1);      \
>                 ret = bpf_snprintf_btf(_str, STRSIZE, &_ptr,            \
>                                        sizeof(_ptr), _hflags);          \
>                 if (ret != _expected_ret) {                             \
> @@ -88,7 +88,7 @@ static int __strncmp(const void *m1, const void *m2, size_t len)
>                         ret = -EBADMSG;                                 \
>                         break;                                          \
>                 }                                                       \
> -               TEST_BTF_BY_ID(_str, #_type, _ptr, _hflags);            \
> +               TEST_BTF_BY_ID(_str, _ptr, _ptr, _hflags);              \
>
> But still makes me suspicious of the test. I haven't debugged further.