Re: [PATCH] Drivers: hv: vmbus: Add mutex lock to channel show functions

From: Sasha Levin <sashal@kernel.org>
To: Dexuan Cui <decui@microsoft.com>
Cc: kimbrownkd <kimbrownkd@gmail.com>,
	Michael Kelley <mikelley@microsoft.com>,
	Long Li <longli@microsoft.com>,
	Sasha Levin <Alexander.Levin@microsoft.com>,
	Stephen Hemminger <sthemmin@microsoft.com>,
	KY Srinivasan <kys@microsoft.com>,
	Haiyang Zhang <haiyangz@microsoft.com>,
	"devel@linuxdriverproject.org" <devel@linuxdriverproject.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH] Drivers: hv: vmbus: Add mutex lock to channel show functions
Date: Thu, 31 Jan 2019 10:19:58 -0500	[thread overview]
Message-ID: <20190131151958.GT3973@sasha-vm> (raw)
In-Reply-To: <PU1P153MB01699B96311157EA9DF515E6BF970@PU1P153MB0169.APCP153.PROD.OUTLOOK.COM>

On Tue, Jan 29, 2019 at 07:20:28PM +0000, Dexuan Cui wrote:
>> From: Kimberly Brown <kimbrownkd@gmail.com>
>> > ...
>> > But as you pointed, at least for sub-channels, channel->ringbuffer_page
>> > can indeed disappear in vmbus_close() -> ... -> vmbus_free_ring(), and
>> > the "attribute->show()" could crash when the race happens.
>> > Adding channel_mutex here seems to be able to fix the race for
>> > sub-channels, as the same channel_mutex is used in
>> vmbus_disconnect_ring().
>> >
>> > For a primary channel, vmbus_close() -> vmbus_free_ring() can still
>> > free channel->ringbuffer_page without notifying the "attribute->show()".
>> > We may also need to acquire/release the channel_mutex in vmbus_close()?
>> >
>> > > Actually, there should be checks that "chan" is not null and that
>> > > "chan->state" is CHANNEL_OPENED_STATE within the locked section. I'll
>> > > need to fix that.
>> > I suppose "chan" can not be NULL here (see the above).
>> >
>> > Checking "chan->state" may not help to completely fix the race, because
>> > there is no locking/synchronization code in
>> > vmbus_close_internal() when we test and change "channel->state".
>> >
>>
>> The calls to vmbus_close_internal() for the subchannels and the primary
>> channel are protected with channel_mutex in vmbus_disconnect_ring().
>> This prevents "channel->state" from changing while "attribute->show()" is
>> running.
>Ah, I think you're right.
>
>> > I guess we may need to check if channel->ringbuffer_page is NULL in
>> > the "attribute->show()".
>> >
>>
>> For the primary channel, vmbus_free_ring() is called after the
>> return from vmbus_disconnect_ring(). Therefore, the primary channel's
>> state is changed before "channel->ringbuffer_page" is set to NULL.
>> Checking the channel state should be sufficient to prevent the ring
>> buffers from being freed while "attribute->show()" is running. The
>> ring buffers can't be freed until the channel's state is changed, and
>> the channel state change is protected by the mutex.
>I think you're right (I noticed in a previous mail you mentioned you would
>improve your patch to check "chan->state" with the mutax held).
>
>> I think checking that "channel->ringbuffer_page" is not NULL would
>> also work, but, as you stated, we would need to aquire/release
>> channel_mutex in vmbus_close().
>Then it looks unnecessary to check "channel->ringbuffer_page".
>
>> > PS, to prove that a race condition does exist and can really cause a panic or
>> > something, I usually add some msleep() delays in different paths so that I
>> > can reproduce the crash every time I take a special action, e.g. when I read
>> > the sysfs files of a NIC, I try to remove hv_netvsc driver. This way, I can prove
>> > a patch can indeed help, at least it can fix the crash which would happen
>> > without the patch. :-)
>> >
>>
>> Thanks! I was able to free the ring buffers while "attribute->show()"
>> was running, which caused a null pointer dereference bug. As expected,
>> the mutex lock fixed it.
>Awesome!

I've queued this one for hyper-fixes, thanks all!

--
Thanks,
Sasha