[PATCH 0/2] xen/pvcalls: fix the backend error path and a workqueue leak

[PATCH 0/2] xen/pvcalls: fix the backend error path and a workqueue leak

[Yehyeong Lee]

1/2: every error path in pvcalls_new_active_socket() runs list_del() on a
{NULL, NULL} list_head, holding socket_lock.  2/2: the per-socket workqueue
is never destroyed.  A frontend reaches 1/2 with one bad ring_order and 2/2
by connecting and releasing sockets in a loop.  Nested-Xen lab: 5 of 5
cells oops as shipped and 0 of 5 patched, a normal connect moves the same
bytes both ways on both arms, and backend/pvcalls/<domid>/0/state after
teardown reads 4 before and 6 after.  Reported to the Xen security team
first; PVCalls is not security supported, so posting here as suggested.

Yehyeong Lee (2):
  xen/pvcalls: unwind pvcalls_new_active_socket() step by step
  xen/pvcalls: destroy the per-socket workqueue on release

 drivers/xen/pvcalls-back.c | 26 ++++++++++++++++----------
 1 file changed, 16 insertions(+), 10 deletions(-)


base-commit: 7475ea8b58616b33c04afb976cf0fbaad9dd63fb
-- 
2.43.0

[PATCH 1/2] xen/pvcalls: unwind pvcalls_new_active_socket() step by step

[Yehyeong Lee]

Every error path in pvcalls_new_active_socket() jumps to one shared label
that does list_del(&map->list) and then hands map to
pvcalls_back_release_active().  But map comes from kzalloc() and its
list_head is only published at the very end of the function, after the last
of those jumps:

	down(&fedata->socket_lock);
	list_add_tail(&map->list, &fedata->socket_mappings);
	up(&fedata->socket_lock);

so on the error path map->list is still {NULL, NULL} and __list_del()
writes through a NULL pointer.

A frontend gets there with one value it owns: the ring_order field of the
page it granted.  Anything above MAX_RING_ORDER takes the second goto, and
nothing else in the function has run by then.

The result is an oops in whichever context handled the request - the
pvcalls-back threaded IRQ handler for PVCALLS_CONNECT, mappass->wq for
PVCALLS_ACCEPT - and because it happens after down(&fedata->socket_lock)
that semaphore is never released, so every later operation on the frontend
blocks on it and the backend never completes backend_disconnect().  (With
CONFIG_DEBUG_LIST the list_del() only reports and returns, and
pvcalls_back_release_active() then runs on a map whose irq is still 0 and
whose bytes, ioworker.wq and saved_data_ready are all NULL.)

A frontend asking for ring_order 5, on a dom0 built with KASAN
(the request was issued by a test-only change to the frontend):

[  133.728877] pvcalls_new_active_socket frontend requested ring_order 5, which is > MAX (4)
[  133.732868] Oops: general protection fault, probably for non-canonical address 0xdffffc0000000001: 0000 [#1] SMP KASAN NOPTI
[  133.737910] KASAN: null-ptr-deref in range [0x0000000000000008-0x000000000000000f]
[  133.741666] CPU: 0 UID: 0 PID: 133 Comm: irq/39-pvcalls- Not tainted 7.3.0-kCTLblk2-15482-g21a7f44fd076-dirty #1 PREEMPT(full)
[  133.746986] Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 1.16.3-debian-1.16.3-2 04/01/2014
[  133.751512] RIP: 0010:pvcalls_new_active_socket+0x821/0xb90
[  133.754237] Code: 48 89 fa 48 c1 ea 03 80 3c 02 00 0f 85 f6 01 00 00 48 8d 79 08 4c 8b 6b 08 48 b8 00 00 00 00 00 fc ff df 48 89 fa 48 c1 ea 03 <80> 3c 02 00 0f 85 14 02 00 00 4c 89 ea 4c 89 69 08 48 b8 00 00 00
[  133.763126] RSP: 0018:ffffc900004afb38 EFLAGS: 00010202
[  133.765861] RAX: dffffc0000000000 RBX: ffff88810886c900 RCX: 0000000000000000
[  133.769348] RDX: 0000000000000001 RSI: 0000000000000246 RDI: 0000000000000008
[  133.772787] RBP: 1ffff92000095f6b R08: 0000000000000001 R09: fffff52000095f58
[  133.776260] R10: 0000000000000003 R11: ffff888100077320 R12: ffff88810858b410
[  133.779823] R13: 0000000000000000 R14: ffff88810858b460 R15: ffff88810858b400
[  133.783382] FS:  0000000000000000(0000) GS:ffff8881b034c000(0000) knlGS:0000000000000000
[  133.787351] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[  133.790308] CR2: 00000000006042c0 CR3: 0000000108400001 CR4: 0000000000770ef0
[  133.793822] PKRU: 55555554
[  133.795347] Call Trace:
[  133.796778]  <TASK>
[  133.798082]  ? __pfx_pvcalls_new_active_socket+0x10/0x10
[  133.800801]  ? security_socket_post_create+0x4d/0x60
[  133.803275]  ? release_sock+0x1a/0x1d0
[  133.805182]  pvcalls_back_event+0x1b09/0x23e0
[  133.807431]  ? __pfx_pvcalls_back_event+0x10/0x10
[  133.809814]  ? __pfx__raw_spin_lock_irqsave+0x10/0x10
[  133.812251]  ? __pfx_pvcalls_back_event+0x10/0x10
[  133.814554]  irq_thread_fn+0x86/0x150
[  133.816468]  irq_thread+0x2da/0x580
[  133.818363]  ? __pfx_irq_thread_fn+0x10/0x10
[  133.820586]  ? __pfx_irq_thread+0x10/0x10
[  133.822670]  ? __kthread_parkme+0x88/0x170
[  133.824803]  ? __pfx_irq_thread+0x10/0x10
[  133.827153]  ? __pfx_irq_thread+0x10/0x10
[  133.829304]  kthread+0x2c8/0x3b0
[  133.831176]  ? recalc_sigpending+0x15c/0x1e0
[  133.833570]  ? __pfx_kthread+0x10/0x10
[  133.835572]  ret_from_fork+0x373/0x5b0
[  133.837581]  ? __pfx_ret_from_fork+0x10/0x10
[  133.839978]  ? __switch_to+0x3c4/0xb40
[  133.841984]  ? __pfx_kthread+0x10/0x10
[  133.843970]  ret_from_fork_asm+0x1a/0x30
[  133.846031]  </TASK>
[  133.847420] Modules linked in:
[  133.849240] ---[ end trace 0000000000000000 ]---

Give each failure its own label, so that only resources that were actually
acquired are released and the list the map was never added to is left
alone.  The map is not published anywhere at that point, so the error path
needs no socket_lock at all.  sock_release() stays in the callee on every
error path, the way
commit 8fafac202d18 ("xen/pvcalls-back: fix double frees with pvcalls_new_active_socket()")
arranged it.

Fixes: 5db4d286a8ef ("xen/pvcalls: implement connect command")
Cc: stable@vger.kernel.org
Signed-off-by: Yehyeong Lee <yhlee@isslab.korea.ac.kr>
---
 drivers/xen/pvcalls-back.c | 25 +++++++++++++++----------
 1 file changed, 15 insertions(+), 10 deletions(-)

diff --git a/drivers/xen/pvcalls-back.c b/drivers/xen/pvcalls-back.c
index ba62651a7f338b..dcefa91a40cd05 100644
--- a/drivers/xen/pvcalls-back.c
+++ b/drivers/xen/pvcalls-back.c
@@ -337,7 +337,7 @@ static struct sock_mapping *pvcalls_new_active_socket(
 
 	ret = xenbus_map_ring_valloc(fedata->dev, &ref, 1, &page);
 	if (ret < 0)
-		goto out;
+		goto out_map;
 	map->ring = page;
 	map->ring_order = map->ring->ring_order;
 	/* first read the order, then map the data ring */
@@ -345,19 +345,19 @@ static struct sock_mapping *pvcalls_new_active_socket(
 	if (map->ring_order > MAX_RING_ORDER) {
 		pr_warn("%s frontend requested ring_order %u, which is > MAX (%u)\n",
 				__func__, map->ring_order, MAX_RING_ORDER);
-		goto out;
+		goto out_unmap_ring;
 	}
 	ret = xenbus_map_ring_valloc(fedata->dev, map->ring->ref,
 				     (1 << map->ring_order), &page);
 	if (ret < 0)
-		goto out;
+		goto out_unmap_ring;
 	map->bytes = page;
 
 	ret = bind_interdomain_evtchn_to_irqhandler_lateeoi(
 			fedata->dev, evtchn,
 			pvcalls_back_conn_event, 0, "pvcalls-backend", map);
 	if (ret < 0)
-		goto out;
+		goto out_unmap_bytes;
 	map->irq = ret;
 
 	map->data.in = map->bytes;
@@ -365,7 +365,7 @@ static struct sock_mapping *pvcalls_new_active_socket(
 
 	map->ioworker.wq = alloc_ordered_workqueue("pvcalls_io", 0);
 	if (!map->ioworker.wq)
-		goto out;
+		goto out_unbind;
 	atomic_set(&map->io, 1);
 	INIT_WORK(&map->ioworker.register_work,	pvcalls_back_ioworker);
 
@@ -381,11 +381,16 @@ static struct sock_mapping *pvcalls_new_active_socket(
 	write_unlock_bh(&map->sock->sk->sk_callback_lock);
 
 	return map;
-out:
-	down(&fedata->socket_lock);
-	list_del(&map->list);
-	pvcalls_back_release_active(fedata->dev, fedata, map);
-	up(&fedata->socket_lock);
+
+out_unbind:
+	unbind_from_irqhandler(map->irq, map);
+out_unmap_bytes:
+	xenbus_unmap_ring_vfree(fedata->dev, map->bytes);
+out_unmap_ring:
+	xenbus_unmap_ring_vfree(fedata->dev, (void *)map->ring);
+out_map:
+	sock_release(sock);
+	kfree(map);
 	return NULL;
 }
 
-- 
2.43.0

[PATCH 2/2] xen/pvcalls: destroy the per-socket workqueue on release

[Yehyeong Lee]

pvcalls_new_active_socket() gives every active socket its own ordered
workqueue:

	map->ioworker.wq = alloc_ordered_workqueue("pvcalls_io", 0);

and nothing ever destroys it.  pvcalls_back_release_active() flushes
map->ioworker.register_work and frees map, but the workqueue and its
pool_workqueues stay allocated, so a frontend leaks a workqueue per socket
it connects and releases - a loop a guest can run as long as it likes.

Destroy it after the IRQ is unbound, so no new work can be queued, and
before map is freed.  pvcalls_back_ioworker() takes no locks, in
particular not fedata->socket_lock, which the backend_disconnect() caller
holds here, so draining it from this point is safe; the flush_work() just
above already has the same requirement.

Fixes: 5db4d286a8ef ("xen/pvcalls: implement connect command")
Cc: stable@vger.kernel.org
Signed-off-by: Yehyeong Lee <yhlee@isslab.korea.ac.kr>
---
 drivers/xen/pvcalls-back.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/xen/pvcalls-back.c b/drivers/xen/pvcalls-back.c
index dcefa91a40cd05..27aa06289a7b92 100644
--- a/drivers/xen/pvcalls-back.c
+++ b/drivers/xen/pvcalls-back.c
@@ -456,6 +456,7 @@ static int pvcalls_back_release_active(struct xenbus_device *dev,
 	xenbus_unmap_ring_vfree(dev, map->bytes);
 	xenbus_unmap_ring_vfree(dev, (void *)map->ring);
 	unbind_from_irqhandler(map->irq, map);
+	destroy_workqueue(map->ioworker.wq);
 
 	sock_release(map->sock);
 	kfree(map);
-- 
2.43.0

Re: [PATCH 1/2] xen/pvcalls: unwind pvcalls_new_active_socket() step by step

[Stefano Stabellini]

On Mon, 9 Nov 2026, Yehyeong Lee wrote:
> Every error path in pvcalls_new_active_socket() jumps to one shared label
> that does list_del(&map->list) and then hands map to
> pvcalls_back_release_active().  But map comes from kzalloc() and its
> list_head is only published at the very end of the function, after the last
> of those jumps:
> 
> 	down(&fedata->socket_lock);
> 	list_add_tail(&map->list, &fedata->socket_mappings);
> 	up(&fedata->socket_lock);
> 
> so on the error path map->list is still {NULL, NULL} and __list_del()
> writes through a NULL pointer.
> 
> A frontend gets there with one value it owns: the ring_order field of the
> page it granted.  Anything above MAX_RING_ORDER takes the second goto, and
> nothing else in the function has run by then.
> 
> The result is an oops in whichever context handled the request - the
> pvcalls-back threaded IRQ handler for PVCALLS_CONNECT, mappass->wq for
> PVCALLS_ACCEPT - and because it happens after down(&fedata->socket_lock)
> that semaphore is never released, so every later operation on the frontend
> blocks on it and the backend never completes backend_disconnect().  (With
> CONFIG_DEBUG_LIST the list_del() only reports and returns, and
> pvcalls_back_release_active() then runs on a map whose irq is still 0 and
> whose bytes, ioworker.wq and saved_data_ready are all NULL.)
> 
> A frontend asking for ring_order 5, on a dom0 built with KASAN
> (the request was issued by a test-only change to the frontend):
> 
> [  133.728877] pvcalls_new_active_socket frontend requested ring_order 5, which is > MAX (4)
> [  133.732868] Oops: general protection fault, probably for non-canonical address 0xdffffc0000000001: 0000 [#1] SMP KASAN NOPTI
> [  133.737910] KASAN: null-ptr-deref in range [0x0000000000000008-0x000000000000000f]
> [  133.741666] CPU: 0 UID: 0 PID: 133 Comm: irq/39-pvcalls- Not tainted 7.3.0-kCTLblk2-15482-g21a7f44fd076-dirty #1 PREEMPT(full)
> [  133.746986] Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 1.16.3-debian-1.16.3-2 04/01/2014
> [  133.751512] RIP: 0010:pvcalls_new_active_socket+0x821/0xb90
> [  133.754237] Code: 48 89 fa 48 c1 ea 03 80 3c 02 00 0f 85 f6 01 00 00 48 8d 79 08 4c 8b 6b 08 48 b8 00 00 00 00 00 fc ff df 48 89 fa 48 c1 ea 03 <80> 3c 02 00 0f 85 14 02 00 00 4c 89 ea 4c 89 69 08 48 b8 00 00 00
> [  133.763126] RSP: 0018:ffffc900004afb38 EFLAGS: 00010202
> [  133.765861] RAX: dffffc0000000000 RBX: ffff88810886c900 RCX: 0000000000000000
> [  133.769348] RDX: 0000000000000001 RSI: 0000000000000246 RDI: 0000000000000008
> [  133.772787] RBP: 1ffff92000095f6b R08: 0000000000000001 R09: fffff52000095f58
> [  133.776260] R10: 0000000000000003 R11: ffff888100077320 R12: ffff88810858b410
> [  133.779823] R13: 0000000000000000 R14: ffff88810858b460 R15: ffff88810858b400
> [  133.783382] FS:  0000000000000000(0000) GS:ffff8881b034c000(0000) knlGS:0000000000000000
> [  133.787351] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [  133.790308] CR2: 00000000006042c0 CR3: 0000000108400001 CR4: 0000000000770ef0
> [  133.793822] PKRU: 55555554
> [  133.795347] Call Trace:
> [  133.796778]  <TASK>
> [  133.798082]  ? __pfx_pvcalls_new_active_socket+0x10/0x10
> [  133.800801]  ? security_socket_post_create+0x4d/0x60
> [  133.803275]  ? release_sock+0x1a/0x1d0
> [  133.805182]  pvcalls_back_event+0x1b09/0x23e0
> [  133.807431]  ? __pfx_pvcalls_back_event+0x10/0x10
> [  133.809814]  ? __pfx__raw_spin_lock_irqsave+0x10/0x10
> [  133.812251]  ? __pfx_pvcalls_back_event+0x10/0x10
> [  133.814554]  irq_thread_fn+0x86/0x150
> [  133.816468]  irq_thread+0x2da/0x580
> [  133.818363]  ? __pfx_irq_thread_fn+0x10/0x10
> [  133.820586]  ? __pfx_irq_thread+0x10/0x10
> [  133.822670]  ? __kthread_parkme+0x88/0x170
> [  133.824803]  ? __pfx_irq_thread+0x10/0x10
> [  133.827153]  ? __pfx_irq_thread+0x10/0x10
> [  133.829304]  kthread+0x2c8/0x3b0
> [  133.831176]  ? recalc_sigpending+0x15c/0x1e0
> [  133.833570]  ? __pfx_kthread+0x10/0x10
> [  133.835572]  ret_from_fork+0x373/0x5b0
> [  133.837581]  ? __pfx_ret_from_fork+0x10/0x10
> [  133.839978]  ? __switch_to+0x3c4/0xb40
> [  133.841984]  ? __pfx_kthread+0x10/0x10
> [  133.843970]  ret_from_fork_asm+0x1a/0x30
> [  133.846031]  </TASK>
> [  133.847420] Modules linked in:
> [  133.849240] ---[ end trace 0000000000000000 ]---
> 
> Give each failure its own label, so that only resources that were actually
> acquired are released and the list the map was never added to is left
> alone.  The map is not published anywhere at that point, so the error path
> needs no socket_lock at all.  sock_release() stays in the callee on every
> error path, the way
> commit 8fafac202d18 ("xen/pvcalls-back: fix double frees with pvcalls_new_active_socket()")
> arranged it.
> 
> Fixes: 5db4d286a8ef ("xen/pvcalls: implement connect command")
> Cc: stable@vger.kernel.org
> Signed-off-by: Yehyeong Lee <yhlee@isslab.korea.ac.kr>

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

Re: [PATCH 2/2] xen/pvcalls: destroy the per-socket workqueue on release

[Stefano Stabellini]

On Mon, 9 Nov 2026, Yehyeong Lee wrote:
> pvcalls_new_active_socket() gives every active socket its own ordered
> workqueue:
> 
> 	map->ioworker.wq = alloc_ordered_workqueue("pvcalls_io", 0);
> 
> and nothing ever destroys it.  pvcalls_back_release_active() flushes
> map->ioworker.register_work and frees map, but the workqueue and its
> pool_workqueues stay allocated, so a frontend leaks a workqueue per socket
> it connects and releases - a loop a guest can run as long as it likes.
> 
> Destroy it after the IRQ is unbound, so no new work can be queued, and
> before map is freed.  pvcalls_back_ioworker() takes no locks, in
> particular not fedata->socket_lock, which the backend_disconnect() caller
> holds here, so draining it from this point is safe; the flush_work() just
> above already has the same requirement.
> 
> Fixes: 5db4d286a8ef ("xen/pvcalls: implement connect command")
> Cc: stable@vger.kernel.org
> Signed-off-by: Yehyeong Lee <yhlee@isslab.korea.ac.kr>
> ---
>  drivers/xen/pvcalls-back.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/xen/pvcalls-back.c b/drivers/xen/pvcalls-back.c
> index dcefa91a40cd05..27aa06289a7b92 100644
> --- a/drivers/xen/pvcalls-back.c
> +++ b/drivers/xen/pvcalls-back.c
> @@ -456,6 +456,7 @@ static int pvcalls_back_release_active(struct xenbus_device *dev,
>  	xenbus_unmap_ring_vfree(dev, map->bytes);
>  	xenbus_unmap_ring_vfree(dev, (void *)map->ring);
>  	unbind_from_irqhandler(map->irq, map);
> +	destroy_workqueue(map->ioworker.wq);

Please move the call to destroy_workqueue() just after flush_work().
With that:

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


>  	sock_release(map->sock);
>  	kfree(map);
> -- 
> 2.43.0
>