Re: [PATCH 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: off-by-one in virtqueue_enable_cb_delayed_max() hangs page reporting
- [Medium] virtio_balloon: broken callback suppression in virtballoon_free_page_report()
--

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

virtio_balloon: submit reported pages as individual buffers

This commit submits each reported page in virtballoon_free_page_report() as a
separate virtqueue buffer instead of a single scatter-gather descriptor chain
to avoid exceeding the virtio specification's 2^32 byte limit. It uses
virtqueue_enable_cb_delayed_max() and virtqueue_disable_cb() around
wait_event() when draining the buffers.

> diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
> index ab3e3d887e00f..64999f05f9f1d 100644
> --- a/drivers/virtio/virtio_balloon.c
> +++ b/drivers/virtio/virtio_balloon.c
> @@ -205,25 +205,47 @@ static int virtballoon_free_page_report(struct page_reporting_dev_info *pr_dev_i

[ ... ]

> -	/* When host has read buffer, this completes via balloon_ack */
> -	wait_event(vb->acked, virtqueue_get_buf(vq, &unused));
> +	if (nents) {
> +		virtqueue_kick(vq);
>  
> -	return 0;
> +		/* When host has read buffer, this completes via balloon_ack.
> +		 * We report pages as individual buffers, so arm the callback
> +		 * to only interrupt once all the queued buffers have been
> +		 * used: otherwise we would get one interrupt per page.
> +		 */
> +		for (i = 0; i < nents; i++) {
> +			unsigned int unused;
> +
> +			virtqueue_enable_cb_delayed_max(vq);

[Severity: High]
Can calling virtqueue_enable_cb_delayed_max(vq) here cause wait_event() to
hang indefinitely when VIRTIO_RING_F_EVENT_IDX is negotiated?

In drivers/virtio/virtio_ring.c, virtqueue_enable_cb_delayed_max_split()
computes the event index as:

    bufs = (u16)(vq->split.avail_idx_shadow - vq->last_used_idx);

    virtio_store_mb(vq->weak_barriers,
                    &vring_used_event(&vq->split.vring),
                    cpu_to_virtio16(vq->vq.vdev, vq->last_used_idx + bufs));

    if (unlikely((u16)(virtio16_to_cpu(vq->vq.vdev, vq->split.vring.used->idx)
                                    - vq->last_used_idx) > bufs)) {
            END_USE(vq);
            return false;
    }

In include/uapi/linux/virtio_ring.h, vring_need_event() checks:

    static inline int vring_need_event(__u16 event_idx, __u16 new_idx, __u16 old)
    {
            return (__u16)(new_idx - event_idx - 1) < (__u16)(new_idx - old);
    }

Because event_idx is 0-based, an interrupt is triggered when used->idx
(new_idx) advances to event_idx + 1. When bufs buffers are queued at ring
slots last_used_idx .. last_used_idx + bufs - 1, consuming all queued buffers
advances used->idx to last_used_idx + bufs.

By setting vring_used_event to last_used_idx + bufs instead of
last_used_idx + bufs - 1, when the device finishes consuming all queued
buffers (new_idx == event_idx), (__u16)(new_idx - event_idx - 1) underflows
to 0xffff and vring_need_event() returns false. In addition, the race check
tests (used->idx - last_used_idx) > bufs, which can never be true since the
device cannot consume more than the bufs available buffers.

Similarly, in virtqueue_enable_cb_delayed_max_packed():

    bufs = vq->packed.vring.num - vq->vq.num_free;
    last_used_idx = READ_ONCE(vq->last_used_idx);
    wrap_counter = packed_used_wrap_counter(last_used_idx);

    used_idx = packed_last_used(last_used_idx) + bufs;
    if (used_idx >= vq->packed.vring.num) {
            used_idx -= vq->packed.vring.num;
            wrap_counter ^= 1;
    }

    vq->packed.vring.driver->off_wrap = cpu_to_le16(used_idx |
            (wrap_counter << VRING_PACKED_EVENT_F_WRAP_CTR));

This sets driver->off_wrap to packed_last_used(last_used_idx) + bufs
(next_avail_idx, one descriptor past the last queued descriptor) instead of
the last queued descriptor. Since that descriptor is not available to the
device, the device never marks it used and never interrupts, hanging the
page_reporting_process() -> page_reporting_cycle() ->
virtballoon_free_page_report() worker in wait_event().

> +			wait_event(vb->acked,
> +				   virtqueue_get_buf(vq, &unused));
> +			virtqueue_disable_cb(vq);
> +		}
> +	}

[Severity: Medium]
Does ordering virtqueue_enable_cb_delayed_max(), virtqueue_get_buf(), and
virtqueue_disable_cb() this way defeat the delayed callback suppression?

There appear to be three issues with the callback state machine in
virtballoon_free_page_report():

1. Reporting virtqueue callbacks are enabled by default in init_vqs(), and
virtqueue_kick(vq) is called before callbacks are disabled or delayed, so the
device is notified while interrupts are still armed for the first buffer.

2. Inside the loop, virtqueue_enable_cb_delayed_max(vq) is called before
wait_event(vb->acked, virtqueue_get_buf(vq, &unused)). Because callbacks are
enabled when virtqueue_get_buf() runs, virtqueue_get_buf_ctx_split() (and
update_last_used_idx_packed() for packed rings) overwrites the delayed event
threshold on every dequeued buffer with the very next single buffer:

    vq->last_used_idx++;
    /* If we expect an interrupt for the next entry, tell host
     * by writing event index and flush out the write before
     * the read in the next get_buf call. */
    if (!(vq->split.avail_flags_shadow & VRING_AVAIL_F_NO_INTERRUPT))
            virtio_store_mb(vq->weak_barriers,
                            &vring_used_event(&vq->split.vring),
                            cpu_to_virtio16(vq->vq.vdev, vq->last_used_idx));

3. When an interrupt fires, vring_interrupt() sets vq->event_triggered = true.
After wait_event() wakes and virtqueue_get_buf() writes
vring_used_event = vq->last_used_idx, the subsequent virtqueue_disable_cb(vq)
call hits the early return in virtqueue_disable_cb_split() (and similarly in
virtqueue_disable_cb_packed()):

    if (!(vq->split.avail_flags_shadow & VRING_AVAIL_F_NO_INTERRUPT)) {
            vq->split.avail_flags_shadow |= VRING_AVAIL_F_NO_INTERRUPT;

            /*
             * If device triggered an event already it won't trigger one again:
             * no need to disable.
             */
            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;

This leaves vring_used_event armed at vq->last_used_idx in the shared ring at
the end of the batch, so the next call to virtballoon_free_page_report()
kicks the virtqueue with interrupts armed for the first buffer.

Should callbacks be kept disabled while draining available buffers with
virtqueue_get_buf(), only arming virtqueue_enable_cb_delayed_max() when
virtqueue_get_buf() returns NULL?

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