Re: [PATCH bpf-next 4/4] selftests/bpf: test BPF_CGROUP_INET_SOCK_RELEASE

On Fri, Jun 26, 2020 at 4:52 PM Stanislav Fomichev [off-list ref] wrote:

On Fri, Jun 26, 2020 at 3:06 PM Andrii Nakryiko
[off-list ref] wrote:

quoted

On Thu, Jun 25, 2020 at 5:13 PM Stanislav Fomichev [off-list ref] wrote:

quoted

Simple test that enforces a single SOCK_DGRAM socker per cgroup.

Signed-off-by: Stanislav Fomichev <redacted>
---
 .../selftests/bpf/prog_tests/udp_limit.c      | 71 +++++++++++++++++++
 tools/testing/selftests/bpf/progs/udp_limit.c | 42 +++++++++++
 2 files changed, 113 insertions(+)
 create mode 100644 tools/testing/selftests/bpf/prog_tests/udp_limit.c
 create mode 100644 tools/testing/selftests/bpf/progs/udp_limit.c

diff --git a/tools/testing/selftests/bpf/prog_tests/udp_limit.c b/tools/testing/selftests/bpf/prog_tests/udp_limit.c
new file mode 100644
index 000000000000..fe359a927d92
--- /dev/null
+++ b/tools/testing/selftests/bpf/prog_tests/udp_limit.c

@@ -0,0 +1,71 @@
+// SPDX-License-Identifier: GPL-2.0
+#include <test_progs.h>
+#include "udp_limit.skel.h"
+
+#include <sys/types.h>
+#include <sys/socket.h>
+
+void test_udp_limit(void)
+{
+       struct udp_limit *skel;
+       int cgroup_fd;
+       int fd1, fd2;
+       int err;
+
+       cgroup_fd = test__join_cgroup("/udp_limit");
+       if (CHECK_FAIL(cgroup_fd < 0))
+               return;
+
+       skel = udp_limit__open_and_load();
+       if (CHECK_FAIL(!skel))
+               goto close_cgroup_fd;
+
+       err = bpf_prog_attach(bpf_program__fd(skel->progs.sock),
+                             cgroup_fd, BPF_CGROUP_INET_SOCK_CREATE, 0);
+       if (CHECK_FAIL(err))
+               goto close_skeleton;
+
+       err = bpf_prog_attach(bpf_program__fd(skel->progs.sock_release),
+                             cgroup_fd, BPF_CGROUP_INET_SOCK_RELEASE, 0);
+       if (CHECK_FAIL(err))
+               goto close_skeleton;

Have you tried:

skel->links.sock = bpf_program__attach_cgroup(skel->progs.sock);

and similarly for sock_release?

Ack, I can try that, thanks!

quoted

quoted

+       /* BPF program enforces a single UDP socket per cgroup,
+        * verify that.
+        */
+       fd1 = socket(AF_INET, SOCK_DGRAM, 0);
+       if (CHECK_FAIL(fd1 < 0))
+               goto close_skeleton;
+
+       fd2 = socket(AF_INET, SOCK_DGRAM, 0);
+       if (CHECK_FAIL(fd2 != -1))
+               goto close_fd1;
+
+       /* We can reopen again after close. */
+       close(fd1);
+
+       fd1 = socket(AF_INET, SOCK_DGRAM, 0);
+       if (CHECK_FAIL(fd1 < 0))
+               goto close_skeleton;
+
+       /* Make sure the program was invoked the expected
+        * number of times:
+        * - open fd1           - BPF_CGROUP_INET_SOCK_CREATE
+        * - attempt to openfd2 - BPF_CGROUP_INET_SOCK_CREATE
+        * - close fd1          - BPF_CGROUP_INET_SOCK_RELEASE
+        * - open fd1 again     - BPF_CGROUP_INET_SOCK_CREATE
+        */
+       if (CHECK_FAIL(skel->bss->invocations != 4))
+               goto close_fd1;
+
+       /* We should still have a single socket in use */
+       if (CHECK_FAIL(skel->bss->in_use != 1))
+               goto close_fd1;

Please use a non-silent CHECK() macro for everything that's a proper
and not a high-frequency check. That generates "a log trail" when
running the test in verbose mode, so it's easier to pinpoint where the
failure happened.

IIRC, the problem with CHECK() is that it requires a 'duration'
argument to be defined.
Do you suggest defining it somewhere just to make CHECK() happy?

Yes, that's what most tests are doing. Just `static int duration;` on
top of test file, and you can forget about it.

quoted

quoted

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

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..98fe294d9c21
--- /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;
+
+SEC("cgroup/sock")
+int sock(struct bpf_sock *ctx)
+{
+       __u32 key;
+
+       if (ctx->type != SOCK_DGRAM)
+               return 1;
+
+       __sync_fetch_and_add(&invocations, 1);
+
+       if (&in_use > 0) {

&in_use is supposed to return an address of a variable... this looks
weird and probably not what you wanted?

Oh, good catch! I was about to ask myself "how did the test pass with
that?", but the test fails as well :-/
Not sure how it creeped in and how I ran my tests, sorry about that.

Yeah, I was wondering that myself :) but was too lazy to check the exact logic.