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,
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
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
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
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(); }
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
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;
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
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;
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;
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
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"))
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();-- 2.46.2
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
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
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
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
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!
linux-kselftest-mirror@lists.linaro.org
-
John Fastabend -
Luigi Leonardi -
Michal Luczaj -
patchwork-bot+netdevbpf＠kernel.org -
Stefano Garzarella