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.
> 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?
> + 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.
Thanks,
--
Dominique Martinet | Asmadeus
