Re: [PATCH v2 2/2] virtio_balloon: submit reported pages as individual buffers

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] virtio_balloon: infinite spin in virtballoon_free_page_report() on broken vq
- [Medium] virtio_balloon: callback toggle side effects in wait_event() condition
--

commit bced8a77808ca2850eb132d7f72c3d6fc5ae5935
Author: Michael S. Tsirkin <mst@redhat.com>

virtio_balloon: submit reported pages as individual buffers

This commit submits each reported free page as an individual virtqueue buffer
rather than a single scatter-gather chain to avoid exceeding the virtio 4 GiB
descriptor chain limit. To prevent interrupt storms from per-page buffers, it
disables reporting virtqueue callbacks at initialization and uses
virtqueue_enable_cb_delayed_max() while waiting for completion in
virtballoon_free_page_report().

> diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
> index ab3e3d887e00f..0a331acf2c8ef 100644
> --- a/drivers/virtio/virtio_balloon.c
> +++ b/drivers/virtio/virtio_balloon.c
> @@ -205,25 +205,53 @@ static int virtballoon_free_page_report(struct page_reporting_dev_info *pr_dev_i
>  	struct virtio_balloon *vb =
>  		container_of(pr_dev_info, struct virtio_balloon, pr_dev_info);
>  	struct virtqueue *vq = vb->reporting_vq;
> -	unsigned int unused, err;
> +	unsigned int i, err = 0;
>  
>  	/* We should always be able to add these buffers to an empty queue. */
> -	err = virtqueue_add_inbuf(vq, sg, nents, vb, GFP_NOWAIT);
> +	for (i = 0; i < nents; i++) {
> +		struct scatterlist one;
>  
> -	/*
> -	 * In the extremely unlikely case that something has occurred and we
> -	 * are able to trigger an error we will simply display a warning
> -	 * and exit without actually processing the pages.
> -	 */
> -	if (WARN_ON_ONCE(err))
> -		return err;
> +		sg_init_table(&one, 1);
> +		sg_set_page(&one, sg_page(&sg[i]), sg[i].length,
> +			    sg[i].offset);
> +		err = virtqueue_add_inbuf(vq, &one, 1, &sg[i], GFP_NOWAIT);
>  
> -	virtqueue_kick(vq);
> +		/*
> +		 * In the extremely unlikely case that something has occurred
> +		 * and we are able to trigger an error we will simply display
> +		 * a warning and exit without actually processing the pages.
> +		 */
> +		if (WARN_ON_ONCE(err)) {
> +			nents = i;
> +			break;
> +		}
> +	}
>  
> -	/* When host has read buffer, this completes via balloon_ack */
> -	wait_event(vb->acked, virtqueue_get_buf(vq, &unused));
> +	if (nents) {
> +		unsigned int unused;
>  
> -	return 0;
> +		virtqueue_kick(vq);
> +
> +		/* When host has read buffer, this completes via balloon_ack. */
> +		i = 0;
> +		wait_event(vb->acked, ({
> +			for (;;) {
> +				virtqueue_disable_cb(vq);
> +				while (virtqueue_get_buf(vq, &unused))
> +					i++;
> +
> +				/* Stop waiting if device used all that we added. */
> +				if (i >= nents)
> +					break;
> +
> +				if (virtqueue_enable_cb_delayed_max(vq))
> +					break;
> +			}
> +			i >= nents;
> +		}));

[Severity: High]
Can this inner for (;;) loop spin indefinitely in kernel workqueue context
when vq->broken becomes true before all nents buffers are drained?

In page_reporting_process() -> page_reporting_process_zone() ->
page_reporting_cycle() -> virtballoon_free_page_report(), if the device marks
buffers as used in the ring and vq->broken becomes true before all nents
buffers are consumed (either because virtqueue_get_buf() encounters a bad
descriptor and calls BAD_RING(), or because virtio_break_device() runs
concurrently during surprise removal or reset), virtqueue_get_buf() returns
NULL without advancing vq->last_used_idx or incrementing i.

Because i < nents, the loop then calls virtqueue_enable_cb_delayed_max(vq).
Unlike virtqueue_enable_cb_delayed(), which checks vq->broken:

drivers/virtio/virtio_ring.c:virtqueue_enable_cb_delayed() {
	struct vring_virtqueue *vq = to_vvq(_vq);

	if (vq->event_triggered)
		data_race(vq->event_triggered = false);

	if (unlikely(vq->broken))
		return true;
...
}

virtqueue_enable_cb_delayed_max() omits that check:

drivers/virtio/virtio_ring.c:virtqueue_enable_cb_delayed_max() {
	struct vring_virtqueue *vq = to_vvq(_vq);

	if (vq->event_triggered)
		data_race(vq->event_triggered = false);

	return VIRTQUEUE_CALL(vq, enable_cb_delayed_max);
}

When unconsumed used entries remain in the ring,
virtqueue_enable_cb_delayed_max_split() and
virtqueue_enable_cb_delayed_max_packed() both return false on every call:

drivers/virtio/virtio_ring.c:virtqueue_enable_cb_delayed_max_split() {
...
	if (unlikely((u16)(virtio16_to_cpu(vq->vq.vdev, vq->split.vring.used->idx)
						- vq->last_used_idx) > bufs)) {
		END_USE(vq);
		return false;
	}
	return true;
}

drivers/virtio/virtio_ring.c:virtqueue_enable_cb_delayed_max_packed() {
...
	if (is_used_desc_packed(vq, used_idx, wrap_counter)) {
		END_USE(vq);
		return false;
	}
	return true;
}

As a result, neither break condition in the for (;;) loop is ever met, and the
thread spins in a tight non-sleeping loop inside the wait_event() condition
rather than sleeping as noted in the commit message. Also, should
virtqueue_enable_cb_delayed_max_split() read used->idx via
vring_read_split_used_idx(vq) instead of a plain access?

[Severity: Medium]
Does placing virtqueue_disable_cb() and virtqueue_enable_cb_delayed_max()
inside the wait_event() condition cause unintended callback state changes when
wait_event() evaluates the condition multiple times?

In include/linux/wait.h, wait_event() evaluates the condition once before
calling __wait_event(), and ___wait_event() calls prepare_to_wait_event()
followed immediately by a second evaluation of the condition before calling
schedule():

include/linux/wait.h:wait_event() {
	might_sleep();
	if (condition)
		break;
	__wait_event(wq_head, condition);
}

When the host has not yet consumed all nents buffers on the first check,
virtqueue_enable_cb_delayed_max(vq) clears VRING_AVAIL_F_NO_INTERRUPT in
vq->split.avail_flags_shadow, sets vq->event_triggered = false, writes
vring_used_event = vq->last_used_idx + bufs, and issues virtio_store_mb().

Immediately afterwards, before sleeping in schedule(), the second condition
check in ___wait_event() runs virtqueue_disable_cb(vq):

drivers/virtio/virtio_ring.c:virtqueue_disable_cb_split() {
	if (!(vq->split.avail_flags_shadow & VRING_AVAIL_F_NO_INTERRUPT)) {
		vq->split.avail_flags_shadow |= VRING_AVAIL_F_NO_INTERRUPT;

		if (vq->event_triggered)
			return;

		if (vq->event)
			/* TODO: this is a hack. Figure out a cleaner value to write. */
			vring_used_event(&vq->split.vring) = 0x0;
...
}

Because the first check enabled callbacks and vq->event_triggered is still
false, virtqueue_disable_cb_split() writes vring_used_event = 0x0 into shared
ring memory while the kicked device is actively processing buffers, right
before virtqueue_enable_cb_delayed_max(vq) overwrites it again. When
vq->last_used_idx == 0 (for example, on the first reporting batch after probe
or on 16-bit wrap), transiently writing vring_used_event = 0x0 while the
device processes the first buffer (used->idx advancing from 0 to 1) satisfies
vring_need_event(0x0, 1, 0) on the host and triggers an immediate interrupt on
the first page. The same double evaluation also happens on any wakeup of the
shared vb->acked waitqueue from inflate_vq or deflate_vq.

In addition, on a packed virtqueue, when vring_interrupt() fires and sets
vq->event_triggered = true, waking wait_event() runs
virtqueue_disable_cb_packed(vq):

drivers/virtio/virtio_ring.c:virtqueue_disable_cb_packed() {
	if (vq->packed.event_flags_shadow != VRING_PACKED_EVENT_FLAG_DISABLE) {
		vq->packed.event_flags_shadow = VRING_PACKED_EVENT_FLAG_DISABLE;

		if (vq->event_triggered)
			return;

		vq->packed.vring.driver->flags =
			cpu_to_le16(vq->packed.event_flags_shadow);
	}
}

Because vq->event_triggered is true, virtqueue_disable_cb_packed() updates
event_flags_shadow to VRING_PACKED_EVENT_FLAG_DISABLE and returns early
without updating vq->packed.vring.driver->flags in shared memory. When
i >= nents then breaks out of the loop without calling
virtqueue_enable_cb_delayed_max(vq), driver->flags remains
VRING_PACKED_EVENT_FLAG_DESC after virtballoon_free_page_report() returns, and
subsequent virtqueue_disable_cb(vq) calls become no-ops because
event_flags_shadow is already VRING_PACKED_EVENT_FLAG_DISABLE.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/32e21059adb6cb1fd08c3f4426a313c77cf450ec.1793057483.git.mst@redhat.com?part=2