On 01/21, Andrii Nakryiko wrote:

On Wed, Jan 20, 2021 at 7:16 PM Stanislav Fomichev [off-list ref] wrote:

quoted

BPF rewrites from 111 to 111, but it still should mark the port as
"changed".
We also verify that if port isn't touched by BPF, it's still prohibited.

Signed-off-by: Stanislav Fomichev <redacted>
---
 .../selftests/bpf/prog_tests/bind_perm.c      | 88 +++++++++++++++++++
 tools/testing/selftests/bpf/progs/bind_perm.c | 36 ++++++++
 2 files changed, 124 insertions(+)
 create mode 100644 tools/testing/selftests/bpf/prog_tests/bind_perm.c
 create mode 100644 tools/testing/selftests/bpf/progs/bind_perm.c

diff --git a/tools/testing/selftests/bpf/prog_tests/bind_perm.c  

b/tools/testing/selftests/bpf/prog_tests/bind_perm.c

quoted

new file mode 100644
index 000000000000..840a04ac9042

--- /dev/null
+++ b/tools/testing/selftests/bpf/prog_tests/bind_perm.c

@@ -0,0 +1,88 @@
+// SPDX-License-Identifier: GPL-2.0
+#include <test_progs.h>
+#include "bind_perm.skel.h"
+
+#include <sys/types.h>
+#include <sys/socket.h>
+#include <sys/capability.h>
+
+static int duration;
+
+void try_bind(int port, int expected_errno)
+{
+       struct sockaddr_in sin = {};
+       int fd = -1;
+
+       fd = socket(AF_INET, SOCK_STREAM, 0);
+       if (CHECK(fd < 0, "fd", "errno %d", errno))
+               goto close_socket;
+
+       sin.sin_family = AF_INET;
+       sin.sin_port = htons(port);
+
+       errno = 0;
+       bind(fd, (struct sockaddr *)&sin, sizeof(sin));
+       CHECK(errno != expected_errno, "bind", "errno %d, expected %d",
+             errno, expected_errno);

ASSERT_NEQ() is nicer

Nice, didn't know these existed. Now we need ASSERT_GT/LE/GE/LE to also
get rid of those other CHECKs :-)

quoted

+
+close_socket:
+       if (fd >= 0)
+               close(fd);
+}
+
+void cap_net_bind_service(cap_flag_value_t flag)
+{
+       const cap_value_t cap_net_bind_service = CAP_NET_BIND_SERVICE;
+       cap_t caps;
+
+       caps = cap_get_proc();
+       if (CHECK(!caps, "cap_get_proc", "errno %d", errno))
+               goto free_caps;
+
+       if (CHECK(cap_set_flag(caps, CAP_EFFECTIVE, 1,  

&cap_net_bind_service,

quoted

+                              CAP_CLEAR),
+                 "cap_set_flag", "errno %d", errno))
+               goto free_caps;
+
+       if (CHECK(cap_set_flag(caps, CAP_EFFECTIVE, 1,  

&cap_net_bind_service,

quoted

+                              CAP_CLEAR),
+                 "cap_set_flag", "errno %d", errno))
+               goto free_caps;
+
+       if (CHECK(cap_set_proc(caps), "cap_set_proc", "errno %d",  

errno))

quoted

+               goto free_caps;
+
+free_caps:
+       if (CHECK(cap_free(caps), "cap_free", "errno %d", errno))
+               goto free_caps;
+}
+
+void test_bind_perm(void)
+{
+       struct bind_perm *skel;
+       int cgroup_fd;
+
+       cgroup_fd = test__join_cgroup("/bind_perm");
+       if (CHECK(cgroup_fd < 0, "cg-join", "errno %d", errno))
+               return;
+
+       skel = bind_perm__open_and_load();
+       if (CHECK(!skel, "skel-load", "errno %d", errno))
+               goto close_cgroup_fd;

errno is irrelevant; also use ASSERT_PTR_OK() instead

Ack, it might be worth unconditionally printing it in your ASSERT_XXX
macros. Worst case - it's not used, but in general case avoids
all this "errno %d" boilerplate.

quoted

+
+       skel->links.bind_v4_prog =  

bpf_program__attach_cgroup(skel->progs.bind_v4_prog, cgroup_fd);

quoted

+       if (CHECK(IS_ERR(skel->links.bind_v4_prog),
+                 "cg-attach", "bind4 %ld",
+                 PTR_ERR(skel->links.bind_v4_prog)))

try using ASSERT_PTR_OK instead

Sure, thanks!