| Message ID | 20201120104736.73749-1-sgarzare@redhat.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 3fe356d58efae54dade9ec94ea7c919ed20cf4db |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net] vsock/virtio: discard packets only when socket is really closed | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Link |
| netdev/fixes_present | success | Link |
| netdev/patch_count | success | Link |
| netdev/tree_selection | success | Clearly marked for net |
| netdev/subject_prefix | success | Link |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Link |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | success | Errors and warnings before: 0 this patch: 0 |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/verify_fixes | success | Link |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 21 lines checked |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 0 this patch: 0 |
| netdev/header_inline | success | Link |
| netdev/stable | success | Stable not CCed |
> -----Original Message----- > From: Stefano Garzarella <sgarzare@redhat.com> > Sent: Friday, November 20, 2020 6:48 PM > To: netdev@vger.kernel.org > Cc: Sergio Lopez <slp@redhat.com>; David S. Miller <davem@davemloft.net>; > Stefano Garzarella <sgarzare@redhat.com>; Justin He <Justin.He@arm.com>; > kvm@vger.kernel.org; linux-kernel@vger.kernel.org; Stefan Hajnoczi > <stefanha@redhat.com>; virtualization@lists.linux-foundation.org; Jakub > Kicinski <kuba@kernel.org> > Subject: [PATCH net] vsock/virtio: discard packets only when socket is > really closed > > Starting from commit 8692cefc433f ("virtio_vsock: Fix race condition > in virtio_transport_recv_pkt"), we discard packets in > virtio_transport_recv_pkt() if the socket has been released. > > When the socket is connected, we schedule a delayed work to wait the > RST packet from the other peer, also if SHUTDOWN_MASK is set in > sk->sk_shutdown. > This is done to complete the virtio-vsock shutdown algorithm, releasing > the port assigned to the socket definitively only when the other peer > has consumed all the packets. > > If we discard the RST packet received, the socket will be closed only > when the VSOCK_CLOSE_TIMEOUT is reached. > > Sergio discovered the issue while running ab(1) HTTP benchmark using > libkrun [1] and observing a latency increase with that commit. > > To avoid this issue, we discard packet only if the socket is really > closed (SOCK_DONE flag is set). > We also set SOCK_DONE in virtio_transport_release() when we don't need > to wait any packets from the other peer (we didn't schedule the delayed > work). In this case we remove the socket from the vsock lists, releasing > the port assigned. > > [1] https://github.com/containers/libkrun > > Fixes: 8692cefc433f ("virtio_vsock: Fix race condition in > virtio_transport_recv_pkt") Acked-by: Jia He <justin.he@arm.com> -- Cheers, Justin (Jia He) > Cc: justin.he@arm.com > Reported-by: Sergio Lopez <slp@redhat.com> > Tested-by: Sergio Lopez <slp@redhat.com> > Signed-off-by: Stefano Garzarella <sgarzare@redhat.com> > --- > net/vmw_vsock/virtio_transport_common.c | 8 +++++--- > 1 file changed, 5 insertions(+), 3 deletions(-) > > diff --git a/net/vmw_vsock/virtio_transport_common.c > b/net/vmw_vsock/virtio_transport_common.c > index 0edda1edf988..5956939eebb7 100644 > --- a/net/vmw_vsock/virtio_transport_common.c > +++ b/net/vmw_vsock/virtio_transport_common.c > @@ -841,8 +841,10 @@ void virtio_transport_release(struct vsock_sock *vsk) > virtio_transport_free_pkt(pkt); > } > > -if (remove_sock) > +if (remove_sock) { > +sock_set_flag(sk, SOCK_DONE); > vsock_remove_sock(vsk); > +} > } > EXPORT_SYMBOL_GPL(virtio_transport_release); > > @@ -1132,8 +1134,8 @@ void virtio_transport_recv_pkt(struct > virtio_transport *t, > > lock_sock(sk); > > -/* Check if sk has been released before lock_sock */ > -if (sk->sk_shutdown == SHUTDOWN_MASK) { > +/* Check if sk has been closed before lock_sock */ > +if (sock_flag(sk, SOCK_DONE)) { > (void)virtio_transport_reset_no_sock(t, pkt); > release_sock(sk); > sock_put(sk); > -- > 2.26.2 IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.
Hello: This patch was applied to netdev/net.git (refs/heads/master): On Fri, 20 Nov 2020 11:47:36 +0100 you wrote: > Starting from commit 8692cefc433f ("virtio_vsock: Fix race condition > in virtio_transport_recv_pkt"), we discard packets in > virtio_transport_recv_pkt() if the socket has been released. > > When the socket is connected, we schedule a delayed work to wait the > RST packet from the other peer, also if SHUTDOWN_MASK is set in > sk->sk_shutdown. > This is done to complete the virtio-vsock shutdown algorithm, releasing > the port assigned to the socket definitively only when the other peer > has consumed all the packets. > > [...] Here is the summary with links: - [net] vsock/virtio: discard packets only when socket is really closed https://git.kernel.org/netdev/net/c/3fe356d58efa You are awesome, thank you! -- Deet-doot-dot, I am a bot. https://korg.docs.kernel.org/patchwork/pwbot.html
diff --git a/net/vmw_vsock/virtio_transport_common.c b/net/vmw_vsock/virtio_transport_common.c index 0edda1edf988..5956939eebb7 100644 --- a/net/vmw_vsock/virtio_transport_common.c +++ b/net/vmw_vsock/virtio_transport_common.c @@ -841,8 +841,10 @@ void virtio_transport_release(struct vsock_sock *vsk) virtio_transport_free_pkt(pkt); } - if (remove_sock) + if (remove_sock) { + sock_set_flag(sk, SOCK_DONE); vsock_remove_sock(vsk); + } } EXPORT_SYMBOL_GPL(virtio_transport_release); @@ -1132,8 +1134,8 @@ void virtio_transport_recv_pkt(struct virtio_transport *t, lock_sock(sk); - /* Check if sk has been released before lock_sock */ - if (sk->sk_shutdown == SHUTDOWN_MASK) { + /* Check if sk has been closed before lock_sock */ + if (sock_flag(sk, SOCK_DONE)) { (void)virtio_transport_reset_no_sock(t, pkt); release_sock(sk); sock_put(sk);
Starting from commit 8692cefc433f ("virtio_vsock: Fix race condition in virtio_transport_recv_pkt"), we discard packets in virtio_transport_recv_pkt() if the socket has been released. When the socket is connected, we schedule a delayed work to wait the RST packet from the other peer, also if SHUTDOWN_MASK is set in sk->sk_shutdown. This is done to complete the virtio-vsock shutdown algorithm, releasing the port assigned to the socket definitively only when the other peer has consumed all the packets. If we discard the RST packet received, the socket will be closed only when the VSOCK_CLOSE_TIMEOUT is reached. Sergio discovered the issue while running ab(1) HTTP benchmark using libkrun [1] and observing a latency increase with that commit. To avoid this issue, we discard packet only if the socket is really closed (SOCK_DONE flag is set). We also set SOCK_DONE in virtio_transport_release() when we don't need to wait any packets from the other peer (we didn't schedule the delayed work). In this case we remove the socket from the vsock lists, releasing the port assigned. [1] https://github.com/containers/libkrun Fixes: 8692cefc433f ("virtio_vsock: Fix race condition in virtio_transport_recv_pkt") Cc: justin.he@arm.com Reported-by: Sergio Lopez <slp@redhat.com> Tested-by: Sergio Lopez <slp@redhat.com> Signed-off-by: Stefano Garzarella <sgarzare@redhat.com> --- net/vmw_vsock/virtio_transport_common.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-)