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

[Arseniy Krasnov <avkrasnov@salutedevices.com>]

On 27.09.2023 10:34, Stefano Garzarella wrote:
> On Tue, Sep 26, 2023 at 10:36:58PM +0300, Arseniy Krasnov wrote:
>>
>>
>> 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@salutedevices.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
> 
> Okay, I was confused, I though socket.h was the uapi one.
> If others do the same, it's fine.
> 
> But why adding a new vsock.h instead of reusing vm_sockets.h?

Yes, that's my mistake, I'll use vm_sockets.h. Seems I just forget about
vm_sockets.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...
> 
> Nope, let's follow others definition.
> 
> Sorry for the confusion ;-)

No problem!

Thanks, Arseniy

> 
>>
>>>
>>>> +
>>>> +#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
>>>>
>>>
>>
>