* [PATCH bpf 1/4] bpf, vsock: Fix poll() missing a queue
2024-11-18 21:03 [PATCH bpf 0/4] bpf, vsock: Fix poll() and close() Michal Luczaj
@ 2024-11-18 21:03 ` Michal Luczaj
2024-11-21 9:20 ` Stefano Garzarella
2024-11-22 17:26 ` Luigi Leonardi
2024-11-18 21:03 ` [PATCH bpf 2/4] selftest/bpf: Add test for af_vsock poll() Michal Luczaj
` (4 subsequent siblings)
5 siblings, 2 replies; 18+ messages in thread
From: Michal Luczaj @ 2024-11-18 21:03 UTC (permalink / raw)
To: Stefano Garzarella, David S. Miller, Eric Dumazet, Jakub Kicinski,
Paolo Abeni, Simon Horman, Bobby Eshleman, Michael S. Tsirkin,
Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
Martin KaFai Lau, Eduard Zingerman, Song Liu, Yonghong Song,
John Fastabend, KP Singh, Stanislav Fomichev, Hao Luo, Jiri Olsa,
Mykola Lysenko, Shuah Khan
Cc: netdev, bpf, linux-kselftest, 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
^ permalink raw reply related [flat|nested] 18+ messages in thread* Re: [PATCH bpf 1/4] bpf, vsock: Fix poll() missing a queue
2024-11-18 21:03 ` [PATCH bpf 1/4] bpf, vsock: Fix poll() missing a queue Michal Luczaj
@ 2024-11-21 9:20 ` Stefano Garzarella
2024-11-22 17:26 ` Luigi Leonardi
1 sibling, 0 replies; 18+ messages in thread
From: Stefano Garzarella @ 2024-11-21 9:20 UTC (permalink / raw)
To: Michal Luczaj
Cc: David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Simon Horman, Bobby Eshleman, Michael S. Tsirkin,
Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
Martin KaFai Lau, Eduard Zingerman, Song Liu, Yonghong Song,
John Fastabend, KP Singh, Stanislav Fomichev, Hao Luo, Jiri Olsa,
Mykola Lysenko, Shuah Khan, netdev, bpf, linux-kselftest
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
>
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH bpf 1/4] bpf, vsock: Fix poll() missing a queue
2024-11-18 21:03 ` [PATCH bpf 1/4] bpf, vsock: Fix poll() missing a queue Michal Luczaj
2024-11-21 9:20 ` Stefano Garzarella
@ 2024-11-22 17:26 ` Luigi Leonardi
1 sibling, 0 replies; 18+ messages in thread
From: Luigi Leonardi @ 2024-11-22 17:26 UTC (permalink / raw)
To: mhal
Cc: andrii, ast, bobby.eshleman, bpf, daniel, davem, eddyz87,
edumazet, haoluo, horms, john.fastabend, jolsa, kpsingh, kuba,
linux-kselftest, martin.lau, mst, mykolal, netdev, pabeni, sdf,
sgarzare, shuah, song, yonghong.song, 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>
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH bpf 2/4] selftest/bpf: Add test for af_vsock poll()
2024-11-18 21:03 [PATCH bpf 0/4] bpf, vsock: Fix poll() and close() Michal Luczaj
2024-11-18 21:03 ` [PATCH bpf 1/4] bpf, vsock: Fix poll() missing a queue Michal Luczaj
@ 2024-11-18 21:03 ` Michal Luczaj
2024-11-21 9:21 ` Stefano Garzarella
2024-11-18 21:03 ` [PATCH bpf 3/4] bpf, vsock: Invoke proto::close on close() Michal Luczaj
` (3 subsequent siblings)
5 siblings, 1 reply; 18+ messages in thread
From: Michal Luczaj @ 2024-11-18 21:03 UTC (permalink / raw)
To: Stefano Garzarella, David S. Miller, Eric Dumazet, Jakub Kicinski,
Paolo Abeni, Simon Horman, Bobby Eshleman, Michael S. Tsirkin,
Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
Martin KaFai Lau, Eduard Zingerman, Song Liu, Yonghong Song,
John Fastabend, KP Singh, Stanislav Fomichev, Hao Luo, Jiri Olsa,
Mykola Lysenko, Shuah Khan
Cc: netdev, bpf, linux-kselftest, 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
^ permalink raw reply related [flat|nested] 18+ messages in thread* Re: [PATCH bpf 2/4] selftest/bpf: Add test for af_vsock poll()
2024-11-18 21:03 ` [PATCH bpf 2/4] selftest/bpf: Add test for af_vsock poll() Michal Luczaj
@ 2024-11-21 9:21 ` Stefano Garzarella
0 siblings, 0 replies; 18+ messages in thread
From: Stefano Garzarella @ 2024-11-21 9:21 UTC (permalink / raw)
To: Michal Luczaj
Cc: David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Simon Horman, Bobby Eshleman, Michael S. Tsirkin,
Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
Martin KaFai Lau, Eduard Zingerman, Song Liu, Yonghong Song,
John Fastabend, KP Singh, Stanislav Fomichev, Hao Luo, Jiri Olsa,
Mykola Lysenko, Shuah Khan, netdev, bpf, linux-kselftest
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
>
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH bpf 3/4] bpf, vsock: Invoke proto::close on close()
2024-11-18 21:03 [PATCH bpf 0/4] bpf, vsock: Fix poll() and close() Michal Luczaj
2024-11-18 21:03 ` [PATCH bpf 1/4] bpf, vsock: Fix poll() missing a queue Michal Luczaj
2024-11-18 21:03 ` [PATCH bpf 2/4] selftest/bpf: Add test for af_vsock poll() Michal Luczaj
@ 2024-11-18 21:03 ` Michal Luczaj
2024-11-21 9:22 ` Stefano Garzarella
2024-11-22 16:20 ` Luigi Leonardi
2024-11-18 21:03 ` [PATCH bpf 4/4] selftest/bpf: Add test for vsock removal from sockmap " Michal Luczaj
` (2 subsequent siblings)
5 siblings, 2 replies; 18+ messages in thread
From: Michal Luczaj @ 2024-11-18 21:03 UTC (permalink / raw)
To: Stefano Garzarella, David S. Miller, Eric Dumazet, Jakub Kicinski,
Paolo Abeni, Simon Horman, Bobby Eshleman, Michael S. Tsirkin,
Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
Martin KaFai Lau, Eduard Zingerman, Song Liu, Yonghong Song,
John Fastabend, KP Singh, Stanislav Fomichev, Hao Luo, Jiri Olsa,
Mykola Lysenko, Shuah Khan
Cc: netdev, bpf, linux-kselftest, 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
^ permalink raw reply related [flat|nested] 18+ messages in thread* Re: [PATCH bpf 3/4] bpf, vsock: Invoke proto::close on close()
2024-11-18 21:03 ` [PATCH bpf 3/4] bpf, vsock: Invoke proto::close on close() Michal Luczaj
@ 2024-11-21 9:22 ` Stefano Garzarella
2024-11-21 19:54 ` Michal Luczaj
2024-11-22 16:20 ` Luigi Leonardi
1 sibling, 1 reply; 18+ messages in thread
From: Stefano Garzarella @ 2024-11-21 9:22 UTC (permalink / raw)
To: Michal Luczaj
Cc: David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Simon Horman, Bobby Eshleman, Michael S. Tsirkin,
Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
Martin KaFai Lau, Eduard Zingerman, Song Liu, Yonghong Song,
John Fastabend, KP Singh, Stanislav Fomichev, Hao Luo, Jiri Olsa,
Mykola Lysenko, Shuah Khan, netdev, bpf, linux-kselftest
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
>
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH bpf 3/4] bpf, vsock: Invoke proto::close on close()
2024-11-21 9:22 ` Stefano Garzarella
@ 2024-11-21 19:54 ` Michal Luczaj
2024-11-22 8:13 ` Stefano Garzarella
0 siblings, 1 reply; 18+ messages in thread
From: Michal Luczaj @ 2024-11-21 19:54 UTC (permalink / raw)
To: Stefano Garzarella
Cc: David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Simon Horman, Bobby Eshleman, Michael S. Tsirkin,
Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
Martin KaFai Lau, Eduard Zingerman, Song Liu, Yonghong Song,
John Fastabend, KP Singh, Stanislav Fomichev, Hao Luo, Jiri Olsa,
Mykola Lysenko, Shuah Khan, netdev, bpf, linux-kselftest
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;
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH bpf 3/4] bpf, vsock: Invoke proto::close on close()
2024-11-21 19:54 ` Michal Luczaj
@ 2024-11-22 8:13 ` Stefano Garzarella
0 siblings, 0 replies; 18+ messages in thread
From: Stefano Garzarella @ 2024-11-22 8:13 UTC (permalink / raw)
To: Michal Luczaj
Cc: David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Simon Horman, Bobby Eshleman, Michael S. Tsirkin,
Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
Martin KaFai Lau, Eduard Zingerman, Song Liu, Yonghong Song,
John Fastabend, KP Singh, Stanislav Fomichev, Hao Luo, Jiri Olsa,
Mykola Lysenko, Shuah Khan, netdev, bpf, linux-kselftest
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;
>
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH bpf 3/4] bpf, vsock: Invoke proto::close on close()
2024-11-18 21:03 ` [PATCH bpf 3/4] bpf, vsock: Invoke proto::close on close() Michal Luczaj
2024-11-21 9:22 ` Stefano Garzarella
@ 2024-11-22 16:20 ` Luigi Leonardi
1 sibling, 0 replies; 18+ messages in thread
From: Luigi Leonardi @ 2024-11-22 16:20 UTC (permalink / raw)
To: mhal
Cc: andrii, ast, bobby.eshleman, bpf, daniel, davem, eddyz87,
edumazet, haoluo, horms, john.fastabend, jolsa, kpsingh, kuba,
linux-kselftest, martin.lau, mst, mykolal, netdev, pabeni, sdf,
sgarzare, shuah, song, yonghong.song, 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>
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH bpf 4/4] selftest/bpf: Add test for vsock removal from sockmap on close()
2024-11-18 21:03 [PATCH bpf 0/4] bpf, vsock: Fix poll() and close() Michal Luczaj
` (2 preceding siblings ...)
2024-11-18 21:03 ` [PATCH bpf 3/4] bpf, vsock: Invoke proto::close on close() Michal Luczaj
@ 2024-11-18 21:03 ` Michal Luczaj
2024-11-21 9:22 ` Stefano Garzarella
2024-11-21 6:48 ` [PATCH bpf 0/4] bpf, vsock: Fix poll() and close() John Fastabend
2024-11-25 22:30 ` patchwork-bot+netdevbpf
5 siblings, 1 reply; 18+ messages in thread
From: Michal Luczaj @ 2024-11-18 21:03 UTC (permalink / raw)
To: Stefano Garzarella, David S. Miller, Eric Dumazet, Jakub Kicinski,
Paolo Abeni, Simon Horman, Bobby Eshleman, Michael S. Tsirkin,
Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
Martin KaFai Lau, Eduard Zingerman, Song Liu, Yonghong Song,
John Fastabend, KP Singh, Stanislav Fomichev, Hao Luo, Jiri Olsa,
Mykola Lysenko, Shuah Khan
Cc: netdev, bpf, linux-kselftest, 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
^ permalink raw reply related [flat|nested] 18+ messages in thread* Re: [PATCH bpf 4/4] selftest/bpf: Add test for vsock removal from sockmap on close()
2024-11-18 21:03 ` [PATCH bpf 4/4] selftest/bpf: Add test for vsock removal from sockmap " Michal Luczaj
@ 2024-11-21 9:22 ` Stefano Garzarella
0 siblings, 0 replies; 18+ messages in thread
From: Stefano Garzarella @ 2024-11-21 9:22 UTC (permalink / raw)
To: Michal Luczaj
Cc: David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Simon Horman, Bobby Eshleman, Michael S. Tsirkin,
Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
Martin KaFai Lau, Eduard Zingerman, Song Liu, Yonghong Song,
John Fastabend, KP Singh, Stanislav Fomichev, Hao Luo, Jiri Olsa,
Mykola Lysenko, Shuah Khan, netdev, bpf, linux-kselftest
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
>
^ permalink raw reply [flat|nested] 18+ messages in thread
* RE: [PATCH bpf 0/4] bpf, vsock: Fix poll() and close()
2024-11-18 21:03 [PATCH bpf 0/4] bpf, vsock: Fix poll() and close() Michal Luczaj
` (3 preceding siblings ...)
2024-11-18 21:03 ` [PATCH bpf 4/4] selftest/bpf: Add test for vsock removal from sockmap " Michal Luczaj
@ 2024-11-21 6:48 ` John Fastabend
2024-11-21 8:08 ` Stefano Garzarella
2024-11-25 22:30 ` patchwork-bot+netdevbpf
5 siblings, 1 reply; 18+ messages in thread
From: John Fastabend @ 2024-11-21 6:48 UTC (permalink / raw)
To: Michal Luczaj, Stefano Garzarella, David S. Miller, Eric Dumazet,
Jakub Kicinski, Paolo Abeni, Simon Horman, Bobby Eshleman,
Michael S. Tsirkin, Alexei Starovoitov, Daniel Borkmann,
Andrii Nakryiko, Martin KaFai Lau, Eduard Zingerman, Song Liu,
Yonghong Song, John Fastabend, KP Singh, Stanislav Fomichev,
Hao Luo, Jiri Olsa, Mykola Lysenko, Shuah Khan
Cc: netdev, bpf, linux-kselftest, Michal Luczaj
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>
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH bpf 0/4] bpf, vsock: Fix poll() and close()
2024-11-21 6:48 ` [PATCH bpf 0/4] bpf, vsock: Fix poll() and close() John Fastabend
@ 2024-11-21 8:08 ` Stefano Garzarella
2024-11-21 23:34 ` John Fastabend
0 siblings, 1 reply; 18+ messages in thread
From: Stefano Garzarella @ 2024-11-21 8:08 UTC (permalink / raw)
To: John Fastabend
Cc: Michal Luczaj, David S. Miller, Eric Dumazet, Jakub Kicinski,
Paolo Abeni, Simon Horman, Bobby Eshleman, Michael S. Tsirkin,
Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
Martin KaFai Lau, Eduard Zingerman, Song Liu, Yonghong Song,
KP Singh, Stanislav Fomichev, Hao Luo, Jiri Olsa, Mykola Lysenko,
Shuah Khan, netdev, bpf, linux-kselftest
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>
>
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH bpf 0/4] bpf, vsock: Fix poll() and close()
2024-11-21 8:08 ` Stefano Garzarella
@ 2024-11-21 23:34 ` John Fastabend
2024-11-22 8:16 ` Stefano Garzarella
0 siblings, 1 reply; 18+ messages in thread
From: John Fastabend @ 2024-11-21 23:34 UTC (permalink / raw)
To: Stefano Garzarella, John Fastabend
Cc: Michal Luczaj, David S. Miller, Eric Dumazet, Jakub Kicinski,
Paolo Abeni, Simon Horman, Bobby Eshleman, Michael S. Tsirkin,
Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
Martin KaFai Lau, Eduard Zingerman, Song Liu, Yonghong Song,
KP Singh, Stanislav Fomichev, Hao Luo, Jiri Olsa, Mykola Lysenko,
Shuah Khan, netdev, bpf, linux-kselftest
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>
> >
>
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH bpf 0/4] bpf, vsock: Fix poll() and close()
2024-11-21 23:34 ` John Fastabend
@ 2024-11-22 8:16 ` Stefano Garzarella
0 siblings, 0 replies; 18+ messages in thread
From: Stefano Garzarella @ 2024-11-22 8:16 UTC (permalink / raw)
To: John Fastabend
Cc: Michal Luczaj, David S. Miller, Eric Dumazet, Jakub Kicinski,
Paolo Abeni, Simon Horman, Bobby Eshleman, Michael S. Tsirkin,
Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
Martin KaFai Lau, Eduard Zingerman, Song Liu, Yonghong Song,
KP Singh, Stanislav Fomichev, Hao Luo, Jiri Olsa, Mykola Lysenko,
Shuah Khan, netdev, bpf, linux-kselftest
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>
> > >
> >
>
>
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH bpf 0/4] bpf, vsock: Fix poll() and close()
2024-11-18 21:03 [PATCH bpf 0/4] bpf, vsock: Fix poll() and close() Michal Luczaj
` (4 preceding siblings ...)
2024-11-21 6:48 ` [PATCH bpf 0/4] bpf, vsock: Fix poll() and close() John Fastabend
@ 2024-11-25 22:30 ` patchwork-bot+netdevbpf
5 siblings, 0 replies; 18+ messages in thread
From: patchwork-bot+netdevbpf @ 2024-11-25 22:30 UTC (permalink / raw)
To: Michal Luczaj
Cc: sgarzare, davem, edumazet, kuba, pabeni, horms, bobby.eshleman,
mst, ast, daniel, andrii, martin.lau, eddyz87, song,
yonghong.song, john.fastabend, kpsingh, sdf, haoluo, jolsa,
mykolal, shuah, netdev, bpf, linux-kselftest
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
^ permalink raw reply [flat|nested] 18+ messages in thread