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, 08 Oct 2019 15:00:25 +0200
From:   Vitaly Kuznetsov <vkuznets@...hat.com>
To:     Andrea Parri <parri.andrea@...il.com>
Cc:     linux-kernel@...r.kernel.org, linux-hyperv@...r.kernel.org,
        "K . Y . Srinivasan" <kys@...rosoft.com>,
        Haiyang Zhang <haiyangz@...rosoft.com>,
        Stephen Hemminger <sthemmin@...rosoft.com>,
        Sasha Levin <sashal@...nel.org>,
        Michael Kelley <mikelley@...rosoft.com>,
        Dexuan Cui <decui@...rosoft.com>
Subject: Re: [PATCH 1/2] Drivers: hv: vmbus: Introduce table of VMBus protocol versions

Andrea Parri <parri.andrea@...il.com> writes:

>> > @@ -244,21 +234,18 @@ int vmbus_connect(void)
>> >  	 * version.
>> >  	 */
>> >  
>> > -	version = VERSION_CURRENT;
>> > +	for (i = 0; ; i++) {
>> > +		version = vmbus_versions[i];
>> > +		if (version == VERSION_INVAL)
>> > +			goto cleanup;
>> 
>> If you use e.g. ARRAY_SIZE() you can get rid of VERSION_INVAL - and make
>> this code look more natural.
>
> Thank you for pointing this out, Vitaly.
>
> IIUC, you're suggesting that I do:
>
> 	for (i = 0; i < ARRAY_SIZE(vmbus_versions); i++) {
> 		version = vmbus_versions[i];
>
> 		ret = vmbus_negotiate_version(msginfo, version);
> 		if (ret == -ETIMEDOUT)
> 			goto cleanup;
>
> 		if (vmbus_connection.conn_state == CONNECTED)
> 			break;
> 	}
>
> 	if (vmbus_connection.conn_state != CONNECTED)
> 		break;
>
> and that I remove VERSION_INVAL from vmbus_versions, yes?
>

Yes, something like that.

> 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).

>
> 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! :-) 

>
> Thanks,
>   Andrea
>
>
>> >  
>> > -	do {
>> >  		ret = vmbus_negotiate_version(msginfo, version);
>> >  		if (ret == -ETIMEDOUT)
>> >  			goto cleanup;
>> >  
>> >  		if (vmbus_connection.conn_state == CONNECTED)
>> >  			break;
>> > -
>> > -		version = vmbus_get_next_version(version);
>> > -	} while (version != VERSION_INVAL);
>> > -
>> > -	if (version == VERSION_INVAL)
>> > -		goto cleanup;
>> > +	}
>> >  
>> >  	vmbus_proto_version = version;
>> >  	pr_info("Vmbus version:%d.%d\n",

-- 
Vitaly