On Mon, 24 Feb 2020 11:08:53 +0100 Stefano Garzarella wrote:
> On Sun, Feb 23, 2020 at 03:50:25PM +0800, Hillf Danton wrote:
> >
> > Seems like vsock needs a word to track lock owner in an attempt to
> > avoid trying to lock sock while the current is the lock owner.
>
> Thanks for this possible solution.
> What about using sock_owned_by_user()?
>
No chance for vsock_locked() if it works.
> We should fix also hyperv_transport, because it could suffer from the same
> problem.
>
You're right. My diff is at most for introducing vsk's lock owner.
> At this point, it might be better to call vsk->transport->release(vsk)
> always with the lock taken and remove it in the transports as in the
> following patch.
>
> What do you think?
>
Yes and ... please take a look at the output of grep
grep -n lock_sock linux/net/vmw_vsock/af_vsock.c
as it drove me mad.
>
> diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c
> index 9c5b2a91baad..a073d8efca33 100644
> --- a/net/vmw_vsock/af_vsock.c
> +++ b/net/vmw_vsock/af_vsock.c
> @@ -753,20 +753,18 @@ static void __vsock_release(struct sock *sk, int level)
> vsk = vsock_sk(sk);
> pending = NULL; /* Compiler warning. */
>
> - /* The release call is supposed to use lock_sock_nested()
> - * rather than lock_sock(), if a sock lock should be acquired.
> - */
> - if (vsk->transport)
> - vsk->transport->release(vsk);
> - else if (sk->sk_type == SOCK_STREAM)
> - vsock_remove_sock(vsk);
> -
> /* When "level" is SINGLE_DEPTH_NESTING, use the nested
> * version to avoid the warning "possible recursive locking
> * detected". When "level" is 0, lock_sock_nested(sk, level)
> * is the same as lock_sock(sk).
> */
> lock_sock_nested(sk, level);
> +
> + if (vsk->transport)
> + vsk->transport->release(vsk);
> + else if (sk->sk_type == SOCK_STREAM)
> + vsock_remove_sock(vsk);
> +
> sock_orphan(sk);
> sk->sk_shutdown = SHUTDOWN_MASK;
>
> diff --git a/net/vmw_vsock/hyperv_transport.c
> b/net/vmw_vsock/hyperv_transport.c
> index 3492c021925f..510f25f4a856 100644
> --- a/net/vmw_vsock/hyperv_transport.c
> +++ b/net/vmw_vsock/hyperv_transport.c
> @@ -529,9 +529,7 @@ static void hvs_release(struct vsock_sock *vsk)
> struct sock *sk = sk_vsock(vsk);
> bool remove_sock;
>
> - lock_sock_nested(sk, SINGLE_DEPTH_NESTING);
> remove_sock = hvs_close_lock_held(vsk);
> - release_sock(sk);
> if (remove_sock)
> vsock_remove_sock(vsk);
> }
> diff --git a/net/vmw_vsock/virtio_transport_common.c
> b/net/vmw_vsock/virtio_transport_common.c
> index d9f0c9c5425a..f3c4bab2f737 100644
> --- a/net/vmw_vsock/virtio_transport_common.c
> +++ b/net/vmw_vsock/virtio_transport_common.c
> @@ -829,7 +829,6 @@ void virtio_transport_release(struct vsock_sock *vsk)
> struct sock *sk = &vsk->sk;
> bool remove_sock = true;
>
> - lock_sock_nested(sk, SINGLE_DEPTH_NESTING);
> if (sk->sk_type == SOCK_STREAM)
> remove_sock = virtio_transport_close(vsk);
>
> @@ -837,7 +836,6 @@ void virtio_transport_release(struct vsock_sock *vsk)
> list_del(&pkt->list);
> virtio_transport_free_pkt(pkt);
> }
> - release_sock(sk);
>
> if (remove_sock)
> vsock_remove_sock(vsk);
>
> Thanks,
> Stefano
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
