Re: [PATCH] hv_netvsc: Make sure out channel is fully opened on send

[Stephen Hemminger <stephen@networkplumber.org>]

On Thu, 27 Sep 2018 10:57:05 +0200
Mohammed Gamal <mgamal@redhat.com> wrote:

> On Wed, 2018-09-26 at 17:13 +0000, Haiyang Zhang wrote:
> > > -----Original Message-----
> > > From: Mohammed Gamal <mgamal@redhat.com>
> > > Sent: Wednesday, September 26, 2018 12:34 PM
> > > To: Stephen Hemminger <sthemmin@microsoft.com>; netdev@vger.kernel.
> > > org
> > > Cc: KY Srinivasan <kys@microsoft.com>; Haiyang Zhang
> > > <haiyangz@microsoft.com>; vkuznets <vkuznets@redhat.com>;
> > > otubo@redhat.com; cavery <cavery@redhat.com>; linux-
> > > kernel@vger.kernel.org; devel@linuxdriverproject.org; Mohammed
> > > Gamal
> > > <mgamal@redhat.com>
> > > Subject: [PATCH] hv_netvsc: Make sure out channel is fully opened
> > > on send
> > > 
> > > Dring high network traffic changes to network interface parameters
> > > such as
> > > number of channels or MTU can cause a kernel panic with a NULL
> > > pointer
> > > dereference. This is due to netvsc_device_remove() being called and
> > > deallocating the channel ring buffers, which can then be accessed
> > > by
> > > netvsc_send_pkt() before they're allocated on calling
> > > netvsc_device_add()
> > > 
> > > The patch fixes this problem by checking the channel state and
> > > returning
> > > ENODEV if not yet opened. We also move the call to
> > > hv_ringbuf_avail_percent()
> > > which may access the uninitialized ring buffer.
> > > 
> > > Signed-off-by: Mohammed Gamal <mgamal@redhat.com>
> > > ---
> > >  drivers/net/hyperv/netvsc.c | 7 ++++++-
> > >  1 file changed, 6 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/net/hyperv/netvsc.c
> > > b/drivers/net/hyperv/netvsc.c index
> > > fe01e14..75f1b31 100644
> > > --- a/drivers/net/hyperv/netvsc.c
> > > +++ b/drivers/net/hyperv/netvsc.c
> > > @@ -825,7 +825,12 @@ static inline int netvsc_send_pkt(
> > >  	struct netdev_queue *txq = netdev_get_tx_queue(ndev,
> > > packet->q_idx);
> > >  	u64 req_id;
> > >  	int ret;
> > > -	u32 ring_avail =
> > > hv_get_avail_to_write_percent(&out_channel-  
> > > > outbound);  
> > > 
> > > +	u32 ring_avail;
> > > +
> > > +	if (out_channel->state != CHANNEL_OPENED_STATE)
> > > +		return -ENODEV;
> > > +
> > > +	ring_avail = hv_get_avail_to_write_percent(&out_channel-  
> > > >outbound);  
> > 
> > When you reproducing the NULL ptr panic, does your kernel include the
> > following patch?
> > hv_netvsc: common detach logic
> > https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/c
> > ommit/?id=7b2ee50c0cd513a176a26a71f2989facdd75bfea
> >   
> Yes it is included. And the commit did reduce the occurrence of this
> race condition, but it still nevertheless occurs albeit rarely.
> 
> > We call netif_tx_disable(ndev) and netif_device_detach(ndev) before
> > doing the changes 
> > on MTU or #channels. So there should be no call to start_xmit() when
> > channel is not ready.
> > 
> > If you see the check for CHANNEL_OPENED_STATE is still necessary on
> > upstream kernel (including 
> > the patch " common detach logic "), we should debug further on the
> > code and find out the 
> > root cause.
> > 
> > Thanks,
> > - Haiyang
> >   
> _______________________________________________
> devel mailing list
> devel@linuxdriverproject.org
> http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Is there some workload, that can be used to reproduce this?
The stress test from Vitaly with changing parameters while running network traffic
passes now.

Can you reproduce this with the upstream current kernel?

Adding the check in start xmit is still racy, and won't cure the problem.

Another solution would be to add a grace period in the netvsc detach logic.