Re: [PATCH v3 1/2] 9p/trans_xen: make cleanup idempotent after dataring alloc errors

Stefano Stabellini
2026-04-09T01:22:05Z

On Tue, 24 Mar 2026, Eric-Terminal wrote:
> From: Yufan Chen <ericterminal@gmail.com>
>
> xen_9pfs_front_alloc_dataring() tears down resources on failure but
> leaves ring fields stale. If xen_9pfs_front_init() later jumps to the
> common error path, xen_9pfs_front_free() may touch the same resources
> again, causing duplicate/invalid gnttab_end_foreign_access() calls and
> potentially dereferencing a freed intf pointer.
>
> Initialize dataring sentinels before allocation, gate teardown on those
> sentinels, and clear ref/intf/data/irq immediately after each release.
>
> This keeps cleanup idempotent for partially initialized rings and
> prevents repeated teardown during init failure handling.
>
> Signed-off-by: Yufan Chen <ericterminal@gmail.com>

Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>

> ---
> v3:
> - Split from mixed series into a dedicated 9p/trans_xen series.
> - No functional changes since v2.
>
> net/9p/trans_xen.c | 51 +++++++++++++++++++++++++++++++++-------------
> diff --git a/net/9p/trans_xen.c b/net/9p/trans_xen.c
> index 47af5a10e..85b9ebfaa 100644
> --- a/net/9p/trans_xen.c
> +++ b/net/9p/trans_xen.c
> @@ -283,25 +283,33 @@ static void xen_9pfs_front_free(struct xen_9pfs_front_priv *priv)
>
> cancel_work_sync(&ring->work);
>
> - if (!priv->rings[i].intf)
> + if (!ring->intf)
> break;
> - if (priv->rings[i].irq > 0)
> - unbind_from_irqhandler(priv->rings[i].irq, ring);
> - if (priv->rings[i].data.in) {
> - for (j = 0;
> - j < (1 << priv->rings[i].intf->ring_order);
> + if (ring->irq >= 0) {
> + unbind_from_irqhandler(ring->irq, ring);
> + ring->irq = -1;
> + }
> + if (ring->data.in) {
> + for (j = 0; j < (1 << ring->intf->ring_order);
> j++) {
> grant_ref_t ref;
>
> - ref = priv->rings[i].intf->ref[j];
> + ref = ring->intf->ref[j];
> gnttab_end_foreign_access(ref, NULL);
> + ring->intf->ref[j] = INVALID_GRANT_REF;
> }
> - free_pages_exact(priv->rings[i].data.in,
> - 1UL << (priv->rings[i].intf->ring_order +
> - XEN_PAGE_SHIFT));
> + free_pages_exact(ring->data.in,
> + 1UL << (ring->intf->ring_order +
> + XEN_PAGE_SHIFT));
> + ring->data.in = NULL;
> + ring->data.out = NULL;
> + }
> + if (ring->ref != INVALID_GRANT_REF) {
> + gnttab_end_foreign_access(ring->ref, NULL);
> + ring->ref = INVALID_GRANT_REF;
> }
> - gnttab_end_foreign_access(priv->rings[i].ref, NULL);
> - free_page((unsigned long)priv->rings[i].intf);
> + free_page((unsigned long)ring->intf);
> + ring->intf = NULL;
> }
> kfree(priv->rings);
> }
> @@ -334,6 +342,12 @@ static int xen_9pfs_front_alloc_dataring(struct xenbus_device *dev,
> int ret = -ENOMEM;
> void *bytes = NULL;
>
> + ring->intf = NULL;
> + ring->data.in = NULL;
> + ring->data.out = NULL;
> + ring->ref = INVALID_GRANT_REF;
> + ring->irq = -1;
> +
> init_waitqueue_head(&ring->wq);
> spin_lock_init(&ring->lock);
> INIT_WORK(&ring->work, p9_xen_response);
> @@ -379,9 +393,18 @@ static int xen_9pfs_front_alloc_dataring(struct xenbus_device *dev,
> for (i--; i >= 0; i--)
> gnttab_end_foreign_access(ring->intf->ref[i], NULL);
> free_pages_exact(bytes, 1UL << (order + XEN_PAGE_SHIFT));
> + ring->data.in = NULL;
> + ring->data.out = NULL;
> + }
> + if (ring->ref != INVALID_GRANT_REF) {
> + gnttab_end_foreign_access(ring->ref, NULL);
> + ring->ref = INVALID_GRANT_REF;
> + }
> + if (ring->intf) {
> + free_page((unsigned long)ring->intf);
> + ring->intf = NULL;
> }
> - gnttab_end_foreign_access(ring->ref, NULL);
> - free_page((unsigned long)ring->intf);
> + ring->irq = -1;
> return ret;
> }
>
> --
> 2.47.3
>