* [PATCH] hv_netvsc: Make sure out channel is fully opened on send @ 2018-09-26 16:34 Mohammed Gamal 2018-09-26 17:13 ` Haiyang Zhang 0 siblings, 1 reply; 18+ messages in thread From: Mohammed Gamal @ 2018-09-26 16:34 UTC (permalink / raw) To: sthemmin, netdev Cc: kys, haiyangz, vkuznets, otubo, cavery, linux-kernel, devel, Mohammed Gamal 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); nvmsg.hdr.msg_type = NVSP_MSG1_TYPE_SEND_RNDIS_PKT; if (skb) -- 1.8.3.1 ^ permalink raw reply related [flat|nested] 18+ messages in thread
* RE: [PATCH] hv_netvsc: Make sure out channel is fully opened on send 2018-09-26 16:34 [PATCH] hv_netvsc: Make sure out channel is fully opened on send Mohammed Gamal @ 2018-09-26 17:13 ` Haiyang Zhang 2018-09-27 8:57 ` Mohammed Gamal 0 siblings, 1 reply; 18+ messages in thread From: Haiyang Zhang @ 2018-09-26 17:13 UTC (permalink / raw) To: Mohammed Gamal, Stephen Hemminger, netdev Cc: KY Srinivasan, vkuznets, otubo, cavery, linux-kernel, devel > -----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/commit/?id=7b2ee50c0cd513a176a26a71f2989facdd75bfea 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] hv_netvsc: Make sure out channel is fully opened on send 2018-09-26 17:13 ` Haiyang Zhang @ 2018-09-27 8:57 ` Mohammed Gamal 0 siblings, 0 replies; 18+ messages in thread From: Mohammed Gamal @ 2018-09-27 8:57 UTC (permalink / raw) To: Haiyang Zhang, Stephen Hemminger, netdev Cc: KY Srinivasan, vkuznets, otubo, cavery, linux-kernel, devel 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 > ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] hv_netvsc: Make sure out channel is fully opened on send @ 2018-09-27 8:57 ` Mohammed Gamal 0 siblings, 0 replies; 18+ messages in thread From: Mohammed Gamal @ 2018-09-27 8:57 UTC (permalink / raw) To: Haiyang Zhang, Stephen Hemminger, netdev Cc: otubo, linux-kernel, devel, vkuznets 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] hv_netvsc: Make sure out channel is fully opened on send 2018-09-27 8:57 ` Mohammed Gamal (?) @ 2018-09-27 10:23 ` Stephen Hemminger 2018-09-27 10:31 ` Mohammed Gamal -1 siblings, 1 reply; 18+ messages in thread From: Stephen Hemminger @ 2018-09-27 10:23 UTC (permalink / raw) To: Mohammed Gamal Cc: Haiyang Zhang, Stephen Hemminger, netdev, otubo, linux-kernel, devel, vkuznets 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. ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] hv_netvsc: Make sure out channel is fully opened on send 2018-09-27 10:23 ` Stephen Hemminger @ 2018-09-27 10:31 ` Mohammed Gamal 0 siblings, 0 replies; 18+ messages in thread From: Mohammed Gamal @ 2018-09-27 10:31 UTC (permalink / raw) To: Stephen Hemminger Cc: Haiyang Zhang, Stephen Hemminger, netdev, otubo, linux-kernel, devel, vkuznets On Thu, 2018-09-27 at 12:23 +0200, Stephen Hemminger wrote: > 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.ker > > > > nel. > > > > 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.g > > > it/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. > Steps to reproduce are listed here: https://bugzilla.redhat.com/show_bug.cgi?id=1632653 We've also managed to reproduce the same issue upstream. It's more likely to be reproduced on Windows 2012R2 than 2016. Regards, Mohammed ^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH] hv_netvsc: Make sure out channel is fully opened on send @ 2018-03-13 19:06 ` Mohammed Gamal 0 siblings, 0 replies; 18+ messages in thread From: Mohammed Gamal @ 2018-03-13 19:06 UTC (permalink / raw) To: netdev, sthemmin Cc: otubo, linux-kernel, devel, vkuznets, Mohammed Gamal, davem 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 | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/drivers/net/hyperv/netvsc.c b/drivers/net/hyperv/netvsc.c index 0265d70..44a8358 100644 --- a/drivers/net/hyperv/netvsc.c +++ b/drivers/net/hyperv/netvsc.c @@ -757,7 +757,7 @@ 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_ringbuf_avail_percent(&out_channel->outbound); + u32 ring_avail; nvmsg.hdr.msg_type = NVSP_MSG1_TYPE_SEND_RNDIS_PKT; if (skb) @@ -773,7 +773,7 @@ static inline int netvsc_send_pkt( req_id = (ulong)skb; - if (out_channel->rescind) + if (out_channel->rescind || out_channel->state != CHANNEL_OPENED_STATE) return -ENODEV; if (packet->page_buf_cnt) { @@ -791,6 +791,7 @@ static inline int netvsc_send_pkt( VMBUS_DATA_PACKET_FLAG_COMPLETION_REQUESTED); } + ring_avail = hv_ringbuf_avail_percent(&out_channel->outbound); if (ret == 0) { atomic_inc_return(&nvchan->queue_sends); -- 1.8.3.1 _______________________________________________ devel mailing list devel@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel ^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH] hv_netvsc: Make sure out channel is fully opened on send @ 2018-03-13 19:06 ` Mohammed Gamal 0 siblings, 0 replies; 18+ messages in thread From: Mohammed Gamal @ 2018-03-13 19:06 UTC (permalink / raw) To: netdev, sthemmin Cc: devel, davem, vkuznets, otubo, linux-kernel, Mohammed Gamal 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 | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/drivers/net/hyperv/netvsc.c b/drivers/net/hyperv/netvsc.c index 0265d70..44a8358 100644 --- a/drivers/net/hyperv/netvsc.c +++ b/drivers/net/hyperv/netvsc.c @@ -757,7 +757,7 @@ 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_ringbuf_avail_percent(&out_channel->outbound); + u32 ring_avail; nvmsg.hdr.msg_type = NVSP_MSG1_TYPE_SEND_RNDIS_PKT; if (skb) @@ -773,7 +773,7 @@ static inline int netvsc_send_pkt( req_id = (ulong)skb; - if (out_channel->rescind) + if (out_channel->rescind || out_channel->state != CHANNEL_OPENED_STATE) return -ENODEV; if (packet->page_buf_cnt) { @@ -791,6 +791,7 @@ static inline int netvsc_send_pkt( VMBUS_DATA_PACKET_FLAG_COMPLETION_REQUESTED); } + ring_avail = hv_ringbuf_avail_percent(&out_channel->outbound); if (ret == 0) { atomic_inc_return(&nvchan->queue_sends); -- 1.8.3.1 ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH] hv_netvsc: Make sure out channel is fully opened on send 2018-03-13 19:06 ` Mohammed Gamal @ 2018-03-13 19:35 ` Stephen Hemminger -1 siblings, 0 replies; 18+ messages in thread From: Stephen Hemminger @ 2018-03-13 19:35 UTC (permalink / raw) To: Mohammed Gamal Cc: otubo, sthemmin, netdev, linux-kernel, devel, vkuznets, davem On Tue, 13 Mar 2018 20:06:50 +0100 Mohammed Gamal <mgamal@redhat.com> wrote: > 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 | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > diff --git a/drivers/net/hyperv/netvsc.c b/drivers/net/hyperv/netvsc.c > index 0265d70..44a8358 100644 > --- a/drivers/net/hyperv/netvsc.c > +++ b/drivers/net/hyperv/netvsc.c > @@ -757,7 +757,7 @@ 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_ringbuf_avail_percent(&out_channel->outbound); > + u32 ring_avail; > > nvmsg.hdr.msg_type = NVSP_MSG1_TYPE_SEND_RNDIS_PKT; > if (skb) > @@ -773,7 +773,7 @@ static inline int netvsc_send_pkt( > > req_id = (ulong)skb; > > - if (out_channel->rescind) > + if (out_channel->rescind || out_channel->state != CHANNEL_OPENED_STATE) > return -ENODEV; > > if (packet->page_buf_cnt) { > @@ -791,6 +791,7 @@ static inline int netvsc_send_pkt( > VMBUS_DATA_PACKET_FLAG_COMPLETION_REQUESTED); > } > > + ring_avail = hv_ringbuf_avail_percent(&out_channel->outbound); > if (ret == 0) { > atomic_inc_return(&nvchan->queue_sends); > Thanks for your patch. Yes there are races with the current update logic. The root cause goes higher up in the flow; the send queues should be stopped before netvsc_device_remove is called. Solving it where you tried to is racy and not going to work reliably. Network patches should go to netdev@vger.kernel.org You can't move the ring_avail check until after the vmbus_sendpacket because that will break the flow control logic. Instead, you should just move the avail_read check until just after the existing rescind check. Also, you shouldn't need to check for OPENED_STATE, just rescind is enough. _______________________________________________ devel mailing list devel@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] hv_netvsc: Make sure out channel is fully opened on send @ 2018-03-13 19:35 ` Stephen Hemminger 0 siblings, 0 replies; 18+ messages in thread From: Stephen Hemminger @ 2018-03-13 19:35 UTC (permalink / raw) To: Mohammed Gamal Cc: netdev, sthemmin, otubo, linux-kernel, devel, vkuznets, davem On Tue, 13 Mar 2018 20:06:50 +0100 Mohammed Gamal <mgamal@redhat.com> wrote: > 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 | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > diff --git a/drivers/net/hyperv/netvsc.c b/drivers/net/hyperv/netvsc.c > index 0265d70..44a8358 100644 > --- a/drivers/net/hyperv/netvsc.c > +++ b/drivers/net/hyperv/netvsc.c > @@ -757,7 +757,7 @@ 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_ringbuf_avail_percent(&out_channel->outbound); > + u32 ring_avail; > > nvmsg.hdr.msg_type = NVSP_MSG1_TYPE_SEND_RNDIS_PKT; > if (skb) > @@ -773,7 +773,7 @@ static inline int netvsc_send_pkt( > > req_id = (ulong)skb; > > - if (out_channel->rescind) > + if (out_channel->rescind || out_channel->state != CHANNEL_OPENED_STATE) > return -ENODEV; > > if (packet->page_buf_cnt) { > @@ -791,6 +791,7 @@ static inline int netvsc_send_pkt( > VMBUS_DATA_PACKET_FLAG_COMPLETION_REQUESTED); > } > > + ring_avail = hv_ringbuf_avail_percent(&out_channel->outbound); > if (ret == 0) { > atomic_inc_return(&nvchan->queue_sends); > Thanks for your patch. Yes there are races with the current update logic. The root cause goes higher up in the flow; the send queues should be stopped before netvsc_device_remove is called. Solving it where you tried to is racy and not going to work reliably. Network patches should go to netdev@vger.kernel.org You can't move the ring_avail check until after the vmbus_sendpacket because that will break the flow control logic. Instead, you should just move the avail_read check until just after the existing rescind check. Also, you shouldn't need to check for OPENED_STATE, just rescind is enough. ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] hv_netvsc: Make sure out channel is fully opened on send 2018-03-13 19:35 ` Stephen Hemminger (?) @ 2018-03-14 9:22 ` Mohammed Gamal 2018-03-15 16:24 ` Mohammed Gamal -1 siblings, 1 reply; 18+ messages in thread From: Mohammed Gamal @ 2018-03-14 9:22 UTC (permalink / raw) To: Stephen Hemminger Cc: otubo, sthemmin, netdev, linux-kernel, devel, vkuznets, davem On Tue, 2018-03-13 at 12:35 -0700, Stephen Hemminger wrote: > On Tue, 13 Mar 2018 20:06:50 +0100 > Mohammed Gamal <mgamal@redhat.com> wrote: > > > 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 | 5 +++-- > > 1 file changed, 3 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/net/hyperv/netvsc.c > > b/drivers/net/hyperv/netvsc.c > > index 0265d70..44a8358 100644 > > --- a/drivers/net/hyperv/netvsc.c > > +++ b/drivers/net/hyperv/netvsc.c > > @@ -757,7 +757,7 @@ 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_ringbuf_avail_percent(&out_channel- > > >outbound); > > + u32 ring_avail; > > > > nvmsg.hdr.msg_type = NVSP_MSG1_TYPE_SEND_RNDIS_PKT; > > if (skb) > > @@ -773,7 +773,7 @@ static inline int netvsc_send_pkt( > > > > req_id = (ulong)skb; > > > > - if (out_channel->rescind) > > + if (out_channel->rescind || out_channel->state != > > CHANNEL_OPENED_STATE) > > return -ENODEV; > > > > if (packet->page_buf_cnt) { > > @@ -791,6 +791,7 @@ static inline int netvsc_send_pkt( > > VMBUS_DATA_PACKET_FLAG_COMP > > LETION_REQUESTED); > > } > > > > + ring_avail = hv_ringbuf_avail_percent(&out_channel- > > >outbound); > > if (ret == 0) { > > atomic_inc_return(&nvchan->queue_sends); > > > > Thanks for your patch. Yes there are races with the current update > logic. The root cause goes higher up in the flow; the send queues > should > be stopped before netvsc_device_remove is called. Solving it where > you tried > to is racy and not going to work reliably. > > Network patches should go to netdev@vger.kernel.org > > You can't move the ring_avail check until after the vmbus_sendpacket > because > that will break the flow control logic. > Why? I don't see ring_avail being used before that point. > Instead, you should just move the avail_read check until just after > the existing rescind > check. > > Also, you shouldn't need to check for OPENED_STATE, just rescind is > enough. That rarely mitigated the race. channel->rescind flag is set on vmbus exit - called on module unload - and when a rescind offer is received from the host, which AFAICT doesn't happen on every call to netvsc_device_remove, so it's quite possible that the ringbuffer is accessed before it's allocated again on channel open and hence the check for OPENED_STAT - which is only set after all vmbus data is initialized. > > > > _______________________________________________ devel mailing list devel@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] hv_netvsc: Make sure out channel is fully opened on send 2018-03-14 9:22 ` Mohammed Gamal @ 2018-03-15 16:24 ` Mohammed Gamal 2018-03-15 17:40 ` Stephen Hemminger 0 siblings, 1 reply; 18+ messages in thread From: Mohammed Gamal @ 2018-03-15 16:24 UTC (permalink / raw) To: Stephen Hemminger Cc: otubo, sthemmin, netdev, linux-kernel, devel, vkuznets, davem On Wed, 2018-03-14 at 10:22 +0100, Mohammed Gamal wrote: > On Tue, 2018-03-13 at 12:35 -0700, Stephen Hemminger wrote: > > On Tue, 13 Mar 2018 20:06:50 +0100 > > Mohammed Gamal <mgamal@redhat.com> wrote: > > > > > 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 | 5 +++-- > > > 1 file changed, 3 insertions(+), 2 deletions(-) > > > > > > diff --git a/drivers/net/hyperv/netvsc.c > > > b/drivers/net/hyperv/netvsc.c > > > index 0265d70..44a8358 100644 > > > --- a/drivers/net/hyperv/netvsc.c > > > +++ b/drivers/net/hyperv/netvsc.c > > > @@ -757,7 +757,7 @@ 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_ringbuf_avail_percent(&out_channel- > > > > outbound); > > > > > > + u32 ring_avail; > > > > > > nvmsg.hdr.msg_type = NVSP_MSG1_TYPE_SEND_RNDIS_PKT; > > > if (skb) > > > @@ -773,7 +773,7 @@ static inline int netvsc_send_pkt( > > > > > > req_id = (ulong)skb; > > > > > > - if (out_channel->rescind) > > > + if (out_channel->rescind || out_channel->state != > > > CHANNEL_OPENED_STATE) > > > return -ENODEV; > > > > > > if (packet->page_buf_cnt) { > > > @@ -791,6 +791,7 @@ static inline int netvsc_send_pkt( > > > VMBUS_DATA_PACKET_FLAG_CO > > > MP > > > LETION_REQUESTED); > > > } > > > > > > + ring_avail = hv_ringbuf_avail_percent(&out_channel- > > > > outbound); > > > > > > if (ret == 0) { > > > atomic_inc_return(&nvchan->queue_sends); > > > > > > > Thanks for your patch. Yes there are races with the current update > > logic. The root cause goes higher up in the flow; the send queues > > should > > be stopped before netvsc_device_remove is called. Solving it where > > you tried > > to is racy and not going to work reliably. > > > > Network patches should go to netdev@vger.kernel.org > > > > You can't move the ring_avail check until after the > > vmbus_sendpacket > > because > > that will break the flow control logic. > > > > Why? I don't see ring_avail being used before that point. Ah, stupid me. vmbus_sendpacket() will write to the ring buffer and that means that ring_avail value will be different than the expected. > > > Instead, you should just move the avail_read check until just after > > the existing rescind > > check. > > > > Also, you shouldn't need to check for OPENED_STATE, just rescind is > > enough. > > That rarely mitigated the race. channel->rescind flag is set on vmbus > exit - called on module unload - and when a rescind offer is received > from the host, which AFAICT doesn't happen on every call to > netvsc_device_remove, so it's quite possible that the ringbuffer is > accessed before it's allocated again on channel open and hence the > check for OPENED_STAT - which is only set after all vmbus data is > initialized. > Perhaps I haven't been clear enough. The NULL pointer dereference happens in the call to hv_ringbuf_avail_percent() which is used to calculate ring_avail. So we need to stop the queues before calling it if the channel's ring buffers haven't been allocated yet, but OTOH we should only stop the queues based upon the value of ring_avail, so this leads into a chicken and egg situation. Is my observation here correct? Please correct me if I am wrong, Stephen. _______________________________________________ devel mailing list devel@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] hv_netvsc: Make sure out channel is fully opened on send 2018-03-15 16:24 ` Mohammed Gamal @ 2018-03-15 17:40 ` Stephen Hemminger 0 siblings, 0 replies; 18+ messages in thread From: Stephen Hemminger @ 2018-03-15 17:40 UTC (permalink / raw) To: Mohammed Gamal Cc: otubo, sthemmin, netdev, linux-kernel, devel, vkuznets, davem On Thu, 15 Mar 2018 17:24:13 +0100 Mohammed Gamal <mgamal@redhat.com> wrote: > On Wed, 2018-03-14 at 10:22 +0100, Mohammed Gamal wrote: > > On Tue, 2018-03-13 at 12:35 -0700, Stephen Hemminger wrote: > > > On Tue, 13 Mar 2018 20:06:50 +0100 > > > Mohammed Gamal <mgamal@redhat.com> wrote: > > > > > > > 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 | 5 +++-- > > > > 1 file changed, 3 insertions(+), 2 deletions(-) > > > > > > > > diff --git a/drivers/net/hyperv/netvsc.c > > > > b/drivers/net/hyperv/netvsc.c > > > > index 0265d70..44a8358 100644 > > > > --- a/drivers/net/hyperv/netvsc.c > > > > +++ b/drivers/net/hyperv/netvsc.c > > > > @@ -757,7 +757,7 @@ 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_ringbuf_avail_percent(&out_channel- > > > > > outbound); > > > > > > > > + u32 ring_avail; > > > > > > > > nvmsg.hdr.msg_type = NVSP_MSG1_TYPE_SEND_RNDIS_PKT; > > > > if (skb) > > > > @@ -773,7 +773,7 @@ static inline int netvsc_send_pkt( > > > > > > > > req_id = (ulong)skb; > > > > > > > > - if (out_channel->rescind) > > > > + if (out_channel->rescind || out_channel->state != > > > > CHANNEL_OPENED_STATE) > > > > return -ENODEV; > > > > > > > > if (packet->page_buf_cnt) { > > > > @@ -791,6 +791,7 @@ static inline int netvsc_send_pkt( > > > > VMBUS_DATA_PACKET_FLAG_CO > > > > MP > > > > LETION_REQUESTED); > > > > } > > > > > > > > + ring_avail = hv_ringbuf_avail_percent(&out_channel- > > > > > outbound); > > > > > > > > if (ret == 0) { > > > > atomic_inc_return(&nvchan->queue_sends); > > > > > > > > > > Thanks for your patch. Yes there are races with the current update > > > logic. The root cause goes higher up in the flow; the send queues > > > should > > > be stopped before netvsc_device_remove is called. Solving it where > > > you tried > > > to is racy and not going to work reliably. > > > > > > Network patches should go to netdev@vger.kernel.org > > > > > > You can't move the ring_avail check until after the > > > vmbus_sendpacket > > > because > > > that will break the flow control logic. > > > > > > > Why? I don't see ring_avail being used before that point. > > Ah, stupid me. vmbus_sendpacket() will write to the ring buffer and > that means that ring_avail value will be different than the expected. > > > > > > Instead, you should just move the avail_read check until just after > > > the existing rescind > > > check. > > > > > > Also, you shouldn't need to check for OPENED_STATE, just rescind is > > > enough. > > > > That rarely mitigated the race. channel->rescind flag is set on vmbus > > exit - called on module unload - and when a rescind offer is received > > from the host, which AFAICT doesn't happen on every call to > > netvsc_device_remove, so it's quite possible that the ringbuffer is > > accessed before it's allocated again on channel open and hence the > > check for OPENED_STAT - which is only set after all vmbus data is > > initialized. > > > > Perhaps I haven't been clear enough. The NULL pointer dereference > happens in the call to hv_ringbuf_avail_percent() which is used to > calculate ring_avail. > > So we need to stop the queues before calling it if the channel's ring > buffers haven't been allocated yet, but OTOH we should only stop the > queues based upon the value of ring_avail, so this leads into a chicken > and egg situation. > > Is my observation here correct? Please correct me if I am wrong, > Stephen. This is a far more drastic work of the shutdown logic which I am still working on. The current netvsc driver is not doing a good job of handling concurrent send/receives during ring changes. From a22da18b41ad5024340dddcc989d918987836f6d Mon Sep 17 00:00:00 2001 From: Stephen Hemminger <sthemmin@microsoft.com> Date: Tue, 6 Feb 2018 15:05:19 -0800 Subject: [PATCH] hv_netvsc: common detach logic Make common function for detaching internals of device during changes to MTU and RSS. Make sure no more packets are transmitted and all packets have been received before doing device teardown. Changes transmit enabling logic so that transmit queues are disabled during the period when lower device is being changed. And enabled only after sub channels are setup. This avoids issue where it could be that a packet was being sent while subchannel was not initialized. Signed-off-by: Stephen Hemminger <sthemmin@microsoft.com> --- drivers/net/hyperv/hyperv_net.h | 1 - drivers/net/hyperv/netvsc.c | 20 +-- drivers/net/hyperv/netvsc_drv.c | 268 +++++++++++++++++++++----------------- drivers/net/hyperv/rndis_filter.c | 14 +- 4 files changed, 160 insertions(+), 143 deletions(-) diff --git a/drivers/net/hyperv/hyperv_net.h b/drivers/net/hyperv/hyperv_net.h index cd538d5a7986..32861036c3fc 100644 --- a/drivers/net/hyperv/hyperv_net.h +++ b/drivers/net/hyperv/hyperv_net.h @@ -212,7 +212,6 @@ void netvsc_channel_cb(void *context); int netvsc_poll(struct napi_struct *napi, int budget); void rndis_set_subchannel(struct work_struct *w); -bool rndis_filter_opened(const struct netvsc_device *nvdev); int rndis_filter_open(struct netvsc_device *nvdev); int rndis_filter_close(struct netvsc_device *nvdev); struct netvsc_device *rndis_filter_device_add(struct hv_device *dev, diff --git a/drivers/net/hyperv/netvsc.c b/drivers/net/hyperv/netvsc.c index 8c95a3797b2f..31b1c6c430bb 100644 --- a/drivers/net/hyperv/netvsc.c +++ b/drivers/net/hyperv/netvsc.c @@ -573,8 +573,6 @@ void netvsc_device_remove(struct hv_device *device) = rtnl_dereference(net_device_ctx->nvdev); int i; - cancel_work_sync(&net_device->subchan_work); - netvsc_revoke_buf(device, net_device); RCU_INIT_POINTER(net_device_ctx->nvdev, NULL); @@ -661,14 +659,18 @@ static void netvsc_send_tx_complete(struct netvsc_device *net_device, queue_sends = atomic_dec_return(&net_device->chan_table[q_idx].queue_sends); - if (net_device->destroy && queue_sends == 0) - wake_up(&net_device->wait_drain); + if (unlikely(net_device->destroy)) { + if (queue_sends == 0) + wake_up(&net_device->wait_drain); + } else { + struct netdev_queue *txq = netdev_get_tx_queue(ndev, q_idx); - if (netif_tx_queue_stopped(netdev_get_tx_queue(ndev, q_idx)) && - (hv_ringbuf_avail_percent(&channel->outbound) > RING_AVAIL_PERCENT_HIWATER || - queue_sends < 1)) { - netif_tx_wake_queue(netdev_get_tx_queue(ndev, q_idx)); - ndev_ctx->eth_stats.wake_queue++; + if (netif_tx_queue_stopped(txq) && + (hv_ringbuf_avail_percent(&channel->outbound) > RING_AVAIL_PERCENT_HIWATER || + queue_sends < 1)) { + netif_tx_wake_queue(txq); + ndev_ctx->eth_stats.wake_queue++; + } } } diff --git a/drivers/net/hyperv/netvsc_drv.c b/drivers/net/hyperv/netvsc_drv.c index faea0be18924..721fac7cad81 100644 --- a/drivers/net/hyperv/netvsc_drv.c +++ b/drivers/net/hyperv/netvsc_drv.c @@ -46,7 +46,8 @@ #include "hyperv_net.h" -#define RING_SIZE_MIN 64 +#define RING_SIZE_MIN 64 +#define RETRY_MAX 2000 #define LINKCHANGE_INT (2 * HZ) #define VF_TAKEOVER_INT (HZ / 10) @@ -123,10 +124,8 @@ static int netvsc_open(struct net_device *net) } rdev = nvdev->extension; - if (!rdev->link_state) { + if (!rdev->link_state) netif_carrier_on(net); - netif_tx_wake_all_queues(net); - } if (vf_netdev) { /* Setting synthetic device up transparently sets @@ -142,36 +141,25 @@ static int netvsc_open(struct net_device *net) return 0; } -static int netvsc_close(struct net_device *net) +static int netvsc_wait_until_empty(struct netvsc_device *nvdev) { - struct net_device_context *net_device_ctx = netdev_priv(net); - struct net_device *vf_netdev - = rtnl_dereference(net_device_ctx->vf_netdev); - struct netvsc_device *nvdev = rtnl_dereference(net_device_ctx->nvdev); - int ret = 0; - u32 aread, i, msec = 10, retry = 0, retry_max = 20; - struct vmbus_channel *chn; - - netif_tx_disable(net); - - /* No need to close rndis filter if it is removed already */ - if (!nvdev) - goto out; - - ret = rndis_filter_close(nvdev); - if (ret != 0) { - netdev_err(net, "unable to close device (ret %d).\n", ret); - return ret; - } + unsigned int retry = 0; + int i; /* Ensure pending bytes in ring are read */ - while (true) { - aread = 0; + for (;;) { + u32 aread = 0; + for (i = 0; i < nvdev->num_chn; i++) { - chn = nvdev->chan_table[i].channel; + struct vmbus_channel *chn + = nvdev->chan_table[i].channel; + if (!chn) continue; + /* make sure receive not running now */ + napi_synchronize(&nvdev->chan_table[i].napi); + aread = hv_get_bytes_to_read(&chn->inbound); if (aread) break; @@ -181,22 +169,40 @@ static int netvsc_close(struct net_device *net) break; } - retry++; - if (retry > retry_max || aread == 0) - break; + if (aread == 0) + return 0; - msleep(msec); + if (++retry > RETRY_MAX) + return -ETIMEDOUT; - if (msec < 1000) - msec *= 2; + usleep_range(1000, 2000); } +} - if (aread) { - netdev_err(net, "Ring buffer not empty after closing rndis\n"); - ret = -ETIMEDOUT; +static int netvsc_close(struct net_device *net) +{ + struct net_device_context *net_device_ctx = netdev_priv(net); + struct net_device *vf_netdev + = rtnl_dereference(net_device_ctx->vf_netdev); + struct netvsc_device *nvdev = rtnl_dereference(net_device_ctx->nvdev); + int ret; + + netif_tx_disable(net); + + /* No need to close rndis filter if it is removed already */ + if (!nvdev) + return 0; + + ret = rndis_filter_close(nvdev); + if (ret != 0) { + netdev_err(net, "unable to close device (ret %d).\n", ret); + return ret; } -out: + ret = netvsc_wait_until_empty(nvdev); + if (ret) + netdev_err(net, "Ring buffer not empty after closing rndis\n"); + if (vf_netdev) dev_close(vf_netdev); @@ -845,16 +851,76 @@ static void netvsc_get_channels(struct net_device *net, } } +static int netvsc_detach(struct net_device *ndev, + struct netvsc_device *nvdev) +{ + struct net_device_context *ndev_ctx = netdev_priv(ndev); + struct hv_device *hdev = ndev_ctx->device_ctx; + int ret; + + /* Don't try continuing to try and setup sub channels */ + if (cancel_work_sync(&nvdev->subchan_work)) + nvdev->num_chn = 1; + + /* If device was up (receiving) then shutdown */ + if (netif_running(ndev)) { + netif_tx_disable(ndev); + + ret = rndis_filter_close(nvdev); + if (ret) { + netdev_err(ndev, + "unable to close device (ret %d).\n", ret); + return ret; + } + + ret = netvsc_wait_until_empty(nvdev); + if (ret) { + netdev_err(ndev, + "Ring buffer not empty after closing rndis\n"); + return ret; + } + } + + rndis_filter_device_remove(hdev, nvdev); + return 0; +} + +static int netvsc_attach(struct net_device *ndev, + struct netvsc_device_info *dev_info) +{ + struct net_device_context *ndev_ctx = netdev_priv(ndev); + struct hv_device *hdev = ndev_ctx->device_ctx; + struct netvsc_device *nvdev; + struct rndis_device *rdev; + int ret; + + nvdev = rndis_filter_device_add(hdev, dev_info); + if (IS_ERR(nvdev)) + return PTR_ERR(nvdev); + + netif_carrier_off(ndev); + + if (netif_running(ndev)) { + ret = rndis_filter_open(nvdev); + if (ret) + return ret; + + rdev = nvdev->extension; + if (!rdev->link_state) + netif_carrier_on(ndev); + } + + return 0; +} + static int netvsc_set_channels(struct net_device *net, struct ethtool_channels *channels) { struct net_device_context *net_device_ctx = netdev_priv(net); - struct hv_device *dev = net_device_ctx->device_ctx; struct netvsc_device *nvdev = rtnl_dereference(net_device_ctx->nvdev); unsigned int orig, count = channels->combined_count; struct netvsc_device_info device_info; - bool was_opened; - int ret = 0; + int ret; /* We do not support separate count for rx, tx, or other */ if (count == 0 || @@ -871,9 +937,6 @@ static int netvsc_set_channels(struct net_device *net, return -EINVAL; orig = nvdev->num_chn; - was_opened = rndis_filter_opened(nvdev); - if (was_opened) - rndis_filter_close(nvdev); memset(&device_info, 0, sizeof(device_info)); device_info.num_chn = count; @@ -882,28 +945,17 @@ static int netvsc_set_channels(struct net_device *net, device_info.recv_sections = nvdev->recv_section_cnt; device_info.recv_section_size = nvdev->recv_section_size; - rndis_filter_device_remove(dev, nvdev); + ret = netvsc_detach(net, nvdev); + if (ret) + return ret; - nvdev = rndis_filter_device_add(dev, &device_info); - if (IS_ERR(nvdev)) { - ret = PTR_ERR(nvdev); + ret = netvsc_attach(net, &device_info); + if (ret) { device_info.num_chn = orig; - nvdev = rndis_filter_device_add(dev, &device_info); - - if (IS_ERR(nvdev)) { - netdev_err(net, "restoring channel setting failed: %ld\n", - PTR_ERR(nvdev)); - return ret; - } + if (netvsc_attach(net, &device_info)) + netdev_err(net, "restoring channel setting failed\n"); } - if (was_opened) - rndis_filter_open(nvdev); - - /* We may have missed link change notifications */ - net_device_ctx->last_reconfig = 0; - schedule_delayed_work(&net_device_ctx->dwork, 0); - return ret; } @@ -969,10 +1021,8 @@ static int netvsc_change_mtu(struct net_device *ndev, int mtu) struct net_device_context *ndevctx = netdev_priv(ndev); struct net_device *vf_netdev = rtnl_dereference(ndevctx->vf_netdev); struct netvsc_device *nvdev = rtnl_dereference(ndevctx->nvdev); - struct hv_device *hdev = ndevctx->device_ctx; int orig_mtu = ndev->mtu; struct netvsc_device_info device_info; - bool was_opened; int ret = 0; if (!nvdev || nvdev->destroy) @@ -985,11 +1035,6 @@ static int netvsc_change_mtu(struct net_device *ndev, int mtu) return ret; } - netif_device_detach(ndev); - was_opened = rndis_filter_opened(nvdev); - if (was_opened) - rndis_filter_close(nvdev); - memset(&device_info, 0, sizeof(device_info)); device_info.num_chn = nvdev->num_chn; device_info.send_sections = nvdev->send_section_cnt; @@ -997,35 +1042,27 @@ static int netvsc_change_mtu(struct net_device *ndev, int mtu) device_info.recv_sections = nvdev->recv_section_cnt; device_info.recv_section_size = nvdev->recv_section_size; - rndis_filter_device_remove(hdev, nvdev); + ret = netvsc_detach(ndev, nvdev); + if (ret) + goto rollback_vf; ndev->mtu = mtu; - nvdev = rndis_filter_device_add(hdev, &device_info); - if (IS_ERR(nvdev)) { - ret = PTR_ERR(nvdev); - - /* Attempt rollback to original MTU */ - ndev->mtu = orig_mtu; - nvdev = rndis_filter_device_add(hdev, &device_info); - - if (vf_netdev) - dev_set_mtu(vf_netdev, orig_mtu); - - if (IS_ERR(nvdev)) { - netdev_err(ndev, "restoring mtu failed: %ld\n", - PTR_ERR(nvdev)); - return ret; - } - } + ret = netvsc_attach(ndev, &device_info); + if (ret) + goto rollback; - if (was_opened) - rndis_filter_open(nvdev); + return 0; - netif_device_attach(ndev); +rollback: + /* Attempt rollback to original MTU */ + ndev->mtu = orig_mtu; - /* We may have missed link change notifications */ - schedule_delayed_work(&ndevctx->dwork, 0); + if (netvsc_attach(ndev, &device_info)) + netdev_err(ndev, "restoring mtu failed\n"); +rollback_vf: + if (vf_netdev) + dev_set_mtu(vf_netdev, orig_mtu); return ret; } @@ -1531,11 +1568,9 @@ static int netvsc_set_ringparam(struct net_device *ndev, { struct net_device_context *ndevctx = netdev_priv(ndev); struct netvsc_device *nvdev = rtnl_dereference(ndevctx->nvdev); - struct hv_device *hdev = ndevctx->device_ctx; struct netvsc_device_info device_info; struct ethtool_ringparam orig; u32 new_tx, new_rx; - bool was_opened; int ret = 0; if (!nvdev || nvdev->destroy) @@ -1560,34 +1595,18 @@ static int netvsc_set_ringparam(struct net_device *ndev, device_info.recv_sections = new_rx; device_info.recv_section_size = nvdev->recv_section_size; - netif_device_detach(ndev); - was_opened = rndis_filter_opened(nvdev); - if (was_opened) - rndis_filter_close(nvdev); - - rndis_filter_device_remove(hdev, nvdev); - - nvdev = rndis_filter_device_add(hdev, &device_info); - if (IS_ERR(nvdev)) { - ret = PTR_ERR(nvdev); + ret = netvsc_detach(ndev, nvdev); + if (ret) + return ret; + ret = netvsc_attach(ndev, &device_info); + if (ret) { device_info.send_sections = orig.tx_pending; device_info.recv_sections = orig.rx_pending; - nvdev = rndis_filter_device_add(hdev, &device_info); - if (IS_ERR(nvdev)) { - netdev_err(ndev, "restoring ringparam failed: %ld\n", - PTR_ERR(nvdev)); - return ret; - } - } - - if (was_opened) - rndis_filter_open(nvdev); - netif_device_attach(ndev); - /* We may have missed link change notifications */ - ndevctx->last_reconfig = 0; - schedule_delayed_work(&ndevctx->dwork, 0); + if (netvsc_attach(ndev, &device_info)) + netdev_err(ndev, "restoring ringparam failed"); + } return ret; } @@ -2072,8 +2091,8 @@ static int netvsc_probe(struct hv_device *dev, static int netvsc_remove(struct hv_device *dev) { struct net_device_context *ndev_ctx; - struct net_device *vf_netdev; - struct net_device *net; + struct net_device *vf_netdev, *net; + struct netvsc_device *nvdev; net = hv_get_drvdata(dev); if (net == NULL) { @@ -2083,8 +2102,6 @@ static int netvsc_remove(struct hv_device *dev) ndev_ctx = netdev_priv(net); - netif_device_detach(net); - cancel_delayed_work_sync(&ndev_ctx->dwork); /* @@ -2092,14 +2109,19 @@ static int netvsc_remove(struct hv_device *dev) * removed. Also blocks mtu and channel changes. */ rtnl_lock(); + vf_netdev = rtnl_dereference(ndev_ctx->vf_netdev); if (vf_netdev) netvsc_unregister_vf(vf_netdev); + nvdev = rtnl_dereference(ndev_ctx->nvdev); + if (nvdev) { + cancel_work_sync(&nvdev->subchan_work); + rndis_filter_device_remove(dev, nvdev); + } + unregister_netdevice(net); - rndis_filter_device_remove(dev, - rtnl_dereference(ndev_ctx->nvdev)); rtnl_unlock(); hv_set_drvdata(dev, NULL); diff --git a/drivers/net/hyperv/rndis_filter.c b/drivers/net/hyperv/rndis_filter.c index a9746f9fbf4b..3ccb0c6263a8 100644 --- a/drivers/net/hyperv/rndis_filter.c +++ b/drivers/net/hyperv/rndis_filter.c @@ -1122,6 +1122,7 @@ void rndis_set_subchannel(struct work_struct *w) for (i = 0; i < VRSS_SEND_TAB_SIZE; i++) ndev_ctx->tx_table[i] = i % nvdev->num_chn; + netif_tx_wake_all_queues(ndev); rtnl_unlock(); return; @@ -1132,6 +1133,7 @@ void rndis_set_subchannel(struct work_struct *w) nvdev->max_chn = 1; nvdev->num_chn = 1; + netif_wake_queue(ndev); unlock: rtnl_unlock(); } @@ -1326,6 +1328,8 @@ struct netvsc_device *rndis_filter_device_add(struct hv_device *dev, if (net_device->num_chn > 1) schedule_work(&net_device->subchan_work); + else + netif_wake_queue(net); out: /* if unavailable, just proceed with one queue */ @@ -1346,9 +1350,6 @@ void rndis_filter_device_remove(struct hv_device *dev, { struct rndis_device *rndis_dev = net_dev->extension; - /* Don't try and setup sub channels if about to halt */ - cancel_work_sync(&net_dev->subchan_work); - /* Halt and release the rndis device */ rndis_filter_halt_device(net_dev, rndis_dev); @@ -1372,10 +1373,3 @@ int rndis_filter_close(struct netvsc_device *nvdev) return rndis_filter_close_device(nvdev->extension); } - -bool rndis_filter_opened(const struct netvsc_device *nvdev) -{ - const struct rndis_device *dev = nvdev->extension; - - return dev->state == RNDIS_DEV_DATAINITIALIZED; -} -- 2.16.1 _______________________________________________ devel mailing list devel@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH] hv_netvsc: Make sure out channel is fully opened on send @ 2018-03-15 17:40 ` Stephen Hemminger 0 siblings, 0 replies; 18+ messages in thread From: Stephen Hemminger @ 2018-03-15 17:40 UTC (permalink / raw) To: Mohammed Gamal Cc: netdev, sthemmin, otubo, linux-kernel, devel, vkuznets, davem On Thu, 15 Mar 2018 17:24:13 +0100 Mohammed Gamal <mgamal@redhat.com> wrote: > On Wed, 2018-03-14 at 10:22 +0100, Mohammed Gamal wrote: > > On Tue, 2018-03-13 at 12:35 -0700, Stephen Hemminger wrote: > > > On Tue, 13 Mar 2018 20:06:50 +0100 > > > Mohammed Gamal <mgamal@redhat.com> wrote: > > > > > > > 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 | 5 +++-- > > > > 1 file changed, 3 insertions(+), 2 deletions(-) > > > > > > > > diff --git a/drivers/net/hyperv/netvsc.c > > > > b/drivers/net/hyperv/netvsc.c > > > > index 0265d70..44a8358 100644 > > > > --- a/drivers/net/hyperv/netvsc.c > > > > +++ b/drivers/net/hyperv/netvsc.c > > > > @@ -757,7 +757,7 @@ 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_ringbuf_avail_percent(&out_channel- > > > > > outbound); > > > > > > > > + u32 ring_avail; > > > > > > > > nvmsg.hdr.msg_type = NVSP_MSG1_TYPE_SEND_RNDIS_PKT; > > > > if (skb) > > > > @@ -773,7 +773,7 @@ static inline int netvsc_send_pkt( > > > > > > > > req_id = (ulong)skb; > > > > > > > > - if (out_channel->rescind) > > > > + if (out_channel->rescind || out_channel->state != > > > > CHANNEL_OPENED_STATE) > > > > return -ENODEV; > > > > > > > > if (packet->page_buf_cnt) { > > > > @@ -791,6 +791,7 @@ static inline int netvsc_send_pkt( > > > > VMBUS_DATA_PACKET_FLAG_CO > > > > MP > > > > LETION_REQUESTED); > > > > } > > > > > > > > + ring_avail = hv_ringbuf_avail_percent(&out_channel- > > > > > outbound); > > > > > > > > if (ret == 0) { > > > > atomic_inc_return(&nvchan->queue_sends); > > > > > > > > > > Thanks for your patch. Yes there are races with the current update > > > logic. The root cause goes higher up in the flow; the send queues > > > should > > > be stopped before netvsc_device_remove is called. Solving it where > > > you tried > > > to is racy and not going to work reliably. > > > > > > Network patches should go to netdev@vger.kernel.org > > > > > > You can't move the ring_avail check until after the > > > vmbus_sendpacket > > > because > > > that will break the flow control logic. > > > > > > > Why? I don't see ring_avail being used before that point. > > Ah, stupid me. vmbus_sendpacket() will write to the ring buffer and > that means that ring_avail value will be different than the expected. > > > > > > Instead, you should just move the avail_read check until just after > > > the existing rescind > > > check. > > > > > > Also, you shouldn't need to check for OPENED_STATE, just rescind is > > > enough. > > > > That rarely mitigated the race. channel->rescind flag is set on vmbus > > exit - called on module unload - and when a rescind offer is received > > from the host, which AFAICT doesn't happen on every call to > > netvsc_device_remove, so it's quite possible that the ringbuffer is > > accessed before it's allocated again on channel open and hence the > > check for OPENED_STAT - which is only set after all vmbus data is > > initialized. > > > > Perhaps I haven't been clear enough. The NULL pointer dereference > happens in the call to hv_ringbuf_avail_percent() which is used to > calculate ring_avail. > > So we need to stop the queues before calling it if the channel's ring > buffers haven't been allocated yet, but OTOH we should only stop the > queues based upon the value of ring_avail, so this leads into a chicken > and egg situation. > > Is my observation here correct? Please correct me if I am wrong, > Stephen. This is a far more drastic work of the shutdown logic which I am still working on. The current netvsc driver is not doing a good job of handling concurrent send/receives during ring changes. From a22da18b41ad5024340dddcc989d918987836f6d Mon Sep 17 00:00:00 2001 From: Stephen Hemminger <sthemmin@microsoft.com> Date: Tue, 6 Feb 2018 15:05:19 -0800 Subject: [PATCH] hv_netvsc: common detach logic Make common function for detaching internals of device during changes to MTU and RSS. Make sure no more packets are transmitted and all packets have been received before doing device teardown. Changes transmit enabling logic so that transmit queues are disabled during the period when lower device is being changed. And enabled only after sub channels are setup. This avoids issue where it could be that a packet was being sent while subchannel was not initialized. Signed-off-by: Stephen Hemminger <sthemmin@microsoft.com> --- drivers/net/hyperv/hyperv_net.h | 1 - drivers/net/hyperv/netvsc.c | 20 +-- drivers/net/hyperv/netvsc_drv.c | 268 +++++++++++++++++++++----------------- drivers/net/hyperv/rndis_filter.c | 14 +- 4 files changed, 160 insertions(+), 143 deletions(-) diff --git a/drivers/net/hyperv/hyperv_net.h b/drivers/net/hyperv/hyperv_net.h index cd538d5a7986..32861036c3fc 100644 --- a/drivers/net/hyperv/hyperv_net.h +++ b/drivers/net/hyperv/hyperv_net.h @@ -212,7 +212,6 @@ void netvsc_channel_cb(void *context); int netvsc_poll(struct napi_struct *napi, int budget); void rndis_set_subchannel(struct work_struct *w); -bool rndis_filter_opened(const struct netvsc_device *nvdev); int rndis_filter_open(struct netvsc_device *nvdev); int rndis_filter_close(struct netvsc_device *nvdev); struct netvsc_device *rndis_filter_device_add(struct hv_device *dev, diff --git a/drivers/net/hyperv/netvsc.c b/drivers/net/hyperv/netvsc.c index 8c95a3797b2f..31b1c6c430bb 100644 --- a/drivers/net/hyperv/netvsc.c +++ b/drivers/net/hyperv/netvsc.c @@ -573,8 +573,6 @@ void netvsc_device_remove(struct hv_device *device) = rtnl_dereference(net_device_ctx->nvdev); int i; - cancel_work_sync(&net_device->subchan_work); - netvsc_revoke_buf(device, net_device); RCU_INIT_POINTER(net_device_ctx->nvdev, NULL); @@ -661,14 +659,18 @@ static void netvsc_send_tx_complete(struct netvsc_device *net_device, queue_sends = atomic_dec_return(&net_device->chan_table[q_idx].queue_sends); - if (net_device->destroy && queue_sends == 0) - wake_up(&net_device->wait_drain); + if (unlikely(net_device->destroy)) { + if (queue_sends == 0) + wake_up(&net_device->wait_drain); + } else { + struct netdev_queue *txq = netdev_get_tx_queue(ndev, q_idx); - if (netif_tx_queue_stopped(netdev_get_tx_queue(ndev, q_idx)) && - (hv_ringbuf_avail_percent(&channel->outbound) > RING_AVAIL_PERCENT_HIWATER || - queue_sends < 1)) { - netif_tx_wake_queue(netdev_get_tx_queue(ndev, q_idx)); - ndev_ctx->eth_stats.wake_queue++; + if (netif_tx_queue_stopped(txq) && + (hv_ringbuf_avail_percent(&channel->outbound) > RING_AVAIL_PERCENT_HIWATER || + queue_sends < 1)) { + netif_tx_wake_queue(txq); + ndev_ctx->eth_stats.wake_queue++; + } } } diff --git a/drivers/net/hyperv/netvsc_drv.c b/drivers/net/hyperv/netvsc_drv.c index faea0be18924..721fac7cad81 100644 --- a/drivers/net/hyperv/netvsc_drv.c +++ b/drivers/net/hyperv/netvsc_drv.c @@ -46,7 +46,8 @@ #include "hyperv_net.h" -#define RING_SIZE_MIN 64 +#define RING_SIZE_MIN 64 +#define RETRY_MAX 2000 #define LINKCHANGE_INT (2 * HZ) #define VF_TAKEOVER_INT (HZ / 10) @@ -123,10 +124,8 @@ static int netvsc_open(struct net_device *net) } rdev = nvdev->extension; - if (!rdev->link_state) { + if (!rdev->link_state) netif_carrier_on(net); - netif_tx_wake_all_queues(net); - } if (vf_netdev) { /* Setting synthetic device up transparently sets @@ -142,36 +141,25 @@ static int netvsc_open(struct net_device *net) return 0; } -static int netvsc_close(struct net_device *net) +static int netvsc_wait_until_empty(struct netvsc_device *nvdev) { - struct net_device_context *net_device_ctx = netdev_priv(net); - struct net_device *vf_netdev - = rtnl_dereference(net_device_ctx->vf_netdev); - struct netvsc_device *nvdev = rtnl_dereference(net_device_ctx->nvdev); - int ret = 0; - u32 aread, i, msec = 10, retry = 0, retry_max = 20; - struct vmbus_channel *chn; - - netif_tx_disable(net); - - /* No need to close rndis filter if it is removed already */ - if (!nvdev) - goto out; - - ret = rndis_filter_close(nvdev); - if (ret != 0) { - netdev_err(net, "unable to close device (ret %d).\n", ret); - return ret; - } + unsigned int retry = 0; + int i; /* Ensure pending bytes in ring are read */ - while (true) { - aread = 0; + for (;;) { + u32 aread = 0; + for (i = 0; i < nvdev->num_chn; i++) { - chn = nvdev->chan_table[i].channel; + struct vmbus_channel *chn + = nvdev->chan_table[i].channel; + if (!chn) continue; + /* make sure receive not running now */ + napi_synchronize(&nvdev->chan_table[i].napi); + aread = hv_get_bytes_to_read(&chn->inbound); if (aread) break; @@ -181,22 +169,40 @@ static int netvsc_close(struct net_device *net) break; } - retry++; - if (retry > retry_max || aread == 0) - break; + if (aread == 0) + return 0; - msleep(msec); + if (++retry > RETRY_MAX) + return -ETIMEDOUT; - if (msec < 1000) - msec *= 2; + usleep_range(1000, 2000); } +} - if (aread) { - netdev_err(net, "Ring buffer not empty after closing rndis\n"); - ret = -ETIMEDOUT; +static int netvsc_close(struct net_device *net) +{ + struct net_device_context *net_device_ctx = netdev_priv(net); + struct net_device *vf_netdev + = rtnl_dereference(net_device_ctx->vf_netdev); + struct netvsc_device *nvdev = rtnl_dereference(net_device_ctx->nvdev); + int ret; + + netif_tx_disable(net); + + /* No need to close rndis filter if it is removed already */ + if (!nvdev) + return 0; + + ret = rndis_filter_close(nvdev); + if (ret != 0) { + netdev_err(net, "unable to close device (ret %d).\n", ret); + return ret; } -out: + ret = netvsc_wait_until_empty(nvdev); + if (ret) + netdev_err(net, "Ring buffer not empty after closing rndis\n"); + if (vf_netdev) dev_close(vf_netdev); @@ -845,16 +851,76 @@ static void netvsc_get_channels(struct net_device *net, } } +static int netvsc_detach(struct net_device *ndev, + struct netvsc_device *nvdev) +{ + struct net_device_context *ndev_ctx = netdev_priv(ndev); + struct hv_device *hdev = ndev_ctx->device_ctx; + int ret; + + /* Don't try continuing to try and setup sub channels */ + if (cancel_work_sync(&nvdev->subchan_work)) + nvdev->num_chn = 1; + + /* If device was up (receiving) then shutdown */ + if (netif_running(ndev)) { + netif_tx_disable(ndev); + + ret = rndis_filter_close(nvdev); + if (ret) { + netdev_err(ndev, + "unable to close device (ret %d).\n", ret); + return ret; + } + + ret = netvsc_wait_until_empty(nvdev); + if (ret) { + netdev_err(ndev, + "Ring buffer not empty after closing rndis\n"); + return ret; + } + } + + rndis_filter_device_remove(hdev, nvdev); + return 0; +} + +static int netvsc_attach(struct net_device *ndev, + struct netvsc_device_info *dev_info) +{ + struct net_device_context *ndev_ctx = netdev_priv(ndev); + struct hv_device *hdev = ndev_ctx->device_ctx; + struct netvsc_device *nvdev; + struct rndis_device *rdev; + int ret; + + nvdev = rndis_filter_device_add(hdev, dev_info); + if (IS_ERR(nvdev)) + return PTR_ERR(nvdev); + + netif_carrier_off(ndev); + + if (netif_running(ndev)) { + ret = rndis_filter_open(nvdev); + if (ret) + return ret; + + rdev = nvdev->extension; + if (!rdev->link_state) + netif_carrier_on(ndev); + } + + return 0; +} + static int netvsc_set_channels(struct net_device *net, struct ethtool_channels *channels) { struct net_device_context *net_device_ctx = netdev_priv(net); - struct hv_device *dev = net_device_ctx->device_ctx; struct netvsc_device *nvdev = rtnl_dereference(net_device_ctx->nvdev); unsigned int orig, count = channels->combined_count; struct netvsc_device_info device_info; - bool was_opened; - int ret = 0; + int ret; /* We do not support separate count for rx, tx, or other */ if (count == 0 || @@ -871,9 +937,6 @@ static int netvsc_set_channels(struct net_device *net, return -EINVAL; orig = nvdev->num_chn; - was_opened = rndis_filter_opened(nvdev); - if (was_opened) - rndis_filter_close(nvdev); memset(&device_info, 0, sizeof(device_info)); device_info.num_chn = count; @@ -882,28 +945,17 @@ static int netvsc_set_channels(struct net_device *net, device_info.recv_sections = nvdev->recv_section_cnt; device_info.recv_section_size = nvdev->recv_section_size; - rndis_filter_device_remove(dev, nvdev); + ret = netvsc_detach(net, nvdev); + if (ret) + return ret; - nvdev = rndis_filter_device_add(dev, &device_info); - if (IS_ERR(nvdev)) { - ret = PTR_ERR(nvdev); + ret = netvsc_attach(net, &device_info); + if (ret) { device_info.num_chn = orig; - nvdev = rndis_filter_device_add(dev, &device_info); - - if (IS_ERR(nvdev)) { - netdev_err(net, "restoring channel setting failed: %ld\n", - PTR_ERR(nvdev)); - return ret; - } + if (netvsc_attach(net, &device_info)) + netdev_err(net, "restoring channel setting failed\n"); } - if (was_opened) - rndis_filter_open(nvdev); - - /* We may have missed link change notifications */ - net_device_ctx->last_reconfig = 0; - schedule_delayed_work(&net_device_ctx->dwork, 0); - return ret; } @@ -969,10 +1021,8 @@ static int netvsc_change_mtu(struct net_device *ndev, int mtu) struct net_device_context *ndevctx = netdev_priv(ndev); struct net_device *vf_netdev = rtnl_dereference(ndevctx->vf_netdev); struct netvsc_device *nvdev = rtnl_dereference(ndevctx->nvdev); - struct hv_device *hdev = ndevctx->device_ctx; int orig_mtu = ndev->mtu; struct netvsc_device_info device_info; - bool was_opened; int ret = 0; if (!nvdev || nvdev->destroy) @@ -985,11 +1035,6 @@ static int netvsc_change_mtu(struct net_device *ndev, int mtu) return ret; } - netif_device_detach(ndev); - was_opened = rndis_filter_opened(nvdev); - if (was_opened) - rndis_filter_close(nvdev); - memset(&device_info, 0, sizeof(device_info)); device_info.num_chn = nvdev->num_chn; device_info.send_sections = nvdev->send_section_cnt; @@ -997,35 +1042,27 @@ static int netvsc_change_mtu(struct net_device *ndev, int mtu) device_info.recv_sections = nvdev->recv_section_cnt; device_info.recv_section_size = nvdev->recv_section_size; - rndis_filter_device_remove(hdev, nvdev); + ret = netvsc_detach(ndev, nvdev); + if (ret) + goto rollback_vf; ndev->mtu = mtu; - nvdev = rndis_filter_device_add(hdev, &device_info); - if (IS_ERR(nvdev)) { - ret = PTR_ERR(nvdev); - - /* Attempt rollback to original MTU */ - ndev->mtu = orig_mtu; - nvdev = rndis_filter_device_add(hdev, &device_info); - - if (vf_netdev) - dev_set_mtu(vf_netdev, orig_mtu); - - if (IS_ERR(nvdev)) { - netdev_err(ndev, "restoring mtu failed: %ld\n", - PTR_ERR(nvdev)); - return ret; - } - } + ret = netvsc_attach(ndev, &device_info); + if (ret) + goto rollback; - if (was_opened) - rndis_filter_open(nvdev); + return 0; - netif_device_attach(ndev); +rollback: + /* Attempt rollback to original MTU */ + ndev->mtu = orig_mtu; - /* We may have missed link change notifications */ - schedule_delayed_work(&ndevctx->dwork, 0); + if (netvsc_attach(ndev, &device_info)) + netdev_err(ndev, "restoring mtu failed\n"); +rollback_vf: + if (vf_netdev) + dev_set_mtu(vf_netdev, orig_mtu); return ret; } @@ -1531,11 +1568,9 @@ static int netvsc_set_ringparam(struct net_device *ndev, { struct net_device_context *ndevctx = netdev_priv(ndev); struct netvsc_device *nvdev = rtnl_dereference(ndevctx->nvdev); - struct hv_device *hdev = ndevctx->device_ctx; struct netvsc_device_info device_info; struct ethtool_ringparam orig; u32 new_tx, new_rx; - bool was_opened; int ret = 0; if (!nvdev || nvdev->destroy) @@ -1560,34 +1595,18 @@ static int netvsc_set_ringparam(struct net_device *ndev, device_info.recv_sections = new_rx; device_info.recv_section_size = nvdev->recv_section_size; - netif_device_detach(ndev); - was_opened = rndis_filter_opened(nvdev); - if (was_opened) - rndis_filter_close(nvdev); - - rndis_filter_device_remove(hdev, nvdev); - - nvdev = rndis_filter_device_add(hdev, &device_info); - if (IS_ERR(nvdev)) { - ret = PTR_ERR(nvdev); + ret = netvsc_detach(ndev, nvdev); + if (ret) + return ret; + ret = netvsc_attach(ndev, &device_info); + if (ret) { device_info.send_sections = orig.tx_pending; device_info.recv_sections = orig.rx_pending; - nvdev = rndis_filter_device_add(hdev, &device_info); - if (IS_ERR(nvdev)) { - netdev_err(ndev, "restoring ringparam failed: %ld\n", - PTR_ERR(nvdev)); - return ret; - } - } - - if (was_opened) - rndis_filter_open(nvdev); - netif_device_attach(ndev); - /* We may have missed link change notifications */ - ndevctx->last_reconfig = 0; - schedule_delayed_work(&ndevctx->dwork, 0); + if (netvsc_attach(ndev, &device_info)) + netdev_err(ndev, "restoring ringparam failed"); + } return ret; } @@ -2072,8 +2091,8 @@ static int netvsc_probe(struct hv_device *dev, static int netvsc_remove(struct hv_device *dev) { struct net_device_context *ndev_ctx; - struct net_device *vf_netdev; - struct net_device *net; + struct net_device *vf_netdev, *net; + struct netvsc_device *nvdev; net = hv_get_drvdata(dev); if (net == NULL) { @@ -2083,8 +2102,6 @@ static int netvsc_remove(struct hv_device *dev) ndev_ctx = netdev_priv(net); - netif_device_detach(net); - cancel_delayed_work_sync(&ndev_ctx->dwork); /* @@ -2092,14 +2109,19 @@ static int netvsc_remove(struct hv_device *dev) * removed. Also blocks mtu and channel changes. */ rtnl_lock(); + vf_netdev = rtnl_dereference(ndev_ctx->vf_netdev); if (vf_netdev) netvsc_unregister_vf(vf_netdev); + nvdev = rtnl_dereference(ndev_ctx->nvdev); + if (nvdev) { + cancel_work_sync(&nvdev->subchan_work); + rndis_filter_device_remove(dev, nvdev); + } + unregister_netdevice(net); - rndis_filter_device_remove(dev, - rtnl_dereference(ndev_ctx->nvdev)); rtnl_unlock(); hv_set_drvdata(dev, NULL); diff --git a/drivers/net/hyperv/rndis_filter.c b/drivers/net/hyperv/rndis_filter.c index a9746f9fbf4b..3ccb0c6263a8 100644 --- a/drivers/net/hyperv/rndis_filter.c +++ b/drivers/net/hyperv/rndis_filter.c @@ -1122,6 +1122,7 @@ void rndis_set_subchannel(struct work_struct *w) for (i = 0; i < VRSS_SEND_TAB_SIZE; i++) ndev_ctx->tx_table[i] = i % nvdev->num_chn; + netif_tx_wake_all_queues(ndev); rtnl_unlock(); return; @@ -1132,6 +1133,7 @@ void rndis_set_subchannel(struct work_struct *w) nvdev->max_chn = 1; nvdev->num_chn = 1; + netif_wake_queue(ndev); unlock: rtnl_unlock(); } @@ -1326,6 +1328,8 @@ struct netvsc_device *rndis_filter_device_add(struct hv_device *dev, if (net_device->num_chn > 1) schedule_work(&net_device->subchan_work); + else + netif_wake_queue(net); out: /* if unavailable, just proceed with one queue */ @@ -1346,9 +1350,6 @@ void rndis_filter_device_remove(struct hv_device *dev, { struct rndis_device *rndis_dev = net_dev->extension; - /* Don't try and setup sub channels if about to halt */ - cancel_work_sync(&net_dev->subchan_work); - /* Halt and release the rndis device */ rndis_filter_halt_device(net_dev, rndis_dev); @@ -1372,10 +1373,3 @@ int rndis_filter_close(struct netvsc_device *nvdev) return rndis_filter_close_device(nvdev->extension); } - -bool rndis_filter_opened(const struct netvsc_device *nvdev) -{ - const struct rndis_device *dev = nvdev->extension; - - return dev->state == RNDIS_DEV_DATAINITIALIZED; -} -- 2.16.1 ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH] hv_netvsc: Make sure out channel is fully opened on send 2018-03-13 19:06 ` Mohammed Gamal @ 2018-03-14 8:27 ` Dan Carpenter -1 siblings, 0 replies; 18+ messages in thread From: Dan Carpenter @ 2018-03-14 8:27 UTC (permalink / raw) To: Mohammed Gamal Cc: otubo, sthemmin, netdev, linux-kernel, devel, vkuznets, davem On Tue, Mar 13, 2018 at 08:06:50PM +0100, Mohammed Gamal wrote: > @@ -791,6 +791,7 @@ static inline int netvsc_send_pkt( > VMBUS_DATA_PACKET_FLAG_COMPLETION_REQUESTED); > } > > + ring_avail = hv_ringbuf_avail_percent(&out_channel->outbound); > if (ret == 0) { > atomic_inc_return(&nvchan->queue_sends); > Could you move the assignment inside the "ret == 0" path closer to where it's used? regards, dan carpenter _______________________________________________ devel mailing list devel@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] hv_netvsc: Make sure out channel is fully opened on send @ 2018-03-14 8:27 ` Dan Carpenter 0 siblings, 0 replies; 18+ messages in thread From: Dan Carpenter @ 2018-03-14 8:27 UTC (permalink / raw) To: Mohammed Gamal Cc: otubo, sthemmin, netdev, linux-kernel, devel, vkuznets, davem On Tue, Mar 13, 2018 at 08:06:50PM +0100, Mohammed Gamal wrote: > @@ -791,6 +791,7 @@ static inline int netvsc_send_pkt( > VMBUS_DATA_PACKET_FLAG_COMPLETION_REQUESTED); > } > > + ring_avail = hv_ringbuf_avail_percent(&out_channel->outbound); > if (ret == 0) { > atomic_inc_return(&nvchan->queue_sends); > Could you move the assignment inside the "ret == 0" path closer to where it's used? regards, dan carpenter ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] hv_netvsc: Make sure out channel is fully opened on send 2018-03-13 19:06 ` Mohammed Gamal @ 2018-03-16 14:16 ` David Miller -1 siblings, 0 replies; 18+ messages in thread From: David Miller @ 2018-03-16 14:16 UTC (permalink / raw) To: mgamal; +Cc: otubo, sthemmin, netdev, linux-kernel, devel, vkuznets From: Mohammed Gamal <mgamal@redhat.com> Date: Tue, 13 Mar 2018 20:06:50 +0100 > 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> Based upon the discusion on this patch, it looks like this will be fixed in some other way. _______________________________________________ devel mailing list devel@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] hv_netvsc: Make sure out channel is fully opened on send @ 2018-03-16 14:16 ` David Miller 0 siblings, 0 replies; 18+ messages in thread From: David Miller @ 2018-03-16 14:16 UTC (permalink / raw) To: mgamal; +Cc: netdev, sthemmin, devel, vkuznets, otubo, linux-kernel From: Mohammed Gamal <mgamal@redhat.com> Date: Tue, 13 Mar 2018 20:06:50 +0100 > 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> Based upon the discusion on this patch, it looks like this will be fixed in some other way. ^ permalink raw reply [flat|nested] 18+ messages in thread
end of thread, other threads:[~2018-09-27 10:31 UTC | newest] Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-09-26 16:34 [PATCH] hv_netvsc: Make sure out channel is fully opened on send Mohammed Gamal 2018-09-26 17:13 ` Haiyang Zhang 2018-09-27 8:57 ` Mohammed Gamal 2018-09-27 8:57 ` Mohammed Gamal 2018-09-27 10:23 ` Stephen Hemminger 2018-09-27 10:31 ` Mohammed Gamal -- strict thread matches above, loose matches on Subject: below -- 2018-03-13 19:06 Mohammed Gamal 2018-03-13 19:06 ` Mohammed Gamal 2018-03-13 19:35 ` Stephen Hemminger 2018-03-13 19:35 ` Stephen Hemminger 2018-03-14 9:22 ` Mohammed Gamal 2018-03-15 16:24 ` Mohammed Gamal 2018-03-15 17:40 ` Stephen Hemminger 2018-03-15 17:40 ` Stephen Hemminger 2018-03-14 8:27 ` Dan Carpenter 2018-03-14 8:27 ` Dan Carpenter 2018-03-16 14:16 ` David Miller 2018-03-16 14:16 ` David Miller
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.