1. Patchew
  2. Xen
  3. View series

[PATCH] xen-netback: Check for hotplug-status existence before watching

Michael Brown posted 1 patch 3 years, 7 months ago
Failed in applying to current master (apply log)
drivers/net/xen-netback/xenbus.c | 12 ++++++++----
1 file changed, 8 insertions(+), 4 deletions(-)
[PATCH] xen-netback: Check for hotplug-status existence before watching
Posted by Michael Brown 3 years, 7 months ago 
The logic in connect() is currently written with the assumption that
xenbus_watch_pathfmt() will return an error for a node that does not
exist.  This assumption is incorrect: xenstore does allow a watch to
be registered for a nonexistent node (and will send notifications
should the node be subsequently created).

As of commit 1f2565780 ("xen-netback: remove 'hotplug-status' once it
has served its purpose"), this leads to a failure when a domU
transitions into XenbusStateConnected more than once.  On the first
domU transition into Connected state, the "hotplug-status" node will
be deleted by the hotplug_status_changed() callback in dom0.  On the
second or subsequent domU transition into Connected state, the
hotplug_status_changed() callback will therefore never be invoked, and
so the backend will remain stuck in InitWait.

This failure prevents scenarios such as reloading the xen-netfront
module within a domU, or booting a domU via iPXE.  There is
unfortunately no way for the domU to work around this dom0 bug.

Fix by explicitly checking for existence of the "hotplug-status" node,
thereby creating the behaviour that was previously assumed to exist.

Signed-off-by: Michael Brown <mbrown@fensystems.co.uk>
---
 drivers/net/xen-netback/xenbus.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/drivers/net/xen-netback/xenbus.c b/drivers/net/xen-netback/xenbus.c
index a5439c130130..d24b7a7993aa 100644
--- a/drivers/net/xen-netback/xenbus.c
+++ b/drivers/net/xen-netback/xenbus.c
@@ -824,11 +824,15 @@ static void connect(struct backend_info *be)
 	xenvif_carrier_on(be->vif);
 
 	unregister_hotplug_status_watch(be);
-	err = xenbus_watch_pathfmt(dev, &be->hotplug_status_watch, NULL,
-				   hotplug_status_changed,
-				   "%s/%s", dev->nodename, "hotplug-status");
-	if (!err)
+	if (xenbus_exists(XBT_NIL, dev->nodename, "hotplug-status")) {
+		err = xenbus_watch_pathfmt(dev, &be->hotplug_status_watch,
+					   NULL, hotplug_status_changed,
+					   "%s/%s", dev->nodename,
+					   "hotplug-status");
+		if (err)
+			goto err;
 		be->have_hotplug_status_watch = 1;
+	}
 
 	netif_tx_wake_all_queues(be->vif->dev);
 
-- 
2.29.2
Re: [PATCH] xen-netback: Check for hotplug-status existence before watching
Posted by Paul Durrant 3 years, 7 months ago 
On 13/04/2021 16:25, Michael Brown wrote:
> The logic in connect() is currently written with the assumption that
> xenbus_watch_pathfmt() will return an error for a node that does not
> exist.  This assumption is incorrect: xenstore does allow a watch to
> be registered for a nonexistent node (and will send notifications
> should the node be subsequently created).
> 
> As of commit 1f2565780 ("xen-netback: remove 'hotplug-status' once it
> has served its purpose"), this leads to a failure when a domU
> transitions into XenbusStateConnected more than once.  On the first
> domU transition into Connected state, the "hotplug-status" node will
> be deleted by the hotplug_status_changed() callback in dom0.  On the
> second or subsequent domU transition into Connected state, the
> hotplug_status_changed() callback will therefore never be invoked, and
> so the backend will remain stuck in InitWait.
> 
> This failure prevents scenarios such as reloading the xen-netfront
> module within a domU, or booting a domU via iPXE.  There is
> unfortunately no way for the domU to work around this dom0 bug.
> 
> Fix by explicitly checking for existence of the "hotplug-status" node,
> thereby creating the behaviour that was previously assumed to exist.
> 
> Signed-off-by: Michael Brown <mbrown@fensystems.co.uk>

Reviewed-by: Paul Durrant <paul@xen.org>

> ---
>   drivers/net/xen-netback/xenbus.c | 12 ++++++++----
>   1 file changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/xen-netback/xenbus.c b/drivers/net/xen-netback/xenbus.c
> index a5439c130130..d24b7a7993aa 100644
> --- a/drivers/net/xen-netback/xenbus.c
> +++ b/drivers/net/xen-netback/xenbus.c
> @@ -824,11 +824,15 @@ static void connect(struct backend_info *be)
>   	xenvif_carrier_on(be->vif);
>   
>   	unregister_hotplug_status_watch(be);
> -	err = xenbus_watch_pathfmt(dev, &be->hotplug_status_watch, NULL,
> -				   hotplug_status_changed,
> -				   "%s/%s", dev->nodename, "hotplug-status");
> -	if (!err)
> +	if (xenbus_exists(XBT_NIL, dev->nodename, "hotplug-status")) {
> +		err = xenbus_watch_pathfmt(dev, &be->hotplug_status_watch,
> +					   NULL, hotplug_status_changed,
> +					   "%s/%s", dev->nodename,
> +					   "hotplug-status");
> +		if (err)
> +			goto err;
>   		be->have_hotplug_status_watch = 1;
> +	}
>   
>   	netif_tx_wake_all_queues(be->vif->dev);
>   
>
Re: [PATCH] xen-netback: Check for hotplug-status existence before watching
Posted by Marek Marczykowski-Górecki 3 years, 6 months ago 
On Tue, Apr 13, 2021 at 04:25:12PM +0100, Michael Brown wrote:
> The logic in connect() is currently written with the assumption that
> xenbus_watch_pathfmt() will return an error for a node that does not
> exist.  This assumption is incorrect: xenstore does allow a watch to
> be registered for a nonexistent node (and will send notifications
> should the node be subsequently created).
> 
> As of commit 1f2565780 ("xen-netback: remove 'hotplug-status' once it
> has served its purpose"), this leads to a failure when a domU
> transitions into XenbusStateConnected more than once.  On the first
> domU transition into Connected state, the "hotplug-status" node will
> be deleted by the hotplug_status_changed() callback in dom0.  On the
> second or subsequent domU transition into Connected state, the
> hotplug_status_changed() callback will therefore never be invoked, and
> so the backend will remain stuck in InitWait.
> 
> This failure prevents scenarios such as reloading the xen-netfront
> module within a domU, or booting a domU via iPXE.  There is
> unfortunately no way for the domU to work around this dom0 bug.
> 
> Fix by explicitly checking for existence of the "hotplug-status" node,
> thereby creating the behaviour that was previously assumed to exist.

This change is wrong. The 'hotplug-status' node is created _only_ by a
hotplug script and done so when it's executed. When kernel waits for
hotplug script to be executed it waits for the node to _appear_, not
_change_. So, this change basically made the kernel not waiting for the
hotplug script at all.

Furthermore, there is an additional side effect: in case of a driver
domain, xl devd may be started after the backend node is created (this
may happen if you start the frontend domain in parallel with the
backend). In this case, 'xl devd' will see the vif backend in
XenbusStateConnected state already and will not execute hotplug script
at all.

I think the proper fix is to re-register the watch when necessary,
instead of not registering it at all.

> Signed-off-by: Michael Brown <mbrown@fensystems.co.uk>
> ---
>  drivers/net/xen-netback/xenbus.c | 12 ++++++++----
>  1 file changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/xen-netback/xenbus.c b/drivers/net/xen-netback/xenbus.c
> index a5439c130130..d24b7a7993aa 100644
> --- a/drivers/net/xen-netback/xenbus.c
> +++ b/drivers/net/xen-netback/xenbus.c
> @@ -824,11 +824,15 @@ static void connect(struct backend_info *be)
>  	xenvif_carrier_on(be->vif);
>  
>  	unregister_hotplug_status_watch(be);
> -	err = xenbus_watch_pathfmt(dev, &be->hotplug_status_watch, NULL,
> -				   hotplug_status_changed,
> -				   "%s/%s", dev->nodename, "hotplug-status");
> -	if (!err)
> +	if (xenbus_exists(XBT_NIL, dev->nodename, "hotplug-status")) {
> +		err = xenbus_watch_pathfmt(dev, &be->hotplug_status_watch,
> +					   NULL, hotplug_status_changed,
> +					   "%s/%s", dev->nodename,
> +					   "hotplug-status");
> +		if (err)
> +			goto err;
>  		be->have_hotplug_status_watch = 1;
> +	}
>  
>  	netif_tx_wake_all_queues(be->vif->dev);
>  

-- 
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab
Re: [PATCH] xen-netback: Check for hotplug-status existence before watching
Posted by Michael Brown 3 years, 6 months ago 
On 10/05/2021 19:32, Marek Marczykowski-Górecki wrote:
> On Tue, Apr 13, 2021 at 04:25:12PM +0100, Michael Brown wrote:
>> The logic in connect() is currently written with the assumption that
>> xenbus_watch_pathfmt() will return an error for a node that does not
>> exist.  This assumption is incorrect: xenstore does allow a watch to
>> be registered for a nonexistent node (and will send notifications
>> should the node be subsequently created).
>>
>> As of commit 1f2565780 ("xen-netback: remove 'hotplug-status' once it
>> has served its purpose"), this leads to a failure when a domU
>> transitions into XenbusStateConnected more than once.  On the first
>> domU transition into Connected state, the "hotplug-status" node will
>> be deleted by the hotplug_status_changed() callback in dom0.  On the
>> second or subsequent domU transition into Connected state, the
>> hotplug_status_changed() callback will therefore never be invoked, and
>> so the backend will remain stuck in InitWait.
>>
>> This failure prevents scenarios such as reloading the xen-netfront
>> module within a domU, or booting a domU via iPXE.  There is
>> unfortunately no way for the domU to work around this dom0 bug.
>>
>> Fix by explicitly checking for existence of the "hotplug-status" node,
>> thereby creating the behaviour that was previously assumed to exist.
> 
> This change is wrong. The 'hotplug-status' node is created _only_ by a
> hotplug script and done so when it's executed. When kernel waits for
> hotplug script to be executed it waits for the node to _appear_, not
> _change_. So, this change basically made the kernel not waiting for the
> hotplug script at all.

That doesn't sound plausible to me.  In the setup as you describe, how 
is the kernel expected to differentiate between "hotplug script has not 
yet created the node" and "hotplug script does not exist and will 
therefore never create any node"?

Michael
Re: [PATCH] xen-netback: Check for hotplug-status existence before watching
Posted by Marek Marczykowski-Górecki 3 years, 6 months ago 
On Mon, May 10, 2021 at 07:47:01PM +0100, Michael Brown wrote:
> On 10/05/2021 19:32, Marek Marczykowski-Górecki wrote:
> > On Tue, Apr 13, 2021 at 04:25:12PM +0100, Michael Brown wrote:
> > > The logic in connect() is currently written with the assumption that
> > > xenbus_watch_pathfmt() will return an error for a node that does not
> > > exist.  This assumption is incorrect: xenstore does allow a watch to
> > > be registered for a nonexistent node (and will send notifications
> > > should the node be subsequently created).
> > > 
> > > As of commit 1f2565780 ("xen-netback: remove 'hotplug-status' once it
> > > has served its purpose"), this leads to a failure when a domU
> > > transitions into XenbusStateConnected more than once.  On the first
> > > domU transition into Connected state, the "hotplug-status" node will
> > > be deleted by the hotplug_status_changed() callback in dom0.  On the
> > > second or subsequent domU transition into Connected state, the
> > > hotplug_status_changed() callback will therefore never be invoked, and
> > > so the backend will remain stuck in InitWait.
> > > 
> > > This failure prevents scenarios such as reloading the xen-netfront
> > > module within a domU, or booting a domU via iPXE.  There is
> > > unfortunately no way for the domU to work around this dom0 bug.
> > > 
> > > Fix by explicitly checking for existence of the "hotplug-status" node,
> > > thereby creating the behaviour that was previously assumed to exist.
> > 
> > This change is wrong. The 'hotplug-status' node is created _only_ by a
> > hotplug script and done so when it's executed. When kernel waits for
> > hotplug script to be executed it waits for the node to _appear_, not
> > _change_. So, this change basically made the kernel not waiting for the
> > hotplug script at all.
> 
> That doesn't sound plausible to me.  In the setup as you describe, how is
> the kernel expected to differentiate between "hotplug script has not yet
> created the node" and "hotplug script does not exist and will therefore
> never create any node"?

Is the later valid at all? From what I can see in the toolstack, it
always sets some hotplug script (if not specified explicitly - then
"vif-bridge"),

-- 
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab
Re: [PATCH] xen-netback: Check for hotplug-status existence before watching
Posted by Michael Brown 3 years, 6 months ago 
On 10/05/2021 19:53, Marek Marczykowski-Górecki wrote:
> On Mon, May 10, 2021 at 07:47:01PM +0100, Michael Brown wrote:
>> That doesn't sound plausible to me.  In the setup as you describe, how is
>> the kernel expected to differentiate between "hotplug script has not yet
>> created the node" and "hotplug script does not exist and will therefore
>> never create any node"?
> 
> Is the later valid at all? From what I can see in the toolstack, it
> always sets some hotplug script (if not specified explicitly - then
> "vif-bridge"),

I don't see any definitive documentation around that so I can't answer 
for sure.  It's probably best to let one of the Xen guys answer that.

If you have a suggested patch, I'm happy to test that it doesn't 
reintroduce the regression bug that was fixed by this commit.

Michael
Re: [PATCH] xen-netback: Check for hotplug-status existence before watching
Posted by Marek Marczykowski-Górecki 3 years, 6 months ago 
On Mon, May 10, 2021 at 08:06:55PM +0100, Michael Brown wrote:
> If you have a suggested patch, I'm happy to test that it doesn't reintroduce
> the regression bug that was fixed by this commit.

Actually, I've just tested with a simple reloading xen-netfront module. It
seems in this case, the hotplug script is not re-executed. In fact, I
think it should not be re-executed at all, since the vif interface
remains in place (it just gets NO-CARRIER flag).

This brings a question, why removing hotplug-status in the first place?
The interface remains correctly configured by the hotplug script after
all. From the commit message:

    xen-netback: remove 'hotplug-status' once it has served its purpose

    Removing the 'hotplug-status' node in netback_remove() is wrong; the script
    may not have completed. Only remove the node once the watch has fired and
    has been unregistered.

I think the intention was to remove 'hotplug-status' node _later_ in
case of quickly adding and removing the interface. Is that right, Paul?
In that case, letting hotplug_status_changed() remove the entry wont
work, because the watch was unregistered few lines earlier in
netback_remove(). And keeping the watch is not an option, because the
whole backend_info struct is going to be free-ed already.

If my guess about the original reason for the change is right, I think
it should be fixed at the hotplug script level - it should check if the
device is still there before writing 'hotplug-status' node. I'm not sure
if doing it race-free is possible from a shell script (I think it
requires doing xenstore read _and_ write in a single transaction). But
in the worst case, the aftermath of loosing the race is leaving stray
'hotplug-status' xenstore node - not ideal, but also less harmful than
failing to bring up an interface. At this point, the toolstack could cleanup
it later, perhaps while setting up that interface again (if it gets
re-connected)?

Anyway, perhaps the best thing to do now, is to revert both commits, and
think of an alternative solution for the original issue? That of course
assumes I guessed correctly why it was done in the first place...

-- 
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab
RE: [PATCH] xen-netback: Check for hotplug-status existence before watching
Posted by Durrant, Paul 3 years, 6 months ago 
> -----Original Message-----
> From: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
> Sent: 10 May 2021 20:43
> To: Michael Brown <mbrown@fensystems.co.uk>; paul@xen.org
> Cc: paul@xen.org; xen-devel@lists.xenproject.org; netdev@vger.kernel.org; wei.liu@kernel.org; Durrant,
> Paul <pdurrant@amazon.co.uk>
> Subject: RE: [EXTERNAL] [PATCH] xen-netback: Check for hotplug-status existence before watching
> 
> On Mon, May 10, 2021 at 08:06:55PM +0100, Michael Brown wrote:
> > If you have a suggested patch, I'm happy to test that it doesn't reintroduce
> > the regression bug that was fixed by this commit.
> 
> Actually, I've just tested with a simple reloading xen-netfront module. It
> seems in this case, the hotplug script is not re-executed. In fact, I
> think it should not be re-executed at all, since the vif interface
> remains in place (it just gets NO-CARRIER flag).
> 
> This brings a question, why removing hotplug-status in the first place?
> The interface remains correctly configured by the hotplug script after
> all. From the commit message:
> 
>     xen-netback: remove 'hotplug-status' once it has served its purpose
> 
>     Removing the 'hotplug-status' node in netback_remove() is wrong; the script
>     may not have completed. Only remove the node once the watch has fired and
>     has been unregistered.
> 
> I think the intention was to remove 'hotplug-status' node _later_ in
> case of quickly adding and removing the interface. Is that right, Paul?

The removal was done to allow unbind/bind to function correctly. IIRC before the original patch doing a bind would stall forever waiting for the hotplug status to change, which would never happen.

> In that case, letting hotplug_status_changed() remove the entry wont
> work, because the watch was unregistered few lines earlier in
> netback_remove(). And keeping the watch is not an option, because the
> whole backend_info struct is going to be free-ed already.
> 
> If my guess about the original reason for the change is right, I think
> it should be fixed at the hotplug script level - it should check if the
> device is still there before writing 'hotplug-status' node.
> I'm not sure if doing it race-free is possible from a shell script (I think it
> requires doing xenstore read _and_ write in a single transaction). But
> in the worst case, the aftermath of loosing the race is leaving stray
> 'hotplug-status' xenstore node - not ideal, but also less harmful than
> failing to bring up an interface. At this point, the toolstack could cleanup
> it later, perhaps while setting up that interface again (if it gets
> re-connected)?
> 
> Anyway, perhaps the best thing to do now, is to revert both commits, and
> think of an alternative solution for the original issue? That of course
> assumes I guessed correctly why it was done in the first place...
> 

Simply reverting everything would likely break the ability to do unbind and bind (which is useful e.g to allow update the netback module whilst guests are still running) so I don't think that's an option.

  Paul
Re: [PATCH] xen-netback: Check for hotplug-status existence before watching
Posted by Marek Marczykowski-Górecki 3 years, 6 months ago 
On Tue, May 11, 2021 at 07:06:55AM +0000, Durrant, Paul wrote:
> > -----Original Message-----
> > From: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
> > Sent: 10 May 2021 20:43
> > To: Michael Brown <mbrown@fensystems.co.uk>; paul@xen.org
> > Cc: paul@xen.org; xen-devel@lists.xenproject.org; netdev@vger.kernel.org; wei.liu@kernel.org; Durrant,
> > Paul <pdurrant@amazon.co.uk>
> > Subject: RE: [EXTERNAL] [PATCH] xen-netback: Check for hotplug-status existence before watching
> > 
> > On Mon, May 10, 2021 at 08:06:55PM +0100, Michael Brown wrote:
> > > If you have a suggested patch, I'm happy to test that it doesn't reintroduce
> > > the regression bug that was fixed by this commit.
> > 
> > Actually, I've just tested with a simple reloading xen-netfront module. It
> > seems in this case, the hotplug script is not re-executed. In fact, I
> > think it should not be re-executed at all, since the vif interface
> > remains in place (it just gets NO-CARRIER flag).
> > 
> > This brings a question, why removing hotplug-status in the first place?
> > The interface remains correctly configured by the hotplug script after
> > all. From the commit message:
> > 
> >     xen-netback: remove 'hotplug-status' once it has served its purpose
> > 
> >     Removing the 'hotplug-status' node in netback_remove() is wrong; the script
> >     may not have completed. Only remove the node once the watch has fired and
> >     has been unregistered.
> > 
> > I think the intention was to remove 'hotplug-status' node _later_ in
> > case of quickly adding and removing the interface. Is that right, Paul?
> 
> The removal was done to allow unbind/bind to function correctly. IIRC before the original patch doing a bind would stall forever waiting for the hotplug status to change, which would never happen.

Hmm, in that case maybe don't remove it at all in the backend, and let
it be cleaned up by the toolstack, when it removes other backend-related
nodes?

-- 
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab
Re: [PATCH] xen-netback: Check for hotplug-status existence before watching
Posted by Marek Marczykowski-Górecki 3 years, 6 months ago 
On Tue, May 11, 2021 at 12:40:54PM +0200, Marek Marczykowski-Górecki wrote:
> On Tue, May 11, 2021 at 07:06:55AM +0000, Durrant, Paul wrote:
> > > -----Original Message-----
> > > From: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
> > > Sent: 10 May 2021 20:43
> > > To: Michael Brown <mbrown@fensystems.co.uk>; paul@xen.org
> > > Cc: paul@xen.org; xen-devel@lists.xenproject.org; netdev@vger.kernel.org; wei.liu@kernel.org; Durrant,
> > > Paul <pdurrant@amazon.co.uk>
> > > Subject: RE: [EXTERNAL] [PATCH] xen-netback: Check for hotplug-status existence before watching
> > > 
> > > On Mon, May 10, 2021 at 08:06:55PM +0100, Michael Brown wrote:
> > > > If you have a suggested patch, I'm happy to test that it doesn't reintroduce
> > > > the regression bug that was fixed by this commit.
> > > 
> > > Actually, I've just tested with a simple reloading xen-netfront module. It
> > > seems in this case, the hotplug script is not re-executed. In fact, I
> > > think it should not be re-executed at all, since the vif interface
> > > remains in place (it just gets NO-CARRIER flag).
> > > 
> > > This brings a question, why removing hotplug-status in the first place?
> > > The interface remains correctly configured by the hotplug script after
> > > all. From the commit message:
> > > 
> > >     xen-netback: remove 'hotplug-status' once it has served its purpose
> > > 
> > >     Removing the 'hotplug-status' node in netback_remove() is wrong; the script
> > >     may not have completed. Only remove the node once the watch has fired and
> > >     has been unregistered.
> > > 
> > > I think the intention was to remove 'hotplug-status' node _later_ in
> > > case of quickly adding and removing the interface. Is that right, Paul?
> > 
> > The removal was done to allow unbind/bind to function correctly. IIRC before the original patch doing a bind would stall forever waiting for the hotplug status to change, which would never happen.
> 
> Hmm, in that case maybe don't remove it at all in the backend, and let
> it be cleaned up by the toolstack, when it removes other backend-related
> nodes?

No, unbind/bind _does_ require hotplug script to be called again.

When exactly vif interface appears in the system (starts to be available
for the hotplug script)? Maybe remove 'hotplug-status' just before that
point?


-- 
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab
RE: [PATCH] xen-netback: Check for hotplug-status existence before watching
Posted by Durrant, Paul 3 years, 6 months ago 
> -----Original Message-----
> From: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
> Sent: 11 May 2021 11:45
> To: Durrant, Paul <pdurrant@amazon.co.uk>
> Cc: Michael Brown <mbrown@fensystems.co.uk>; paul@xen.org; xen-devel@lists.xenproject.org;
> netdev@vger.kernel.org; wei.liu@kernel.org
> Subject: RE: [EXTERNAL] [PATCH] xen-netback: Check for hotplug-status existence before watching
> 
> On Tue, May 11, 2021 at 12:40:54PM +0200, Marek Marczykowski-Górecki wrote:
> > On Tue, May 11, 2021 at 07:06:55AM +0000, Durrant, Paul wrote:
> > > > -----Original Message-----
> > > > From: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
> > > > Sent: 10 May 2021 20:43
> > > > To: Michael Brown <mbrown@fensystems.co.uk>; paul@xen.org
> > > > Cc: paul@xen.org; xen-devel@lists.xenproject.org; netdev@vger.kernel.org; wei.liu@kernel.org;
> Durrant,
> > > > Paul <pdurrant@amazon.co.uk>
> > > > Subject: RE: [EXTERNAL] [PATCH] xen-netback: Check for hotplug-status existence before watching
> > > >
> > > > On Mon, May 10, 2021 at 08:06:55PM +0100, Michael Brown wrote:
> > > > > If you have a suggested patch, I'm happy to test that it doesn't reintroduce
> > > > > the regression bug that was fixed by this commit.
> > > >
> > > > Actually, I've just tested with a simple reloading xen-netfront module. It
> > > > seems in this case, the hotplug script is not re-executed. In fact, I
> > > > think it should not be re-executed at all, since the vif interface
> > > > remains in place (it just gets NO-CARRIER flag).
> > > >
> > > > This brings a question, why removing hotplug-status in the first place?
> > > > The interface remains correctly configured by the hotplug script after
> > > > all. From the commit message:
> > > >
> > > >     xen-netback: remove 'hotplug-status' once it has served its purpose
> > > >
> > > >     Removing the 'hotplug-status' node in netback_remove() is wrong; the script
> > > >     may not have completed. Only remove the node once the watch has fired and
> > > >     has been unregistered.
> > > >
> > > > I think the intention was to remove 'hotplug-status' node _later_ in
> > > > case of quickly adding and removing the interface. Is that right, Paul?
> > >
> > > The removal was done to allow unbind/bind to function correctly. IIRC before the original patch
> doing a bind would stall forever waiting for the hotplug status to change, which would never happen.
> >
> > Hmm, in that case maybe don't remove it at all in the backend, and let
> > it be cleaned up by the toolstack, when it removes other backend-related
> > nodes?
> 
> No, unbind/bind _does_ require hotplug script to be called again.
> 

Yes, sorry I was misremembering. My memory is hazy but there was definitely a problem at the time with leaving the node in place.

> When exactly vif interface appears in the system (starts to be available
> for the hotplug script)? Maybe remove 'hotplug-status' just before that
> point?
> 

I really can't remember any detail. Perhaps try reverting both patches then and check that the unbind/rmmod/modprobe/bind sequence still works (and the backend actually makes it into connected state).

  Paul
Re: [PATCH] xen-netback: Check for hotplug-status existence before watching
Posted by Marek Marczykowski-Górecki 3 years, 6 months ago 
On Tue, May 11, 2021 at 12:46:38PM +0000, Durrant, Paul wrote:
> > -----Original Message-----
> > From: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
> > Sent: 11 May 2021 11:45
> > To: Durrant, Paul <pdurrant@amazon.co.uk>
> > Cc: Michael Brown <mbrown@fensystems.co.uk>; paul@xen.org; xen-devel@lists.xenproject.org;
> > netdev@vger.kernel.org; wei.liu@kernel.org
> > Subject: RE: [EXTERNAL] [PATCH] xen-netback: Check for hotplug-status existence before watching
> > 
> > On Tue, May 11, 2021 at 12:40:54PM +0200, Marek Marczykowski-Górecki wrote:
> > > On Tue, May 11, 2021 at 07:06:55AM +0000, Durrant, Paul wrote:
> > > > > -----Original Message-----
> > > > > From: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
> > > > > Sent: 10 May 2021 20:43
> > > > > To: Michael Brown <mbrown@fensystems.co.uk>; paul@xen.org
> > > > > Cc: paul@xen.org; xen-devel@lists.xenproject.org; netdev@vger.kernel.org; wei.liu@kernel.org;
> > Durrant,
> > > > > Paul <pdurrant@amazon.co.uk>
> > > > > Subject: RE: [EXTERNAL] [PATCH] xen-netback: Check for hotplug-status existence before watching
> > > > >
> > > > > On Mon, May 10, 2021 at 08:06:55PM +0100, Michael Brown wrote:
> > > > > > If you have a suggested patch, I'm happy to test that it doesn't reintroduce
> > > > > > the regression bug that was fixed by this commit.
> > > > >
> > > > > Actually, I've just tested with a simple reloading xen-netfront module. It
> > > > > seems in this case, the hotplug script is not re-executed. In fact, I
> > > > > think it should not be re-executed at all, since the vif interface
> > > > > remains in place (it just gets NO-CARRIER flag).
> > > > >
> > > > > This brings a question, why removing hotplug-status in the first place?
> > > > > The interface remains correctly configured by the hotplug script after
> > > > > all. From the commit message:
> > > > >
> > > > >     xen-netback: remove 'hotplug-status' once it has served its purpose
> > > > >
> > > > >     Removing the 'hotplug-status' node in netback_remove() is wrong; the script
> > > > >     may not have completed. Only remove the node once the watch has fired and
> > > > >     has been unregistered.
> > > > >
> > > > > I think the intention was to remove 'hotplug-status' node _later_ in
> > > > > case of quickly adding and removing the interface. Is that right, Paul?
> > > >
> > > > The removal was done to allow unbind/bind to function correctly. IIRC before the original patch
> > doing a bind would stall forever waiting for the hotplug status to change, which would never happen.
> > >
> > > Hmm, in that case maybe don't remove it at all in the backend, and let
> > > it be cleaned up by the toolstack, when it removes other backend-related
> > > nodes?
> > 
> > No, unbind/bind _does_ require hotplug script to be called again.
> > 
> 
> Yes, sorry I was misremembering. My memory is hazy but there was definitely a problem at the time with leaving the node in place.
> 
> > When exactly vif interface appears in the system (starts to be available
> > for the hotplug script)? Maybe remove 'hotplug-status' just before that
> > point?
> > 
> 
> I really can't remember any detail. Perhaps try reverting both patches then and check that the unbind/rmmod/modprobe/bind sequence still works (and the backend actually makes it into connected state).

Ok, I've tried this. I've reverted both commits, then used your test
script from the 9476654bd5e8ad42abe8ee9f9e90069ff8e60c17:
    
    This has been tested by running iperf as a server in the test VM and
    then running a client against it in a continuous loop, whilst also
    running:
    
    while true;
      do echo vif-$DOMID-$VIF >unbind;
      echo down;
      rmmod xen-netback;
      echo unloaded;
      modprobe xen-netback;
      cd $(pwd);
      brctl addif xenbr0 vif$DOMID.$VIF;
      ip link set vif$DOMID.$VIF up;
      echo up;
      sleep 5;
      done
    
    in dom0 from /sys/bus/xen-backend/drivers/vif to continuously unbind,
    unload, re-load, re-bind and re-plumb the backend.
    
In fact, the need to call `brctl` and `ip link` manually is exactly
because the hotplug script isn't executed. When I execute it manually,
the backend properly gets back to working. So, removing 'hotplug-status'
was in the correct place (netback_remove). The missing part is the toolstack
calling the hotplug script on xen-netback re-bind.

In this case, I'm not sure what is the proper way. If I restart
xendriverdomain service (I do run the backend in domU), it properly
executes hotplug script and the backend interface gets properly
configured. But it doesn't do it on its own. It seems to be related to
device "state" in xenstore. The specific part of the libxl is
backend_watch_callback():
https://github.com/xen-project/xen/blob/master/tools/libs/light/libxl_device.c#L1664

    ddev = search_for_device(dguest, dev);
    if (ddev == NULL && state == XenbusStateClosed) {
        /*
         * Spurious state change, device has already been disconnected
         * or never attached.
         */
        goto skip;
    } else if (ddev == NULL) {
        rc = add_device(egc, nested_ao, dguest, dev);
        if (rc > 0)
            free_ao = true;
    } else if (state == XenbusStateClosed && online == 0) {
        rc = remove_device(egc, nested_ao, dguest, ddev);
        if (rc > 0)
            free_ao = true;
        check_and_maybe_remove_guest(gc, ddomain, dguest);
    }

In short: if device gets XenbusStateInitWait for the first time (ddev ==
NULL case), it goes to add_device() which executes the hotplug script
and stores the device.
Then, if device goes to XenbusStateClosed + online==0 state, then it
executes hotplug script again (with "offline" parameter) and forgets the
device. If you unbind the driver, the device stays in
XenbusStateConnected state (in xenstore), and after you bind it again,
it goes to XenbusStateInitWait. It don't think it goes through
XenbusStateClosed, and online stays at 1 too, so libxl doesn't execute
the hotplug script again.

Some solution could be to add an extra case at the end, like "ddev !=
NULL && state == XenbusStateInitWait && hotplug-status != connected".
And make sure xl devd won't call the same hotplug script multiple times
for the same device _at the same time_ (I'm not sure about the async
machinery here).

But even if xl devd (aka xendriverdomain service) gets "fixes" to
execute hotplug script in that case, I don't think it would work in
backend in dom0 case - there, I think nothing watches already configured
vif interfaces (there is no xl devd daemon in dom0, and xl background
process watches only domain death and cdrom eject events). 

I'm adding toolstack maintainers, maybe they'll have some idea...

In any case, the issue is not calling the hotplug script, responsible
for configuring newly created vif interface. Not kernel waiting for it.
So, I think both commits should still be reverted.

-- 
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab
Re: [PATCH] xen-netback: Check for hotplug-status existence before watching
Posted by Michael Brown 3 years, 6 months ago 
On 17/05/2021 22:43, Marek Marczykowski-Górecki wrote:
> In any case, the issue is not calling the hotplug script, responsible
> for configuring newly created vif interface. Not kernel waiting for it.
> So, I think both commits should still be reverted.

Did you also test the ability for a domU to have the netfront driver 
reloaded?  (That should be roughly equivalent to my original test 
scenario comprising the iPXE netfront driver used to boot a kernel that 
then loads the Linux netfront driver.)

Michael
Re: [PATCH] xen-netback: Check for hotplug-status existence before watching
Posted by Marek Marczykowski-Górecki 3 years, 6 months ago 
On Mon, May 17, 2021 at 10:51:38PM +0100, Michael Brown wrote:
> On 17/05/2021 22:43, Marek Marczykowski-Górecki wrote:
> > In any case, the issue is not calling the hotplug script, responsible
> > for configuring newly created vif interface. Not kernel waiting for it.
> > So, I think both commits should still be reverted.
> 
> Did you also test the ability for a domU to have the netfront driver
> reloaded?  (That should be roughly equivalent to my original test scenario
> comprising the iPXE netfront driver used to boot a kernel that then loads
> the Linux netfront driver.)

Yes, with both commits reverted, it just works. Without the need to do
anything at the backend side.

-- 
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab
Re: [PATCH] xen-netback: Check for hotplug-status existence before watching
Posted by Paul Durrant 3 years, 6 months ago 
On 17/05/2021 22:43, Marek Marczykowski-Górecki wrote:
> On Tue, May 11, 2021 at 12:46:38PM +0000, Durrant, Paul wrote:
>>> -----Original Message-----
>>> From: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
>>> Sent: 11 May 2021 11:45
>>> To: Durrant, Paul <pdurrant@amazon.co.uk>
>>> Cc: Michael Brown <mbrown@fensystems.co.uk>; paul@xen.org; xen-devel@lists.xenproject.org;
>>> netdev@vger.kernel.org; wei.liu@kernel.org
>>> Subject: RE: [EXTERNAL] [PATCH] xen-netback: Check for hotplug-status existence before watching
>>>
>>> On Tue, May 11, 2021 at 12:40:54PM +0200, Marek Marczykowski-Górecki wrote:
>>>> On Tue, May 11, 2021 at 07:06:55AM +0000, Durrant, Paul wrote:
>>>>>> -----Original Message-----
>>>>>> From: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
>>>>>> Sent: 10 May 2021 20:43
>>>>>> To: Michael Brown <mbrown@fensystems.co.uk>; paul@xen.org
>>>>>> Cc: paul@xen.org; xen-devel@lists.xenproject.org; netdev@vger.kernel.org; wei.liu@kernel.org;
>>> Durrant,
>>>>>> Paul <pdurrant@amazon.co.uk>
>>>>>> Subject: RE: [EXTERNAL] [PATCH] xen-netback: Check for hotplug-status existence before watching
>>>>>>
>>>>>> On Mon, May 10, 2021 at 08:06:55PM +0100, Michael Brown wrote:
>>>>>>> If you have a suggested patch, I'm happy to test that it doesn't reintroduce
>>>>>>> the regression bug that was fixed by this commit.
>>>>>>
>>>>>> Actually, I've just tested with a simple reloading xen-netfront module. It
>>>>>> seems in this case, the hotplug script is not re-executed. In fact, I
>>>>>> think it should not be re-executed at all, since the vif interface
>>>>>> remains in place (it just gets NO-CARRIER flag).
>>>>>>
>>>>>> This brings a question, why removing hotplug-status in the first place?
>>>>>> The interface remains correctly configured by the hotplug script after
>>>>>> all. From the commit message:
>>>>>>
>>>>>>      xen-netback: remove 'hotplug-status' once it has served its purpose
>>>>>>
>>>>>>      Removing the 'hotplug-status' node in netback_remove() is wrong; the script
>>>>>>      may not have completed. Only remove the node once the watch has fired and
>>>>>>      has been unregistered.
>>>>>>
>>>>>> I think the intention was to remove 'hotplug-status' node _later_ in
>>>>>> case of quickly adding and removing the interface. Is that right, Paul?
>>>>>
>>>>> The removal was done to allow unbind/bind to function correctly. IIRC before the original patch
>>> doing a bind would stall forever waiting for the hotplug status to change, which would never happen.
>>>>
>>>> Hmm, in that case maybe don't remove it at all in the backend, and let
>>>> it be cleaned up by the toolstack, when it removes other backend-related
>>>> nodes?
>>>
>>> No, unbind/bind _does_ require hotplug script to be called again.
>>>
>>
>> Yes, sorry I was misremembering. My memory is hazy but there was definitely a problem at the time with leaving the node in place.
>>
>>> When exactly vif interface appears in the system (starts to be available
>>> for the hotplug script)? Maybe remove 'hotplug-status' just before that
>>> point?
>>>
>>
>> I really can't remember any detail. Perhaps try reverting both patches then and check that the unbind/rmmod/modprobe/bind sequence still works (and the backend actually makes it into connected state).
> 
> Ok, I've tried this. I've reverted both commits, then used your test
> script from the 9476654bd5e8ad42abe8ee9f9e90069ff8e60c17:
>      
>      This has been tested by running iperf as a server in the test VM and
>      then running a client against it in a continuous loop, whilst also
>      running:
>      
>      while true;
>        do echo vif-$DOMID-$VIF >unbind;
>        echo down;
>        rmmod xen-netback;
>        echo unloaded;
>        modprobe xen-netback;
>        cd $(pwd);
>        brctl addif xenbr0 vif$DOMID.$VIF;
>        ip link set vif$DOMID.$VIF up;
>        echo up;
>        sleep 5;
>        done
>      
>      in dom0 from /sys/bus/xen-backend/drivers/vif to continuously unbind,
>      unload, re-load, re-bind and re-plumb the backend.
>      
> In fact, the need to call `brctl` and `ip link` manually is exactly
> because the hotplug script isn't executed. When I execute it manually,
> the backend properly gets back to working. So, removing 'hotplug-status'
> was in the correct place (netback_remove). The missing part is the toolstack
> calling the hotplug script on xen-netback re-bind.
> 

Why is that missing? We're going behind the back of the toolstack to do 
the unbind and bind so why should we expect it to re-execute a hotplug 
script?

> In this case, I'm not sure what is the proper way. If I restart
> xendriverdomain service (I do run the backend in domU), it properly
> executes hotplug script and the backend interface gets properly
> configured. But it doesn't do it on its own. It seems to be related to
> device "state" in xenstore. The specific part of the libxl is
> backend_watch_callback():
> https://github.com/xen-project/xen/blob/master/tools/libs/light/libxl_device.c#L1664
> 
>      ddev = search_for_device(dguest, dev);
>      if (ddev == NULL && state == XenbusStateClosed) {
>          /*
>           * Spurious state change, device has already been disconnected
>           * or never attached.
>           */
>          goto skip;
>      } else if (ddev == NULL) {
>          rc = add_device(egc, nested_ao, dguest, dev);
>          if (rc > 0)
>              free_ao = true;
>      } else if (state == XenbusStateClosed && online == 0) {
>          rc = remove_device(egc, nested_ao, dguest, ddev);
>          if (rc > 0)
>              free_ao = true;
>          check_and_maybe_remove_guest(gc, ddomain, dguest);
>      }
> 
> In short: if device gets XenbusStateInitWait for the first time (ddev ==
> NULL case), it goes to add_device() which executes the hotplug script
> and stores the device.
> Then, if device goes to XenbusStateClosed + online==0 state, then it
> executes hotplug script again (with "offline" parameter) and forgets the
> device. If you unbind the driver, the device stays in
> XenbusStateConnected state (in xenstore), and after you bind it again,
> it goes to XenbusStateInitWait. It don't think it goes through
> XenbusStateClosed, and online stays at 1 too, so libxl doesn't execute
> the hotplug script again.

This is pretty key. The frontend should not notice an unbind/bind i.e. 
there should be no evidence of it happening by examining states in 
xenstore (from the guest side).

   Paul

> 
> Some solution could be to add an extra case at the end, like "ddev !=
> NULL && state == XenbusStateInitWait && hotplug-status != connected".
> And make sure xl devd won't call the same hotplug script multiple times
> for the same device _at the same time_ (I'm not sure about the async
> machinery here).
> 
> But even if xl devd (aka xendriverdomain service) gets "fixes" to
> execute hotplug script in that case, I don't think it would work in
> backend in dom0 case - there, I think nothing watches already configured
> vif interfaces (there is no xl devd daemon in dom0, and xl background
> process watches only domain death and cdrom eject events).
> 
> I'm adding toolstack maintainers, maybe they'll have some idea...
> 
> In any case, the issue is not calling the hotplug script, responsible
> for configuring newly created vif interface. Not kernel waiting for it.
> So, I think both commits should still be reverted.
>
Re: [PATCH] xen-netback: Check for hotplug-status existence before watching
Posted by Marek Marczykowski-Górecki 3 years, 6 months ago 
On Tue, May 18, 2021 at 07:57:16AM +0100, Paul Durrant wrote:
> On 17/05/2021 22:43, Marek Marczykowski-Górecki wrote:
> > On Tue, May 11, 2021 at 12:46:38PM +0000, Durrant, Paul wrote:
> > > I really can't remember any detail. Perhaps try reverting both patches then and check that the unbind/rmmod/modprobe/bind sequence still works (and the backend actually makes it into connected state).
> > 
> > Ok, I've tried this. I've reverted both commits, then used your test
> > script from the 9476654bd5e8ad42abe8ee9f9e90069ff8e60c17:
> >      This has been tested by running iperf as a server in the test VM and
> >      then running a client against it in a continuous loop, whilst also
> >      running:
> >      while true;
> >        do echo vif-$DOMID-$VIF >unbind;
> >        echo down;
> >        rmmod xen-netback;
> >        echo unloaded;
> >        modprobe xen-netback;
> >        cd $(pwd);
> >        brctl addif xenbr0 vif$DOMID.$VIF;
> >        ip link set vif$DOMID.$VIF up;
> >        echo up;
> >        sleep 5;
> >        done
> >      in dom0 from /sys/bus/xen-backend/drivers/vif to continuously unbind,
> >      unload, re-load, re-bind and re-plumb the backend.
> > In fact, the need to call `brctl` and `ip link` manually is exactly
> > because the hotplug script isn't executed. When I execute it manually,
> > the backend properly gets back to working. So, removing 'hotplug-status'
> > was in the correct place (netback_remove). The missing part is the toolstack
> > calling the hotplug script on xen-netback re-bind.
> > 
> 
> Why is that missing? We're going behind the back of the toolstack to do the
> unbind and bind so why should we expect it to re-execute a hotplug script?

Ok, then simply execute the whole hotplug script (instead of its subset)
after re-loading the backend module and everything will be fine.

For example like this:
    XENBUS_PATH=backend/vif/$DOMID/$VIF \
    XENBUS_TYPE=vif \
    XENBUS_BASE_PATH=backend \
    script=/etc/xen/scripts/vif-bridge \
    vif=vif.$DOMID.$VIF \
    /etc/xen/scripts/vif-bridge online

(...)

> > In short: if device gets XenbusStateInitWait for the first time (ddev ==
> > NULL case), it goes to add_device() which executes the hotplug script
> > and stores the device.
> > Then, if device goes to XenbusStateClosed + online==0 state, then it
> > executes hotplug script again (with "offline" parameter) and forgets the
> > device. If you unbind the driver, the device stays in
> > XenbusStateConnected state (in xenstore), and after you bind it again,
> > it goes to XenbusStateInitWait. It don't think it goes through
> > XenbusStateClosed, and online stays at 1 too, so libxl doesn't execute
> > the hotplug script again.
> 
> This is pretty key. The frontend should not notice an unbind/bind i.e. there
> should be no evidence of it happening by examining states in xenstore (from
> the guest side).

If you update the backend module, I think the frontend needs at least to
re-evaluate feature-* nodes. In case of applying just a bug fix, they
should not change (in theory), but technically that would be the correct
thing to do.

-- 
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab
Re: [PATCH] xen-netback: Check for hotplug-status existence before watching
Posted by patchwork-bot+netdevbpf@kernel.org 3 years, 7 months ago 
Hello:

This patch was applied to netdev/net.git (refs/heads/master):

On Tue, 13 Apr 2021 16:25:12 +0100 you wrote:
> The logic in connect() is currently written with the assumption that
> xenbus_watch_pathfmt() will return an error for a node that does not
> exist.  This assumption is incorrect: xenstore does allow a watch to
> be registered for a nonexistent node (and will send notifications
> should the node be subsequently created).
> 
> As of commit 1f2565780 ("xen-netback: remove 'hotplug-status' once it
> has served its purpose"), this leads to a failure when a domU
> transitions into XenbusStateConnected more than once.  On the first
> domU transition into Connected state, the "hotplug-status" node will
> be deleted by the hotplug_status_changed() callback in dom0.  On the
> second or subsequent domU transition into Connected state, the
> hotplug_status_changed() callback will therefore never be invoked, and
> so the backend will remain stuck in InitWait.
> 
> [...]

Here is the summary with links:
  - xen-netback: Check for hotplug-status existence before watching
    https://git.kernel.org/netdev/net/c/2afeec08ab5c

You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

Patchew 2.1-devel-b67e4c2

© 2016 - 2024 Red Hat, Inc.