* [PATCH bpf-next 1/4] bpf: add BPF_CGROUP_INET_SOCK_RELEASE hook
@ 2020-06-26 0:09 Stanislav Fomichev
2020-06-26 0:09 ` [PATCH bpf-next 2/4] libbpf: add support for BPF_CGROUP_INET_SOCK_RELEASE Stanislav Fomichev
` (4 more replies)
0 siblings, 5 replies; 11+ messages in thread
From: Stanislav Fomichev @ 2020-06-26 0:09 UTC (permalink / raw)
To: netdev, bpf; +Cc: davem, ast, daniel, Stanislav Fomichev
Sometimes it's handy to know when the socket gets freed.
In particular, we'd like to try to use a smarter allocation
of ports for bpf_bind and explore the possibility of
limiting the number of SOCK_DGRAM sockets the process can have.
Adding a release pair to existing BPF_CGROUP_INET_SOCK_CREATE
can unlock both of the mentioned features.
The only questionable part here is the sock->sk check
in the inet_release. Looking at the places where we
do 'sock->sk = NULL', I don't understand how it can race
with inet_release and why the check is there (it's been
there since the initial git import). Otherwise, the
change itself is pretty simple, we add a BPF hook
to the inet_release and avoid calling it for kernel
sockets.
Signed-off-by: Stanislav Fomichev <sdf@google.com>
---
include/linux/bpf-cgroup.h | 3 +++
include/uapi/linux/bpf.h | 1 +
kernel/bpf/syscall.c | 3 +++
net/core/filter.c | 1 +
net/ipv4/af_inet.c | 3 +++
5 files changed, 11 insertions(+)
diff --git a/include/linux/bpf-cgroup.h b/include/linux/bpf-cgroup.h
index c66c545e161a..b4fd09fe67bd 100644
--- a/include/linux/bpf-cgroup.h
+++ b/include/linux/bpf-cgroup.h
@@ -210,6 +210,9 @@ int bpf_percpu_cgroup_storage_update(struct bpf_map *map, void *key,
#define BPF_CGROUP_RUN_PROG_INET_SOCK(sk) \
BPF_CGROUP_RUN_SK_PROG(sk, BPF_CGROUP_INET_SOCK_CREATE)
+#define BPF_CGROUP_RUN_PROG_INET_SOCK_RELEASE(sk) \
+ BPF_CGROUP_RUN_SK_PROG(sk, BPF_CGROUP_INET_SOCK_RELEASE)
+
#define BPF_CGROUP_RUN_PROG_INET4_POST_BIND(sk) \
BPF_CGROUP_RUN_SK_PROG(sk, BPF_CGROUP_INET4_POST_BIND)
diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
index c65b374a5090..d7aea1d0167a 100644
--- a/include/uapi/linux/bpf.h
+++ b/include/uapi/linux/bpf.h
@@ -226,6 +226,7 @@ enum bpf_attach_type {
BPF_CGROUP_INET4_GETSOCKNAME,
BPF_CGROUP_INET6_GETSOCKNAME,
BPF_XDP_DEVMAP,
+ BPF_CGROUP_INET_SOCK_RELEASE,
__MAX_BPF_ATTACH_TYPE
};
diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
index 4d530b1d5683..2a3d4b8f95c7 100644
--- a/kernel/bpf/syscall.c
+++ b/kernel/bpf/syscall.c
@@ -1994,6 +1994,7 @@ bpf_prog_load_check_attach(enum bpf_prog_type prog_type,
case BPF_PROG_TYPE_CGROUP_SOCK:
switch (expected_attach_type) {
case BPF_CGROUP_INET_SOCK_CREATE:
+ case BPF_CGROUP_INET_SOCK_RELEASE:
case BPF_CGROUP_INET4_POST_BIND:
case BPF_CGROUP_INET6_POST_BIND:
return 0;
@@ -2792,6 +2793,7 @@ attach_type_to_prog_type(enum bpf_attach_type attach_type)
return BPF_PROG_TYPE_CGROUP_SKB;
break;
case BPF_CGROUP_INET_SOCK_CREATE:
+ case BPF_CGROUP_INET_SOCK_RELEASE:
case BPF_CGROUP_INET4_POST_BIND:
case BPF_CGROUP_INET6_POST_BIND:
return BPF_PROG_TYPE_CGROUP_SOCK;
@@ -2942,6 +2944,7 @@ static int bpf_prog_query(const union bpf_attr *attr,
case BPF_CGROUP_INET_INGRESS:
case BPF_CGROUP_INET_EGRESS:
case BPF_CGROUP_INET_SOCK_CREATE:
+ case BPF_CGROUP_INET_SOCK_RELEASE:
case BPF_CGROUP_INET4_BIND:
case BPF_CGROUP_INET6_BIND:
case BPF_CGROUP_INET4_POST_BIND:
diff --git a/net/core/filter.c b/net/core/filter.c
index 209482a4eaa2..7bcac182383c 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -6855,6 +6855,7 @@ static bool __sock_filter_check_attach_type(int off,
case offsetof(struct bpf_sock, priority):
switch (attach_type) {
case BPF_CGROUP_INET_SOCK_CREATE:
+ case BPF_CGROUP_INET_SOCK_RELEASE:
goto full_access;
default:
return false;
diff --git a/net/ipv4/af_inet.c b/net/ipv4/af_inet.c
index 02aa5cb3a4fd..965a96ea1168 100644
--- a/net/ipv4/af_inet.c
+++ b/net/ipv4/af_inet.c
@@ -411,6 +411,9 @@ int inet_release(struct socket *sock)
if (sk) {
long timeout;
+ if (!sk->sk_kern_sock)
+ BPF_CGROUP_RUN_PROG_INET_SOCK_RELEASE(sk);
+
/* Applications forget to leave groups before exiting */
ip_mc_drop_socket(sk);
--
2.27.0.111.gc72c7da667-goog
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH bpf-next 2/4] libbpf: add support for BPF_CGROUP_INET_SOCK_RELEASE
2020-06-26 0:09 [PATCH bpf-next 1/4] bpf: add BPF_CGROUP_INET_SOCK_RELEASE hook Stanislav Fomichev
@ 2020-06-26 0:09 ` Stanislav Fomichev
2020-06-26 22:02 ` Andrii Nakryiko
2020-06-26 0:09 ` [PATCH bpf-next 3/4] bpftool: support BPF_CGROUP_INET_SOCK_RELEASE Stanislav Fomichev
` (3 subsequent siblings)
4 siblings, 1 reply; 11+ messages in thread
From: Stanislav Fomichev @ 2020-06-26 0:09 UTC (permalink / raw)
To: netdev, bpf; +Cc: davem, ast, daniel, Stanislav Fomichev
Add auto-detection for the cgroup/sock_release programs.
Signed-off-by: Stanislav Fomichev <sdf@google.com>
---
tools/include/uapi/linux/bpf.h | 1 +
tools/lib/bpf/libbpf.c | 2 ++
2 files changed, 3 insertions(+)
diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h
index c65b374a5090..d7aea1d0167a 100644
--- a/tools/include/uapi/linux/bpf.h
+++ b/tools/include/uapi/linux/bpf.h
@@ -226,6 +226,7 @@ enum bpf_attach_type {
BPF_CGROUP_INET4_GETSOCKNAME,
BPF_CGROUP_INET6_GETSOCKNAME,
BPF_XDP_DEVMAP,
+ BPF_CGROUP_INET_SOCK_RELEASE,
__MAX_BPF_ATTACH_TYPE
};
diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
index 7f01be2b88b8..acbab6d0672d 100644
--- a/tools/lib/bpf/libbpf.c
+++ b/tools/lib/bpf/libbpf.c
@@ -6670,6 +6670,8 @@ static const struct bpf_sec_def section_defs[] = {
BPF_APROG_SEC("cgroup_skb/egress", BPF_PROG_TYPE_CGROUP_SKB,
BPF_CGROUP_INET_EGRESS),
BPF_APROG_COMPAT("cgroup/skb", BPF_PROG_TYPE_CGROUP_SKB),
+ BPF_EAPROG_SEC("cgroup/sock_release", BPF_PROG_TYPE_CGROUP_SOCK,
+ BPF_CGROUP_INET_SOCK_RELEASE),
BPF_APROG_SEC("cgroup/sock", BPF_PROG_TYPE_CGROUP_SOCK,
BPF_CGROUP_INET_SOCK_CREATE),
BPF_EAPROG_SEC("cgroup/post_bind4", BPF_PROG_TYPE_CGROUP_SOCK,
--
2.27.0.111.gc72c7da667-goog
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH bpf-next 3/4] bpftool: support BPF_CGROUP_INET_SOCK_RELEASE
2020-06-26 0:09 [PATCH bpf-next 1/4] bpf: add BPF_CGROUP_INET_SOCK_RELEASE hook Stanislav Fomichev
2020-06-26 0:09 ` [PATCH bpf-next 2/4] libbpf: add support for BPF_CGROUP_INET_SOCK_RELEASE Stanislav Fomichev
@ 2020-06-26 0:09 ` Stanislav Fomichev
2020-06-26 0:09 ` [PATCH bpf-next 4/4] selftests/bpf: test BPF_CGROUP_INET_SOCK_RELEASE Stanislav Fomichev
` (2 subsequent siblings)
4 siblings, 0 replies; 11+ messages in thread
From: Stanislav Fomichev @ 2020-06-26 0:09 UTC (permalink / raw)
To: netdev, bpf; +Cc: davem, ast, daniel, Stanislav Fomichev
Support attaching to sock_release from the bpftool.
Signed-off-by: Stanislav Fomichev <sdf@google.com>
---
tools/bpf/bpftool/main.h | 1 +
1 file changed, 1 insertion(+)
diff --git a/tools/bpf/bpftool/main.h b/tools/bpf/bpftool/main.h
index 5cdf0bc049bd..0a281d3cceb8 100644
--- a/tools/bpf/bpftool/main.h
+++ b/tools/bpf/bpftool/main.h
@@ -92,6 +92,7 @@ static const char * const attach_type_name[__MAX_BPF_ATTACH_TYPE] = {
[BPF_CGROUP_INET_INGRESS] = "ingress",
[BPF_CGROUP_INET_EGRESS] = "egress",
[BPF_CGROUP_INET_SOCK_CREATE] = "sock_create",
+ [BPF_CGROUP_INET_SOCK_RELEASE] = "sock_release",
[BPF_CGROUP_SOCK_OPS] = "sock_ops",
[BPF_CGROUP_DEVICE] = "device",
[BPF_CGROUP_INET4_BIND] = "bind4",
--
2.27.0.111.gc72c7da667-goog
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH bpf-next 4/4] selftests/bpf: test BPF_CGROUP_INET_SOCK_RELEASE
2020-06-26 0:09 [PATCH bpf-next 1/4] bpf: add BPF_CGROUP_INET_SOCK_RELEASE hook Stanislav Fomichev
2020-06-26 0:09 ` [PATCH bpf-next 2/4] libbpf: add support for BPF_CGROUP_INET_SOCK_RELEASE Stanislav Fomichev
2020-06-26 0:09 ` [PATCH bpf-next 3/4] bpftool: support BPF_CGROUP_INET_SOCK_RELEASE Stanislav Fomichev
@ 2020-06-26 0:09 ` Stanislav Fomichev
2020-06-26 22:06 ` Andrii Nakryiko
2020-06-26 2:30 ` [PATCH bpf-next 1/4] bpf: add BPF_CGROUP_INET_SOCK_RELEASE hook kernel test robot
2020-06-26 7:00 ` kernel test robot
4 siblings, 1 reply; 11+ messages in thread
From: Stanislav Fomichev @ 2020-06-26 0:09 UTC (permalink / raw)
To: netdev, bpf; +Cc: davem, ast, daniel, Stanislav Fomichev
Simple test that enforces a single SOCK_DGRAM socker per cgroup.
Signed-off-by: Stanislav Fomichev <sdf@google.com>
---
.../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;
+
+ /* 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;
+
+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) {
+ /* BPF_CGROUP_INET_SOCK_RELEASE is _not_ called
+ * when we return an error from the BPF
+ * program!
+ */
+ return 0;
+ }
+
+ __sync_fetch_and_add(&in_use, 1);
+ return 1;
+}
+
+SEC("cgroup/sock_release")
+int sock_release(struct bpf_sock *ctx)
+{
+ __u32 key;
+
+ if (ctx->type != SOCK_DGRAM)
+ return 1;
+
+ __sync_fetch_and_add(&invocations, 1);
+ __sync_fetch_and_add(&in_use, -1);
+ return 1;
+}
--
2.27.0.111.gc72c7da667-goog
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH bpf-next 1/4] bpf: add BPF_CGROUP_INET_SOCK_RELEASE hook
2020-06-26 0:09 [PATCH bpf-next 1/4] bpf: add BPF_CGROUP_INET_SOCK_RELEASE hook Stanislav Fomichev
` (2 preceding siblings ...)
2020-06-26 0:09 ` [PATCH bpf-next 4/4] selftests/bpf: test BPF_CGROUP_INET_SOCK_RELEASE Stanislav Fomichev
@ 2020-06-26 2:30 ` kernel test robot
2020-06-26 7:00 ` kernel test robot
4 siblings, 0 replies; 11+ messages in thread
From: kernel test robot @ 2020-06-26 2:30 UTC (permalink / raw)
To: Stanislav Fomichev, netdev, bpf
Cc: kbuild-all, davem, ast, daniel, Stanislav Fomichev
[-- Attachment #1: Type: text/plain, Size: 3903 bytes --]
Hi Stanislav,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on bpf/master]
[also build test ERROR on net/master net-next/master v5.8-rc2 next-20200625]
[cannot apply to bpf-next/master]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Stanislav-Fomichev/bpf-add-BPF_CGROUP_INET_SOCK_RELEASE-hook/20200626-081210
base: https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf.git master
config: nds32-defconfig (attached as .config)
compiler: nds32le-linux-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=nds32
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All errors (new ones prefixed by >>):
In file included from ./arch/nds32/include/generated/asm/bug.h:1,
from include/linux/bug.h:5,
from include/linux/thread_info.h:12,
from include/linux/uio.h:9,
from include/linux/socket.h:8,
from net/ipv4/af_inet.c:69:
include/linux/dma-mapping.h: In function 'dma_map_resource':
arch/nds32/include/asm/memory.h:82:32: warning: comparison of unsigned expression >= 0 is always true [-Wtype-limits]
82 | #define pfn_valid(pfn) ((pfn) >= PHYS_PFN_OFFSET && (pfn) < (PHYS_PFN_OFFSET + max_mapnr))
| ^~
include/asm-generic/bug.h:144:27: note: in definition of macro 'WARN_ON_ONCE'
144 | int __ret_warn_once = !!(condition); \
| ^~~~~~~~~
include/linux/dma-mapping.h:352:19: note: in expansion of macro 'pfn_valid'
352 | if (WARN_ON_ONCE(pfn_valid(PHYS_PFN(phys_addr))))
| ^~~~~~~~~
net/ipv4/af_inet.c: In function 'inet_release':
>> net/ipv4/af_inet.c:415:4: error: implicit declaration of function 'BPF_CGROUP_RUN_PROG_INET_SOCK_RELEASE'; did you mean 'BPF_CGROUP_RUN_PROG_INET_SOCK'? [-Werror=implicit-function-declaration]
415 | BPF_CGROUP_RUN_PROG_INET_SOCK_RELEASE(sk);
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
| BPF_CGROUP_RUN_PROG_INET_SOCK
cc1: some warnings being treated as errors
vim +415 net/ipv4/af_inet.c
400
401
402 /*
403 * The peer socket should always be NULL (or else). When we call this
404 * function we are destroying the object and from then on nobody
405 * should refer to it.
406 */
407 int inet_release(struct socket *sock)
408 {
409 struct sock *sk = sock->sk;
410
411 if (sk) {
412 long timeout;
413
414 if (!sk->sk_kern_sock)
> 415 BPF_CGROUP_RUN_PROG_INET_SOCK_RELEASE(sk);
416
417 /* Applications forget to leave groups before exiting */
418 ip_mc_drop_socket(sk);
419
420 /* If linger is set, we don't return until the close
421 * is complete. Otherwise we return immediately. The
422 * actually closing is done the same either way.
423 *
424 * If the close is due to the process exiting, we never
425 * linger..
426 */
427 timeout = 0;
428 if (sock_flag(sk, SOCK_LINGER) &&
429 !(current->flags & PF_EXITING))
430 timeout = sk->sk_lingertime;
431 sk->sk_prot->close(sk, timeout);
432 sock->sk = NULL;
433 }
434 return 0;
435 }
436 EXPORT_SYMBOL(inet_release);
437
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 10888 bytes --]
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH bpf-next 1/4] bpf: add BPF_CGROUP_INET_SOCK_RELEASE hook
2020-06-26 0:09 [PATCH bpf-next 1/4] bpf: add BPF_CGROUP_INET_SOCK_RELEASE hook Stanislav Fomichev
` (3 preceding siblings ...)
2020-06-26 2:30 ` [PATCH bpf-next 1/4] bpf: add BPF_CGROUP_INET_SOCK_RELEASE hook kernel test robot
@ 2020-06-26 7:00 ` kernel test robot
4 siblings, 0 replies; 11+ messages in thread
From: kernel test robot @ 2020-06-26 7:00 UTC (permalink / raw)
To: Stanislav Fomichev, netdev, bpf
Cc: kbuild-all, clang-built-linux, davem, ast, daniel, Stanislav Fomichev
[-- Attachment #1: Type: text/plain, Size: 2937 bytes --]
Hi Stanislav,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on bpf/master]
[also build test ERROR on net/master net-next/master v5.8-rc2 next-20200625]
[cannot apply to bpf-next/master]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Stanislav-Fomichev/bpf-add-BPF_CGROUP_INET_SOCK_RELEASE-hook/20200626-081210
base: https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf.git master
config: x86_64-defconfig (attached as .config)
compiler: clang version 11.0.0 (https://github.com/llvm/llvm-project 6e11ed52057ffc39941cb2de6d93cae522db4782)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# install x86_64 cross compiling tool for clang build
# apt-get install binutils-x86-64-linux-gnu
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=x86_64
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All errors (new ones prefixed by >>):
>> net/ipv4/af_inet.c:415:4: error: implicit declaration of function 'BPF_CGROUP_RUN_PROG_INET_SOCK_RELEASE' [-Werror,-Wimplicit-function-declaration]
BPF_CGROUP_RUN_PROG_INET_SOCK_RELEASE(sk);
^
1 error generated.
vim +/BPF_CGROUP_RUN_PROG_INET_SOCK_RELEASE +415 net/ipv4/af_inet.c
400
401
402 /*
403 * The peer socket should always be NULL (or else). When we call this
404 * function we are destroying the object and from then on nobody
405 * should refer to it.
406 */
407 int inet_release(struct socket *sock)
408 {
409 struct sock *sk = sock->sk;
410
411 if (sk) {
412 long timeout;
413
414 if (!sk->sk_kern_sock)
> 415 BPF_CGROUP_RUN_PROG_INET_SOCK_RELEASE(sk);
416
417 /* Applications forget to leave groups before exiting */
418 ip_mc_drop_socket(sk);
419
420 /* If linger is set, we don't return until the close
421 * is complete. Otherwise we return immediately. The
422 * actually closing is done the same either way.
423 *
424 * If the close is due to the process exiting, we never
425 * linger..
426 */
427 timeout = 0;
428 if (sock_flag(sk, SOCK_LINGER) &&
429 !(current->flags & PF_EXITING))
430 timeout = sk->sk_lingertime;
431 sk->sk_prot->close(sk, timeout);
432 sock->sk = NULL;
433 }
434 return 0;
435 }
436 EXPORT_SYMBOL(inet_release);
437
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 29433 bytes --]
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH bpf-next 2/4] libbpf: add support for BPF_CGROUP_INET_SOCK_RELEASE
2020-06-26 0:09 ` [PATCH bpf-next 2/4] libbpf: add support for BPF_CGROUP_INET_SOCK_RELEASE Stanislav Fomichev
@ 2020-06-26 22:02 ` Andrii Nakryiko
2020-06-26 23:51 ` Stanislav Fomichev
0 siblings, 1 reply; 11+ messages in thread
From: Andrii Nakryiko @ 2020-06-26 22:02 UTC (permalink / raw)
To: Stanislav Fomichev
Cc: Networking, bpf, David S. Miller, Alexei Starovoitov, Daniel Borkmann
On Thu, Jun 25, 2020 at 5:13 PM Stanislav Fomichev <sdf@google.com> wrote:
>
> Add auto-detection for the cgroup/sock_release programs.
>
> Signed-off-by: Stanislav Fomichev <sdf@google.com>
> ---
Acked-by: Andrii Nakryiko <andriin@fb.com>
> tools/include/uapi/linux/bpf.h | 1 +
> tools/lib/bpf/libbpf.c | 2 ++
> 2 files changed, 3 insertions(+)
>
> diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h
> index c65b374a5090..d7aea1d0167a 100644
> --- a/tools/include/uapi/linux/bpf.h
> +++ b/tools/include/uapi/linux/bpf.h
> @@ -226,6 +226,7 @@ enum bpf_attach_type {
> BPF_CGROUP_INET4_GETSOCKNAME,
> BPF_CGROUP_INET6_GETSOCKNAME,
> BPF_XDP_DEVMAP,
> + BPF_CGROUP_INET_SOCK_RELEASE,
> __MAX_BPF_ATTACH_TYPE
> };
>
> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> index 7f01be2b88b8..acbab6d0672d 100644
> --- a/tools/lib/bpf/libbpf.c
> +++ b/tools/lib/bpf/libbpf.c
> @@ -6670,6 +6670,8 @@ static const struct bpf_sec_def section_defs[] = {
> BPF_APROG_SEC("cgroup_skb/egress", BPF_PROG_TYPE_CGROUP_SKB,
> BPF_CGROUP_INET_EGRESS),
> BPF_APROG_COMPAT("cgroup/skb", BPF_PROG_TYPE_CGROUP_SKB),
> + BPF_EAPROG_SEC("cgroup/sock_release", BPF_PROG_TYPE_CGROUP_SOCK,
> + BPF_CGROUP_INET_SOCK_RELEASE),
> BPF_APROG_SEC("cgroup/sock", BPF_PROG_TYPE_CGROUP_SOCK,
might want to add another alias to match _release: "cgroup/sock_create"?
> BPF_CGROUP_INET_SOCK_CREATE),
> BPF_EAPROG_SEC("cgroup/post_bind4", BPF_PROG_TYPE_CGROUP_SOCK,
> --
> 2.27.0.111.gc72c7da667-goog
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH bpf-next 4/4] selftests/bpf: test BPF_CGROUP_INET_SOCK_RELEASE
2020-06-26 0:09 ` [PATCH bpf-next 4/4] selftests/bpf: test BPF_CGROUP_INET_SOCK_RELEASE Stanislav Fomichev
@ 2020-06-26 22:06 ` Andrii Nakryiko
2020-06-26 23:52 ` Stanislav Fomichev
0 siblings, 1 reply; 11+ messages in thread
From: Andrii Nakryiko @ 2020-06-26 22:06 UTC (permalink / raw)
To: Stanislav Fomichev
Cc: Networking, bpf, David S. Miller, Alexei Starovoitov, Daniel Borkmann
On Thu, Jun 25, 2020 at 5:13 PM Stanislav Fomichev <sdf@google.com> wrote:
>
> Simple test that enforces a single SOCK_DGRAM socker per cgroup.
>
> Signed-off-by: Stanislav Fomichev <sdf@google.com>
> ---
> .../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?
> +
> + /* 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.
> +
> +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?
> + /* BPF_CGROUP_INET_SOCK_RELEASE is _not_ called
> + * when we return an error from the BPF
> + * program!
> + */
> + return 0;
> + }
> +
> + __sync_fetch_and_add(&in_use, 1);
> + return 1;
> +}
> +
> +SEC("cgroup/sock_release")
> +int sock_release(struct bpf_sock *ctx)
> +{
> + __u32 key;
> +
> + if (ctx->type != SOCK_DGRAM)
> + return 1;
> +
> + __sync_fetch_and_add(&invocations, 1);
> + __sync_fetch_and_add(&in_use, -1);
> + return 1;
> +}
> --
> 2.27.0.111.gc72c7da667-goog
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH bpf-next 2/4] libbpf: add support for BPF_CGROUP_INET_SOCK_RELEASE
2020-06-26 22:02 ` Andrii Nakryiko
@ 2020-06-26 23:51 ` Stanislav Fomichev
0 siblings, 0 replies; 11+ messages in thread
From: Stanislav Fomichev @ 2020-06-26 23:51 UTC (permalink / raw)
To: Andrii Nakryiko
Cc: Networking, bpf, David S. Miller, Alexei Starovoitov, Daniel Borkmann
On Fri, Jun 26, 2020 at 3:02 PM Andrii Nakryiko
<andrii.nakryiko@gmail.com> wrote:
>
> On Thu, Jun 25, 2020 at 5:13 PM Stanislav Fomichev <sdf@google.com> wrote:
> >
> > Add auto-detection for the cgroup/sock_release programs.
> >
> > Signed-off-by: Stanislav Fomichev <sdf@google.com>
> > ---
>
> Acked-by: Andrii Nakryiko <andriin@fb.com>
>
>
> > tools/include/uapi/linux/bpf.h | 1 +
> > tools/lib/bpf/libbpf.c | 2 ++
> > 2 files changed, 3 insertions(+)
> >
> > diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h
> > index c65b374a5090..d7aea1d0167a 100644
> > --- a/tools/include/uapi/linux/bpf.h
> > +++ b/tools/include/uapi/linux/bpf.h
> > @@ -226,6 +226,7 @@ enum bpf_attach_type {
> > BPF_CGROUP_INET4_GETSOCKNAME,
> > BPF_CGROUP_INET6_GETSOCKNAME,
> > BPF_XDP_DEVMAP,
> > + BPF_CGROUP_INET_SOCK_RELEASE,
> > __MAX_BPF_ATTACH_TYPE
> > };
> >
> > diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> > index 7f01be2b88b8..acbab6d0672d 100644
> > --- a/tools/lib/bpf/libbpf.c
> > +++ b/tools/lib/bpf/libbpf.c
> > @@ -6670,6 +6670,8 @@ static const struct bpf_sec_def section_defs[] = {
> > BPF_APROG_SEC("cgroup_skb/egress", BPF_PROG_TYPE_CGROUP_SKB,
> > BPF_CGROUP_INET_EGRESS),
> > BPF_APROG_COMPAT("cgroup/skb", BPF_PROG_TYPE_CGROUP_SKB),
> > + BPF_EAPROG_SEC("cgroup/sock_release", BPF_PROG_TYPE_CGROUP_SOCK,
> > + BPF_CGROUP_INET_SOCK_RELEASE),
> > BPF_APROG_SEC("cgroup/sock", BPF_PROG_TYPE_CGROUP_SOCK,
>
> might want to add another alias to match _release: "cgroup/sock_create"?
Sure, will do!
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH bpf-next 4/4] selftests/bpf: test BPF_CGROUP_INET_SOCK_RELEASE
2020-06-26 22:06 ` Andrii Nakryiko
@ 2020-06-26 23:52 ` Stanislav Fomichev
2020-06-27 0:57 ` Andrii Nakryiko
0 siblings, 1 reply; 11+ messages in thread
From: Stanislav Fomichev @ 2020-06-26 23:52 UTC (permalink / raw)
To: Andrii Nakryiko
Cc: Networking, bpf, David S. Miller, Alexei Starovoitov, Daniel Borkmann
On Fri, Jun 26, 2020 at 3:06 PM Andrii Nakryiko
<andrii.nakryiko@gmail.com> wrote:
>
> On Thu, Jun 25, 2020 at 5:13 PM Stanislav Fomichev <sdf@google.com> wrote:
> >
> > Simple test that enforces a single SOCK_DGRAM socker per cgroup.
> >
> > Signed-off-by: Stanislav Fomichev <sdf@google.com>
> > ---
> > .../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!
> > + /* 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?
> > +
> > +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.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH bpf-next 4/4] selftests/bpf: test BPF_CGROUP_INET_SOCK_RELEASE
2020-06-26 23:52 ` Stanislav Fomichev
@ 2020-06-27 0:57 ` Andrii Nakryiko
0 siblings, 0 replies; 11+ messages in thread
From: Andrii Nakryiko @ 2020-06-27 0:57 UTC (permalink / raw)
To: Stanislav Fomichev
Cc: Networking, bpf, David S. Miller, Alexei Starovoitov, Daniel Borkmann
On Fri, Jun 26, 2020 at 4:52 PM Stanislav Fomichev <sdf@google.com> wrote:
>
> On Fri, Jun 26, 2020 at 3:06 PM Andrii Nakryiko
> <andrii.nakryiko@gmail.com> wrote:
> >
> > On Thu, Jun 25, 2020 at 5:13 PM Stanislav Fomichev <sdf@google.com> wrote:
> > >
> > > Simple test that enforces a single SOCK_DGRAM socker per cgroup.
> > >
> > > Signed-off-by: Stanislav Fomichev <sdf@google.com>
> > > ---
> > > .../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!
>
> > > + /* 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.
>
> > > +
> > > +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.
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2020-06-27 0:58 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-06-26 0:09 [PATCH bpf-next 1/4] bpf: add BPF_CGROUP_INET_SOCK_RELEASE hook Stanislav Fomichev
2020-06-26 0:09 ` [PATCH bpf-next 2/4] libbpf: add support for BPF_CGROUP_INET_SOCK_RELEASE Stanislav Fomichev
2020-06-26 22:02 ` Andrii Nakryiko
2020-06-26 23:51 ` Stanislav Fomichev
2020-06-26 0:09 ` [PATCH bpf-next 3/4] bpftool: support BPF_CGROUP_INET_SOCK_RELEASE Stanislav Fomichev
2020-06-26 0:09 ` [PATCH bpf-next 4/4] selftests/bpf: test BPF_CGROUP_INET_SOCK_RELEASE Stanislav Fomichev
2020-06-26 22:06 ` Andrii Nakryiko
2020-06-26 23:52 ` Stanislav Fomichev
2020-06-27 0:57 ` Andrii Nakryiko
2020-06-26 2:30 ` [PATCH bpf-next 1/4] bpf: add BPF_CGROUP_INET_SOCK_RELEASE hook kernel test robot
2020-06-26 7:00 ` kernel test robot
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).