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

From: Kimberly Brown <kimbrownkd@gmail.com>
To: Dexuan Cui <decui@microsoft.com>
Cc: 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: Mon, 28 Jan 2019 14:58:45 -0500	[thread overview]
Message-ID: <20190128195845.GA3723@ubu-Virtual-Machine> (raw)
In-Reply-To: <PU1P153MB01696D28CDDC098307EBD5AFBF980@PU1P153MB0169.APCP153.PROD.OUTLOOK.COM>

On Tue, Jan 22, 2019 at 06:40:30PM +0000, Dexuan Cui wrote:
> > From: Kimberly Brown <kimbrownkd@gmail.com>
> > Sent: Monday, January 21, 2019 10:43 PM
> > > > @@ -1421,7 +1422,10 @@ static ssize_t vmbus_chan_attr_show(struct
> > > > kobject *kobj,
> > > >  	if (chan->state != CHANNEL_OPENED_STATE)
> > > >  		return -EINVAL;
> > > >
> > > > -	return attribute->show(chan, buf);
> > > > +	mutex_lock(&vmbus_connection.channel_mutex);
> > > > +	ret = attribute->show(chan, buf);
> > > > +	mutex_unlock(&vmbus_connection.channel_mutex);
> > > > +	return ret;
> > > >  }
> > >
> > > It looks this patch is already able to fix the original issue:
> > > 6712cc9c2211 ("vmbus: don't return values for uninitalized channels"),
> > > as chan->state can't be CHANNEL_OPENED_STATE when
> > > channel->ringbuffer_page is not set up yet, or has been freed.
> > > -- Dexuan
> > 
> > I think that patch 6712cc9c2211 fixes the problem when the channel is
> > not set up yet, but it doesn't fix the problem when the channel is being
> > closed
> Yeah, now I see your point.
> 
> > The channel could be freed after the check that "chan->state" is
> > CHANNEL_OPENED_STATE, while the "attribute->show()" function is running.
> 
> IMO the channel struct itself can't be freed while the "attribute->show()"
> function is running, because I suppose the sysfs interface should have an extra
> reference to channel->kobj (see vmbus_add_channel_kobj()), so before the sysfs
> files are removed, the channel struct itself can't disappear.
> (I didn't check the related code very carefully, so I could be wrong. :-) )
> 

I think that you're correct that the channel struct can't be freed while
the "attribute->show()" function is running. I tested this by calling
msleep() in chan_attr_show(), opening a subchannel sysfs file, and
unloading hv_netvsc. Unloading hv_netvsc should result in calls to
vmbus_chan_release() for each subchannel. I confirmed that
vmbus_chan_release() isn't called until the "attribute->show()" function
returns.

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

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

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

> -- Dexuan