Re: [Xen-devel] [PATCH 1/2] libxl/devd: fix a race with concurrent device addition/removal

[Roger Pau Monne <roger.pau@xxxxxxxxxx>]

On Wed, May 10, 2017 at 11:32:45AM +0100, Ian Jackson wrote:
> Roger Pau Monne writes ("[PATCH 1/2] libxl/devd: fix a race with concurrent 
> device addition/removal"):
> > Current code can free the libxl__device inside of the libxl__ddomain_device
> > before the addition has finished if a removal happens while an addition is
> > still in process:
> > 
> >   backend_watch_callback
> >             |
> >             v
> >        add_device
> >             |                 backend_watch_callback
> >     (async operation)                   |
> >             |                           v
> >             |                     remove_device
> >             |                           |
> >             |                           V
> >             |                    device_complete
> >             |                 (free libxl__device)
> >             v
> >      device_complete
> >   (deref libxl__device)
> > 
> > Fix this by creating a temporary copy of the libxl__device, that's
> > tracked by the GC of the nested async operation. This ensures that
> > the libxl__device used by the async operations cannot be freed while
> > being used.
> 
> Doesn't this arrange that the remove hotplug script will be invoked
> while the add hotplug script is still running ?

That's indeed possible (either with the current code or with this patch),
although unlikely. The async code called by remove_device will wait for the
backend to switch to state 6, while the add_device code will wait for state 2
IIRC (one can change those states to make them clash probably).

> Is that really desirable (or allowed!) ?

Hm, no, I don't think it's desirable at all. I still think this is better that
the previous code (at lest it doesn't dereference libxl__device anymore), but
clearly needs further improvements.

Also, it seems to me the same can happen even without driver domains, if a user
executes concurrent block-{attach/detach} operations, but maybe I'm missing
something?

Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel