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] [day] [month] [year] [list]

Message-ID: <1380193530.29483.67.camel@kazak.uk.xensource.com>
Date:	Thu, 26 Sep 2013 12:05:30 +0100
From:	Ian Campbell <Ian.Campbell@...rix.com>
To:	Paul Durrant <Paul.Durrant@...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

On Thu, 2013-09-26 at 11:51 +0100, Paul Durrant wrote:
> > -----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.

OK.

> 
> > > -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.

OK.


--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html