netdev - Re: [PATCH bpf-next v3 4/4] selftests/bpf: test BPF_CGROUP_INET_SOCK

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAEf4BzYqQ_mSpadEoj2SD8QM8CSmfaiprERBjub2tGa6NxKvNQ@mail.gmail.com>
Date:   Wed, 1 Jul 2020 14:01:08 -0700
From:   Andrii Nakryiko <andrii.nakryiko@...il.com>
To:     Stanislav Fomichev <sdf@...gle.com>
Cc:     Networking <netdev@...r.kernel.org>, bpf <bpf@...r.kernel.org>,
        "David S. Miller" <davem@...emloft.net>,
        Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>
Subject: Re: [PATCH bpf-next v3 4/4] selftests/bpf: test BPF_CGROUP_INET_SOCK_RELEASE

On Wed, Jul 1, 2020 at 1:14 PM Stanislav Fomichev <sdf@...gle.com> wrote:
>
> Simple test that enforces a single SOCK_DGRAM socker per cgroup.
>
> Signed-off-by: Stanislav Fomichev <sdf@...gle.com>
> ---
>  .../selftests/bpf/prog_tests/udp_limit.c      | 72 +++++++++++++++++++
>  tools/testing/selftests/bpf/progs/udp_limit.c | 42 +++++++++++
>  2 files changed, 114 insertions(+)
>  create mode 100644 tools/testing/selftests/bpf/prog_tests/udp_limit.c
>  create mode 100644 tools/testing/selftests/bpf/progs/udp_limit.c
>

[...]

> +       fd1 = socket(AF_INET, SOCK_DGRAM, 0);
> +       if (CHECK(fd1 < 0, "fd1", "errno %d", errno))
> +               goto close_skeleton;
> +
> +       fd2 = socket(AF_INET, SOCK_DGRAM, 0);
> +       if (CHECK(fd2 >= 0, "fd2", "errno %d", errno))

close(fd2);

> +               goto close_fd1;
> +

[...]

> +close_fd1:
> +       close(fd1);
> +close_skeleton:
> +       udp_limit__destroy(skel);
> +close_cgroup_fd:
> +       close(cgroup_fd);

nit: choosing between close_fd1 and close_skeleton (which also
alternates!) is hard to keep track of. When clean up gets one step
beyond trivial, I usually just initialize variables properly and do
clean up for all exit scenario in one block:

if (fd1 >= 0)
    close(fd1);
udp_limit__destroy(skel);
close(cgroup_fd);

It also makes later extensions simpler.


> +}
> diff --git a/tools/testing/selftests/bpf/progs/udp_limit.c b/tools/testing/selftests/bpf/progs/udp_limit.c
> new file mode 100644
> index 000000000000..af1154bfb946
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/progs/udp_limit.c
> @@ -0,0 +1,42 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +
> +#include <sys/socket.h>
> +#include <linux/bpf.h>
> +#include <bpf/bpf_helpers.h>
> +
> +int invocations, in_use;

uninitialized globals can leads to libbpf refusing to load them (due
to COM section), Daniel just recently had this problem. So better to
always zero-initialize them.

[...]