| Message ID | 20190528105623.27983-2-sgarzare@redhat.com |
|---|---|
| State | Changes Requested |
| Delegated to: | David Miller |
| Headers | show |
| Series | vsock/virtio: several fixes in the .probe() and .remove() | expand |
From: Stefano Garzarella <sgarzare@redhat.com> Date: Tue, 28 May 2019 12:56:20 +0200 > @@ -68,7 +68,13 @@ struct virtio_vsock { > > static struct virtio_vsock *virtio_vsock_get(void) > { > - return the_virtio_vsock; > + struct virtio_vsock *vsock; > + > + mutex_lock(&the_virtio_vsock_mutex); > + vsock = the_virtio_vsock; > + mutex_unlock(&the_virtio_vsock_mutex); > + > + return vsock; This doesn't do anything as far as I can tell. No matter what, you will either get the value before it's changed or after it's changed. Since you should never publish the pointer by assigning it until the object is fully initialized, this can never be a problem even without the mutex being there. Even if you sampled the the_virtio_sock value right before it's being set to NULL by the remove function, that still can happen with the mutex held too. This function is also terribly named btw, it implies that a reference count is being taken. But that's not what this function does, it just returns the pointer value as-is.
On Wed, May 29, 2019 at 09:28:52PM -0700, David Miller wrote: > From: Stefano Garzarella <sgarzare@redhat.com> > Date: Tue, 28 May 2019 12:56:20 +0200 > > > @@ -68,7 +68,13 @@ struct virtio_vsock { > > > > static struct virtio_vsock *virtio_vsock_get(void) > > { > > - return the_virtio_vsock; > > + struct virtio_vsock *vsock; > > + > > + mutex_lock(&the_virtio_vsock_mutex); > > + vsock = the_virtio_vsock; > > + mutex_unlock(&the_virtio_vsock_mutex); > > + > > + return vsock; > > This doesn't do anything as far as I can tell. > > No matter what, you will either get the value before it's changed or > after it's changed. > > Since you should never publish the pointer by assigning it until the > object is fully initialized, this can never be a problem even without > the mutex being there. > > Even if you sampled the the_virtio_sock value right before it's being > set to NULL by the remove function, that still can happen with the > mutex held too. Yes, I found out when I was answering Jason's question [1]. :( I proposed to use RCU to solve this issue, do you agree? Let me know if there is a better way. > > This function is also terribly named btw, it implies that a reference > count is being taken. But that's not what this function does, it > just returns the pointer value as-is. What do you think if I remove the function, using directly the_virtio_vsock? (should be easier to use with RCU API) Thanks, Stefano [1] https://lore.kernel.org/netdev/20190529105832.oz3sagbne5teq3nt@steredhat
diff --git a/net/vmw_vsock/virtio_transport.c b/net/vmw_vsock/virtio_transport.c index 96ab344f17bb..d3ba7747aa73 100644 --- a/net/vmw_vsock/virtio_transport.c +++ b/net/vmw_vsock/virtio_transport.c @@ -68,7 +68,13 @@ struct virtio_vsock { static struct virtio_vsock *virtio_vsock_get(void) { - return the_virtio_vsock; + struct virtio_vsock *vsock; + + mutex_lock(&the_virtio_vsock_mutex); + vsock = the_virtio_vsock; + mutex_unlock(&the_virtio_vsock_mutex); + + return vsock; } static u32 virtio_transport_get_local_cid(void) @@ -592,7 +598,6 @@ static int virtio_vsock_probe(struct virtio_device *vdev) atomic_set(&vsock->queued_replies, 0); vdev->priv = vsock; - the_virtio_vsock = vsock; mutex_init(&vsock->tx_lock); mutex_init(&vsock->rx_lock); mutex_init(&vsock->event_lock); @@ -614,6 +619,8 @@ static int virtio_vsock_probe(struct virtio_device *vdev) virtio_vsock_event_fill(vsock); mutex_unlock(&vsock->event_lock); + the_virtio_vsock = vsock; + mutex_unlock(&the_virtio_vsock_mutex); return 0; @@ -628,6 +635,9 @@ static void virtio_vsock_remove(struct virtio_device *vdev) struct virtio_vsock *vsock = vdev->priv; struct virtio_vsock_pkt *pkt; + mutex_lock(&the_virtio_vsock_mutex); + the_virtio_vsock = NULL; + flush_work(&vsock->loopback_work); flush_work(&vsock->rx_work); flush_work(&vsock->tx_work); @@ -667,13 +677,10 @@ static void virtio_vsock_remove(struct virtio_device *vdev) } spin_unlock_bh(&vsock->loopback_list_lock); - mutex_lock(&the_virtio_vsock_mutex); - the_virtio_vsock = NULL; - mutex_unlock(&the_virtio_vsock_mutex); - vdev->config->del_vqs(vdev); kfree(vsock); + mutex_unlock(&the_virtio_vsock_mutex); } static struct virtio_device_id id_table[] = {
This patch protects the reading of 'the_virtio_vsock' taking the mutex used when it is set. We also move the 'the_virtio_vsock' assignment at the end of the .probe(), when we finished all the initialization, and at the beginning of .remove(), before to release resources, taking the lock until the end of the function. Signed-off-by: Stefano Garzarella <sgarzare@redhat.com> --- net/vmw_vsock/virtio_transport.c | 19 +++++++++++++------ 1 file changed, 13 insertions(+), 6 deletions(-)