Re: [PATCH net] vsock: keep poll shutdown state consistent

[Stefano Garzarella <sgarzare@redhat.com>]

On Mon, May 18, 2026 at 08:39:37PM +0800, ziyu zhang wrote:
>On Mon, 18 May 2026 at 18:16, Stefano Garzarella <sgarzare@redhat.com> wrote:
>>
>> On Sat, May 16, 2026 at 11:47:45AM +0800, Ziyu Zhang wrote:
>> >vsock_poll() reads vsk->peer_shutdown before taking the socket
>> >lock to set EPOLLHUP and EPOLLRDHUP, then reads it again under the
>> >lock to report EOF readability. A shutdown packet can update
>> >peer_shutdown while poll is waiting for the lock, so one poll invocation
>> >can report EPOLLIN without the corresponding HUP/RDHUP bits.
>> >
>> >Keep non-connectible sockets on a single lockless READ_ONCE()
>>
>> Should this be paired with WRITE_ONCE() on writers?
>
>Yes, since the poll path uses lockless READ_ONCE() snapshots of
>peer_shutdown, the writer side should be annotated with WRITE_ONCE() as
>well. I will add that in v2.
>
>>
>> >snapshot. For connectible sockets, defer shutdown-derived poll bits
>> >until after lock_sock() and use one READ_ONCE() snapshot for both EOF
>> >readability and HUP/RDHUP. This preserves shutdowns that arrive before
>> >the lock is acquired and keeps all peer-shutdown-derived bits consistent
>> >for a poll pass.
>> >
>> >Fixes: d021c344051a ("VSOCK: Introduce VM Sockets")
>> >Signed-off-by: Ziyu Zhang <ziyuzhang201@gmail.com>
>> >---
>> > net/vmw_vsock/af_vsock.c | 40 ++++++++++++++++++++++++++--------------
>> > 1 file changed, 26 insertions(+), 14 deletions(-)
>> >
>> >diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c
>> >index adcba1b7b..bed42347b 100644
>> >--- a/net/vmw_vsock/af_vsock.c
>> >+++ b/net/vmw_vsock/af_vsock.c
>> >@@ -1122,6 +1122,25 @@ static int vsock_shutdown(struct socket *sock, int mode)
>> >       return err;
>> > }
>> >
>> >+static __poll_t vsock_poll_shutdown(struct sock *sk, u32 peer_shutdown)
>> >+{
>> >+      __poll_t mask = 0;
>> >+
>> >+      /* INET sockets treat local write shutdown and peer write shutdown as a
>> >+       * case of EPOLLHUP set.
>> >+       */
>> >+      if (sk->sk_shutdown == SHUTDOWN_MASK ||
>> >+          ((sk->sk_shutdown & SEND_SHUTDOWN) &&
>> >+           (peer_shutdown & SEND_SHUTDOWN)))
>> >+              mask |= EPOLLHUP;
>> >+
>> >+      if (sk->sk_shutdown & RCV_SHUTDOWN ||
>> >+          peer_shutdown & SEND_SHUTDOWN)
>> >+              mask |= EPOLLRDHUP;
>> >+
>> >+      return mask;
>> >+}
>> >+
>> > static __poll_t vsock_poll(struct file *file, struct socket *sock,
>> >                              poll_table *wait)
>> > {
>> >@@ -1139,19 +1158,9 @@ static __poll_t vsock_poll(struct file *file, struct socket *sock,
>> >               /* Signify that there has been an error on this socket. */
>> >               mask |= EPOLLERR;
>> >
>> >-      /* INET sockets treat local write shutdown and peer write shutdown as a
>> >-       * case of EPOLLHUP set.
>> >-       */
>> >-      if ((sk->sk_shutdown == SHUTDOWN_MASK) ||
>> >-          ((sk->sk_shutdown & SEND_SHUTDOWN) &&
>> >-           (vsk->peer_shutdown & SEND_SHUTDOWN))) {
>> >-              mask |= EPOLLHUP;
>> >-      }
>> >-
>> >-      if (sk->sk_shutdown & RCV_SHUTDOWN ||
>> >-          vsk->peer_shutdown & SEND_SHUTDOWN) {
>> >-              mask |= EPOLLRDHUP;
>> >-      }
>> >+      if (!sock_type_connectible(sk->sk_type))
>> >+              mask |= vsock_poll_shutdown(sk,
>> >+                                          READ_ONCE(vsk->peer_shutdown));
>>
>> Can we move this in the `if (sock->type == SOCK_DGRAM)` branch ?
>>
>> Not a strong opinion about that, but in any case IMO we should add a
>> comment here to explain why we are doing only for not connectible
>> sockets.
>>
>> That said, if we use WRITE_ONCE in the writers, do we really need to
>> move this after the lock_sock for the connectable ones?
>
>Yes, I will move the non-connectible handling into the SOCK_DGRAM branch
>and add a comment there.
>
>For connectible sockets, my current understanding is that
>READ_ONCE()/WRITE_ONCE() make the individual lockless accesses explicit, but
>they do not make two separate reads in one vsock_poll() invocation observe the
>same peer_shutdown value. So I still think using one peer_shutdown snapshot
>after lock_sock() is useful for keeping the returned mask internally
>consistent. Please let me know if you think WRITE_ONCE() is enough for this
>case.

What will be the issue of "do not make two separate reads in one 
vsock_poll() invocation observe the same peer_shutdown value." ?

In any case, I'm not against it; I just want to understand the issue 
better.

Thanks,
Stefano