netdev - Re: [PATCH net-next v1 02/12] vsock: read from socket's error queue

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <ed26b2b7-bafc-a964-00e2-70da66640e46@salutedevices.com>
Date: Tue, 26 Sep 2023 22:36:58 +0300
From: Arseniy Krasnov <avkrasnov@...utedevices.com>
To: Stefano Garzarella <sgarzare@...hat.com>
CC: Stefan Hajnoczi <stefanha@...hat.com>, "David S. Miller"
	<davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski
	<kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>, "Michael S. Tsirkin"
	<mst@...hat.com>, Jason Wang <jasowang@...hat.com>, Bobby Eshleman
	<bobby.eshleman@...edance.com>, <kvm@...r.kernel.org>,
	<virtualization@...ts.linux-foundation.org>, <netdev@...r.kernel.org>,
	<linux-kernel@...r.kernel.org>, <kernel@...rdevices.ru>, <oxffffaa@...il.com>
Subject: Re: [PATCH net-next v1 02/12] vsock: read from socket's error queue



On 26.09.2023 15:55, Stefano Garzarella wrote:
> On Fri, Sep 22, 2023 at 08:24:18AM +0300, Arseniy Krasnov wrote:
>> This adds handling of MSG_ERRQUEUE input flag in receive call. This flag
>> is used to read socket's error queue instead of data queue. Possible
>> scenario of error queue usage is receiving completions for transmission
>> with MSG_ZEROCOPY flag. This patch also adds new defines: 'SOL_VSOCK'
>> and 'VSOCK_RECVERR'.
>>
>> Signed-off-by: Arseniy Krasnov <avkrasnov@...utedevices.com>
>> ---
>> Changelog:
>> v5(big patchset) -> v1:
>>  * R-b tag removed, due to added defines to 'include/uapi/linux/vsock.h'.
>>    Both 'SOL_VSOCK' and 'VSOCK_RECVERR' are needed by userspace, so
>>    they were placed to 'include/uapi/linux/vsock.h'. At the same time,
>>    the same define for 'SOL_VSOCK' was placed to 'include/linux/socket.h'.
>>    This is needed because this file contains SOL_XXX defines for different
>>    types of socket, so it prevents situation when another new SOL_XXX
>>    will use constant 287.
>>
>> include/linux/socket.h     | 1 +
>> include/uapi/linux/vsock.h | 9 +++++++++
>> net/vmw_vsock/af_vsock.c   | 6 ++++++
>> 3 files changed, 16 insertions(+)
>> create mode 100644 include/uapi/linux/vsock.h
>>
>> diff --git a/include/linux/socket.h b/include/linux/socket.h
>> index 39b74d83c7c4..cfcb7e2c3813 100644
>> --- a/include/linux/socket.h
>> +++ b/include/linux/socket.h
>> @@ -383,6 +383,7 @@ struct ucred {
>> #define SOL_MPTCP    284
>> #define SOL_MCTP    285
>> #define SOL_SMC        286
>> +#define SOL_VSOCK    287
>>
>> /* IPX options */
>> #define IPX_TYPE    1
>> diff --git a/include/uapi/linux/vsock.h b/include/uapi/linux/vsock.h
>> new file mode 100644
>> index 000000000000..b25c1347a3b8
>> --- /dev/null
>> +++ b/include/uapi/linux/vsock.h
> 
> We already have include/uapi/linux/vm_sockets.h
> 
> Should we include these changes there instead of creating a new header?
> 
>> @@ -0,0 +1,9 @@
>> +/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
>> +#ifndef _UAPI_LINUX_VSOCK_H
>> +#define _UAPI_LINUX_VSOCK_H
>> +
>> +#define SOL_VSOCK    287
> 
> Why we need to re-define this also here?

Reason of this re-define is that SOL_VSOCK must be exported to userspace, so
i place it to include/uapi/XXX. At the same time include/linux/socket.h contains
constants for SOL_XXX and they goes sequentially in this file (e.g. one by one,
each new value is +1 to the previous). So if I add SOL_VSOCK to include/uapi/XXX
only, it is possible that someone will add new SOL_VERY_NEW_SOCKET == 287 to
include/linux/socket.h in future. I think it is not good that two SOL_XXX will
have same value.

For example SOL_RDS and SOL_TIPS uses the same approach - there are two same defines:
one in include/uapi/ and another is in include/linux/socket.h

> 
> In that case, should we protect with some guards to avoid double
> defines?

May be:

in include/linux/socket.h

#ifndef SOL_VSOCK
#define SOL_VSOCK 287
#endif

But not sure...

> 
>> +
>> +#define VSOCK_RECVERR    1
>> +
>> +#endif /* _UAPI_LINUX_VSOCK_H */
>> diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c
>> index d841f4de33b0..4fd11bf34bc7 100644
>> --- a/net/vmw_vsock/af_vsock.c
>> +++ b/net/vmw_vsock/af_vsock.c
>> @@ -110,6 +110,8 @@
>> #include <linux/workqueue.h>
>> #include <net/sock.h>
>> #include <net/af_vsock.h>
>> +#include <linux/errqueue.h>
>> +#include <uapi/linux/vsock.h>
>>
>> static int __vsock_bind(struct sock *sk, struct sockaddr_vm *addr);
>> static void vsock_sk_destruct(struct sock *sk);
>> @@ -2137,6 +2139,10 @@ vsock_connectible_recvmsg(struct socket *sock, struct msghdr *msg, size_t len,
>>     int err;
>>
>>     sk = sock->sk;
>> +
>> +    if (unlikely(flags & MSG_ERRQUEUE))
>> +        return sock_recv_errqueue(sk, msg, len, SOL_VSOCK, VSOCK_RECVERR);
>> +
>>     vsk = vsock_sk(sk);
>>     err = 0;
>>
>> -- 
>> 2.25.1
>>
>