RE: [PATCH 3/4] Drivers: hv: vmbus: Compute ring byte counts from a caller-held snapshot

[Michael Kelley]

From: Kameron Carr <kameroncarr@xxxxxxxxxxxxxxxxxxx> Sent: Thursday, October 1, 2026 3:11 PM
> To: Haiyang Zhang <haiyangz@xxxxxxxxxxxxx>; Wei Liu <wei.liu@xxxxxxxxxx>; Dexuan Cui
> <decui@xxxxxxxxxxxxx>; Long Li <longli@xxxxxxxxxxxxx>; Michael Kelley
> <mikelley@xxxxxxxxxxxxx>
> Cc: linux-hyperv@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx
> Subject: [PATCH 3/4] Drivers: hv: vmbus: Compute ring byte counts from a caller-held
> snapshot
> 
> hv_get_ringbuffer_availbytes() reads the indices directly, so a caller
> that also wants the index values has to read them a second time. The
> host can change them in between, resulting in the byte counts and the
> indices reflecting two different states of the ring.
> 
> Replace it with hv_ringbuffer_avail_write() and
> hv_ringbuffer_avail_read(), which take the indices as arguments and
> return a single count. They are separate because most callers want only
> one of the two values. Use them in hv_ringbuffer_write() in place of the
> open-coded equivalent. hv_get_bytes_to_read() and
> hv_get_bytes_to_write() duplicated the same arithmetic, so put the
> helpers in include/linux/hyperv.h and use them there too.
> 
> Add a hv_ringbuffer_index_valid() helper for bounds checking and use it
> for the checks in hv_ringbuffer_write(). 

With all patches in this series applied, there are 5 places that
hv_ringbuffer_index_valid() is used. Four of those places check two indices --
a read index and a write index. One place checks just a single index. I'd
suggest having hv_ringbuffer_index_valid() check two indices instead of
just one. For the one place that checks a single index, just pass zero for the
other index. And flip the polarity -- make it hv_ringbuffer_indices_invalid()
so that it doesn't need a "!" in front of every usage. (For me anyway,
that's slightly less cognitive load when reading the code.)

This is just a suggestion. If you prefer to keep the code like it is,
I won't object.

> Convert the remaining callers
> to use a single snapshot:
> 
>   - hv_ringbuffer_get_debuginfo() now reports the indices and the
>     computed byte counts from the same snapshot.
> 
>   - hv_pkt_iter_close() computes the free space from priv_read_index
>     instead of re-reading the shared read index. The two agree unless a
>     misbehaving host has rewritten the value.
> 
> No functional change intended for a well-behaved host.
> 
> Signed-off-by: Kameron Carr <kameroncarr@xxxxxxxxxxxxxxxxxxx>

Regardless of whether you want to take my suggestion,

Reviewed-by: Michael Kelley <mhklinux@xxxxxxxxxxx>

> ---
>  drivers/hv/ring_buffer.c | 63 ++++++++++++++++--------------------------------
>  include/linux/hyperv.h   | 48 +++++++++++++++++++++++++++---------
>  2 files changed, 58 insertions(+), 53 deletions(-)
> 
> diff --git a/drivers/hv/ring_buffer.c b/drivers/hv/ring_buffer.c
> index 7f466c5..29edab9 100644
> --- a/drivers/hv/ring_buffer.c
> +++ b/drivers/hv/ring_buffer.c
> @@ -107,35 +107,12 @@ static u32 hv_copyto_ringbuffer(
>  	return start_write_offset;
>  }
> 
> -/*
> - *
> - * hv_get_ringbuffer_availbytes()
> - *
> - * Get number of bytes available to read and to write to
> - * for the specified ring buffer
> - */
> -static void
> -hv_get_ringbuffer_availbytes(const struct hv_ring_buffer_info *rbi,
> -			     u32 *read, u32 *write)
> -{
> -	u32 read_loc, write_loc, dsize;
> -
> -	/* Capture the read/write indices before they changed */
> -	read_loc = READ_ONCE(rbi->ring_buffer->read_index);
> -	write_loc = READ_ONCE(rbi->ring_buffer->write_index);
> -	dsize = rbi->ring_datasize;
> -
> -	*write = write_loc >= read_loc ? dsize - (write_loc - read_loc) :
> -		read_loc - write_loc;
> -	*read = dsize - *write;
> -}
> -
>  /* Get various debug metrics for the specified ring buffer. */
>  int hv_ringbuffer_get_debuginfo(struct hv_ring_buffer_info *ring_info,
>  				struct hv_ring_buffer_debug_info *debug_info)
>  {
> -	u32 bytes_avail_towrite;
> -	u32 bytes_avail_toread;
> +	u32 read_index;
> +	u32 write_index;
> 
>  	mutex_lock(&ring_info->ring_buffer_mutex);
> 
> @@ -144,13 +121,14 @@ int hv_ringbuffer_get_debuginfo(struct hv_ring_buffer_info
> *ring_info,
>  		return -EINVAL;
>  	}
> 
> -	hv_get_ringbuffer_availbytes(ring_info,
> -				     &bytes_avail_toread,
> -				     &bytes_avail_towrite);
> -	debug_info->bytes_avail_toread = bytes_avail_toread;
> -	debug_info->bytes_avail_towrite = bytes_avail_towrite;
> -	debug_info->current_read_index = ring_info->ring_buffer->read_index;
> -	debug_info->current_write_index = ring_info->ring_buffer->write_index;
> +	read_index = READ_ONCE(ring_info->ring_buffer->read_index);
> +	write_index = READ_ONCE(ring_info->ring_buffer->write_index);
> +	debug_info->bytes_avail_toread =
> +		hv_ringbuffer_avail_read(ring_info, read_index, write_index);
> +	debug_info->bytes_avail_towrite =
> +		hv_ringbuffer_avail_write(ring_info, read_index, write_index);
> +	debug_info->current_read_index = read_index;
> +	debug_info->current_write_index = write_index;
>  	debug_info->current_interrupt_mask
>  		= ring_info->ring_buffer->interrupt_mask;
>  	mutex_unlock(&ring_info->ring_buffer_mutex);
> @@ -282,8 +260,8 @@ int hv_ringbuffer_write(struct vmbus_channel *channel,
> 
>  	read_index = READ_ONCE(outring_info->ring_buffer->read_index);
>  	old_write = READ_ONCE(outring_info->ring_buffer->write_index);
> -	if (unlikely(read_index >= outring_info->ring_datasize ||
> -		     old_write >= outring_info->ring_datasize)) {
> +	if (unlikely(!hv_ringbuffer_index_valid(outring_info, read_index) ||
> +		     !hv_ringbuffer_index_valid(outring_info, old_write))) {
>  		spin_unlock_irqrestore(&outring_info->ring_lock, flags);
>  		pr_err_ratelimited("outbound ring indices out of range: relid %u read %u
> write %u size %u\n",
>  				   channel->offermsg.child_relid, read_index,
> @@ -291,9 +269,8 @@ int hv_ringbuffer_write(struct vmbus_channel *channel,
>  		return -EIO;
>  	}
> 
> -	bytes_avail_towrite = old_write >= read_index ?
> -		outring_info->ring_datasize - (old_write - read_index) :
> -		read_index - old_write;
> +	bytes_avail_towrite = hv_ringbuffer_avail_write(outring_info, read_index,
> +							old_write);
> 
>  	/*
>  	 * If there is only room for the packet, assume it is full.
> @@ -568,6 +545,7 @@ void hv_pkt_iter_close(struct vmbus_channel *channel)
>  {
>  	struct hv_ring_buffer_info *rbi = &channel->inbound;
>  	u32 curr_write_sz, pending_sz, bytes_read, start_read_index;
> +	u32 write_index;
> 
>  	/*
>  	 * Make sure all reads are done before we update the read index since
> @@ -607,11 +585,13 @@ void hv_pkt_iter_close(struct vmbus_channel *channel)
>  		return;
> 
>  	/*
> -	 * Ensure the read of write_index in hv_get_bytes_to_write()
> -	 * happens after the read of pending_send_sz.
> +	 * Ensure the read of write_index happens after the read of
> +	 * pending_send_sz.
>  	 */
>  	virt_rmb();
> -	curr_write_sz = hv_get_bytes_to_write(rbi);
> +	write_index = READ_ONCE(rbi->ring_buffer->write_index);
> +	curr_write_sz = hv_ringbuffer_avail_write(rbi, rbi->priv_read_index,
> +						  write_index);
>  	bytes_read = hv_pkt_iter_bytes_read(rbi, start_read_index);
> 
>  	/*
> @@ -627,8 +607,7 @@ void hv_pkt_iter_close(struct vmbus_channel *channel)
>  	 * Exactly filling the ring buffer is treated as "not enough
>  	 * space". The ring buffer always must have at least one byte
>  	 * empty so the empty and full conditions are distinguishable.
> -	 * hv_get_bytes_to_write() doesn't fully tell the truth in
> -	 * this regard.
> +	 * curr_write_sz doesn't fully tell the truth in this regard.
>  	 *
>  	 * So first check if we were in the "enough free space" state
>  	 * before we began the iteration. If so, the host was not
> diff --git a/include/linux/hyperv.h b/include/linux/hyperv.h
> index 5c65820..9d7d09c 100644
> --- a/include/linux/hyperv.h
> +++ b/include/linux/hyperv.h
> @@ -209,31 +209,57 @@ struct hv_ring_buffer_info {
>  };
> 
> 
> +/*
> + * The indices live in memory shared with the untrusted host, so check one
> + * before using it as an offset or to compute a byte count.
> + */
> +static inline bool
> +hv_ringbuffer_index_valid(const struct hv_ring_buffer_info *rbi, u32 index)
> +{
> +	return index < rbi->ring_datasize;
> +}
> +
> +/*
> + * Byte counts for a caller-supplied snapshot of the indices, so that the
> + * counts and the indices the caller goes on to use describe one state of the
> + * ring.
> + */
> +static inline u32
> +hv_ringbuffer_avail_write(const struct hv_ring_buffer_info *rbi,
> +			  u32 read_loc, u32 write_loc)
> +{
> +	u32 dsize = rbi->ring_datasize;
> +
> +	return write_loc >= read_loc ? dsize - (write_loc - read_loc) :
> +		read_loc - write_loc;
> +}
> +
> +static inline u32
> +hv_ringbuffer_avail_read(const struct hv_ring_buffer_info *rbi,
> +			 u32 read_loc, u32 write_loc)
> +{
> +	return rbi->ring_datasize -
> +		hv_ringbuffer_avail_write(rbi, read_loc, write_loc);
> +}
> +
>  static inline u32 hv_get_bytes_to_read(const struct hv_ring_buffer_info *rbi)
>  {
> -	u32 read_loc, write_loc, dsize, read;
> +	u32 read_loc, write_loc;
> 
> -	dsize = rbi->ring_datasize;
>  	read_loc = READ_ONCE(rbi->ring_buffer->read_index);
>  	write_loc = READ_ONCE(rbi->ring_buffer->write_index);
> 
> -	read = write_loc >= read_loc ? (write_loc - read_loc) :
> -		(dsize - read_loc) + write_loc;
> -
> -	return read;
> +	return hv_ringbuffer_avail_read(rbi, read_loc, write_loc);
>  }
> 
>  static inline u32 hv_get_bytes_to_write(const struct hv_ring_buffer_info *rbi)
>  {
> -	u32 read_loc, write_loc, dsize, write;
> +	u32 read_loc, write_loc;
> 
> -	dsize = rbi->ring_datasize;
>  	read_loc = READ_ONCE(rbi->ring_buffer->read_index);
>  	write_loc = READ_ONCE(rbi->ring_buffer->write_index);
> 
> -	write = write_loc >= read_loc ? dsize - (write_loc - read_loc) :
> -		read_loc - write_loc;
> -	return write;
> +	return hv_ringbuffer_avail_write(rbi, read_loc, write_loc);
>  }
> 
>  static inline u32 hv_get_avail_to_write_percent(
> 
> --
> 2.45.4