Re: [PATCH] 9p/xen: protect xen_9pfs_front_free against concurrent calls

[Dominique Martinet <asmadeus@codewreck.org>]

Stefano Stabellini wrote on Mon, Jan 26, 2026 at 02:09:01PM -0800:
> > I don't understand this priv->rings != NULL check here;
> > if it's guarding for front_free() called before front_init() then it
> > doesn't need to be checked at every iteration, and if it can change in
> > another thread I don't see why it would be safe.
> 
> xen_9pfs_front_free() can be reached from the error paths before any
> rings are allocated, so we need to handle a NULL priv->rings but it
> doesn't have to be checked at every iteration. I can move it before the
> for loop as you suggested.

Yes, please move it above the loop

> > > @@ -310,9 +306,18 @@ static void xen_9pfs_front_free(struct xen_9pfs_front_priv *priv)
> > >  
> > >  static void xen_9pfs_front_remove(struct xenbus_device *dev)
> > >  {
> > > -	struct xen_9pfs_front_priv *priv = dev_get_drvdata(&dev->dev);
> > > +	struct xen_9pfs_front_priv *priv;
> > >  
> > > +	write_lock(&xen_9pfs_lock);
> > > +	priv = dev_get_drvdata(&dev->dev);
> > > +	if (priv == NULL) {
> > > +		write_unlock(&xen_9pfs_lock);
> > > +		return;
> > > +	}
> > >  	dev_set_drvdata(&dev->dev, NULL);
> > > +	list_del_init(&priv->list);
> > 
> > There's nothing wrong with using the del_init() variant here, but it
> > would imply someone else could still access it after the unlock here,
> > which means to me they could still access it after priv is freed in
> > xen_9pfs_front_free().
> > >From your commit message I think the priv == NULL check and
> > dev_set_drvdata() under lock above is enough, can you confirm?
> 
> Yes you are right. I can replace list_del_init with list_del if you
> think it is clearer.

Since you'll send a v2 for the loop check, might as well do this as well
if you don't mind.


> > > @@ -473,6 +482,11 @@ static int xen_9pfs_front_init(struct xenbus_device *dev)
> > >  		goto error;
> > >  	}
> > >  
> > > +	write_lock(&xen_9pfs_lock);
> > > +	dev_set_drvdata(&dev->dev, priv);
> > 
> > Honest question: could xen_9pfs_front_init() also be called multiple
> > times as well?
> > (if so this should check for the previous drvdata and free the priv
> > that was just built if it was non-null)
> > 
> > Please ignore this if you think that can't happen, I really don't
> > know.
> 
> That should not be possible before freeing priv first:
> xen_9pfs_front_init is only called when the frontend is in the
> XenbusStateInitialising state (see xen_9pfs_front_changed). Once we
> store priv we immediately switch the state to XenbusStateInitialised,
> and there will be no more calls to xen_9pfs_front_init. If the backend
> forces a re-probe, xenbus invokes xen_9pfs_front_remove first, which
> frees priv.

Ok, this makes sense to me.
I don't have any setup to test xen so trusting you here, but this looks
sane enough so will apply v2 when you send it

-- 
Dominique Martinet | Asmadeus