[PATCH net-next 0/2] xen-netback: windows frontend compatibility fixes

[PATCH net-next 0/2] xen-netback: windows frontend compatibility fixes

[Paul Durrant]

The following patches fix a couple more issues found when testing with
Windows frontends.

v2:
- Add comment in 2/2 to note that state transitions from Connected to Closed
  are incorrect.

[PATCH net-next v2 1/2] xen-netback: add a vif-is-connected flag

[Paul Durrant]

Having applied my patch to separate vif disconnect and free, I ran into a
BUG when testing resume from S3 with a Windows frontend because the vif task
pointer was not cleared by xenvif_disconnect() and so a double call to this
function tries to stop the thread twice.
Rather than applying a point fix for that issue it seems better to introduce
a boolean to indicate whether the vif is connected or not such that repeated
calls to either xenvif_connect() or xenvif_disconnect() behave appropriately.

Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
Cc: David Vrabel <david.vrabel@citrix.com>
Cc: Wei Liu <wei.liu2@citrix.com>
Cc: Ian Campbell <ian.campbell@citrix.com>
---
 drivers/net/xen-netback/common.h    |    1 +
 drivers/net/xen-netback/interface.c |   24 +++++++++++++-----------
 2 files changed, 14 insertions(+), 11 deletions(-)

diff --git a/drivers/net/xen-netback/common.h b/drivers/net/xen-netback/common.h
index 5715318..860d92c 100644
--- a/drivers/net/xen-netback/common.h
+++ b/drivers/net/xen-netback/common.h
@@ -169,6 +169,7 @@ struct xenvif {
 
 	/* Miscellaneous private stuff. */
 	struct net_device *dev;
+	bool connected;
 };
 
 static inline struct xenbus_device *xenvif_to_xenbus_device(struct xenvif *vif)
diff --git a/drivers/net/xen-netback/interface.c b/drivers/net/xen-netback/interface.c
index 01bb854..94b47f5 100644
--- a/drivers/net/xen-netback/interface.c
+++ b/drivers/net/xen-netback/interface.c
@@ -366,7 +366,7 @@ int xenvif_connect(struct xenvif *vif, unsigned long tx_ring_ref,
 	int err = -ENOMEM;
 
 	/* Already connected through? */
-	if (vif->tx_irq)
+	if (vif->connected)
 		return 0;
 
 	err = xenvif_map_frontend_rings(vif, tx_ring_ref, rx_ring_ref);
@@ -425,6 +425,7 @@ int xenvif_connect(struct xenvif *vif, unsigned long tx_ring_ref,
 
 	wake_up_process(vif->task);
 
+	vif->connected = 1;
 	return 0;
 
 err_rx_unbind:
@@ -453,23 +454,24 @@ void xenvif_carrier_off(struct xenvif *vif)
 
 void xenvif_disconnect(struct xenvif *vif)
 {
+	if (!vif->connected)
+		return;
+
 	if (netif_carrier_ok(vif->dev))
 		xenvif_carrier_off(vif);
 
-	if (vif->tx_irq) {
-		if (vif->tx_irq == vif->rx_irq)
-			unbind_from_irqhandler(vif->tx_irq, vif);
-		else {
-			unbind_from_irqhandler(vif->tx_irq, vif);
-			unbind_from_irqhandler(vif->rx_irq, vif);
-		}
-		vif->tx_irq = 0;
+	if (vif->tx_irq == vif->rx_irq)
+		unbind_from_irqhandler(vif->tx_irq, vif);
+	else {
+		unbind_from_irqhandler(vif->tx_irq, vif);
+		unbind_from_irqhandler(vif->rx_irq, vif);
 	}
 
-	if (vif->task)
-		kthread_stop(vif->task);
+	kthread_stop(vif->task);
 
 	xenvif_unmap_frontend_rings(vif);
+
+	vif->connected = 0;
 }
 
 void xenvif_free(struct xenvif *vif)
-- 
1.7.10.4

Re: [PATCH net-next v2 1/2] xen-netback: add a vif-is-connected flag

[David Miller]

From: Paul Durrant <paul.durrant@citrix.com>
Date: Fri, 20 Sep 2013 14:57:39 +0100

> @@ -169,6 +169,7 @@ struct xenvif {
>  
>  	/* Miscellaneous private stuff. */
>  	struct net_device *dev;
> +	bool connected;
>  };
>  
>  static inline struct xenbus_device *xenvif_to_xenbus_device(struct xenvif *vif)
...
> +	vif->connected = 1;
 ...
> +	vif->connected = 0;

Please use 'true' and 'false' when assigning values to something
of "bool" type.

Also these look like bug fixes, and are thus more appropriately
targetted at 'net' and even potentially -stable as well.

Re: [Xen-devel] [PATCH net-next v2 1/2] xen-netback: add a vif-is-connected flag

[annie li]

On 2013-9-20 21:57, Paul Durrant wrote:
> Having applied my patch to separate vif disconnect and free, I ran into a
> BUG when testing resume from S3 with a Windows frontend because the vif task
> pointer was not cleared by xenvif_disconnect() and so a double call to this
> function tries to stop the thread twice.
Or it is better to do more implements in windows netfront? For example, 
when the windows vm hibernates, disconnect the vif as required by 
netback: connect-> closing-> closed.

Thanks
Annie

> Rather than applying a point fix for that issue it seems better to introduce
> a boolean to indicate whether the vif is connected or not such that repeated
> calls to either xenvif_connect() or xenvif_disconnect() behave appropriately.
>
> Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
> Cc: David Vrabel <david.vrabel@citrix.com>
> Cc: Wei Liu <wei.liu2@citrix.com>
> Cc: Ian Campbell <ian.campbell@citrix.com>
> ---
>   drivers/net/xen-netback/common.h    |    1 +
>   drivers/net/xen-netback/interface.c |   24 +++++++++++++-----------
>   2 files changed, 14 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/net/xen-netback/common.h b/drivers/net/xen-netback/common.h
> index 5715318..860d92c 100644
> --- a/drivers/net/xen-netback/common.h
> +++ b/drivers/net/xen-netback/common.h
> @@ -169,6 +169,7 @@ struct xenvif {
>   
>   	/* Miscellaneous private stuff. */
>   	struct net_device *dev;
> +	bool connected;
>   };
>   
>   static inline struct xenbus_device *xenvif_to_xenbus_device(struct xenvif *vif)
> diff --git a/drivers/net/xen-netback/interface.c b/drivers/net/xen-netback/interface.c
> index 01bb854..94b47f5 100644
> --- a/drivers/net/xen-netback/interface.c
> +++ b/drivers/net/xen-netback/interface.c
> @@ -366,7 +366,7 @@ int xenvif_connect(struct xenvif *vif, unsigned long tx_ring_ref,
>   	int err = -ENOMEM;
>   
>   	/* Already connected through? */
> -	if (vif->tx_irq)
> +	if (vif->connected)
>   		return 0;
>   
>   	err = xenvif_map_frontend_rings(vif, tx_ring_ref, rx_ring_ref);
> @@ -425,6 +425,7 @@ int xenvif_connect(struct xenvif *vif, unsigned long tx_ring_ref,
>   
>   	wake_up_process(vif->task);
>   
> +	vif->connected = 1;
>   	return 0;
>   
>   err_rx_unbind:
> @@ -453,23 +454,24 @@ void xenvif_carrier_off(struct xenvif *vif)
>   
>   void xenvif_disconnect(struct xenvif *vif)
>   {
> +	if (!vif->connected)
> +		return;
> +
>   	if (netif_carrier_ok(vif->dev))
>   		xenvif_carrier_off(vif);
>   
> -	if (vif->tx_irq) {
> -		if (vif->tx_irq == vif->rx_irq)
> -			unbind_from_irqhandler(vif->tx_irq, vif);
> -		else {
> -			unbind_from_irqhandler(vif->tx_irq, vif);
> -			unbind_from_irqhandler(vif->rx_irq, vif);
> -		}
> -		vif->tx_irq = 0;
> +	if (vif->tx_irq == vif->rx_irq)
> +		unbind_from_irqhandler(vif->tx_irq, vif);
> +	else {
> +		unbind_from_irqhandler(vif->tx_irq, vif);
> +		unbind_from_irqhandler(vif->rx_irq, vif);
>   	}
>   
> -	if (vif->task)
> -		kthread_stop(vif->task);
> +	kthread_stop(vif->task);
>   
>   	xenvif_unmap_frontend_rings(vif);
> +
> +	vif->connected = 0;
>   }
>   
>   void xenvif_free(struct xenvif *vif)

RE: [Xen-devel] [PATCH net-next v2 1/2] xen-netback: add a vif-is-connected flag

[Paul Durrant]

> -----Original Message-----
> From: annie li [mailto:annie.li@oracle.com]
> Sent: 22 September 2013 03:57
> To: Paul Durrant
> Cc: netdev@vger.kernel.org; xen-devel@lists.xen.org; Wei Liu; David Vrabel;
> Ian Campbell
> Subject: Re: [Xen-devel] [PATCH net-next v2 1/2] xen-netback: add a vif-is-
> connected flag
> 
> 
> On 2013-9-20 21:57, Paul Durrant wrote:
> > Having applied my patch to separate vif disconnect and free, I ran into a
> > BUG when testing resume from S3 with a Windows frontend because the
> vif task
> > pointer was not cleared by xenvif_disconnect() and so a double call to this
> > function tries to stop the thread twice.
> Or it is better to do more implements in windows netfront? For example,
> when the windows vm hibernates, disconnect the vif as required by
> netback: connect-> closing-> closed.
> 

S3 != hibernation; that is S4. The backend does not go away when the VM goes into S3 as the domain remains intact. We do go through the correct closing->closed transition on the way down but, because of the way the D3->D0 code in the frontend needs to be generalized, we attempt a second closing->closed transition on the way back up. In the S4 case this ok because we have a fresh backend, but in the S3 case we don't and therefore hit the double-disconnect issue. The fact the backend BUGs in this case clearly shows a vulnerability in the backend and thus that is where the fix needs to be made; the frontend is doing nothing wrong.

  Paul

[PATCH net-next v2 2/2] xen-netback: handle frontends that fail to transition through Closing

[Paul Durrant]

Some old Windows frontends fail to transition through the xenbus Closing
state and move directly from Connected to Closed. Handle this case properly.

Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
Cc: David Vrabel <david.vrabel@citrix.com>
Cc: Wei Liu <wei.liu2@citrix.com>
Cc: Ian Campbell <ian.campbell@citrix.com>
---
 drivers/net/xen-netback/xenbus.c |    7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/net/xen-netback/xenbus.c b/drivers/net/xen-netback/xenbus.c
index a53782e..684b0f6 100644
--- a/drivers/net/xen-netback/xenbus.c
+++ b/drivers/net/xen-netback/xenbus.c
@@ -265,6 +265,13 @@ static void frontend_changed(struct xenbus_device *dev,
 		break;
 
 	case XenbusStateClosed:
+		/*
+		 * Handle frontends which erroneously transition
+		 * directly from Connected to Closed.
+		 */
+		if (dev->state == XenbusStateConnected)
+			disconnect_backend(dev);
+
 		xenbus_switch_state(dev, XenbusStateClosed);
 		if (xenbus_dev_is_online(dev))
 			break;
-- 
1.7.10.4

Re: [PATCH net-next v2 2/2] xen-netback: handle frontends that fail to transition through Closing

[Bastian Blank]

On Fri, Sep 20, 2013 at 02:57:40PM +0100, Paul Durrant wrote:
> Some old Windows frontends fail to transition through the xenbus Closing
> state and move directly from Connected to Closed. Handle this case properly.

What happens in this case? Are there other state changes that will do
unwanted things?

Bastian

-- 
If there are self-made purgatories, then we all have to live in them.
		-- Spock, "This Side of Paradise", stardate 3417.7

RE: [Xen-devel] [PATCH net-next v2 2/2] xen-netback: handle frontends that fail to transition through Closing

[Paul Durrant]

> -----Original Message-----
> From: Bastian Blank [mailto:bastian@waldi.eu.org]
> Sent: 20 September 2013 17:15
> To: Paul Durrant
> Cc: netdev@vger.kernel.org; xen-devel@lists.xen.org; Wei Liu; David Vrabel;
> Ian Campbell
> Subject: Re: [Xen-devel] [PATCH net-next v2 2/2] xen-netback: handle
> frontends that fail to transition through Closing
> 
> On Fri, Sep 20, 2013 at 02:57:40PM +0100, Paul Durrant wrote:
> > Some old Windows frontends fail to transition through the xenbus Closing
> > state and move directly from Connected to Closed. Handle this case
> properly.
> 
> What happens in this case? Are there other state changes that will do
> unwanted things?
> 

Hmm, now you mention it I suspect a transition directly to an unknown state may also not do the right thing. Perhaps it would be better to go straight to a more robust state model as suggested by David Vrabel.

  Paul