On Sat, 24 Jan 2026, Dominique Martinet wrote:
> Stefano Stabellini wrote on Fri, Jan 23, 2026 at 10:40:09AM -0800:
> > The xenwatch thread can race with other back-end change notifications
> > and call xen_9pfs_front_free() twice, hitting the observed general
> > protection fault due to a double-free. Guard the teardown path so only
> > one caller can release the front-end state at a time, preventing the
> > crash.
>
> Thank you, just a couple of nitpicks below
>
> > diff --git a/net/9p/trans_xen.c b/net/9p/trans_xen.c
> > index 280f686f60fbb..8809f3c06ec95 100644
> > --- a/net/9p/trans_xen.c
> > +++ b/net/9p/trans_xen.c
> > @@ -274,11 +274,7 @@ static void xen_9pfs_front_free(struct
> > xen_9pfs_front_priv *priv)
> > {
> > int i, j;
> >
> > - write_lock(&xen_9pfs_lock);
> > - list_del(&priv->list);
> > - write_unlock(&xen_9pfs_lock);
> > -
> > - for (i = 0; i < XEN_9PFS_NUM_RINGS; i++) {
> > + for (i = 0; priv->rings != NULL && i < XEN_9PFS_NUM_RINGS; i++) {
>
>
> 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.
>
> > struct xen_9pfs_dataring *ring = &priv->rings[i];
> >
> > cancel_work_sync(&ring->work);
> > @@ -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.
> > + write_unlock(&xen_9pfs_lock);
> > +
> > xen_9pfs_front_free(priv);
> > }
> >
> > @@ -387,7 +392,7 @@ static int xen_9pfs_front_init(struct xenbus_device
> > *dev)
> > {
> > int ret, i;
> > struct xenbus_transaction xbt;
> > - struct xen_9pfs_front_priv *priv = dev_get_drvdata(&dev->dev);
> > + struct xen_9pfs_front_priv *priv;
> > char *versions, *v;
> > unsigned int max_rings, max_ring_order, len = 0;
> >
> > @@ -415,6 +420,10 @@ static int xen_9pfs_front_init(struct xenbus_device
> > *dev)
> > if (p9_xen_trans.maxsize > XEN_FLEX_RING_SIZE(max_ring_order))
> > p9_xen_trans.maxsize = XEN_FLEX_RING_SIZE(max_ring_order) / 2;
> >
> > + priv = kzalloc(sizeof(*priv), GFP_KERNEL);
> > + if (!priv)
> > + return -ENOMEM;
> > + priv->dev = dev;
> > priv->rings = kcalloc(XEN_9PFS_NUM_RINGS, sizeof(*priv->rings),
> > GFP_KERNEL);
> > if (!priv->rings) {
> > @@ -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.
