linux-kernel - RE: [PATCH 1/2] Drivers: hv: vmbus: Introduce table of VMBus protocol versions

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] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 8 Oct 2019 22:41:42 +0000
From:   Dexuan Cui <decui@...rosoft.com>
To:     vkuznets <vkuznets@...hat.com>,
        Andrea Parri <parri.andrea@...il.com>
CC:     "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-hyperv@...r.kernel.org" <linux-hyperv@...r.kernel.org>,
        KY Srinivasan <kys@...rosoft.com>,
        Haiyang Zhang <haiyangz@...rosoft.com>,
        Stephen Hemminger <sthemmin@...rosoft.com>,
        Sasha Levin <sashal@...nel.org>,
        Michael Kelley <mikelley@...rosoft.com>
Subject: RE: [PATCH 1/2] Drivers: hv: vmbus: Introduce table of VMBus protocol
 versions

> From: Vitaly Kuznetsov <vkuznets@...hat.com>
> Sent: Tuesday, October 8, 2019 6:00 AM
>  ...
> > Looking at the uses of VERSION_INVAL, I find one remaining occurrence
> > of this macro in vmbus_bus_resume(), which does:
> >
> > 	if (vmbus_proto_version == VERSION_INVAL ||
> > 	    vmbus_proto_version == 0) {
> > 		...
> > 	}
> >
> > TBH I'm looking at vmbus_bus_resume() and vmbus_bus_suspend() for the
> > first time and I'm not sure about how to change such check yet... any
> > suggestions?
> 
> Hm, I don't think vmbus_proto_version can ever become == VERSION_INVAL
> if we rewrite the code the way you suggest, right? So you'll reduce this
> to 'if (!vmbus_proto_version)' meaning we haven't negotiated any version
> (yet).

Yeah, Vitaly is correct. The check may be a little paranoid as I believe 
"vmbus_proto_version" must be a negotiated value in vmbus_bus_resume()
and vmbus_bus_suspend().  I added the check just in case.

> > Mmh, I see that vmbus_bus_resume() and vmbus_bus_suspend() can access
> > vmbus_connection.conn_state: can such accesses race with a concurrent
> > vmbus_connect()?
> 
> Let's summon Dexuan who's the author! :-)

There should not be an issue:

vmbus_connect() is called in the early subsys_initcall(hv_acpi_init).

vmbus_bus_suspend() is called late in the PM code after the kernel boots up, e.g.
in the hibernation function hibernation_snapshot() -> dpm_suspend(). 

vmbus_bus_resume() is also called later in late_initcall_sync(software_resume).

In the hibernatin process, vmbus_bus_suspend()/resume() can also be called a
few times, and vmbus_bus_resume() calls vmbus_negotiate_version(). As I
checked, there is no issue, either.

Thanks,
Dexuan