| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <350e3a3f-7ebd-471e-95fa-05225d786f1c@rbox.co>
Date: Thu, 21 Nov 2024 20:54:19 +0100
From: Michal Luczaj <mhal@...x.co>
To: Stefano Garzarella <sgarzare@...hat.com>
Cc: "David S. Miller" <davem@...emloft.net>,
Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>,
Paolo Abeni <pabeni@...hat.com>, Simon Horman <horms@...nel.org>,
Bobby Eshleman <bobby.eshleman@...edance.com>,
"Michael S. Tsirkin" <mst@...hat.com>, Alexei Starovoitov <ast@...nel.org>,
Daniel Borkmann <daniel@...earbox.net>, Andrii Nakryiko <andrii@...nel.org>,
Martin KaFai Lau <martin.lau@...ux.dev>, Eduard Zingerman
<eddyz87@...il.com>, Song Liu <song@...nel.org>,
Yonghong Song <yonghong.song@...ux.dev>,
John Fastabend <john.fastabend@...il.com>, KP Singh <kpsingh@...nel.org>,
Stanislav Fomichev <sdf@...ichev.me>, Hao Luo <haoluo@...gle.com>,
Jiri Olsa <jolsa@...nel.org>, Mykola Lysenko <mykolal@...com>,
Shuah Khan <shuah@...nel.org>, netdev@...r.kernel.org, bpf@...r.kernel.org,
linux-kselftest@...r.kernel.org
Subject: Re: [PATCH bpf 3/4] bpf, vsock: Invoke proto::close on close()
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@...x.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@...hat.com>
>
>> +
>> + sk->sk_prot->close(sk, 0);
>> + __vsock_release(sk, 0);
>> sock->sk = NULL;
>> sock->state = SS_FREE;
Powered by blists - more mailing lists