Messages in this thread | | | Date | Thu, 30 May 2019 12:27:40 +0200 | From | Stefano Garzarella <> | Subject | Re: [PATCH 1/4] vsock/virtio: fix locking around 'the_virtio_vsock' |
| |
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
| |