From: Haiyang Zhang <[email protected]>
After queue stopped, the wakeup mechanism may wake it up again
when ring buffer usage is lower than a threshold. This may cause
send path panic on NULL pointer when we stopped all tx queues in
netvsc_detach and start removing the netvsc device.
This patch fix it by adding a tx_disable flag to prevent unwanted
queue wakeup.
Fixes: 7b2ee50c0cd5 ("hv_netvsc: common detach logic")
Reported-by: Mohammed Gamal <[email protected]>
Signed-off-by: Haiyang Zhang <[email protected]>
---
drivers/net/hyperv/hyperv_net.h | 1 +
drivers/net/hyperv/netvsc.c | 6 ++++--
drivers/net/hyperv/netvsc_drv.c | 32 ++++++++++++++++++++++++++------
3 files changed, 31 insertions(+), 8 deletions(-)
diff --git a/drivers/net/hyperv/hyperv_net.h b/drivers/net/hyperv/hyperv_net.h
index e859ae2..49f41b6 100644
--- a/drivers/net/hyperv/hyperv_net.h
+++ b/drivers/net/hyperv/hyperv_net.h
@@ -987,6 +987,7 @@ struct netvsc_device {
wait_queue_head_t wait_drain;
bool destroy;
+ bool tx_disable; /* if true, do not wake up queue again */
/* Receive buffer allocated by us but manages by NetVSP */
void *recv_buf;
diff --git a/drivers/net/hyperv/netvsc.c b/drivers/net/hyperv/netvsc.c
index 813d195..e0dce37 100644
--- a/drivers/net/hyperv/netvsc.c
+++ b/drivers/net/hyperv/netvsc.c
@@ -110,6 +110,7 @@ static struct netvsc_device *alloc_net_device(void)
init_waitqueue_head(&net_device->wait_drain);
net_device->destroy = false;
+ net_device->tx_disable = false;
net_device->max_pkt = RNDIS_MAX_PKT_DEFAULT;
net_device->pkt_align = RNDIS_PKT_ALIGN_DEFAULT;
@@ -719,7 +720,7 @@ static void netvsc_send_tx_complete(struct net_device *ndev,
} else {
struct netdev_queue *txq = netdev_get_tx_queue(ndev, q_idx);
- if (netif_tx_queue_stopped(txq) &&
+ if (netif_tx_queue_stopped(txq) && !net_device->tx_disable &&
(hv_get_avail_to_write_percent(&channel->outbound) >
RING_AVAIL_PERCENT_HIWATER || queue_sends < 1)) {
netif_tx_wake_queue(txq);
@@ -874,7 +875,8 @@ static inline int netvsc_send_pkt(
} else if (ret == -EAGAIN) {
netif_tx_stop_queue(txq);
ndev_ctx->eth_stats.stop_queue++;
- if (atomic_read(&nvchan->queue_sends) < 1) {
+ if (atomic_read(&nvchan->queue_sends) < 1 &&
+ !net_device->tx_disable) {
netif_tx_wake_queue(txq);
ndev_ctx->eth_stats.wake_queue++;
ret = -ENOSPC;
diff --git a/drivers/net/hyperv/netvsc_drv.c b/drivers/net/hyperv/netvsc_drv.c
index 1a08679..0824155 100644
--- a/drivers/net/hyperv/netvsc_drv.c
+++ b/drivers/net/hyperv/netvsc_drv.c
@@ -109,6 +109,15 @@ static void netvsc_set_rx_mode(struct net_device *net)
rcu_read_unlock();
}
+static inline void netvsc_tx_enable(struct netvsc_device *nvscdev,
+ struct net_device *ndev)
+{
+ nvscdev->tx_disable = false;
+ mb(); /* ensure queue wake up mechanism is on */
+
+ netif_tx_wake_all_queues(ndev);
+}
+
static int netvsc_open(struct net_device *net)
{
struct net_device_context *ndev_ctx = netdev_priv(net);
@@ -129,7 +138,7 @@ static int netvsc_open(struct net_device *net)
rdev = nvdev->extension;
if (!rdev->link_state) {
netif_carrier_on(net);
- netif_tx_wake_all_queues(net);
+ netvsc_tx_enable(nvdev, net);
}
if (vf_netdev) {
@@ -184,6 +193,17 @@ static int netvsc_wait_until_empty(struct netvsc_device *nvdev)
}
}
+static inline void netvsc_tx_disable(struct netvsc_device *nvscdev,
+ struct net_device *ndev)
+{
+ if (nvscdev) {
+ nvscdev->tx_disable = true;
+ mb(); /* ensure txq will not wake up after stop */
+ }
+
+ netif_tx_disable(ndev);
+}
+
static int netvsc_close(struct net_device *net)
{
struct net_device_context *net_device_ctx = netdev_priv(net);
@@ -192,7 +212,7 @@ static int netvsc_close(struct net_device *net)
struct netvsc_device *nvdev = rtnl_dereference(net_device_ctx->nvdev);
int ret;
- netif_tx_disable(net);
+ netvsc_tx_disable(nvdev, net);
/* No need to close rndis filter if it is removed already */
if (!nvdev)
@@ -918,7 +938,7 @@ static int netvsc_detach(struct net_device *ndev,
/* If device was up (receiving) then shutdown */
if (netif_running(ndev)) {
- netif_tx_disable(ndev);
+ netvsc_tx_disable(nvdev, ndev);
ret = rndis_filter_close(nvdev);
if (ret) {
@@ -1906,7 +1926,7 @@ static void netvsc_link_change(struct work_struct *w)
if (rdev->link_state) {
rdev->link_state = false;
netif_carrier_on(net);
- netif_tx_wake_all_queues(net);
+ netvsc_tx_enable(net_device, net);
} else {
notify = true;
}
@@ -1916,7 +1936,7 @@ static void netvsc_link_change(struct work_struct *w)
if (!rdev->link_state) {
rdev->link_state = true;
netif_carrier_off(net);
- netif_tx_stop_all_queues(net);
+ netvsc_tx_disable(net_device, net);
}
kfree(event);
break;
@@ -1925,7 +1945,7 @@ static void netvsc_link_change(struct work_struct *w)
if (!rdev->link_state) {
rdev->link_state = true;
netif_carrier_off(net);
- netif_tx_stop_all_queues(net);
+ netvsc_tx_disable(net_device, net);
event->event = RNDIS_STATUS_MEDIA_CONNECT;
spin_lock_irqsave(&ndev_ctx->lock, flags);
list_add(&event->list, &ndev_ctx->reconfig_events);
--
1.8.3.1
On Thu, 28 Mar 2019 17:48:45 +0000
Haiyang Zhang <[email protected]> wrote:
> +static inline void netvsc_tx_enable(struct netvsc_device *nvscdev,
> + struct net_device *ndev)
> +{
> + nvscdev->tx_disable = false;
> + mb(); /* ensure queue wake up mechanism is on */
> +
> + netif_tx_wake_all_queues(ndev);
> +}
You don't need a full mb(). virt_wmb() should be sufficient.
Could I suggest an alternative approach.
You don't need to introduce a local tx_disable flag, the only place where a wakeup
could cause problems is after a send_completion was processed during detach state.
Instead, just avoid wakeup in that place.
--- a/drivers/net/hyperv/netvsc.c
+++ b/drivers/net/hyperv/netvsc.c
@@ -720,6 +720,7 @@ static void netvsc_send_tx_complete(struct net_device *ndev,
struct netdev_queue *txq = netdev_get_tx_queue(ndev, q_idx);
if (netif_tx_queue_stopped(txq) &&
+ netif_device_present(ndev) &&
(hv_get_avail_to_write_percent(&channel->outbound) >
RING_AVAIL_PERCENT_HIWATER || queue_sends < 1)) {
netif_tx_wake_queue(txq);
> -----Original Message-----
> From: Stephen Hemminger <[email protected]>
> Sent: Thursday, March 28, 2019 2:38 PM
> To: Haiyang Zhang <[email protected]>
> Cc: [email protected]; [email protected]; Haiyang Zhang
> <[email protected]>; KY Srinivasan <[email protected]>; Stephen
> Hemminger <[email protected]>; [email protected]; vkuznets
> <[email protected]>; [email protected]; [email protected];
> [email protected]
> Subject: Re: [PATCH hyperv-fixes] hv_netvsc: Fix unwanted wakeup after
> tx_disable
>
> On Thu, 28 Mar 2019 17:48:45 +0000
> Haiyang Zhang <[email protected]> wrote:
>
> > +static inline void netvsc_tx_enable(struct netvsc_device *nvscdev,
> > + struct net_device *ndev)
> > +{
> > + nvscdev->tx_disable = false;
> > + mb(); /* ensure queue wake up mechanism is on */
> > +
> > + netif_tx_wake_all_queues(ndev);
> > +}
>
> You don't need a full mb(). virt_wmb() should be sufficient.
I will make this change.
> Could I suggest an alternative approach.
> You don't need to introduce a local tx_disable flag, the only place where a
> wakeup could cause problems is after a send_completion was processed
> during detach state.
>
> Instead, just avoid wakeup in that place.
In netvsc_detach(), after netif_tx_disable(), we call netvsc_wait_until_empty(nvdev);
TX patch should not be waken up again while waiting for in/out ring to becomes empty.
In my tests before this patch, there are wakeup happens before netif_device_detach(),
so netif_device_present(ndev) is still true at that time.
In other places, like netvsc_close(), link_change(), we also don't want wakeup after tx_disable.
Thanks.
- Haiyang
>
> --- a/drivers/net/hyperv/netvsc.c
> +++ b/drivers/net/hyperv/netvsc.c
> @@ -720,6 +720,7 @@ static void netvsc_send_tx_complete(struct
> net_device *ndev,
> struct netdev_queue *txq = netdev_get_tx_queue(ndev, q_idx);
>
> if (netif_tx_queue_stopped(txq) &&
> + netif_device_present(ndev) &&
> (hv_get_avail_to_write_percent(&channel->outbound) >
> RING_AVAIL_PERCENT_HIWATER || queue_sends < 1)) {
> netif_tx_wake_queue(txq);
From: Haiyang Zhang <[email protected]>
Date: Thu, 28 Mar 2019 17:48:45 +0000
> diff --git a/drivers/net/hyperv/netvsc_drv.c b/drivers/net/hyperv/netvsc_drv.c
> index 1a08679..0824155 100644
> --- a/drivers/net/hyperv/netvsc_drv.c
> +++ b/drivers/net/hyperv/netvsc_drv.c
> @@ -109,6 +109,15 @@ static void netvsc_set_rx_mode(struct net_device *net)
> rcu_read_unlock();
> }
>
> +static inline void netvsc_tx_enable(struct netvsc_device *nvscdev,
> + struct net_device *ndev)
Do not use inline in foo.c files, let the compiler decide.
> @@ -184,6 +193,17 @@ static int netvsc_wait_until_empty(struct netvsc_device *nvdev)
> }
> }
>
> +static inline void netvsc_tx_disable(struct netvsc_device *nvscdev,
> + struct net_device *ndev)
> +{
Likewise.
On Thu, 28 Mar 2019 19:00:18 +0000
Haiyang Zhang <[email protected]> wrote:
> > -----Original Message-----
> > From: Stephen Hemminger <[email protected]>
> > Sent: Thursday, March 28, 2019 2:38 PM
> > To: Haiyang Zhang <[email protected]>
> > Cc: [email protected]; [email protected]; Haiyang Zhang
> > <[email protected]>; KY Srinivasan <[email protected]>; Stephen
> > Hemminger <[email protected]>; [email protected]; vkuznets
> > <[email protected]>; [email protected]; [email protected];
> > [email protected]
> > Subject: Re: [PATCH hyperv-fixes] hv_netvsc: Fix unwanted wakeup after
> > tx_disable
> >
> > On Thu, 28 Mar 2019 17:48:45 +0000
> > Haiyang Zhang <[email protected]> wrote:
> >
> > > +static inline void netvsc_tx_enable(struct netvsc_device *nvscdev,
> > > + struct net_device *ndev)
> > > +{
> > > + nvscdev->tx_disable = false;
> > > + mb(); /* ensure queue wake up mechanism is on */
> > > +
> > > + netif_tx_wake_all_queues(ndev);
> > > +}
> >
> > You don't need a full mb(). virt_wmb() should be sufficient.
>
> I will make this change.
>
> > Could I suggest an alternative approach.
> > You don't need to introduce a local tx_disable flag, the only place where a
> > wakeup could cause problems is after a send_completion was processed
> > during detach state.
> >
> > Instead, just avoid wakeup in that place.
>
> In netvsc_detach(), after netif_tx_disable(), we call netvsc_wait_until_empty(nvdev);
> TX patch should not be waken up again while waiting for in/out ring to becomes empty.
>
> In my tests before this patch, there are wakeup happens before netif_device_detach(),
> so netif_device_present(ndev) is still true at that time.
>
> In other places, like netvsc_close(), link_change(), we also don't want wakeup after tx_disable.
>
> Thanks.
> - Haiyang
>
> >
> > --- a/drivers/net/hyperv/netvsc.c
> > +++ b/drivers/net/hyperv/netvsc.c
> > @@ -720,6 +720,7 @@ static void netvsc_send_tx_complete(struct
> > net_device *ndev,
> > struct netdev_queue *txq = netdev_get_tx_queue(ndev, q_idx);
> >
> > if (netif_tx_queue_stopped(txq) &&
> > + netif_device_present(ndev) &&
> > (hv_get_avail_to_write_percent(&channel->outbound) >
> > RING_AVAIL_PERCENT_HIWATER || queue_sends < 1)) {
> > netif_tx_wake_queue(txq);
Then what about doing netif_detach earlier in netvsc_detach.
The state management is already (too) complex in netvsc and adding another
boolean flag just makes it harder to understand.
> -----Original Message-----
> From: Stephen Hemminger <[email protected]>
> Sent: Thursday, March 28, 2019 3:42 PM
> To: Haiyang Zhang <[email protected]>
> Cc: Haiyang Zhang <[email protected]>; [email protected];
> [email protected]; KY Srinivasan <[email protected]>; Stephen
> Hemminger <[email protected]>; [email protected]; vkuznets
> <[email protected]>; [email protected]; [email protected];
> [email protected]
> Subject: Re: [PATCH hyperv-fixes] hv_netvsc: Fix unwanted wakeup after
> tx_disable
>
> On Thu, 28 Mar 2019 19:00:18 +0000
> Haiyang Zhang <[email protected]> wrote:
>
> > > -----Original Message-----
> > > From: Stephen Hemminger <[email protected]>
> > > Sent: Thursday, March 28, 2019 2:38 PM
> > > To: Haiyang Zhang <[email protected]>
> > > Cc: [email protected]; [email protected]; Haiyang Zhang
> > > <[email protected]>; KY Srinivasan <[email protected]>;
> Stephen
> > > Hemminger <[email protected]>; [email protected]; vkuznets
> > > <[email protected]>; [email protected];
> [email protected];
> > > [email protected]
> > > Subject: Re: [PATCH hyperv-fixes] hv_netvsc: Fix unwanted wakeup
> > > after tx_disable
> > >
> > > On Thu, 28 Mar 2019 17:48:45 +0000
> > > Haiyang Zhang <[email protected]> wrote:
> > >
> > > > +static inline void netvsc_tx_enable(struct netvsc_device *nvscdev,
> > > > + struct net_device *ndev)
> > > > +{
> > > > + nvscdev->tx_disable = false;
> > > > + mb(); /* ensure queue wake up mechanism is on */
> > > > +
> > > > + netif_tx_wake_all_queues(ndev);
> > > > +}
> > >
> > > You don't need a full mb(). virt_wmb() should be sufficient.
> >
> > I will make this change.
> >
> > > Could I suggest an alternative approach.
> > > You don't need to introduce a local tx_disable flag, the only place
> > > where a wakeup could cause problems is after a send_completion was
> > > processed during detach state.
> > >
> > > Instead, just avoid wakeup in that place.
> >
> > In netvsc_detach(), after netif_tx_disable(), we call
> > netvsc_wait_until_empty(nvdev); TX patch should not be waken up again
> while waiting for in/out ring to becomes empty.
> >
> > In my tests before this patch, there are wakeup happens before
> > netif_device_detach(), so netif_device_present(ndev) is still true at that
> time.
> >
> > In other places, like netvsc_close(), link_change(), we also don't want
> wakeup after tx_disable.
> >
> > Thanks.
> > - Haiyang
> >
> > >
> > > --- a/drivers/net/hyperv/netvsc.c
> > > +++ b/drivers/net/hyperv/netvsc.c
> > > @@ -720,6 +720,7 @@ static void netvsc_send_tx_complete(struct
> > > net_device *ndev,
> > > struct netdev_queue *txq = netdev_get_tx_queue(ndev,
> > > q_idx);
> > >
> > > if (netif_tx_queue_stopped(txq) &&
> > > + netif_device_present(ndev) &&
> > > (hv_get_avail_to_write_percent(&channel->outbound) >
> > > RING_AVAIL_PERCENT_HIWATER || queue_sends < 1)) {
> > > netif_tx_wake_queue(txq);
>
>
> Then what about doing netif_detach earlier in netvsc_detach.
>
> The state management is already (too) complex in netvsc and adding another
> boolean flag just makes it harder to understand.
If we move netif_device_detach() before the netvsc_wait_until_empty(), the remaining
Packets in the receive buffer will be passed to a detached device.
Also, in case of netvsc_close() and link_change(), we don't call netif_device_detach().
Thanks,
- Haiyang