Re: [PATCH] vsock: remove vsock from connected table when connect is interrupted by a signal

[Stefano Garzarella]

On Wed, Feb 16, 2022 at 08:14:59PM -0800, Jakub Kicinski wrote:
> On Wed, 16 Feb 2022 17:11:22 +0100 Stefano Garzarella wrote:
> > On Wed, Feb 16, 2022 at 08:32:22AM -0600, Seth Forshee wrote:
> > >vsock_connect() expects that the socket could already be in the
> > >TCP_ESTABLISHED state when the connecting task wakes up with a signal
> > >pending. If this happens the socket will be in the connected table, and
> > >it is not removed when the socket state is reset. In this situation it's
> > >common for the process to retry connect(), and if the connection is
> > >successful the socket will be added to the connected table a second
> > >time, corrupting the list.
> > >
> > >Prevent this by calling vsock_remove_connected() if a signal is received
> > >while waiting for a connection. This is harmless if the socket is not in
> > >the connected table, and if it is in the table then removing it will
> > >prevent list corruption from a double add.
> > >
> > >Signed-off-by: Seth Forshee <sforshee@xxxxxxxxxxxxxxxx>
> > >---
> > > net/vmw_vsock/af_vsock.c | 1 +
> > > 1 file changed, 1 insertion(+)
> > >
> > >diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c
> > >index 3235261f138d..38baeb189d4e 100644
> > >--- a/net/vmw_vsock/af_vsock.c
> > >+++ b/net/vmw_vsock/af_vsock.c
> > >@@ -1401,6 +1401,7 @@ static int vsock_connect(struct socket *sock, struct sockaddr *addr,
> > > 			sk->sk_state = sk->sk_state == TCP_ESTABLISHED ? TCP_CLOSING : TCP_CLOSE;
> > > 			sock->state = SS_UNCONNECTED;
> > > 			vsock_transport_cancel_pkt(vsk);
> > >+			vsock_remove_connected(vsk);
> > > 			goto out_wait;
> > > 		} else if (timeout == 0) {
> > > 			err = -ETIMEDOUT;
> >
> > Thanks for this fix! The patch LGTM:
> >
> > Reviewed-by: Stefano Garzarella <sgarzare@xxxxxxxxxx>
> >
> >
> > @Dave, @Jakub, since we need this also in stable branches, I was going
> > to suggest adding a Fixes tag, but I'm a little confused: the issue
> > seems to have always been there, so from commit d021c344051a ("VSOCK:
> > Introduce VM Sockets"), but to use vsock_remove_connected() as we are
> > using in this patch, we really need commit d5afa82c977e ("vsock: correct
> > removal of socket from the list").
> >
> > Commit d5afa82c977e was introduces in v5.3 and it was backported in
> > v4.19 and v4.14, but not in v4.9.
> > So if we want to backport this patch also for v4.9, I think we need
> > commit d5afa82c977e as well.
>
> The fixes tag sounds good. Dunno what's the best way to handle this
> case. We can add a mention of the dependency to the patch description.
> Personally I'd keep things simple, add the Fixes tag and keep an eye
> on the backports, if 4.9 doesn't get it - email Greg and explain.

Okay, I'll keep an eye on this patch for 4.9.

@Seth, can you send a v2 mentioning the dependency with commit 
d5afa82c977e ("vsock: correct removal of socket from the list") and 
adding the following fixes tag?

    Fixes: d021c344051a ("VSOCK: Introduce VM Sockets")

Thanks,
Stefano