Re: [PATCH 5/5] 9p/virtio: keep zero-copy buffers until the device returns them

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider.

Critical: 0 · High: 1 · Medium: 0 · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] The patch adds an unconditional `wait_event_idle(req->wq,
  !smp_load_acquire(&req->zc_pages))` to p9_client_zc_rpc()…

--

9p/virtio: keep zero-copy buffers until the device returns them

This patch fixes a use-after-free. If a fatal signal ends the wait in
p9_virtio_zc_request() while the device still holds the request, a
misbehaving device can later write into zero-copy buffers that have
already been freed. Freeing of the pinned pages moves to req_done() via
a per-request struct p9_zc_pages, and p9_client_zc_rpc() now waits until
the device returns the request.

> diff --git a/net/9p/client.c b/net/9p/client.c
> index 15bb939087165..34be0d2652d4e 100644
> --- a/net/9p/client.c
> +++ b/net/9p/client.c

[ ... ]

> @@ -707,6 +708,8 @@ static struct p9_req_t *p9_client_zc_rpc(struct p9_client *c, int8_t type,
>  		 * into rc only while it waits.
>  		 */
>  	}
> +	/* The caller may reuse the payload once the transport releases it */
> +	wait_event_idle(req->wq, !smp_load_acquire(&req->zc_pages));

[Severity: High]

Can this wait leave a killed task stuck forever?

wait_event_idle() sleeps in TASK_IDLE and has no timeout, so SIGKILL
cannot end it. khungtaskd also skips TASK_NOLOAD tasks, so the hang
would never be reported.

The only code that clears req->zc_pages and wakes req->wq is in
req_done(). It runs only after virtqueue_get_buf() returns this exact
request:

net/9p/trans_virtio.c:req_done() {
    ...
	while ((req = virtqueue_get_buf(chan->vq, &len)) != NULL) {
		...
		if (req->zc_pages) {
			if (req->zc_pages->held)
				p9_zc_release(req->zc_pages);
			smp_store_release(&req->zc_pages, NULL);
			wake_up(&req->wq);
		}
    ...
}

On a fatal signal, p9_virtio_zc_request() hands the pages to req_done()
and returns with req->zc_pages still set:

		held = req->zc_pages;
		if (held)
			zc->held = true;
		spin_unlock_irqrestore(&chan->lock, flags);
		if (held)
			return err;

p9_client_zc_rpc() sends a TFLUSH only when c->status == Connected.
There are two cases where no flush is sent but the new wait still runs:

  - after umount -f, via
    v9fs_session_begin_cancel()->p9_client_begin_disconnect()
  - after an earlier -EIO has set Disconnected

If a flush is sent, p9_client_rpc(TFLUSH) can itself return early on the
fatal signal. In every case the wait above runs.

If the device never returns the original descriptor, nothing else clears
zc_pages. That can happen in several ways:

  - virtqueue_kick() fails and sets vq->broken. Its return value is
    ignored right after req->zc_pages = zc in p9_virtio_zc_request().
  - surprise PCI removal. virtio_pci_remove() calls
    virtio_break_device(), and vring_interrupt() then stops calling the
    callback.
  - a hung backend, or a device that answers the TFLUSH but keeps the
    original descriptor. This is the misbehaving-device case the commit
    message describes.

Before this patch, a killed task always left this path.

Once stuck, the task keeps any VFS locks its caller holds and cannot
finish exiting or coredumping. Its pinned pages stay counted in
vp_pinned, which throttles p9_get_mapped_pages() on other mounts. It
also keeps the mount busy, so p9_virtio_close() never runs and
chan->inuse stays true. p9_virtio_remove() then loops here and never
reaches virtio_reset_device()/del_vqs():

	while (chan->inuse) {
		mutex_unlock(&virtio_9p_lock);
		msleep(250);

That is a circular wait. Remove waits for umount, umount waits for the
task, and the task waits for a req_done() that never runs for this
request.

For pinned user pages (need_drop == 1), the zc->held hand-off already
keeps the pages alive until req_done(), so the unconditional wait seems
to be needed only for kvec buffers. Could the wait be limited to that
case, or bounded or made killable? Alternatively, could the transport
clear zc_pages and wake waiters when the vq is broken or torn down?

The commit message also does not mention that killed tasks can now
block on the device with no time limit. Should it?

>  recalc_sigpending:
>  	if (sigpending) {
>  		spin_lock_irqsave(&current->sighand->siglock, flags);

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261109105251.2545764-1-lian%40is.s.u-tokyo.ac.jp