* [PATCH] vsock: remove vsock from connected table when connect is interrupted by a signal @ 2022-02-16 14:32 Seth Forshee 2022-02-16 16:11 ` Stefano Garzarella 0 siblings, 1 reply; 4+ messages in thread From: Seth Forshee @ 2022-02-16 14:32 UTC (permalink / raw) To: Stefano Garzarella, David S. Miller, Jakub Kicinski Cc: virtualization, netdev, linux-kernel 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@digitalocean.com> --- 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; -- 2.32.0 ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] vsock: remove vsock from connected table when connect is interrupted by a signal 2022-02-16 14:32 [PATCH] vsock: remove vsock from connected table when connect is interrupted by a signal Seth Forshee @ 2022-02-16 16:11 ` Stefano Garzarella 2022-02-17 4:14 ` Jakub Kicinski 0 siblings, 1 reply; 4+ messages in thread From: Stefano Garzarella @ 2022-02-16 16:11 UTC (permalink / raw) To: Seth Forshee Cc: David S. Miller, Jakub Kicinski, virtualization, netdev, linux-kernel Hi Seth, 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@digitalocean.com> >--- > 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; >-- >2.32.0 > Thanks for this fix! The patch LGTM: Reviewed-by: Stefano Garzarella <sgarzare@redhat.com> @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. Thanks, Stefano ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] vsock: remove vsock from connected table when connect is interrupted by a signal 2022-02-16 16:11 ` Stefano Garzarella @ 2022-02-17 4:14 ` Jakub Kicinski 2022-02-17 8:34 ` Stefano Garzarella 0 siblings, 1 reply; 4+ messages in thread From: Jakub Kicinski @ 2022-02-17 4:14 UTC (permalink / raw) To: Stefano Garzarella Cc: Seth Forshee, David S. Miller, virtualization, netdev, linux-kernel 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@digitalocean.com> > >--- > > 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@redhat.com> > > > @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. ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] vsock: remove vsock from connected table when connect is interrupted by a signal 2022-02-17 4:14 ` Jakub Kicinski @ 2022-02-17 8:34 ` Stefano Garzarella 0 siblings, 0 replies; 4+ messages in thread From: Stefano Garzarella @ 2022-02-17 8:34 UTC (permalink / raw) To: Jakub Kicinski, Seth Forshee Cc: David S. Miller, virtualization, netdev, linux-kernel 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@digitalocean.com> >> >--- >> > 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@redhat.com> >> >> >> @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 ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2022-02-17 8:34 UTC | newest] Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-02-16 14:32 [PATCH] vsock: remove vsock from connected table when connect is interrupted by a signal Seth Forshee 2022-02-16 16:11 ` Stefano Garzarella 2022-02-17 4:14 ` Jakub Kicinski 2022-02-17 8:34 ` Stefano Garzarella
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).