[PATCH bpf 0/4] bpf, vsock: Fix poll() and close()

[PATCH bpf 0/4] bpf, vsock: Fix poll() and close()

[Michal Luczaj]

Two small fixes for vsock: poll() missing a queue check, and close() not
invoking sockmap cleanup.

Signed-off-by: Michal Luczaj <mhal@rbox.co>
---
Michal Luczaj (4):
      bpf, vsock: Fix poll() missing a queue
      selftest/bpf: Add test for af_vsock poll()
      bpf, vsock: Invoke proto::close on close()
      selftest/bpf: Add test for vsock removal from sockmap on close()

 net/vmw_vsock/af_vsock.c                           | 70 ++++++++++++--------
 .../selftests/bpf/prog_tests/sockmap_basic.c       | 77 ++++++++++++++++++++++
 2 files changed, 120 insertions(+), 27 deletions(-)
---
base-commit: 6c4139b0f19b7397286897caee379f8321e78272
change-id: 20241118-vsock-bpf-poll-close-64f432e682ec

Best regards,
-- 
Michal Luczaj <mhal@rbox.co>

[PATCH bpf 1/4] bpf, vsock: Fix poll() missing a queue

[Michal Luczaj]

When a verdict program simply passes a packet without redirection, sk_msg
is enqueued on sk_psock::ingress_msg. Add a missing check to poll().

Fixes: 634f1a7110b4 ("vsock: support sockmap")
Signed-off-by: Michal Luczaj <mhal@rbox.co>
---
 net/vmw_vsock/af_vsock.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c
index dfd29160fe11c4675f872c1ee123d65b2da0dae6..919da8edd03c838cbcdbf1618425da6c5ec2df1a 100644
--- a/net/vmw_vsock/af_vsock.c
+++ b/net/vmw_vsock/af_vsock.c
@@ -1054,6 +1054,9 @@ static __poll_t vsock_poll(struct file *file, struct socket *sock,
 		mask |= EPOLLRDHUP;
 	}
 
+	if (sk_is_readable(sk))
+		mask |= EPOLLIN | EPOLLRDNORM;
+
 	if (sock->type == SOCK_DGRAM) {
 		/* For datagram sockets we can read if there is something in
 		 * the queue and write as long as the socket isn't shutdown for

-- 
2.46.2

[PATCH bpf 2/4] selftest/bpf: Add test for af_vsock poll()

[Michal Luczaj]

Verify that vsock's poll() notices when sk_psock::ingress_msg isn't empty.

Signed-off-by: Michal Luczaj <mhal@rbox.co>
---
 .../selftests/bpf/prog_tests/sockmap_basic.c       | 46 ++++++++++++++++++++++
 1 file changed, 46 insertions(+)

diff --git a/tools/testing/selftests/bpf/prog_tests/sockmap_basic.c b/tools/testing/selftests/bpf/prog_tests/sockmap_basic.c
index 82bfb266741cfaceafa2a407cd2ccc937708c613..21d1e2e2308433e7475952dcab034e92f2f6101a 100644
--- a/tools/testing/selftests/bpf/prog_tests/sockmap_basic.c
+++ b/tools/testing/selftests/bpf/prog_tests/sockmap_basic.c
@@ -885,6 +885,50 @@ static void test_sockmap_same_sock(void)
 	test_sockmap_pass_prog__destroy(skel);
 }
 
+static void test_sockmap_skb_verdict_vsock_poll(void)
+{
+	struct test_sockmap_pass_prog *skel;
+	int err, map, conn, peer;
+	struct bpf_program *prog;
+	struct bpf_link *link;
+	char buf = 'x';
+	int zero = 0;
+
+	skel = test_sockmap_pass_prog__open_and_load();
+	if (!ASSERT_OK_PTR(skel, "open_and_load"))
+		return;
+
+	if (create_pair(AF_VSOCK, SOCK_STREAM, &conn, &peer))
+		goto destroy;
+
+	prog = skel->progs.prog_skb_verdict;
+	map = bpf_map__fd(skel->maps.sock_map_rx);
+	link = bpf_program__attach_sockmap(prog, map);
+	if (!ASSERT_OK_PTR(link, "bpf_program__attach_sockmap"))
+		goto close;
+
+	err = bpf_map_update_elem(map, &zero, &conn, BPF_ANY);
+	if (!ASSERT_OK(err, "bpf_map_update_elem"))
+		goto detach;
+
+	if (xsend(peer, &buf, 1, 0) != 1)
+		goto detach;
+
+	err = poll_read(conn, IO_TIMEOUT_SEC);
+	if (!ASSERT_OK(err, "poll"))
+		goto detach;
+
+	if (xrecv_nonblock(conn, &buf, 1, 0) != 1)
+		FAIL("xrecv_nonblock");
+detach:
+	bpf_link__detach(link);
+close:
+	xclose(conn);
+	xclose(peer);
+destroy:
+	test_sockmap_pass_prog__destroy(skel);
+}
+
 void test_sockmap_basic(void)
 {
 	if (test__start_subtest("sockmap create_update_free"))
@@ -943,4 +987,6 @@ void test_sockmap_basic(void)
 		test_skmsg_helpers_with_link(BPF_MAP_TYPE_SOCKMAP);
 	if (test__start_subtest("sockhash sk_msg attach sockhash helpers with link"))
 		test_skmsg_helpers_with_link(BPF_MAP_TYPE_SOCKHASH);
+	if (test__start_subtest("sockmap skb_verdict vsock poll"))
+		test_sockmap_skb_verdict_vsock_poll();
 }

-- 
2.46.2

[PATCH bpf 3/4] bpf, vsock: Invoke proto::close on close()

[Michal Luczaj]

vsock defines a BPF callback to be invoked when close() is called. However,
this callback is never actually executed. As a result, a closed vsock
socket is not automatically removed from the sockmap/sockhash.

Introduce a dummy vsock_close() and make vsock_release() call proto::close.

Note: changes in __vsock_release() look messy, but it's only due to indent
level reduction and variables xmas tree reorder.

Fixes: 634f1a7110b4 ("vsock: support sockmap")
Signed-off-by: Michal Luczaj <mhal@rbox.co>
---
 net/vmw_vsock/af_vsock.c | 67 +++++++++++++++++++++++++++++-------------------
 1 file changed, 40 insertions(+), 27 deletions(-)

diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c
index 919da8edd03c838cbcdbf1618425da6c5ec2df1a..b52b798aa4c2926c3f233aad6cd31b4056f6fee2 100644
--- a/net/vmw_vsock/af_vsock.c
+++ b/net/vmw_vsock/af_vsock.c
@@ -117,12 +117,14 @@
 static int __vsock_bind(struct sock *sk, struct sockaddr_vm *addr);
 static void vsock_sk_destruct(struct sock *sk);
 static int vsock_queue_rcv_skb(struct sock *sk, struct sk_buff *skb);
+static void vsock_close(struct sock *sk, long timeout);
 
 /* Protocol family. */
 struct proto vsock_proto = {
 	.name = "AF_VSOCK",
 	.owner = THIS_MODULE,
 	.obj_size = sizeof(struct vsock_sock),
+	.close = vsock_close,
 #ifdef CONFIG_BPF_SYSCALL
 	.psock_update_sk_prot = vsock_bpf_update_proto,
 #endif
@@ -797,39 +799,37 @@ static bool sock_type_connectible(u16 type)
 
 static void __vsock_release(struct sock *sk, int level)
 {
-	if (sk) {
-		struct sock *pending;
-		struct vsock_sock *vsk;
-
-		vsk = vsock_sk(sk);
-		pending = NULL;	/* Compiler warning. */
+	struct vsock_sock *vsk;
+	struct sock *pending;
 
-		/* When "level" is SINGLE_DEPTH_NESTING, use the nested
-		 * version to avoid the warning "possible recursive locking
-		 * detected". When "level" is 0, lock_sock_nested(sk, level)
-		 * is the same as lock_sock(sk).
-		 */
-		lock_sock_nested(sk, level);
+	vsk = vsock_sk(sk);
+	pending = NULL;	/* Compiler warning. */
 
-		if (vsk->transport)
-			vsk->transport->release(vsk);
-		else if (sock_type_connectible(sk->sk_type))
-			vsock_remove_sock(vsk);
+	/* When "level" is SINGLE_DEPTH_NESTING, use the nested
+	 * version to avoid the warning "possible recursive locking
+	 * detected". When "level" is 0, lock_sock_nested(sk, level)
+	 * is the same as lock_sock(sk).
+	 */
+	lock_sock_nested(sk, level);
 
-		sock_orphan(sk);
-		sk->sk_shutdown = SHUTDOWN_MASK;
+	if (vsk->transport)
+		vsk->transport->release(vsk);
+	else if (sock_type_connectible(sk->sk_type))
+		vsock_remove_sock(vsk);
 
-		skb_queue_purge(&sk->sk_receive_queue);
+	sock_orphan(sk);
+	sk->sk_shutdown = SHUTDOWN_MASK;
 
-		/* Clean up any sockets that never were accepted. */
-		while ((pending = vsock_dequeue_accept(sk)) != NULL) {
-			__vsock_release(pending, SINGLE_DEPTH_NESTING);
-			sock_put(pending);
-		}
+	skb_queue_purge(&sk->sk_receive_queue);
 
-		release_sock(sk);
-		sock_put(sk);
+	/* Clean up any sockets that never were accepted. */
+	while ((pending = vsock_dequeue_accept(sk)) != NULL) {
+		__vsock_release(pending, SINGLE_DEPTH_NESTING);
+		sock_put(pending);
 	}
+
+	release_sock(sk);
+	sock_put(sk);
 }
 
 static void vsock_sk_destruct(struct sock *sk)
@@ -901,9 +901,22 @@ void vsock_data_ready(struct sock *sk)
 }
 EXPORT_SYMBOL_GPL(vsock_data_ready);
 
+/* Dummy callback required by sockmap.
+ * See unconditional call of saved_close() in sock_map_close().
+ */
+static void vsock_close(struct sock *sk, long timeout)
+{
+}
+
 static int vsock_release(struct socket *sock)
 {
-	__vsock_release(sock->sk, 0);
+	struct sock *sk = sock->sk;
+
+	if (!sk)
+		return 0;
+
+	sk->sk_prot->close(sk, 0);
+	__vsock_release(sk, 0);
 	sock->sk = NULL;
 	sock->state = SS_FREE;
 

-- 
2.46.2

[PATCH bpf 4/4] selftest/bpf: Add test for vsock removal from sockmap on close()

[Michal Luczaj]

Make sure the proto::close callback gets invoked on vsock release.

Signed-off-by: Michal Luczaj <mhal@rbox.co>
---
 .../selftests/bpf/prog_tests/sockmap_basic.c       | 31 ++++++++++++++++++++++
 1 file changed, 31 insertions(+)

diff --git a/tools/testing/selftests/bpf/prog_tests/sockmap_basic.c b/tools/testing/selftests/bpf/prog_tests/sockmap_basic.c
index 21d1e2e2308433e7475952dcab034e92f2f6101a..c502e1590dcc1d8b06c82673e060839479d99590 100644
--- a/tools/testing/selftests/bpf/prog_tests/sockmap_basic.c
+++ b/tools/testing/selftests/bpf/prog_tests/sockmap_basic.c
@@ -108,6 +108,35 @@ static void test_sockmap_create_update_free(enum bpf_map_type map_type)
 	close(s);
 }
 
+static void test_sockmap_vsock_delete_on_close(void)
+{
+	int err, c, p, map;
+	const int zero = 0;
+
+	err = create_pair(AF_VSOCK, SOCK_STREAM, &c, &p);
+	if (!ASSERT_OK(err, "create_pair(AF_VSOCK)"))
+		return;
+
+	map = bpf_map_create(BPF_MAP_TYPE_SOCKMAP, NULL, sizeof(int),
+			     sizeof(int), 1, NULL);
+	if (!ASSERT_GE(map, 0, "bpf_map_create")) {
+		close(c);
+		goto out;
+	}
+
+	err = bpf_map_update_elem(map, &zero, &c, BPF_NOEXIST);
+	close(c);
+	if (!ASSERT_OK(err, "bpf_map_update"))
+		goto out;
+
+	err = bpf_map_update_elem(map, &zero, &p, BPF_NOEXIST);
+	ASSERT_OK(err, "after close(), bpf_map_update");
+
+out:
+	close(p);
+	close(map);
+}
+
 static void test_skmsg_helpers(enum bpf_map_type map_type)
 {
 	struct test_skmsg_load_helpers *skel;
@@ -935,6 +964,8 @@ void test_sockmap_basic(void)
 		test_sockmap_create_update_free(BPF_MAP_TYPE_SOCKMAP);
 	if (test__start_subtest("sockhash create_update_free"))
 		test_sockmap_create_update_free(BPF_MAP_TYPE_SOCKHASH);
+	if (test__start_subtest("sockmap vsock delete on close"))
+		test_sockmap_vsock_delete_on_close();
 	if (test__start_subtest("sockmap sk_msg load helpers"))
 		test_skmsg_helpers(BPF_MAP_TYPE_SOCKMAP);
 	if (test__start_subtest("sockhash sk_msg load helpers"))

-- 
2.46.2

RE: [PATCH bpf 0/4] bpf, vsock: Fix poll() and close()

[John Fastabend]

Michal Luczaj wrote:
> Two small fixes for vsock: poll() missing a queue check, and close() not
> invoking sockmap cleanup.
> 
> Signed-off-by: Michal Luczaj <mhal@rbox.co>
> ---
> Michal Luczaj (4):
>       bpf, vsock: Fix poll() missing a queue
>       selftest/bpf: Add test for af_vsock poll()
>       bpf, vsock: Invoke proto::close on close()
>       selftest/bpf: Add test for vsock removal from sockmap on close()
> 
>  net/vmw_vsock/af_vsock.c                           | 70 ++++++++++++--------
>  .../selftests/bpf/prog_tests/sockmap_basic.c       | 77 ++++++++++++++++++++++
>  2 files changed, 120 insertions(+), 27 deletions(-)
> ---
> base-commit: 6c4139b0f19b7397286897caee379f8321e78272
> change-id: 20241118-vsock-bpf-poll-close-64f432e682ec
> 
> Best regards,
> -- 
> Michal Luczaj <mhal@rbox.co>
> 

LGTM, would be nice to get an ack from someone on the vsock side
though.

Acked-by: John Fastabend <john.fastabend@gmail.com>

Re: [PATCH bpf 0/4] bpf, vsock: Fix poll() and close()

[Stefano Garzarella]

On Wed, Nov 20, 2024 at 10:48:25PM -0800, John Fastabend wrote:
>Michal Luczaj wrote:
>> Two small fixes for vsock: poll() missing a queue check, and close() not
>> invoking sockmap cleanup.
>>
>> Signed-off-by: Michal Luczaj <mhal@rbox.co>
>> ---
>> Michal Luczaj (4):
>>       bpf, vsock: Fix poll() missing a queue
>>       selftest/bpf: Add test for af_vsock poll()
>>       bpf, vsock: Invoke proto::close on close()
>>       selftest/bpf: Add test for vsock removal from sockmap on close()
>>
>>  net/vmw_vsock/af_vsock.c                           | 70 ++++++++++++--------
>>  .../selftests/bpf/prog_tests/sockmap_basic.c       | 77 ++++++++++++++++++++++
>>  2 files changed, 120 insertions(+), 27 deletions(-)
>> ---
>> base-commit: 6c4139b0f19b7397286897caee379f8321e78272
>> change-id: 20241118-vsock-bpf-poll-close-64f432e682ec
>>
>> Best regards,
>> --
>> Michal Luczaj <mhal@rbox.co>
>>
>
>LGTM, would be nice to get an ack from someone on the vsock side
>though.

Sorry, is at the top of my list but other urgent things have come up.

I will review it by today.

Stefano

>
>Acked-by: John Fastabend <john.fastabend@gmail.com>
>

Re: [PATCH bpf 1/4] bpf, vsock: Fix poll() missing a queue

[Stefano Garzarella]

On Mon, Nov 18, 2024 at 10:03:41PM +0100, Michal Luczaj wrote:
>When a verdict program simply passes a packet without redirection, sk_msg
>is enqueued on sk_psock::ingress_msg. Add a missing check to poll().
>
>Fixes: 634f1a7110b4 ("vsock: support sockmap")
>Signed-off-by: Michal Luczaj <mhal@rbox.co>
>---
> net/vmw_vsock/af_vsock.c | 3 +++
> 1 file changed, 3 insertions(+)

Yep, in vsock_bpf.c we set `prot->sock_is_readable = sk_msg_is_readable`,
so it LGTM!

Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>

>
>diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c
>index dfd29160fe11c4675f872c1ee123d65b2da0dae6..919da8edd03c838cbcdbf1618425da6c5ec2df1a 100644
>--- a/net/vmw_vsock/af_vsock.c
>+++ b/net/vmw_vsock/af_vsock.c
>@@ -1054,6 +1054,9 @@ static __poll_t vsock_poll(struct file *file, struct socket *sock,
> 		mask |= EPOLLRDHUP;
> 	}
>
>+	if (sk_is_readable(sk))
>+		mask |= EPOLLIN | EPOLLRDNORM;
>+
> 	if (sock->type == SOCK_DGRAM) {
> 		/* For datagram sockets we can read if there is something in
> 		 * the queue and write as long as the socket isn't shutdown for
>
>-- 
>2.46.2
>

Re: [PATCH bpf 2/4] selftest/bpf: Add test for af_vsock poll()

[Stefano Garzarella]

On Mon, Nov 18, 2024 at 10:03:42PM +0100, Michal Luczaj wrote:
>Verify that vsock's poll() notices when sk_psock::ingress_msg isn't empty.
>
>Signed-off-by: Michal Luczaj <mhal@rbox.co>
>---
> .../selftests/bpf/prog_tests/sockmap_basic.c       | 46 ++++++++++++++++++++++
> 1 file changed, 46 insertions(+)

Acked-by: Stefano Garzarella <sgarzare@redhat.com>

>
>diff --git a/tools/testing/selftests/bpf/prog_tests/sockmap_basic.c b/tools/testing/selftests/bpf/prog_tests/sockmap_basic.c
>index 82bfb266741cfaceafa2a407cd2ccc937708c613..21d1e2e2308433e7475952dcab034e92f2f6101a 100644
>--- a/tools/testing/selftests/bpf/prog_tests/sockmap_basic.c
>+++ b/tools/testing/selftests/bpf/prog_tests/sockmap_basic.c
>@@ -885,6 +885,50 @@ static void test_sockmap_same_sock(void)
> 	test_sockmap_pass_prog__destroy(skel);
> }
>
>+static void test_sockmap_skb_verdict_vsock_poll(void)
>+{
>+	struct test_sockmap_pass_prog *skel;
>+	int err, map, conn, peer;
>+	struct bpf_program *prog;
>+	struct bpf_link *link;
>+	char buf = 'x';
>+	int zero = 0;
>+
>+	skel = test_sockmap_pass_prog__open_and_load();
>+	if (!ASSERT_OK_PTR(skel, "open_and_load"))
>+		return;
>+
>+	if (create_pair(AF_VSOCK, SOCK_STREAM, &conn, &peer))
>+		goto destroy;
>+
>+	prog = skel->progs.prog_skb_verdict;
>+	map = bpf_map__fd(skel->maps.sock_map_rx);
>+	link = bpf_program__attach_sockmap(prog, map);
>+	if (!ASSERT_OK_PTR(link, "bpf_program__attach_sockmap"))
>+		goto close;
>+
>+	err = bpf_map_update_elem(map, &zero, &conn, BPF_ANY);
>+	if (!ASSERT_OK(err, "bpf_map_update_elem"))
>+		goto detach;
>+
>+	if (xsend(peer, &buf, 1, 0) != 1)
>+		goto detach;
>+
>+	err = poll_read(conn, IO_TIMEOUT_SEC);
>+	if (!ASSERT_OK(err, "poll"))
>+		goto detach;
>+
>+	if (xrecv_nonblock(conn, &buf, 1, 0) != 1)
>+		FAIL("xrecv_nonblock");
>+detach:
>+	bpf_link__detach(link);
>+close:
>+	xclose(conn);
>+	xclose(peer);
>+destroy:
>+	test_sockmap_pass_prog__destroy(skel);
>+}
>+
> void test_sockmap_basic(void)
> {
> 	if (test__start_subtest("sockmap create_update_free"))
>@@ -943,4 +987,6 @@ void test_sockmap_basic(void)
> 		test_skmsg_helpers_with_link(BPF_MAP_TYPE_SOCKMAP);
> 	if (test__start_subtest("sockhash sk_msg attach sockhash helpers with link"))
> 		test_skmsg_helpers_with_link(BPF_MAP_TYPE_SOCKHASH);
>+	if (test__start_subtest("sockmap skb_verdict vsock poll"))
>+		test_sockmap_skb_verdict_vsock_poll();
> }
>
>-- 
>2.46.2
>

Re: [PATCH bpf 3/4] bpf, vsock: Invoke proto::close on close()

[Stefano Garzarella]

On Mon, Nov 18, 2024 at 10:03:43PM +0100, Michal Luczaj wrote:
>vsock defines a BPF callback to be invoked when close() is called. However,
>this callback is never actually executed. As a result, a closed vsock
>socket is not automatically removed from the sockmap/sockhash.
>
>Introduce a dummy vsock_close() and make vsock_release() call proto::close.
>
>Note: changes in __vsock_release() look messy, but it's only due to indent
>level reduction and variables xmas tree reorder.
>
>Fixes: 634f1a7110b4 ("vsock: support sockmap")
>Signed-off-by: Michal Luczaj <mhal@rbox.co>
>---
> net/vmw_vsock/af_vsock.c | 67 +++++++++++++++++++++++++++++-------------------
> 1 file changed, 40 insertions(+), 27 deletions(-)
>
>diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c
>index 919da8edd03c838cbcdbf1618425da6c5ec2df1a..b52b798aa4c2926c3f233aad6cd31b4056f6fee2 100644
>--- a/net/vmw_vsock/af_vsock.c
>+++ b/net/vmw_vsock/af_vsock.c
>@@ -117,12 +117,14 @@
> static int __vsock_bind(struct sock *sk, struct sockaddr_vm *addr);
> static void vsock_sk_destruct(struct sock *sk);
> static int vsock_queue_rcv_skb(struct sock *sk, struct sk_buff *skb);
>+static void vsock_close(struct sock *sk, long timeout);
>
> /* Protocol family. */
> struct proto vsock_proto = {
> 	.name = "AF_VSOCK",
> 	.owner = THIS_MODULE,
> 	.obj_size = sizeof(struct vsock_sock),
>+	.close = vsock_close,
> #ifdef CONFIG_BPF_SYSCALL
> 	.psock_update_sk_prot = vsock_bpf_update_proto,
> #endif
>@@ -797,39 +799,37 @@ static bool sock_type_connectible(u16 type)
>
> static void __vsock_release(struct sock *sk, int level)
> {
>-	if (sk) {
>-		struct sock *pending;
>-		struct vsock_sock *vsk;
>-
>-		vsk = vsock_sk(sk);
>-		pending = NULL;	/* Compiler warning. */
>+	struct vsock_sock *vsk;
>+	struct sock *pending;
>
>-		/* When "level" is SINGLE_DEPTH_NESTING, use the nested
>-		 * version to avoid the warning "possible recursive locking
>-		 * detected". When "level" is 0, lock_sock_nested(sk, level)
>-		 * is the same as lock_sock(sk).
>-		 */
>-		lock_sock_nested(sk, level);
>+	vsk = vsock_sk(sk);
>+	pending = NULL;	/* Compiler warning. */
>
>-		if (vsk->transport)
>-			vsk->transport->release(vsk);
>-		else if (sock_type_connectible(sk->sk_type))
>-			vsock_remove_sock(vsk);
>+	/* When "level" is SINGLE_DEPTH_NESTING, use the nested
>+	 * version to avoid the warning "possible recursive locking
>+	 * detected". When "level" is 0, lock_sock_nested(sk, level)
>+	 * is the same as lock_sock(sk).
>+	 */
>+	lock_sock_nested(sk, level);
>
>-		sock_orphan(sk);
>-		sk->sk_shutdown = SHUTDOWN_MASK;
>+	if (vsk->transport)
>+		vsk->transport->release(vsk);
>+	else if (sock_type_connectible(sk->sk_type))
>+		vsock_remove_sock(vsk);
>
>-		skb_queue_purge(&sk->sk_receive_queue);
>+	sock_orphan(sk);
>+	sk->sk_shutdown = SHUTDOWN_MASK;
>
>-		/* Clean up any sockets that never were accepted. */
>-		while ((pending = vsock_dequeue_accept(sk)) != NULL) {
>-			__vsock_release(pending, SINGLE_DEPTH_NESTING);
>-			sock_put(pending);
>-		}
>+	skb_queue_purge(&sk->sk_receive_queue);
>
>-		release_sock(sk);
>-		sock_put(sk);
>+	/* Clean up any sockets that never were accepted. */
>+	while ((pending = vsock_dequeue_accept(sk)) != NULL) {
>+		__vsock_release(pending, SINGLE_DEPTH_NESTING);
>+		sock_put(pending);
> 	}
>+
>+	release_sock(sk);
>+	sock_put(sk);
> }
>
> static void vsock_sk_destruct(struct sock *sk)
>@@ -901,9 +901,22 @@ void vsock_data_ready(struct sock *sk)
> }
> EXPORT_SYMBOL_GPL(vsock_data_ready);
>
>+/* Dummy callback required by sockmap.
>+ * See unconditional call of saved_close() in sock_map_close().
>+ */
>+static void vsock_close(struct sock *sk, long timeout)
>+{
>+}
>+
> static int vsock_release(struct socket *sock)
> {
>-	__vsock_release(sock->sk, 0);
>+	struct sock *sk = sock->sk;
>+
>+	if (!sk)
>+		return 0;

Compared with before, now we return earlier and so we don't set SS_FREE, 
could it be risky?

I think no, because in theory we have already set it in a previous call, 
right?

Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>

>+
>+	sk->sk_prot->close(sk, 0);
>+	__vsock_release(sk, 0);
> 	sock->sk = NULL;
> 	sock->state = SS_FREE;
>
>
>-- 
>2.46.2
>

Re: [PATCH bpf 4/4] selftest/bpf: Add test for vsock removal from sockmap on close()

[Stefano Garzarella]

On Mon, Nov 18, 2024 at 10:03:44PM +0100, Michal Luczaj wrote:
>Make sure the proto::close callback gets invoked on vsock release.
>
>Signed-off-by: Michal Luczaj <mhal@rbox.co>
>---
> .../selftests/bpf/prog_tests/sockmap_basic.c       | 31 ++++++++++++++++++++++
> 1 file changed, 31 insertions(+)

Acked-by: Stefano Garzarella <sgarzare@redhat.com>

>
>diff --git a/tools/testing/selftests/bpf/prog_tests/sockmap_basic.c b/tools/testing/selftests/bpf/prog_tests/sockmap_basic.c
>index 21d1e2e2308433e7475952dcab034e92f2f6101a..c502e1590dcc1d8b06c82673e060839479d99590 100644
>--- a/tools/testing/selftests/bpf/prog_tests/sockmap_basic.c
>+++ b/tools/testing/selftests/bpf/prog_tests/sockmap_basic.c
>@@ -108,6 +108,35 @@ static void test_sockmap_create_update_free(enum bpf_map_type map_type)
> 	close(s);
> }
>
>+static void test_sockmap_vsock_delete_on_close(void)
>+{
>+	int err, c, p, map;
>+	const int zero = 0;
>+
>+	err = create_pair(AF_VSOCK, SOCK_STREAM, &c, &p);
>+	if (!ASSERT_OK(err, "create_pair(AF_VSOCK)"))
>+		return;
>+
>+	map = bpf_map_create(BPF_MAP_TYPE_SOCKMAP, NULL, sizeof(int),
>+			     sizeof(int), 1, NULL);
>+	if (!ASSERT_GE(map, 0, "bpf_map_create")) {
>+		close(c);
>+		goto out;
>+	}
>+
>+	err = bpf_map_update_elem(map, &zero, &c, BPF_NOEXIST);
>+	close(c);
>+	if (!ASSERT_OK(err, "bpf_map_update"))
>+		goto out;
>+
>+	err = bpf_map_update_elem(map, &zero, &p, BPF_NOEXIST);
>+	ASSERT_OK(err, "after close(), bpf_map_update");
>+
>+out:
>+	close(p);
>+	close(map);
>+}
>+
> static void test_skmsg_helpers(enum bpf_map_type map_type)
> {
> 	struct test_skmsg_load_helpers *skel;
>@@ -935,6 +964,8 @@ void test_sockmap_basic(void)
> 		test_sockmap_create_update_free(BPF_MAP_TYPE_SOCKMAP);
> 	if (test__start_subtest("sockhash create_update_free"))
> 		test_sockmap_create_update_free(BPF_MAP_TYPE_SOCKHASH);
>+	if (test__start_subtest("sockmap vsock delete on close"))
>+		test_sockmap_vsock_delete_on_close();
> 	if (test__start_subtest("sockmap sk_msg load helpers"))
> 		test_skmsg_helpers(BPF_MAP_TYPE_SOCKMAP);
> 	if (test__start_subtest("sockhash sk_msg load helpers"))
>
>-- 
>2.46.2
>

Re: [PATCH bpf 3/4] bpf, vsock: Invoke proto::close on close()

[Michal Luczaj]

On 11/21/24 10:22, Stefano Garzarella wrote:
> On Mon, Nov 18, 2024 at 10:03:43PM +0100, Michal Luczaj wrote:
>> vsock defines a BPF callback to be invoked when close() is called. However,
>> this callback is never actually executed. As a result, a closed vsock
>> socket is not automatically removed from the sockmap/sockhash.
>>
>> Introduce a dummy vsock_close() and make vsock_release() call proto::close.
>>
>> Note: changes in __vsock_release() look messy, but it's only due to indent
>> level reduction and variables xmas tree reorder.
>>
>> Fixes: 634f1a7110b4 ("vsock: support sockmap")
>> Signed-off-by: Michal Luczaj <mhal@rbox.co>
>> ---
>> net/vmw_vsock/af_vsock.c | 67 +++++++++++++++++++++++++++++-------------------
>> 1 file changed, 40 insertions(+), 27 deletions(-)
>>
>> diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c
>> index 919da8edd03c838cbcdbf1618425da6c5ec2df1a..b52b798aa4c2926c3f233aad6cd31b4056f6fee2 100644
>> --- a/net/vmw_vsock/af_vsock.c
>> +++ b/net/vmw_vsock/af_vsock.c
>> @@ -117,12 +117,14 @@
>> static int __vsock_bind(struct sock *sk, struct sockaddr_vm *addr);
>> static void vsock_sk_destruct(struct sock *sk);
>> static int vsock_queue_rcv_skb(struct sock *sk, struct sk_buff *skb);
>> +static void vsock_close(struct sock *sk, long timeout);
>>
>> /* Protocol family. */
>> struct proto vsock_proto = {
>> 	.name = "AF_VSOCK",
>> 	.owner = THIS_MODULE,
>> 	.obj_size = sizeof(struct vsock_sock),
>> +	.close = vsock_close,
>> #ifdef CONFIG_BPF_SYSCALL
>> 	.psock_update_sk_prot = vsock_bpf_update_proto,
>> #endif
>> @@ -797,39 +799,37 @@ static bool sock_type_connectible(u16 type)
>>
>> static void __vsock_release(struct sock *sk, int level)
>> {
>> -	if (sk) {
>> -		struct sock *pending;
>> -		struct vsock_sock *vsk;
>> -
>> -		vsk = vsock_sk(sk);
>> -		pending = NULL;	/* Compiler warning. */
>> +	struct vsock_sock *vsk;
>> +	struct sock *pending;
>>
>> -		/* When "level" is SINGLE_DEPTH_NESTING, use the nested
>> -		 * version to avoid the warning "possible recursive locking
>> -		 * detected". When "level" is 0, lock_sock_nested(sk, level)
>> -		 * is the same as lock_sock(sk).
>> -		 */
>> -		lock_sock_nested(sk, level);
>> +	vsk = vsock_sk(sk);
>> +	pending = NULL;	/* Compiler warning. */
>>
>> -		if (vsk->transport)
>> -			vsk->transport->release(vsk);
>> -		else if (sock_type_connectible(sk->sk_type))
>> -			vsock_remove_sock(vsk);
>> +	/* When "level" is SINGLE_DEPTH_NESTING, use the nested
>> +	 * version to avoid the warning "possible recursive locking
>> +	 * detected". When "level" is 0, lock_sock_nested(sk, level)
>> +	 * is the same as lock_sock(sk).
>> +	 */
>> +	lock_sock_nested(sk, level);
>>
>> -		sock_orphan(sk);
>> -		sk->sk_shutdown = SHUTDOWN_MASK;
>> +	if (vsk->transport)
>> +		vsk->transport->release(vsk);
>> +	else if (sock_type_connectible(sk->sk_type))
>> +		vsock_remove_sock(vsk);
>>
>> -		skb_queue_purge(&sk->sk_receive_queue);
>> +	sock_orphan(sk);
>> +	sk->sk_shutdown = SHUTDOWN_MASK;
>>
>> -		/* Clean up any sockets that never were accepted. */
>> -		while ((pending = vsock_dequeue_accept(sk)) != NULL) {
>> -			__vsock_release(pending, SINGLE_DEPTH_NESTING);
>> -			sock_put(pending);
>> -		}
>> +	skb_queue_purge(&sk->sk_receive_queue);
>>
>> -		release_sock(sk);
>> -		sock_put(sk);
>> +	/* Clean up any sockets that never were accepted. */
>> +	while ((pending = vsock_dequeue_accept(sk)) != NULL) {
>> +		__vsock_release(pending, SINGLE_DEPTH_NESTING);
>> +		sock_put(pending);
>> 	}
>> +
>> +	release_sock(sk);
>> +	sock_put(sk);
>> }
>>
>> static void vsock_sk_destruct(struct sock *sk)
>> @@ -901,9 +901,22 @@ void vsock_data_ready(struct sock *sk)
>> }
>> EXPORT_SYMBOL_GPL(vsock_data_ready);
>>
>> +/* Dummy callback required by sockmap.
>> + * See unconditional call of saved_close() in sock_map_close().
>> + */
>> +static void vsock_close(struct sock *sk, long timeout)
>> +{
>> +}
>> +
>> static int vsock_release(struct socket *sock)
>> {
>> -	__vsock_release(sock->sk, 0);
>> +	struct sock *sk = sock->sk;
>> +
>> +	if (!sk)
>> +		return 0;
> 
> Compared with before, now we return earlier and so we don't set SS_FREE, 
> could it be risky?
>
> I think no, because in theory we have already set it in a previous call, 
> right?

Yeah, and is there actually a way to call vsock_release() for a second
time? The only caller I see is __sock_release(), which won't allow that.

As for the sockets that never had ->sk assigned, I assume it doesn't matter.

> Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>
> 
>> +
>> +	sk->sk_prot->close(sk, 0);
>> +	__vsock_release(sk, 0);
>> 	sock->sk = NULL;
>> 	sock->state = SS_FREE;

Re: [PATCH bpf 0/4] bpf, vsock: Fix poll() and close()

[John Fastabend]

Stefano Garzarella wrote:
> On Wed, Nov 20, 2024 at 10:48:25PM -0800, John Fastabend wrote:
> >Michal Luczaj wrote:
> >> Two small fixes for vsock: poll() missing a queue check, and close() not
> >> invoking sockmap cleanup.
> >>
> >> Signed-off-by: Michal Luczaj <mhal@rbox.co>
> >> ---
> >> Michal Luczaj (4):
> >>       bpf, vsock: Fix poll() missing a queue
> >>       selftest/bpf: Add test for af_vsock poll()
> >>       bpf, vsock: Invoke proto::close on close()
> >>       selftest/bpf: Add test for vsock removal from sockmap on close()
> >>
> >>  net/vmw_vsock/af_vsock.c                           | 70 ++++++++++++--------
> >>  .../selftests/bpf/prog_tests/sockmap_basic.c       | 77 ++++++++++++++++++++++
> >>  2 files changed, 120 insertions(+), 27 deletions(-)
> >> ---
> >> base-commit: 6c4139b0f19b7397286897caee379f8321e78272
> >> change-id: 20241118-vsock-bpf-poll-close-64f432e682ec
> >>
> >> Best regards,
> >> --
> >> Michal Luczaj <mhal@rbox.co>
> >>
> >
> >LGTM, would be nice to get an ack from someone on the vsock side
> >though.
> 
> Sorry, is at the top of my list but other urgent things have come up.
> 
> I will review it by today.

Thanks a lot Stefano much appreciated! I was also slow to review as I
was travelling and on PTO.

> 
> Stefano
> 
> >
> >Acked-by: John Fastabend <john.fastabend@gmail.com>
> >
> 

Re: [PATCH bpf 3/4] bpf, vsock: Invoke proto::close on close()

[Stefano Garzarella]

On Thu, Nov 21, 2024 at 8:54 PM Michal Luczaj <mhal@rbox.co> wrote:
>
> On 11/21/24 10:22, Stefano Garzarella wrote:
> > On Mon, Nov 18, 2024 at 10:03:43PM +0100, Michal Luczaj wrote:
> >> vsock defines a BPF callback to be invoked when close() is called. However,
> >> this callback is never actually executed. As a result, a closed vsock
> >> socket is not automatically removed from the sockmap/sockhash.
> >>
> >> Introduce a dummy vsock_close() and make vsock_release() call proto::close.
> >>
> >> Note: changes in __vsock_release() look messy, but it's only due to indent
> >> level reduction and variables xmas tree reorder.
> >>
> >> Fixes: 634f1a7110b4 ("vsock: support sockmap")
> >> Signed-off-by: Michal Luczaj <mhal@rbox.co>
> >> ---
> >> net/vmw_vsock/af_vsock.c | 67 +++++++++++++++++++++++++++++-------------------
> >> 1 file changed, 40 insertions(+), 27 deletions(-)
> >>
> >> diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c
> >> index 919da8edd03c838cbcdbf1618425da6c5ec2df1a..b52b798aa4c2926c3f233aad6cd31b4056f6fee2 100644
> >> --- a/net/vmw_vsock/af_vsock.c
> >> +++ b/net/vmw_vsock/af_vsock.c
> >> @@ -117,12 +117,14 @@
> >> static int __vsock_bind(struct sock *sk, struct sockaddr_vm *addr);
> >> static void vsock_sk_destruct(struct sock *sk);
> >> static int vsock_queue_rcv_skb(struct sock *sk, struct sk_buff *skb);
> >> +static void vsock_close(struct sock *sk, long timeout);
> >>
> >> /* Protocol family. */
> >> struct proto vsock_proto = {
> >>      .name = "AF_VSOCK",
> >>      .owner = THIS_MODULE,
> >>      .obj_size = sizeof(struct vsock_sock),
> >> +    .close = vsock_close,
> >> #ifdef CONFIG_BPF_SYSCALL
> >>      .psock_update_sk_prot = vsock_bpf_update_proto,
> >> #endif
> >> @@ -797,39 +799,37 @@ static bool sock_type_connectible(u16 type)
> >>
> >> static void __vsock_release(struct sock *sk, int level)
> >> {
> >> -    if (sk) {
> >> -            struct sock *pending;
> >> -            struct vsock_sock *vsk;
> >> -
> >> -            vsk = vsock_sk(sk);
> >> -            pending = NULL; /* Compiler warning. */
> >> +    struct vsock_sock *vsk;
> >> +    struct sock *pending;
> >>
> >> -            /* When "level" is SINGLE_DEPTH_NESTING, use the nested
> >> -             * version to avoid the warning "possible recursive locking
> >> -             * detected". When "level" is 0, lock_sock_nested(sk, level)
> >> -             * is the same as lock_sock(sk).
> >> -             */
> >> -            lock_sock_nested(sk, level);
> >> +    vsk = vsock_sk(sk);
> >> +    pending = NULL; /* Compiler warning. */
> >>
> >> -            if (vsk->transport)
> >> -                    vsk->transport->release(vsk);
> >> -            else if (sock_type_connectible(sk->sk_type))
> >> -                    vsock_remove_sock(vsk);
> >> +    /* When "level" is SINGLE_DEPTH_NESTING, use the nested
> >> +     * version to avoid the warning "possible recursive locking
> >> +     * detected". When "level" is 0, lock_sock_nested(sk, level)
> >> +     * is the same as lock_sock(sk).
> >> +     */
> >> +    lock_sock_nested(sk, level);
> >>
> >> -            sock_orphan(sk);
> >> -            sk->sk_shutdown = SHUTDOWN_MASK;
> >> +    if (vsk->transport)
> >> +            vsk->transport->release(vsk);
> >> +    else if (sock_type_connectible(sk->sk_type))
> >> +            vsock_remove_sock(vsk);
> >>
> >> -            skb_queue_purge(&sk->sk_receive_queue);
> >> +    sock_orphan(sk);
> >> +    sk->sk_shutdown = SHUTDOWN_MASK;
> >>
> >> -            /* Clean up any sockets that never were accepted. */
> >> -            while ((pending = vsock_dequeue_accept(sk)) != NULL) {
> >> -                    __vsock_release(pending, SINGLE_DEPTH_NESTING);
> >> -                    sock_put(pending);
> >> -            }
> >> +    skb_queue_purge(&sk->sk_receive_queue);
> >>
> >> -            release_sock(sk);
> >> -            sock_put(sk);
> >> +    /* Clean up any sockets that never were accepted. */
> >> +    while ((pending = vsock_dequeue_accept(sk)) != NULL) {
> >> +            __vsock_release(pending, SINGLE_DEPTH_NESTING);
> >> +            sock_put(pending);
> >>      }
> >> +
> >> +    release_sock(sk);
> >> +    sock_put(sk);
> >> }
> >>
> >> static void vsock_sk_destruct(struct sock *sk)
> >> @@ -901,9 +901,22 @@ void vsock_data_ready(struct sock *sk)
> >> }
> >> EXPORT_SYMBOL_GPL(vsock_data_ready);
> >>
> >> +/* Dummy callback required by sockmap.
> >> + * See unconditional call of saved_close() in sock_map_close().
> >> + */
> >> +static void vsock_close(struct sock *sk, long timeout)
> >> +{
> >> +}
> >> +
> >> static int vsock_release(struct socket *sock)
> >> {
> >> -    __vsock_release(sock->sk, 0);
> >> +    struct sock *sk = sock->sk;
> >> +
> >> +    if (!sk)
> >> +            return 0;
> >
> > Compared with before, now we return earlier and so we don't set SS_FREE,
> > could it be risky?
> >
> > I think no, because in theory we have already set it in a previous call,
> > right?
>
> Yeah, and is there actually a way to call vsock_release() for a second
> time? The only caller I see is __sock_release(), which won't allow that.

Maybe no, but the `sock->sk` check made me think so.

>
> As for the sockets that never had ->sk assigned, I assume it doesn't matter.

Yep, so my R-b stays here ;-)

Thanks for these great fixes,
Stefano

>
> > Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>
> >
> >> +
> >> +    sk->sk_prot->close(sk, 0);
> >> +    __vsock_release(sk, 0);
> >>      sock->sk = NULL;
> >>      sock->state = SS_FREE;
>

Re: [PATCH bpf 0/4] bpf, vsock: Fix poll() and close()

[Stefano Garzarella]

On Fri, Nov 22, 2024 at 12:34 AM John Fastabend
<john.fastabend@gmail.com> wrote:
>
> Stefano Garzarella wrote:
> > On Wed, Nov 20, 2024 at 10:48:25PM -0800, John Fastabend wrote:
> > >Michal Luczaj wrote:
> > >> Two small fixes for vsock: poll() missing a queue check, and close() not
> > >> invoking sockmap cleanup.
> > >>
> > >> Signed-off-by: Michal Luczaj <mhal@rbox.co>
> > >> ---
> > >> Michal Luczaj (4):
> > >>       bpf, vsock: Fix poll() missing a queue
> > >>       selftest/bpf: Add test for af_vsock poll()
> > >>       bpf, vsock: Invoke proto::close on close()
> > >>       selftest/bpf: Add test for vsock removal from sockmap on close()
> > >>
> > >>  net/vmw_vsock/af_vsock.c                           | 70 ++++++++++++--------
> > >>  .../selftests/bpf/prog_tests/sockmap_basic.c       | 77 ++++++++++++++++++++++
> > >>  2 files changed, 120 insertions(+), 27 deletions(-)
> > >> ---
> > >> base-commit: 6c4139b0f19b7397286897caee379f8321e78272
> > >> change-id: 20241118-vsock-bpf-poll-close-64f432e682ec
> > >>
> > >> Best regards,
> > >> --
> > >> Michal Luczaj <mhal@rbox.co>
> > >>
> > >
> > >LGTM, would be nice to get an ack from someone on the vsock side
> > >though.
> >
> > Sorry, is at the top of my list but other urgent things have come up.
> >
> > I will review it by today.
>
> Thanks a lot Stefano much appreciated! I was also slow to review as I
> was travelling and on PTO.
>

You're welcome :-) Thanks for reviewing the bpf part!

Hope you enjoyed your PTO!

Ciao,
Stefano

> >
> > Stefano
> >
> > >
> > >Acked-by: John Fastabend <john.fastabend@gmail.com>
> > >
> >
>
>

Re: [PATCH bpf 3/4] bpf, vsock: Invoke proto::close on close()

[Luigi Leonardi]

I spent some time checking and nobody but __sock_create (net/socket.c)
and vsock_release can set sock->sk to NULL.

I also ran checkpatch, everything LGTM.
Thanks for the fix!

Reviewed-by: Luigi Leonardi <leonardi@redhat.com>

Re: [PATCH bpf 1/4] bpf, vsock: Fix poll() missing a queue

[Luigi Leonardi]

>When a verdict program simply passes a packet without redirection, sk_msg
>is enqueued on sk_psock::ingress_msg. Add a missing check to poll().
>
>Fixes: 634f1a7110b4 ("vsock: support sockmap")
>Signed-off-by: Michal Luczaj <mhal@rbox.co>
>---
> net/vmw_vsock/af_vsock.c | 3 +++
> 1 file changed, 3 insertions(+)
>
>diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c
>index dfd29160fe11c4675f872c1ee123d65b2da0dae6..919da8edd03c838cbcdbf1618425da6c5ec2df1a 100644
>--- a/net/vmw_vsock/af_vsock.c
>+++ b/net/vmw_vsock/af_vsock.c
>@@ -1054,6 +1054,9 @@ static __poll_t vsock_poll(struct file *file, struct socket *sock,
> 		mask |= EPOLLRDHUP;
> 	}
> 
>+	if (sk_is_readable(sk))
>+		mask |= EPOLLIN | EPOLLRDNORM;
>+
> 	if (sock->type == SOCK_DGRAM) {
> 		/* For datagram sockets we can read if there is something in
> 		 * the queue and write as long as the socket isn't shutdown for

LGTM, thanks!

Reviewed-by: Luigi Leonardi <leonardi@redhat.com>

Re: [PATCH bpf 0/4] bpf, vsock: Fix poll() and close()

[patchwork-bot+netdevbpf]

Hello:

This series was applied to bpf/bpf.git (master)
by Alexei Starovoitov <ast@kernel.org>:

On Mon, 18 Nov 2024 22:03:40 +0100 you wrote:
> Two small fixes for vsock: poll() missing a queue check, and close() not
> invoking sockmap cleanup.
> 
> Signed-off-by: Michal Luczaj <mhal@rbox.co>
> ---
> Michal Luczaj (4):
>       bpf, vsock: Fix poll() missing a queue
>       selftest/bpf: Add test for af_vsock poll()
>       bpf, vsock: Invoke proto::close on close()
>       selftest/bpf: Add test for vsock removal from sockmap on close()
> 
> [...]

Here is the summary with links:
  - [bpf,1/4] bpf, vsock: Fix poll() missing a queue
    https://git.kernel.org/bpf/bpf/c/9f0fc9814521
  - [bpf,2/4] selftest/bpf: Add test for af_vsock poll()
    https://git.kernel.org/bpf/bpf/c/9c2a2a45136d
  - [bpf,3/4] bpf, vsock: Invoke proto::close on close()
    https://git.kernel.org/bpf/bpf/c/135ffc7becc8
  - [bpf,4/4] selftest/bpf: Add test for vsock removal from sockmap on close()
    https://git.kernel.org/bpf/bpf/c/515745445e92

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html