[PATCH bpf-next v4 0/4] bpf: add BPF_CGROUP_INET_SOCK_RELEASE hook

[PATCH bpf-next v4 0/4] bpf: add BPF_CGROUP_INET_SOCK_RELEASE hook

[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.

There is already existing BPF_CGROUP_INET_SOCK_CREATE hook
that triggers upon socket creation; let's add new hook
(BPF_CGROUP_INET_SOCK_RELEASE) that triggers on socket release.

v4:
* initialize global BPF vars (Andrii Nakryiko)
* simplify error handling (Andrii Nakryiko)

v3:
* s/CHECK_FAIL/CHECK/ (Andrii Nakryiko)
* s/bpf_prog_attach/bpf_program__attach_cgroup/ (Andrii Nakryiko)
* fix &in_use in BPF program (Andrii Nakryiko)

v2:
* fix compile issue with CONFIG_CGROUP_BPF=n (kernel test robot)

Stanislav Fomichev (4):
  bpf: add BPF_CGROUP_INET_SOCK_RELEASE hook
  libbpf: add support for BPF_CGROUP_INET_SOCK_RELEASE
  bpftool: add support for BPF_CGROUP_INET_SOCK_RELEASE
  selftests/bpf: test BPF_CGROUP_INET_SOCK_RELEASE

 include/linux/bpf-cgroup.h                    |  4 +
 include/uapi/linux/bpf.h                      |  1 +
 kernel/bpf/syscall.c                          |  3 +
 net/core/filter.c                             |  1 +
 net/ipv4/af_inet.c                            |  3 +
 tools/bpf/bpftool/common.c                    |  1 +
 tools/include/uapi/linux/bpf.h                |  1 +
 tools/lib/bpf/libbpf.c                        |  4 +
 .../selftests/bpf/prog_tests/udp_limit.c      | 75 +++++++++++++++++++
 tools/testing/selftests/bpf/progs/udp_limit.c | 42 +++++++++++
 10 files changed, 135 insertions(+)
 create mode 100644 tools/testing/selftests/bpf/prog_tests/udp_limit.c
 create mode 100644 tools/testing/selftests/bpf/progs/udp_limit.c

-- 
2.27.0.212.ge8ba1cc988-goog

[PATCH bpf-next v4 1/4] bpf: add BPF_CGROUP_INET_SOCK_RELEASE hook

[Stanislav Fomichev]

Implement BPF_CGROUP_INET_SOCK_RELEASE hook that triggers
on inet socket release. It triggers only for userspace
sockets, the same semantics as existing BPF_CGROUP_INET_SOCK_CREATE.

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 | 4 ++++
 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, 12 insertions(+)

diff --git a/include/linux/bpf-cgroup.h b/include/linux/bpf-cgroup.h
index c66c545e161a..2c6f26670acc 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)
 
@@ -401,6 +404,7 @@ static inline int bpf_percpu_cgroup_storage_update(struct bpf_map *map,
 #define BPF_CGROUP_RUN_PROG_INET_INGRESS(sk,skb) ({ 0; })
 #define BPF_CGROUP_RUN_PROG_INET_EGRESS(sk,skb) ({ 0; })
 #define BPF_CGROUP_RUN_PROG_INET_SOCK(sk) ({ 0; })
+#define BPF_CGROUP_RUN_PROG_INET_SOCK_RELEASE(sk) ({ 0; })
 #define BPF_CGROUP_RUN_PROG_INET4_BIND(sk, uaddr) ({ 0; })
 #define BPF_CGROUP_RUN_PROG_INET6_BIND(sk, uaddr) ({ 0; })
 #define BPF_CGROUP_RUN_PROG_INET4_POST_BIND(sk) ({ 0; })
diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
index da9bf35a26f8..548a749aebb3 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 8da159936bab..156f51ffada2 100644
--- a/kernel/bpf/syscall.c
+++ b/kernel/bpf/syscall.c
@@ -1981,6 +1981,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;
@@ -2779,6 +2780,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;
@@ -2929,6 +2931,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 c5e696e6c315..ddcc0d6209e1 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -6890,6 +6890,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 ea6ed6d487ed..ff141d630bdf 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.212.ge8ba1cc988-goog

[PATCH bpf-next v4 2/4] libbpf: add support for BPF_CGROUP_INET_SOCK_RELEASE

[Stanislav Fomichev]

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         | 4 ++++
 2 files changed, 5 insertions(+)

diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h
index da9bf35a26f8..548a749aebb3 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 4ea7f4f1a691..88a483627a2b 100644
--- a/tools/lib/bpf/libbpf.c
+++ b/tools/lib/bpf/libbpf.c
@@ -6917,6 +6917,10 @@ 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_create",	BPF_PROG_TYPE_CGROUP_SOCK,
+						BPF_CGROUP_INET_SOCK_CREATE),
+	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.212.ge8ba1cc988-goog

[PATCH bpf-next v4 3/4] bpftool: add support for BPF_CGROUP_INET_SOCK_RELEASE

[Stanislav Fomichev]

Support attaching to BPF_CGROUP_INET_SOCK_RELEASE and properly
display attach type upon prog dump.

Signed-off-by: Stanislav Fomichev <sdf@google.com>
Acked-by: Andrii Nakryiko <andriin@fb.com>
---
 tools/bpf/bpftool/common.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/tools/bpf/bpftool/common.c b/tools/bpf/bpftool/common.c
index 18e5604fe260..29f4e7611ae8 100644
--- a/tools/bpf/bpftool/common.c
+++ b/tools/bpf/bpftool/common.c
@@ -33,6 +33,7 @@ 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.212.ge8ba1cc988-goog

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

[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      | 75 +++++++++++++++++++
 tools/testing/selftests/bpf/progs/udp_limit.c | 42 +++++++++++
 2 files changed, 117 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..2aba09d4d01b
--- /dev/null
+++ b/tools/testing/selftests/bpf/prog_tests/udp_limit.c
@@ -0,0 +1,75 @@
+// SPDX-License-Identifier: GPL-2.0
+#include <test_progs.h>
+#include "udp_limit.skel.h"
+
+#include <sys/types.h>
+#include <sys/socket.h>
+
+static int duration;
+
+void test_udp_limit(void)
+{
+	struct udp_limit *skel;
+	int fd1 = -1, fd2 = -1;
+	int cgroup_fd;
+
+	cgroup_fd = test__join_cgroup("/udp_limit");
+	if (CHECK(cgroup_fd < 0, "cg-join", "errno %d", errno))
+		return;
+
+	skel = udp_limit__open_and_load();
+	if (CHECK(!skel, "skel-load", "errno %d", errno))
+		goto close_cgroup_fd;
+
+	skel->links.sock = bpf_program__attach_cgroup(skel->progs.sock, cgroup_fd);
+	skel->links.sock_release = bpf_program__attach_cgroup(skel->progs.sock_release, cgroup_fd);
+	if (CHECK(IS_ERR(skel->links.sock) || IS_ERR(skel->links.sock_release),
+		  "cg-attach", "sock %ld sock_release %ld",
+		  PTR_ERR(skel->links.sock),
+		  PTR_ERR(skel->links.sock_release)))
+		goto close_skeleton;
+
+	/* BPF program enforces a single UDP socket per cgroup,
+	 * verify that.
+	 */
+	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))
+		goto close_skeleton;
+
+	/* We can reopen again after close. */
+	close(fd1);
+	fd1 = -1;
+
+	fd1 = socket(AF_INET, SOCK_DGRAM, 0);
+	if (CHECK(fd1 < 0, "fd1-again", "errno %d", errno))
+		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(skel->bss->invocations != 4, "bss-invocations",
+		  "invocations=%d", skel->bss->invocations))
+		goto close_skeleton;
+
+	/* We should still have a single socket in use */
+	if (CHECK(skel->bss->in_use != 1, "bss-in_use",
+		  "in_use=%d", skel->bss->in_use))
+		goto close_skeleton;
+
+close_skeleton:
+	if (fd1 >= 0)
+		close(fd1);
+	if (fd2 >= 0)
+		close(fd2);
+	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..edbb30a27e63
--- /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 = 0, in_use = 0;
+
+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.212.ge8ba1cc988-goog

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

[Andrii Nakryiko]

On Mon, Jul 6, 2020 at 4:04 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>
> ---

Acked-by: Andrii Nakryiko <andriin@fb.com>

>  .../selftests/bpf/prog_tests/udp_limit.c      | 75 +++++++++++++++++++
>  tools/testing/selftests/bpf/progs/udp_limit.c | 42 +++++++++++
>  2 files changed, 117 insertions(+)
>  create mode 100644 tools/testing/selftests/bpf/prog_tests/udp_limit.c
>  create mode 100644 tools/testing/selftests/bpf/progs/udp_limit.c
>

[...]

Re: [PATCH bpf-next v4 1/4] bpf: add BPF_CGROUP_INET_SOCK_RELEASE hook

[Andrii Nakryiko]

On Mon, Jul 6, 2020 at 4:02 PM Stanislav Fomichev <sdf@google.com> wrote:
>
> Implement BPF_CGROUP_INET_SOCK_RELEASE hook that triggers
> on inet socket release. It triggers only for userspace
> sockets, the same semantics as existing BPF_CGROUP_INET_SOCK_CREATE.
>
> 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 | 4 ++++
>  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, 12 insertions(+)
>

Looks good overall, but I have no idea about sock->sk NULL case.

Acked-by: Andrii Nakryiko <andriin@fb.com>

> diff --git a/include/linux/bpf-cgroup.h b/include/linux/bpf-cgroup.h
> index c66c545e161a..2c6f26670acc 100644
> --- a/include/linux/bpf-cgroup.h
> +++ b/include/linux/bpf-cgroup.h

[...]

Re: [PATCH bpf-next v4 1/4] bpf: add BPF_CGROUP_INET_SOCK_RELEASE hook

[Daniel Borkmann]

On 7/7/20 1:42 AM, Andrii Nakryiko wrote:
> On Mon, Jul 6, 2020 at 4:02 PM Stanislav Fomichev <sdf@google.com> wrote:
>>
>> Implement BPF_CGROUP_INET_SOCK_RELEASE hook that triggers
>> on inet socket release. It triggers only for userspace
>> sockets, the same semantics as existing BPF_CGROUP_INET_SOCK_CREATE.
>>
>> 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 | 4 ++++
>>   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, 12 insertions(+)
>>
> 
> Looks good overall, but I have no idea about sock->sk NULL case.

+1, looks good & very useful hook. For the sock->sk NULL case here's a related
discussion on why it's needed [0].

   [0] https://lore.kernel.org/netdev/20190221221356.173485-1-ebiggers@kernel.org/

> Acked-by: Andrii Nakryiko <andriin@fb.com>
> 
>> diff --git a/include/linux/bpf-cgroup.h b/include/linux/bpf-cgroup.h
>> index c66c545e161a..2c6f26670acc 100644
>> --- a/include/linux/bpf-cgroup.h
>> +++ b/include/linux/bpf-cgroup.h
> 
> [...]
> 

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

[Daniel Borkmann]

On 7/7/20 1:01 AM, Stanislav Fomichev 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      | 75 +++++++++++++++++++
>   tools/testing/selftests/bpf/progs/udp_limit.c | 42 +++++++++++
>   2 files changed, 117 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..2aba09d4d01b
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/prog_tests/udp_limit.c
> @@ -0,0 +1,75 @@
> +// SPDX-License-Identifier: GPL-2.0
> +#include <test_progs.h>
> +#include "udp_limit.skel.h"
> +
> +#include <sys/types.h>
> +#include <sys/socket.h>
> +
> +static int duration;
> +
> +void test_udp_limit(void)
> +{
> +	struct udp_limit *skel;
> +	int fd1 = -1, fd2 = -1;
> +	int cgroup_fd;
> +
> +	cgroup_fd = test__join_cgroup("/udp_limit");
> +	if (CHECK(cgroup_fd < 0, "cg-join", "errno %d", errno))
> +		return;
> +
> +	skel = udp_limit__open_and_load();
> +	if (CHECK(!skel, "skel-load", "errno %d", errno))
> +		goto close_cgroup_fd;
> +
> +	skel->links.sock = bpf_program__attach_cgroup(skel->progs.sock, cgroup_fd);
> +	skel->links.sock_release = bpf_program__attach_cgroup(skel->progs.sock_release, cgroup_fd);
> +	if (CHECK(IS_ERR(skel->links.sock) || IS_ERR(skel->links.sock_release),
> +		  "cg-attach", "sock %ld sock_release %ld",
> +		  PTR_ERR(skel->links.sock),
> +		  PTR_ERR(skel->links.sock_release)))
> +		goto close_skeleton;
> +
> +	/* BPF program enforces a single UDP socket per cgroup,
> +	 * verify that.
> +	 */
> +	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))
> +		goto close_skeleton;
> +
> +	/* We can reopen again after close. */
> +	close(fd1);
> +	fd1 = -1;
> +
> +	fd1 = socket(AF_INET, SOCK_DGRAM, 0);
> +	if (CHECK(fd1 < 0, "fd1-again", "errno %d", errno))
> +		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(skel->bss->invocations != 4, "bss-invocations",
> +		  "invocations=%d", skel->bss->invocations))
> +		goto close_skeleton;
> +
> +	/* We should still have a single socket in use */
> +	if (CHECK(skel->bss->in_use != 1, "bss-in_use",
> +		  "in_use=%d", skel->bss->in_use))
> +		goto close_skeleton;
> +
> +close_skeleton:
> +	if (fd1 >= 0)
> +		close(fd1);
> +	if (fd2 >= 0)
> +		close(fd2);
> +	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..edbb30a27e63
> --- /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 = 0, in_use = 0;
> +
> +SEC("cgroup/sock")

nit: Doesn't matter overly much, but given you've added `cgroup/sock_create`
earlier in patch 2/4 intention was probably to use it as well. But either is
fine as it resolved to the same.

> +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;
> +}
> 

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

[Andrii Nakryiko]

On Tue, Jul 7, 2020 at 2:45 PM Daniel Borkmann <daniel@iogearbox.net> wrote:
>
> On 7/7/20 1:01 AM, Stanislav Fomichev 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      | 75 +++++++++++++++++++
> >   tools/testing/selftests/bpf/progs/udp_limit.c | 42 +++++++++++
> >   2 files changed, 117 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..2aba09d4d01b
> > --- /dev/null
> > +++ b/tools/testing/selftests/bpf/prog_tests/udp_limit.c
> > @@ -0,0 +1,75 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +#include <test_progs.h>
> > +#include "udp_limit.skel.h"
> > +
> > +#include <sys/types.h>
> > +#include <sys/socket.h>
> > +
> > +static int duration;
> > +
> > +void test_udp_limit(void)
> > +{
> > +     struct udp_limit *skel;
> > +     int fd1 = -1, fd2 = -1;
> > +     int cgroup_fd;
> > +
> > +     cgroup_fd = test__join_cgroup("/udp_limit");
> > +     if (CHECK(cgroup_fd < 0, "cg-join", "errno %d", errno))
> > +             return;
> > +
> > +     skel = udp_limit__open_and_load();
> > +     if (CHECK(!skel, "skel-load", "errno %d", errno))
> > +             goto close_cgroup_fd;
> > +
> > +     skel->links.sock = bpf_program__attach_cgroup(skel->progs.sock, cgroup_fd);
> > +     skel->links.sock_release = bpf_program__attach_cgroup(skel->progs.sock_release, cgroup_fd);
> > +     if (CHECK(IS_ERR(skel->links.sock) || IS_ERR(skel->links.sock_release),
> > +               "cg-attach", "sock %ld sock_release %ld",
> > +               PTR_ERR(skel->links.sock),
> > +               PTR_ERR(skel->links.sock_release)))
> > +             goto close_skeleton;
> > +
> > +     /* BPF program enforces a single UDP socket per cgroup,
> > +      * verify that.
> > +      */
> > +     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))
> > +             goto close_skeleton;
> > +
> > +     /* We can reopen again after close. */
> > +     close(fd1);
> > +     fd1 = -1;
> > +
> > +     fd1 = socket(AF_INET, SOCK_DGRAM, 0);
> > +     if (CHECK(fd1 < 0, "fd1-again", "errno %d", errno))
> > +             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(skel->bss->invocations != 4, "bss-invocations",
> > +               "invocations=%d", skel->bss->invocations))
> > +             goto close_skeleton;
> > +
> > +     /* We should still have a single socket in use */
> > +     if (CHECK(skel->bss->in_use != 1, "bss-in_use",
> > +               "in_use=%d", skel->bss->in_use))
> > +             goto close_skeleton;
> > +
> > +close_skeleton:
> > +     if (fd1 >= 0)
> > +             close(fd1);
> > +     if (fd2 >= 0)
> > +             close(fd2);
> > +     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..edbb30a27e63
> > --- /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 = 0, in_use = 0;
> > +
> > +SEC("cgroup/sock")
>
> nit: Doesn't matter overly much, but given you've added `cgroup/sock_create`
> earlier in patch 2/4 intention was probably to use it as well. But either is
> fine as it resolved to the same.

heh, had the same thought, but didn't want to be too nitpicky :)


>
> > +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;
> > +}
> >
>

Re: [PATCH bpf-next v4 1/4] bpf: add BPF_CGROUP_INET_SOCK_RELEASE hook

[Stanislav Fomichev]

On Tue, Jul 7, 2020 at 2:42 PM Daniel Borkmann <daniel@iogearbox.net> wrote:
>
> On 7/7/20 1:42 AM, Andrii Nakryiko wrote:
> > On Mon, Jul 6, 2020 at 4:02 PM Stanislav Fomichev <sdf@google.com> wrote:
> >>
> >> Implement BPF_CGROUP_INET_SOCK_RELEASE hook that triggers
> >> on inet socket release. It triggers only for userspace
> >> sockets, the same semantics as existing BPF_CGROUP_INET_SOCK_CREATE.
> >>
> >> 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 | 4 ++++
> >>   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, 12 insertions(+)
> >>
> >
> > Looks good overall, but I have no idea about sock->sk NULL case.
>
> +1, looks good & very useful hook. For the sock->sk NULL case here's a related
> discussion on why it's needed [0].
Thanks for the pointer! I'll resend a v5 with s/sock/sock_create/ you
mentioned and will clean up the commit description a bit.

Re: [PATCH bpf-next v4 1/4] bpf: add BPF_CGROUP_INET_SOCK_RELEASE hook

[Daniel Borkmann]

On 7/8/20 1:43 AM, Stanislav Fomichev wrote:
> On Tue, Jul 7, 2020 at 2:42 PM Daniel Borkmann <daniel@iogearbox.net> wrote:
>>
>> On 7/7/20 1:42 AM, Andrii Nakryiko wrote:
>>> On Mon, Jul 6, 2020 at 4:02 PM Stanislav Fomichev <sdf@google.com> wrote:
>>>>
>>>> Implement BPF_CGROUP_INET_SOCK_RELEASE hook that triggers
>>>> on inet socket release. It triggers only for userspace
>>>> sockets, the same semantics as existing BPF_CGROUP_INET_SOCK_CREATE.
>>>>
>>>> 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 | 4 ++++
>>>>    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, 12 insertions(+)
>>>>
>>>
>>> Looks good overall, but I have no idea about sock->sk NULL case.
>>
>> +1, looks good & very useful hook. For the sock->sk NULL case here's a related
>> discussion on why it's needed [0].
> Thanks for the pointer! I'll resend a v5 with s/sock/sock_create/ you
> mentioned and will clean up the commit description a bit.

Already fixed up the selftest and a typo in the commit desc there & applied it. Let
me know if you prefer a respin though and I can toss it taking the respin which would
work just as well. :)

Thanks,
Daniel

Re: [PATCH bpf-next v4 1/4] bpf: add BPF_CGROUP_INET_SOCK_RELEASE hook

[Stanislav Fomichev]

On Tue, Jul 7, 2020 at 4:56 PM Daniel Borkmann <daniel@iogearbox.net> wrote:
>
> On 7/8/20 1:43 AM, Stanislav Fomichev wrote:
> > On Tue, Jul 7, 2020 at 2:42 PM Daniel Borkmann <daniel@iogearbox.net> wrote:
> >>
> >> On 7/7/20 1:42 AM, Andrii Nakryiko wrote:
> >>> On Mon, Jul 6, 2020 at 4:02 PM Stanislav Fomichev <sdf@google.com> wrote:
> >>>>
> >>>> Implement BPF_CGROUP_INET_SOCK_RELEASE hook that triggers
> >>>> on inet socket release. It triggers only for userspace
> >>>> sockets, the same semantics as existing BPF_CGROUP_INET_SOCK_CREATE.
> >>>>
> >>>> 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 | 4 ++++
> >>>>    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, 12 insertions(+)
> >>>>
> >>>
> >>> Looks good overall, but I have no idea about sock->sk NULL case.
> >>
> >> +1, looks good & very useful hook. For the sock->sk NULL case here's a related
> >> discussion on why it's needed [0].
> > Thanks for the pointer! I'll resend a v5 with s/sock/sock_create/ you
> > mentioned and will clean up the commit description a bit.
>
> Already fixed up the selftest and a typo in the commit desc there & applied it.
Oh, awesome, thanks!