linux-kernel - RE: [PATCH V2 10/12] Drivers: hv: vmbus: channge vmbus_connection.channel

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:	Thu, 12 Nov 2015 13:10:51 +0000
From:	Dexuan Cui <decui@...rosoft.com>
To:	Vitaly Kuznetsov <vkuznets@...hat.com>
CC:	"gregkh@...uxfoundation.org" <gregkh@...uxfoundation.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"devel@...uxdriverproject.org" <devel@...uxdriverproject.org>,
	"olaf@...fle.de" <olaf@...fle.de>,
	"apw@...onical.com" <apw@...onical.com>,
	"jasowang@...hat.com" <jasowang@...hat.com>,
	KY Srinivasan <kys@...rosoft.com>
Subject: RE: [PATCH V2 10/12] Drivers: hv: vmbus: channge
 vmbus_connection.channel_lock to mutex

> From: Vitaly Kuznetsov [mailto:vkuznets@...hat.com]
> Sent: Thursday, November 12, 2015 18:41
> To: Dexuan Cui <decui@...rosoft.com>
> Cc: gregkh@...uxfoundation.org; linux-kernel@...r.kernel.org;
> devel@...uxdriverproject.org; olaf@...fle.de; apw@...onical.com;
> jasowang@...hat.com; KY Srinivasan <kys@...rosoft.com>
> Subject: Re: [PATCH V2 10/12] Drivers: hv: vmbus: channge
> vmbus_connection.channel_lock to mutex
>
> "K. Y. Srinivasan" <kys@...rosoft.com> writes:
>
> > From: Dexuan Cui <decui@...rosoft.com>
> >
> > spinlock is unnecessary here.
> > mutex is enough.
>
> Hm, mutex is usually required when we need to sleep and a spinlock is
> enough otherwise :-)

Sorry, I should have written a better changelog. :-)

 > Or are you trying to say we don't need to disable interrupts here? In

Yes.
Here we try to protect vmbus_connection.chn_list and the related
channel pointer (see relid2channel()) from being updated in parallel.

The parallel paths, e.g., vmbus_process_offer() and
vmbus_onoffer_rescind(), are in process context and no irq context is
involved, so we don't need disable interrupts at all.

> that can maybe we can just switch to spin_lock()/spin_unlock()?

Switching to mutex actually makes preparation for a later patch (which
will be posted to LKML once this pachset is accepted):

Drivers: hv: vmbus: add an API vmbus_hvsock_device_unregister()
https://github.com/dcui/linux/commit/185afe8394a9bdae2be11ee1ea2a38d05e373025
(on branch decui/vmsock_1020)

For a vmsock socket connection, the host and the guest can be closing
the connection at the same time.

When the host tries to close the connection, a rescind offer is received
in the VM.

When the guest tries to close the connection, a new vmbus API
vmbus_hvsock_device_unregister(channel) is invoked, so
vmbus_hvsock_device_unregister() -> vmbus_device_unregister() is
invoked and this can be running in parallel with
vmbus_onoffer_rescind() -> vmbus_device_unregister().

The issue of "vmbus_onoffer_rescind () -> relid2channel()" is:
it returns a channel pointer without the spinlock held, so actually
there is no real protection for the channel pointer.

So IMO we need to serialize vmbus_onoffer_rescind() and
vmbus_hvsock_device_unregister().
Here I use mutex (rather than spinlock) because the critical section
can sleep, due to vmbus_device_unregister().

Thanks,
-- Dexuan
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/