netdev - RE: [PATCH net v5 1/1] xen-netback: Handle backend state transitions in a more robust way

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <9AAE0902D5BC7E449B7C8E4E778ABCD0114155@AMSPEX01CL01.citrite.net>
Date:	Thu, 26 Sep 2013 10:51:08 +0000
From:	Paul Durrant <Paul.Durrant@...rix.com>
To:	Ian Campbell <Ian.Campbell@...rix.com>
CC:	"xen-devel@...ts.xen.org" <xen-devel@...ts.xen.org>,
	"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
	Wei Liu <wei.liu2@...rix.com>,
	David Vrabel <david.vrabel@...rix.com>
Subject: RE: [PATCH net v5 1/1] xen-netback: Handle backend state
 transitions in a more robust way

> -----Original Message-----
> From: Ian Campbell
> Sent: 26 September 2013 11:17
> To: Paul Durrant
> Cc: xen-devel@...ts.xen.org; netdev@...r.kernel.org; Wei Liu; David Vrabel
> Subject: Re: [PATCH net v5 1/1] xen-netback: Handle backend state
> transitions in a more robust way
> 
> On Wed, 2013-09-25 at 10:59 +0100, Paul Durrant wrote:
> > When the frontend state changes netback now specifies its desired state
> to
> > a new function, set_backend_state(), which transitions through any
> > necessary intermediate states.
> > This fixes an issue observed with some old Windows frontend drivers
> where
> > they failed to transition through the Closing state and netback would not
> > behave correctly.
> 
> I'm somewhat terrified of breaking compatibility with some old or
> obscure frontend here. I don't think it is reasonable to try and test
> all of those so I guess we'll just have to deal with that when it
> happens...
> 
> > Signed-off-by: Paul Durrant <paul.durrant@...rix.com>
> > Cc: Ian Campbell <ian.campbell@...rix.com>
> > Cc: Wei Liu <wei.liu2@...rix.com>
> > Cc: David Vrabel <david.vrabel@...rix.com>
> > ---
> >  drivers/net/xen-netback/xenbus.c |  143
> ++++++++++++++++++++++++++++++--------
> >  1 file changed, 113 insertions(+), 30 deletions(-)
> >
> > diff --git a/drivers/net/xen-netback/xenbus.c b/drivers/net/xen-
> netback/xenbus.c
> > index a53782e..01329b2 100644
> > --- a/drivers/net/xen-netback/xenbus.c
> > +++ b/drivers/net/xen-netback/xenbus.c
> > @@ -24,6 +24,7 @@
> >  struct backend_info {
> >  	struct xenbus_device *dev;
> >  	struct xenvif *vif;
> > +	enum xenbus_state state;
> 
> This cannot just be read from xenstore because of the
> wait-for-hotplug-script deferral stuff, right?
> 

Yes, that's right.

> I'm not sure it is worth clarifying that, either by a comment or by
> naming it deferred_state_change or something?
> 

It’s the xenbus state that's deferred, the backend state leads so I could name it 'desired_state' or 'target_state' perhaps; probably a bit ugly so I'll add a comment above that field I think.

> > -static void destroy_backend(struct xenbus_device *dev)
> > +static void backend_connect(struct backend_info *be)
> >  {
> > -	struct backend_info *be = dev_get_drvdata(&dev->dev);
> > +	if (be->vif)
> > +		connect(be);
> > +}
> >
> > -	if (be->vif) {
> > -		kobject_uevent(&dev->dev.kobj, KOBJ_OFFLINE);
> > -		xenbus_rm(XBT_NIL, dev->nodename, "hotplug-status");
> > -		xenvif_free(be->vif);
> > -		be->vif = NULL;
> 
> Where did this uevent offlining functionality end up?
> 

It's done in netback_remove() .

> > +static inline void backend_switch_state(struct backend_info *be,
> > +					enum xenbus_state state)
> > +{
> > +	struct xenbus_device *dev = be->dev;
> > +
> > +	pr_debug("%s -> %s\n", dev->nodename, xenbus_strstate(state));
> > +	be->state = state;
> > +
> > +	/* If we are waiting for a hotplug script then defer the
> > +	 * actual xenbus state change.
> 
> Is the right/wise regardless of the state we are moving too? Might the
> state change have effectively "cancelled" the need to wait for the
> script?
> 
> The previous code only deferred when moving to Connected. If we are now
> moving to Closed/Closing do we still need to wait?
> 

I debated about this; we never really dealt with this before. I think it's legitimate for the frontend to give up waiting for a backend hotplug so we should be able to go from InitWait to Closing, so in this case be->state will be Closing when the script (hopefully) finishes. Thus we move to that when the watch fires rather than to Connected. Then the frontend can move to Closed. If the hotplug script never finishes then I don't think we're any more screwed than we were before.

  Paul

> 
> Ian.