bpf.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [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).