[PATCH] 9p/xen: protect xen_9pfs_front_free against concurrent calls

[PATCH] 9p/xen: protect xen_9pfs_front_free against concurrent calls

[Stefano Stabellini]

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.

This is a fix for the following double-free:

[   27.052347] Oops: general protection fault, probably for non-canonical address 0x6b6b6b6b6b6b6b6b: 0000 [#1] SMP DEBUG_PAGEALLOC NOPTI
[   27.052357] CPU: 0 UID: 0 PID: 32 Comm: xenwatch Not tainted 6.18.0-02087-g51ab33fc0a8b-dirty #60 PREEMPT(none)
[   27.052363] RIP: e030:xen_9pfs_front_free+0x1d/0x150
[   27.052368] Code: 90 90 90 90 90 90 90 90 90 90 90 90 90 41 55 41 54 55 48 89 fd 48 c7 c7 48 d0 92 85 53 e8 cb cb 05 00 48 8b 45 08 48 8b 55 00 <48> 3b 28 0f 85 f9 28 35 fe 48 3b 6a 08 0f 85 ef 28 35 fe 48 89 42
[   27.052377] RSP: e02b:ffffc9004016fdd0 EFLAGS: 00010246
[   27.052381] RAX: 6b6b6b6b6b6b6b6b RBX: ffff88800d66e400 RCX: 0000000000000000
[   27.052385] RDX: 6b6b6b6b6b6b6b6b RSI: 0000000000000000 RDI: 0000000000000000
[   27.052389] RBP: ffff88800a887040 R08: 0000000000000000 R09: 0000000000000000
[   27.052393] R10: 0000000000000000 R11: 0000000000000000 R12: ffff888009e46b68
[   27.052397] R13: 0000000000000200 R14: 0000000000000000 R15: ffff88800a887040
[   27.052404] FS:  0000000000000000(0000) GS:ffff88808ca57000(0000) knlGS:0000000000000000
[   27.052408] CS:  e030 DS: 0000 ES: 0000 CR0: 0000000080050033
[   27.052412] CR2: 00007f9714004360 CR3: 0000000004834000 CR4: 0000000000050660
[   27.052418] Call Trace:
[   27.052420]  <TASK>
[   27.052422]  xen_9pfs_front_changed+0x5d5/0x720
[   27.052426]  ? xenbus_otherend_changed+0x72/0x140
[   27.052430]  ? __pfx_xenwatch_thread+0x10/0x10
[   27.052434]  xenwatch_thread+0x94/0x1c0
[   27.052438]  ? __pfx_autoremove_wake_function+0x10/0x10
[   27.052442]  kthread+0xf8/0x240
[   27.052445]  ? __pfx_kthread+0x10/0x10
[   27.052449]  ? __pfx_kthread+0x10/0x10
[   27.052452]  ret_from_fork+0x16b/0x1a0
[   27.052456]  ? __pfx_kthread+0x10/0x10
[   27.052459]  ret_from_fork_asm+0x1a/0x30
[   27.052463]  </TASK>
[   27.052465] Modules linked in:
[   27.052471] ---[ end trace 0000000000000000 ]---

Signed-off-by: Stefano Stabellini <stefano.stabellini@amd.com>
---
 net/9p/trans_xen.c | 41 +++++++++++++++++++++--------------------
 1 file changed, 21 insertions(+), 20 deletions(-)

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++) {
 		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);
+	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);
+	list_add_tail(&priv->list, &xen_9pfs_devs);
+	write_unlock(&xen_9pfs_lock);
+
 	xenbus_switch_state(dev, XenbusStateInitialised);
 	return 0;
 
@@ -487,19 +501,6 @@ static int xen_9pfs_front_init(struct xenbus_device *dev)
 static int xen_9pfs_front_probe(struct xenbus_device *dev,
 				const struct xenbus_device_id *id)
 {
-	struct xen_9pfs_front_priv *priv = NULL;
-
-	priv = kzalloc(sizeof(*priv), GFP_KERNEL);
-	if (!priv)
-		return -ENOMEM;
-
-	priv->dev = dev;
-	dev_set_drvdata(&dev->dev, priv);
-
-	write_lock(&xen_9pfs_lock);
-	list_add_tail(&priv->list, &xen_9pfs_devs);
-	write_unlock(&xen_9pfs_lock);
-
 	return 0;
 }
 
-- 
2.25.1

Re: [PATCH] 9p/xen: protect xen_9pfs_front_free against concurrent calls

[Dominique Martinet]

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

Re: [PATCH] 9p/xen: protect xen_9pfs_front_free against concurrent calls

[Stefano Stabellini]

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.

Re: [PATCH] 9p/xen: protect xen_9pfs_front_free against concurrent calls

[Dominique Martinet]

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