Message ID | KL1P15301MB00082249964D8EF1BAA48790BF8D0@KL1P15301MB0008.APCP153.PROD.OUTLOOK.COM |
---|---|
State | Changes Requested, archived |
Delegated to: | David Miller |
Headers | show |
> On Aug 16, 2017, at 12:15 AM, Dexuan Cui <decui@microsoft.com> wrote: > > > With the current code, when vsock_dequeue_accept() is removing a sock > from the list, nothing prevents vsock_enqueue_accept() from adding a new > sock into the list concurrently. We should add a lock to protect the list. > For the VMCI socket transport, we always lock the sockets before calling into vsock_enqueue_accept and af_vsock.c locks the socket before calling vsock_dequeue_accept, so from our point of view these operations are already protected, but with finer granularity than a single global lock. As far as I can see, the virtio transport also locks the socket before calling vsock_enqueue_accept, so they should be fine with the current version as well, but Stefan can comment on that. Thanks, Jorgen
On Tue, Aug 15, 2017 at 10:15:39PM +0000, Dexuan Cui wrote: > With the current code, when vsock_dequeue_accept() is removing a sock > from the list, nothing prevents vsock_enqueue_accept() from adding a new > sock into the list concurrently. We should add a lock to protect the list. The listener sock is locked, preventing concurrent modification. I have checked both the virtio and vmci transports. Can you post an example where the listener sock isn't locked? Stefan
> > On Aug 16, 2017, at 12:15 AM, Dexuan Cui <decui@microsoft.com> wrote: > > With the current code, when vsock_dequeue_accept() is removing a sock > > from the list, nothing prevents vsock_enqueue_accept() from adding a new > > sock into the list concurrently. We should add a lock to protect the list. > > For the VMCI socket transport, we always lock the sockets before calling into > vsock_enqueue_accept and af_vsock.c locks the socket before calling > vsock_dequeue_accept, so from our point of view these operations are already > protected, but with finer granularity than a single global lock. As far as I can see, > the virtio transport also locks the socket before calling vsock_enqueue_accept, > so they should be fine with the current version as well, but Stefan can comment > on that. > > Jorgen Hi Jorgen, Thanks, you're correct. Please ignore this patch. I'll update the hv_sock driver to add proper lock_sock()/relesae_sock(). Thanks, -- Dexuan
> From: Stefan Hajnoczi [mailto:stefanha@redhat.com] > Sent: Thursday, August 17, 2017 07:06 > > On Tue, Aug 15, 2017 at 10:15:39PM +0000, Dexuan Cui wrote: > > With the current code, when vsock_dequeue_accept() is removing a sock > > from the list, nothing prevents vsock_enqueue_accept() from adding a new > > sock into the list concurrently. We should add a lock to protect the list. > > The listener sock is locked, preventing concurrent modification. I have > checked both the virtio and vmci transports. Can you post an example > where the listener sock isn't locked? > > Stefan Sorry, I was not careful when checking the vmci code. Please ignore the patch. Now I realized the expectation is that the individual transport drivers should do the locking for vsock_enqueue_accept(), but for vsock_dequeue_accept(), the locking is done by the common vsock driver. Thanks, -- Dexuan
diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c index dfc8c51e..b7b2c66 100644 --- a/net/vmw_vsock/af_vsock.c +++ b/net/vmw_vsock/af_vsock.c @@ -126,6 +126,7 @@ static struct proto vsock_proto = { static const struct vsock_transport *transport; static DEFINE_MUTEX(vsock_register_mutex); +static DEFINE_SPINLOCK(vsock_accept_queue_lock); /**** EXPORTS ****/ @@ -406,7 +407,10 @@ void vsock_enqueue_accept(struct sock *listener, struct sock *connected) sock_hold(connected); sock_hold(listener); + + spin_lock(&vsock_accept_queue_lock); list_add_tail(&vconnected->accept_queue, &vlistener->accept_queue); + spin_unlock(&vsock_accept_queue_lock); } EXPORT_SYMBOL_GPL(vsock_enqueue_accept); @@ -423,7 +427,10 @@ static struct sock *vsock_dequeue_accept(struct sock *listener) vconnected = list_entry(vlistener->accept_queue.next, struct vsock_sock, accept_queue); + spin_lock(&vsock_accept_queue_lock); list_del_init(&vconnected->accept_queue); + spin_unlock(&vsock_accept_queue_lock); + sock_put(listener); /* The caller will need a reference on the connected socket so we let * it call sock_put().
With the current code, when vsock_dequeue_accept() is removing a sock from the list, nothing prevents vsock_enqueue_accept() from adding a new sock into the list concurrently. We should add a lock to protect the list. Signed-off-by: Dexuan Cui <decui@microsoft.com> Cc: Andy King <acking@vmware.com> Cc: Dmitry Torokhov <dtor@vmware.com> Cc: George Zhang <georgezhang@vmware.com> Cc: Jorgen Hansen <jhansen@vmware.com> Cc: Reilly Grant <grantr@vmware.com> Cc: Asias He <asias@redhat.com> Cc: Stefan Hajnoczi <stefanha@redhat.com> Cc: Vitaly Kuznetsov <vkuznets@redhat.com> Cc: Cathy Avery <cavery@redhat.com> Cc: K. Y. Srinivasan <kys@microsoft.com> Cc: Haiyang Zhang <haiyangz@microsoft.com> Cc: Stephen Hemminger <sthemmin@microsoft.com> --- net/vmw_vsock/af_vsock.c | 7 +++++++ 1 file changed, 7 insertions(+)