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
