[PATCH v3 0/2] 9p/trans_xen: fixes and parser cleanup

[PATCH v3 0/2] 9p/trans_xen: fixes and parser cleanup

[Eric-Terminal]

From: Yufan Chen <ericterminal@gmail.com>

Hi,

This series provides a stability fix and a parser modernization for the
9p Xen transport. It has been split from the previous mixed series
following feedback from Dominique Martinet to ease review and testing.

Patch 1 fixes a potential double-free/Oops during initialization failure
by making the dataring cleanup idempotent.

Patch 2 replaces deprecated simple_strtoul() with kstrtouint() in the
backend version parsing logic, ensuring stricter input validation.

Testing:
- Verified the series using virtme-ng.
- Patch 1: Tested error paths by forcing init failures on non-Xen systems;
  dmesg confirms the new sentinel-based cleanup correctly prevents Oops.
- Patch 2: Verified with a userspace mock harness that it correctly
  handles empty tokens (,,) and rejects malformed input (1abc).

v3:
- Split into a dedicated 9p/trans_xen series for v9fs.
- No functional changes since v2.

Yufan Chen (2):
  9p/trans_xen: make cleanup idempotent after dataring alloc errors
  9p/trans_xen: replace simple_strto* with kstrtouint

 net/9p/trans_xen.c | 77 +++++++++++++++++++++++++++++++++++-------------------
 1 file changed, 50 insertions(+), 27 deletions(-)

-- 
2.47.3

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

[Eric-Terminal]

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>
---
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

[PATCH v3 2/2] 9p/trans_xen: replace simple_strto* with kstrtouint

[Eric-Terminal]

From: Yufan Chen <ericterminal@gmail.com>

In xen_9pfs_front_init(), parse the backend version list as comma-separated
tokens with kstrtouint(), keep strict token validation, and explicitly
require protocol version 1 to be present.

This replaces the deprecated simple_strtoul(), improves error reporting
consistency, and avoids partially parsed values in control paths.

Signed-off-by: Yufan Chen <ericterminal@gmail.com>
---
v3:
- Split from mixed series into a dedicated 9p/trans_xen series.
- Refined commit message to focus on 9p changes.
- No functional changes since v2.

 net/9p/trans_xen.c | 26 ++++++++++++++++----------
 1 file changed, 16 insertions(+), 10 deletions(-)

diff --git a/net/9p/trans_xen.c b/net/9p/trans_xen.c
index 85b9ebfaa..f9fb2db7a 100644
--- a/net/9p/trans_xen.c
+++ b/net/9p/trans_xen.c
@@ -413,23 +413,29 @@ static int xen_9pfs_front_init(struct xenbus_device *dev)
 	int ret, i;
 	struct xenbus_transaction xbt;
 	struct xen_9pfs_front_priv *priv;
-	char *versions, *v;
-	unsigned int max_rings, max_ring_order, len = 0;
+	char *versions, *v, *token;
+	bool version_1 = false;
+	unsigned int max_rings, max_ring_order, len = 0, version;
 
 	versions = xenbus_read(XBT_NIL, dev->otherend, "versions", &len);
 	if (IS_ERR(versions))
 		return PTR_ERR(versions);
-	for (v = versions; *v; v++) {
-		if (simple_strtoul(v, &v, 10) == 1) {
-			v = NULL;
-			break;
+	for (v = versions; (token = strsep(&v, ",")); ) {
+		if (!*token)
+			continue;
+
+		ret = kstrtouint(token, 10, &version);
+		if (ret) {
+			kfree(versions);
+			return ret;
 		}
-	}
-	if (v) {
-		kfree(versions);
-		return -EINVAL;
+		if (version == 1)
+			version_1 = true;
 	}
 	kfree(versions);
+	if (!version_1)
+		return -EINVAL;
+
 	max_rings = xenbus_read_unsigned(dev->otherend, "max-rings", 0);
 	if (max_rings < XEN_9PFS_NUM_RINGS)

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

[Stefano Stabellini]

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
> 

Re: [PATCH v3 2/2] 9p/trans_xen: replace simple_strto* with kstrtouint

[Stefano Stabellini]

On Tue, 24 Mar 2026, Eric-Terminal wrote:
> From: Yufan Chen <ericterminal@gmail.com>
> 
> In xen_9pfs_front_init(), parse the backend version list as comma-separated
> tokens with kstrtouint(), keep strict token validation, and explicitly
> require protocol version 1 to be present.
> 
> This replaces the deprecated simple_strtoul(), improves error reporting
> consistency, and avoids partially parsed values in control paths.
> 
> Signed-off-by: Yufan Chen <ericterminal@gmail.com>

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