Linux Virtualization - Mar 2023 - [RFC PATCH v1] vsock: check error queue to set EPOLLERR

On Thu, Mar 02, 2023 at 02:41:29PM +0300, Arseniy Krasnov
wrote:
>Hello!
>
>On 02.03.2023 13:06, Stefano Garzarella wrote:
>> On Wed, Mar 01, 2023 at 08:19:45AM +0300, Arseniy Krasnov wrote:
>>> EPOLLERR must be set not only when there is error on the socket,
but also
>>> when error queue of it is not empty (may be it contains some
control
>>> messages). Without this patch 'poll()' won't detect
data in error queue.
>>
>> Do you have a reproducer?
>>
>Dedicated reproducer - no:)
>To reproduce this issue, i used last MSG_ZEROCOPY patches. Completion was
inserted to
>error queue, and 'poll()' didn't report about it. That was the
reason, why this patch
>was included to MSG_ZEROCOPY patchset. But also i think it is better to
reduce number
>of patches in it(i'm working on v2), so it is good to handle this patch
separately.
Yep, absolutely!
>May be one way to reproduce it is use SO_TIMESTAMP(time info about skbuff
will be queued
>to the error queue). IIUC this feature is implemented at socket layer and
may work in
>vsock (but i'm not sure). Ok, i'll check it and try to implement
reproducer.
>
>IIUC, for future, policy for fixes is "for each fix implement
reproducer in vsock_test"?
Nope, but for each fix we should have a Fixes tag.

Usually we use vsock_test to check regressions on features and also the
behaviour of different transports.
My question was more about whether this problem was there before
supporting sk_buff or not, to figure out which Fixes tag to use.
>
>>> This patch is based on 'tcp_poll()'.
>>
>> LGTM but we should add a Fixes tag.
>> It's not clear to me whether the problem depends on when we
switched to using sk_buff or was pre-existing.
>>
>> Do you have any idea when we introduced this issue?
>git blame shows, that this code exists since first commit to vsock:
Okay, but did we use sk_error_queue before supporting sk_buff?

Anyway, if we are not sure I think we can use the following Fixes tag,
I don't see any issue if we backport this patch also before supporting
sk_buff.

Thanks,
Stefano
>
>commit d021c344051af91f42c5ba9fdedc176740cbd238
>Author: Andy King <acking at vmware.com>
>Date:   Wed Feb 6 14:23:56 2013 +0000
>
>    VSOCK: Introduce VM Sockets
>
>For TCP same logic was added by:
>
>commit 4ed2d765dfaccff5ebdac68e2064b59125033a3b
>Author: Willem de Bruijn <willemb at google.com>
>Date:   Mon Aug 4 22:11:49 2014 -0400
>
>    net-timestamp: TCP timestamping
>
>
>>
>> Thanks,
>> Stefano
>>
>
>Thanks Arseniy
>
>>>
>>> Signed-off-by: Arseniy Krasnov <AVKrasnov at sberdevices.ru>
>>> ---
>>> net/vmw_vsock/af_vsock.c | 2 +-
>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c
>>> index 19aea7cba26e..b5e51ef4a74c 100644
>>> --- a/net/vmw_vsock/af_vsock.c
>>> +++ b/net/vmw_vsock/af_vsock.c
>>> @@ -1026,7 +1026,7 @@ static __poll_t vsock_poll(struct file *file,
struct socket *sock,
>>> ????poll_wait(file, sk_sleep(sk), wait);
>>> ????mask = 0;
>>>
>>> -??? if (sk->sk_err)
>>> +??? if (sk->sk_err ||
!skb_queue_empty_lockless(&sk->sk_error_queue))
>>> ??????? /* Signify that there has been an error on this socket. */
>>> ??????? mask |= EPOLLERR;
>>>
>>> --?
>>> 2.25.1
>>>
>>
>