On Fri, Jan 10, 2025 at 09:35:09AM +0100, Stefano Garzarella wrote:
> During virtio_transport_release() we can schedule a delayed work to
> perform the closing of the socket before destruction.
>
> The destructor is called either when the socket is really destroyed
> (reference counter to zero), or it can also be called when we are
> de-assigning the transport.
>
> In the former case, we are sure the delayed work has completed, because
> it holds a reference until it completes, so the destructor will
> definitely be called after the delayed work is finished.
> But in the latter case, the destructor is called by AF_VSOCK core, just
> after the release(), so there may still be delayed work scheduled.
>
> Refactor the code, moving the code to delete the close work already in
> the do_close() to a new function. Invoke it during destruction to make
> sure we don't leave any pending work.
>
> Fixes: c0cfa2d8a788 ("vsock: add multi-transports support")
> Cc: stable@xxxxxxxxxxxxxxx
> Reported-by: Hyunwoo Kim <v4bel@xxxxxxxxx>
> Closes: https://lore.kernel.org/netdev/Z37Sh+utS+iV3+eb@v4bel-B760M-AORUS-ELITE-AX/
> Signed-off-by: Stefano Garzarella <sgarzare@xxxxxxxxxx>
> ---
> net/vmw_vsock/virtio_transport_common.c | 29 ++++++++++++++++++-------
> 1 file changed, 21 insertions(+), 8 deletions(-)
>
> diff --git a/net/vmw_vsock/virtio_transport_common.c b/net/vmw_vsock/virtio_transport_common.c
> index 51a494b69be8..7f7de6d88096 100644
> --- a/net/vmw_vsock/virtio_transport_common.c
> +++ b/net/vmw_vsock/virtio_transport_common.c
> @@ -26,6 +26,9 @@
> /* Threshold for detecting small packets to copy */
> #define GOOD_COPY_LEN 128
>
> +static void virtio_transport_cancel_close_work(struct vsock_sock *vsk,
> + bool cancel_timeout);
> +
> static const struct virtio_transport *
> virtio_transport_get_ops(struct vsock_sock *vsk)
> {
> @@ -1109,6 +1112,8 @@ void virtio_transport_destruct(struct vsock_sock *vsk)
> {
> struct virtio_vsock_sock *vvs = vsk->trans;
>
> + virtio_transport_cancel_close_work(vsk, true);
> +
> kfree(vvs);
> vsk->trans = NULL;
> }
> @@ -1204,17 +1209,11 @@ static void virtio_transport_wait_close(struct sock *sk, long timeout)
> }
> }
>
> -static void virtio_transport_do_close(struct vsock_sock *vsk,
> - bool cancel_timeout)
> +static void virtio_transport_cancel_close_work(struct vsock_sock *vsk,
> + bool cancel_timeout)
> {
> struct sock *sk = sk_vsock(vsk);
>
> - sock_set_flag(sk, SOCK_DONE);
> - vsk->peer_shutdown = SHUTDOWN_MASK;
> - if (vsock_stream_has_data(vsk) <= 0)
> - sk->sk_state = TCP_CLOSING;
> - sk->sk_state_change(sk);
> -
> if (vsk->close_work_scheduled &&
> (!cancel_timeout || cancel_delayed_work(&vsk->close_work))) {
> vsk->close_work_scheduled = false;
> @@ -1226,6 +1225,20 @@ static void virtio_transport_do_close(struct vsock_sock *vsk,
> }
> }
>
> +static void virtio_transport_do_close(struct vsock_sock *vsk,
> + bool cancel_timeout)
> +{
> + struct sock *sk = sk_vsock(vsk);
> +
> + sock_set_flag(sk, SOCK_DONE);
> + vsk->peer_shutdown = SHUTDOWN_MASK;
> + if (vsock_stream_has_data(vsk) <= 0)
> + sk->sk_state = TCP_CLOSING;
> + sk->sk_state_change(sk);
> +
> + virtio_transport_cancel_close_work(vsk, cancel_timeout);
> +}
> +
> static void virtio_transport_close_timeout(struct work_struct *work)
> {
> struct vsock_sock *vsk =
> --
> 2.47.1
>
The two scenarios I presented have been resolved.
Tested-by: Hyunwoo Kim <v4bel@xxxxxxxxx>
Regards,
Hyunwoo Kim
