| Message ID | 20250121-vsock-transport-vs-autobind-v2-2-aad6069a4e8c@rbox.co (mailing list archive) |
|---|---|
| State | New |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | vsock: Transport reassignment and error handling issues | expand |
On Tue, Jan 21, 2025 at 03:44:03PM +0100, Michal Luczaj wrote: >sk_err is set when a (connectible) connect() fails. Effectively, this makes >an otherwise still healthy SS_UNCONNECTED socket impossible to use for any >subsequent connection attempts. > >Clear sk_err upon trying to establish a connection. > >Fixes: d021c344051a ("VSOCK: Introduce VM Sockets") >Reviewed-by: Stefano Garzarella <sgarzare@redhat.com> >Signed-off-by: Michal Luczaj <mhal@rbox.co> >--- > net/vmw_vsock/af_vsock.c | 5 +++++ > 1 file changed, 5 insertions(+) > >diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c >index cfe18bc8fdbe7ced073c6b3644d635fdbfa02610..075695173648d3a4ecbd04e908130efdbb393b41 100644 >--- a/net/vmw_vsock/af_vsock.c >+++ b/net/vmw_vsock/af_vsock.c >@@ -1523,6 +1523,11 @@ static int vsock_connect(struct socket *sock, struct sockaddr *addr, > if (err < 0) > goto out; > >+ /* sk_err might have been set as a result of an earlier >+ * (failed) connect attempt. >+ */ >+ sk->sk_err = 0; Just to understand: Why do you reset sk_error after calling to transport->connect and not before? My worry is that a transport might check this field and return an error. IIUC with virtio-based transports this is not the case. >+ > /* Mark sock as connecting and set the error code to in > * progress in case this is a non-blocking connect. > */ > >-- >2.48.1 > Thanks, Luigi
On 1/22/25 17:28, Luigi Leonardi wrote: > On Tue, Jan 21, 2025 at 03:44:03PM +0100, Michal Luczaj wrote: >> sk_err is set when a (connectible) connect() fails. Effectively, this makes >> an otherwise still healthy SS_UNCONNECTED socket impossible to use for any >> subsequent connection attempts. >> >> Clear sk_err upon trying to establish a connection. >> >> Fixes: d021c344051a ("VSOCK: Introduce VM Sockets") >> Reviewed-by: Stefano Garzarella <sgarzare@redhat.com> >> Signed-off-by: Michal Luczaj <mhal@rbox.co> >> --- >> net/vmw_vsock/af_vsock.c | 5 +++++ >> 1 file changed, 5 insertions(+) >> >> diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c >> index cfe18bc8fdbe7ced073c6b3644d635fdbfa02610..075695173648d3a4ecbd04e908130efdbb393b41 100644 >> --- a/net/vmw_vsock/af_vsock.c >> +++ b/net/vmw_vsock/af_vsock.c >> @@ -1523,6 +1523,11 @@ static int vsock_connect(struct socket *sock, struct sockaddr *addr, >> if (err < 0) >> goto out; >> >> + /* sk_err might have been set as a result of an earlier >> + * (failed) connect attempt. >> + */ >> + sk->sk_err = 0; > > Just to understand: Why do you reset sk_error after calling to > transport->connect and not before? transport->connect() can fail. In such case, I thought, it would be better to keep the old value of sk_err. Otherwise we'd have an early failing vsock_connect() that clears sk_err. > My worry is that a transport might check this field and return an error. > IIUC with virtio-based transports this is not the case. Right, transport might check, but currently none of the transports do.
On Wed, Jan 22, 2025 at 10:06:51PM +0100, Michal Luczaj wrote: >On 1/22/25 17:28, Luigi Leonardi wrote: >> On Tue, Jan 21, 2025 at 03:44:03PM +0100, Michal Luczaj wrote: >>> sk_err is set when a (connectible) connect() fails. Effectively, this makes >>> an otherwise still healthy SS_UNCONNECTED socket impossible to use for any >>> subsequent connection attempts. >>> >>> Clear sk_err upon trying to establish a connection. >>> >>> Fixes: d021c344051a ("VSOCK: Introduce VM Sockets") >>> Reviewed-by: Stefano Garzarella <sgarzare@redhat.com> >>> Signed-off-by: Michal Luczaj <mhal@rbox.co> >>> --- >>> net/vmw_vsock/af_vsock.c | 5 +++++ >>> 1 file changed, 5 insertions(+) >>> >>> diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c >>> index cfe18bc8fdbe7ced073c6b3644d635fdbfa02610..075695173648d3a4ecbd04e908130efdbb393b41 100644 >>> --- a/net/vmw_vsock/af_vsock.c >>> +++ b/net/vmw_vsock/af_vsock.c >>> @@ -1523,6 +1523,11 @@ static int vsock_connect(struct socket *sock, struct sockaddr *addr, >>> if (err < 0) >>> goto out; >>> >>> + /* sk_err might have been set as a result of an earlier >>> + * (failed) connect attempt. >>> + */ >>> + sk->sk_err = 0; >> >> Just to understand: Why do you reset sk_error after calling to >> transport->connect and not before? > >transport->connect() can fail. In such case, I thought, it would be better >to keep the old value of sk_err. Otherwise we'd have an early failing >vsock_connect() that clears sk_err. That's a good point, transport->connect doesn't set sk_err if it fails. Thanks for the clarification :) Reviewed-by: Luigi Leonardi <leonardi@redhat.com> > >> My worry is that a transport might check this field and return an error. >> IIUC with virtio-based transports this is not the case. > >Right, transport might check, but currently none of the transports do. >
diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c index cfe18bc8fdbe7ced073c6b3644d635fdbfa02610..075695173648d3a4ecbd04e908130efdbb393b41 100644 --- a/net/vmw_vsock/af_vsock.c +++ b/net/vmw_vsock/af_vsock.c @@ -1523,6 +1523,11 @@ static int vsock_connect(struct socket *sock, struct sockaddr *addr, if (err < 0) goto out; + /* sk_err might have been set as a result of an earlier + * (failed) connect attempt. + */ + sk->sk_err = 0; + /* Mark sock as connecting and set the error code to in * progress in case this is a non-blocking connect. */
sk_err is set when a (connectible) connect() fails. Effectively, this makes an otherwise still healthy SS_UNCONNECTED socket impossible to use for any subsequent connection attempts. Clear sk_err upon trying to establish a connection. Fixes: d021c344051a ("VSOCK: Introduce VM Sockets") Reviewed-by: Stefano Garzarella <sgarzare@redhat.com> Signed-off-by: Michal Luczaj <mhal@rbox.co> --- net/vmw_vsock/af_vsock.c | 5 +++++ 1 file changed, 5 insertions(+)