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  PHC 
Open Source and information security mailing list archives
 
Hash Suite for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Date:   Fri, 8 May 2020 11:17:31 -0700
From:   Andrii Nakryiko <andrii.nakryiko@...il.com>
To:     Yonghong Song <yhs@...com>
Cc:     Andrii Nakryiko <andriin@...com>, bpf <bpf@...r.kernel.org>,
        Martin KaFai Lau <kafai@...com>,
        Networking <netdev@...r.kernel.org>,
        Alexei Starovoitov <ast@...com>,
        Daniel Borkmann <daniel@...earbox.net>,
        Kernel Team <kernel-team@...com>
Subject: Re: [PATCH bpf-next v2 18/20] tools/bpf: selftests: add iterator
 programs for ipv6_route and netlink

On Wed, May 6, 2020 at 6:09 PM Yonghong Song <yhs@...com> wrote:
>
>
>
> On 5/5/20 11:01 PM, Andrii Nakryiko wrote:
> > On Sun, May 3, 2020 at 11:30 PM Yonghong Song <yhs@...com> wrote:
> >>
> >> Two bpf programs are added in this patch for netlink and ipv6_route
> >> target. On my VM, I am able to achieve identical
> >> results compared to /proc/net/netlink and /proc/net/ipv6_route.
> >>
> >>    $ cat /proc/net/netlink
> >>    sk               Eth Pid        Groups   Rmem     Wmem     Dump  Locks    Drops    Inode
> >>    000000002c42d58b 0   0          00000000 0        0        0     2        0        7
> >>    00000000a4e8b5e1 0   1          00000551 0        0        0     2        0        18719
> >>    00000000e1b1c195 4   0          00000000 0        0        0     2        0        16422
> >>    000000007e6b29f9 6   0          00000000 0        0        0     2        0        16424
> >>    ....
> >>    00000000159a170d 15  1862       00000002 0        0        0     2        0        1886
> >>    000000009aca4bc9 15  3918224839 00000002 0        0        0     2        0        19076
> >>    00000000d0ab31d2 15  1          00000002 0        0        0     2        0        18683
> >>    000000008398fb08 16  0          00000000 0        0        0     2        0        27
> >>    $ cat /sys/fs/bpf/my_netlink
> >>    sk               Eth Pid        Groups   Rmem     Wmem     Dump  Locks    Drops    Inode
> >>    000000002c42d58b 0   0          00000000 0        0        0     2        0        7
> >>    00000000a4e8b5e1 0   1          00000551 0        0        0     2        0        18719
> >>    00000000e1b1c195 4   0          00000000 0        0        0     2        0        16422
> >>    000000007e6b29f9 6   0          00000000 0        0        0     2        0        16424
> >>    ....
> >>    00000000159a170d 15  1862       00000002 0        0        0     2        0        1886
> >>    000000009aca4bc9 15  3918224839 00000002 0        0        0     2        0        19076
> >>    00000000d0ab31d2 15  1          00000002 0        0        0     2        0        18683
> >>    000000008398fb08 16  0          00000000 0        0        0     2        0        27
> >>
> >>    $ cat /proc/net/ipv6_route
> >>    fe800000000000000000000000000000 40 00000000000000000000000000000000 00 00000000000000000000000000000000 00000100 00000001 00000000 00000001     eth0
> >>    00000000000000000000000000000000 00 00000000000000000000000000000000 00 00000000000000000000000000000000 ffffffff 00000001 00000000 00200200       lo
> >>    00000000000000000000000000000001 80 00000000000000000000000000000000 00 00000000000000000000000000000000 00000000 00000003 00000000 80200001       lo
> >>    fe80000000000000c04b03fffe7827ce 80 00000000000000000000000000000000 00 00000000000000000000000000000000 00000000 00000002 00000000 80200001     eth0
> >>    ff000000000000000000000000000000 08 00000000000000000000000000000000 00 00000000000000000000000000000000 00000100 00000003 00000000 00000001     eth0
> >>    00000000000000000000000000000000 00 00000000000000000000000000000000 00 00000000000000000000000000000000 ffffffff 00000001 00000000 00200200       lo
> >>    $ cat /sys/fs/bpf/my_ipv6_route
> >>    fe800000000000000000000000000000 40 00000000000000000000000000000000 00 00000000000000000000000000000000 00000100 00000001 00000000 00000001     eth0
> >>    00000000000000000000000000000000 00 00000000000000000000000000000000 00 00000000000000000000000000000000 ffffffff 00000001 00000000 00200200       lo
> >>    00000000000000000000000000000001 80 00000000000000000000000000000000 00 00000000000000000000000000000000 00000000 00000003 00000000 80200001       lo
> >>    fe80000000000000c04b03fffe7827ce 80 00000000000000000000000000000000 00 00000000000000000000000000000000 00000000 00000002 00000000 80200001     eth0
> >>    ff000000000000000000000000000000 08 00000000000000000000000000000000 00 00000000000000000000000000000000 00000100 00000003 00000000 00000001     eth0
> >>    00000000000000000000000000000000 00 00000000000000000000000000000000 00 00000000000000000000000000000000 ffffffff 00000001 00000000 00200200       lo
> >>
> >> Signed-off-by: Yonghong Song <yhs@...com>
> >> ---
> >
> > Looks good, but something weird with printf below...
> >
> > Acked-by: Andrii Nakryiko <andriin@...com>
> >
> >>   .../selftests/bpf/progs/bpf_iter_ipv6_route.c | 63 ++++++++++++++++
> >>   .../selftests/bpf/progs/bpf_iter_netlink.c    | 74 +++++++++++++++++++
> >>   2 files changed, 137 insertions(+)
> >>   create mode 100644 tools/testing/selftests/bpf/progs/bpf_iter_ipv6_route.c
> >>   create mode 100644 tools/testing/selftests/bpf/progs/bpf_iter_netlink.c
> >>
> >> diff --git a/tools/testing/selftests/bpf/progs/bpf_iter_ipv6_route.c b/tools/testing/selftests/bpf/progs/bpf_iter_ipv6_route.c
> >> new file mode 100644
> >> index 000000000000..0dee4629298f
> >> --- /dev/null
> >> +++ b/tools/testing/selftests/bpf/progs/bpf_iter_ipv6_route.c
> >> @@ -0,0 +1,63 @@
> >> +// SPDX-License-Identifier: GPL-2.0
> >> +/* Copyright (c) 2020 Facebook */
> >> +#include "vmlinux.h"
> >> +#include <bpf/bpf_helpers.h>
> >> +#include <bpf/bpf_tracing.h>
> >> +#include <bpf/bpf_endian.h>
> >> +
> >> +char _license[] SEC("license") = "GPL";
> >> +
> >> +extern bool CONFIG_IPV6_SUBTREES __kconfig __weak;
> >> +
> >> +#define        RTF_GATEWAY             0x0002
> >> +#define IFNAMSIZ               16
> >
> > nit: these look weirdly unaligned :)
> >
> >> +#define fib_nh_gw_family        nh_common.nhc_gw_family
> >> +#define fib_nh_gw6              nh_common.nhc_gw.ipv6
> >> +#define fib_nh_dev              nh_common.nhc_dev
> >> +
> >
> > [...]
> >
> >
> >> +       dev = fib6_nh->fib_nh_dev;
> >> +       if (dev)
> >> +               BPF_SEQ_PRINTF(seq, "%08x %08x %08x %08x %8s\n", rt->fib6_metric,
> >> +                              rt->fib6_ref.refs.counter, 0, flags, dev->name);
> >> +       else
> >> +               BPF_SEQ_PRINTF(seq, "%08x %08x %08x %08x %8s\n", rt->fib6_metric,
> >> +                              rt->fib6_ref.refs.counter, 0, flags);
> >
> > hmm... how does it work? you specify 4 params, but format string
> > expects 5. Shouldn't this fail?
>
> Thanks for catching this. Unfortunately, we can only detech this at
> runtime when BPF_SEQ_PRINTF is executed since only then we do
> format/argument checking.
>
> In the above, if I flip condition "if (dev)" to "if (!dev)", the
> BPF_SEQ_PRRINTF will not print anything and returns -EINVAL.
>
> I am wondering whether verifier should do some verification at prog load
> time to ensure
>    # of args in packed u64 array >= # of format specifier
> This should capture this case. Or we just assume users should do
> adequate testing to capture such cases.
>

My initial thought is that it would be too specific knowledge for
verifier, but maybe as we add more generic logging/printf
capabilities, it might come in handy. But I'd defer for later on.

> Note that this won't affect safety of the program so it is totally
> okay for verifier to delay the checking to runtime.
>
> >
> >> +
> >> +       return 0;
> >> +}
> >> diff --git a/tools/testing/selftests/bpf/progs/bpf_iter_netlink.c b/tools/testing/selftests/bpf/progs/bpf_iter_netlink.c
> >> new file mode 100644
> >> index 000000000000..0a85a621a36d
> >> --- /dev/null
> >> +++ b/tools/testing/selftests/bpf/progs/bpf_iter_netlink.c
> >> @@ -0,0 +1,74 @@
> >> +// SPDX-License-Identifier: GPL-2.0
> >> +/* Copyright (c) 2020 Facebook */
> >> +#include "vmlinux.h"
> >> +#include <bpf/bpf_helpers.h>
> >> +#include <bpf/bpf_tracing.h>
> >> +#include <bpf/bpf_endian.h>
> >> +
> >> +char _license[] SEC("license") = "GPL";
> >> +
> >> +#define sk_rmem_alloc  sk_backlog.rmem_alloc
> >> +#define sk_refcnt      __sk_common.skc_refcnt
> >> +
> >> +#define offsetof(TYPE, MEMBER)  ((size_t)&((TYPE *)0)->MEMBER)
> >> +#define container_of(ptr, type, member)                                \
> >> +       ({                                                      \
> >> +               void *__mptr = (void *)(ptr);                   \
> >> +               ((type *)(__mptr - offsetof(type, member)));    \
> >> +       })
> >
> > we should probably put offsetof(), offsetofend() and container_of()
> > macro into bpf_helpers.h, seems like universal things for kernel
> > datastructs :)
> >
> > [...]
> >

Powered by blists - more mailing lists