[PATCH 1/1] hv: drivers: vmbus: Prevent load re-ordering when reading ring buffer

[PATCH 1/1] hv: drivers: vmbus: Prevent load re-ordering when reading ring buffer

[Michael Kelley]

When reading a packet from a host-to-guest ring buffer, there is no
memory barrier between reading the write index (to see if there is
a packet to read) and reading the contents of the packet. The Hyper-V
host uses store-release when updating the write index to ensure that
writes of the packet data are completed first. On the guest side,
the processor can reorder and read the packet data before the write
index, and sometimes get stale packet data. Getting such stale packet
data has been observed in a reproducible case in a VM on ARM64.

Fix this by using virt_load_acquire() to read the write index,
ensuring that reads of the packet data cannot be reordered
before it. Preventing such reordering is logically correct, and
with this change, getting stale data can no longer be reproduced.

Signed-off-by: Michael Kelley <mikelley@microsoft.com>
---
 drivers/hv/ring_buffer.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/drivers/hv/ring_buffer.c b/drivers/hv/ring_buffer.c
index 71efacb..3d215d9 100644
--- a/drivers/hv/ring_buffer.c
+++ b/drivers/hv/ring_buffer.c
@@ -439,7 +439,16 @@ int hv_ringbuffer_read(struct vmbus_channel *channel,
 static u32 hv_pkt_iter_avail(const struct hv_ring_buffer_info *rbi)
 {
 	u32 priv_read_loc = rbi->priv_read_index;
-	u32 write_loc = READ_ONCE(rbi->ring_buffer->write_index);
+	u32 write_loc;
+
+	/*
+	 * The Hyper-V host writes the packet data, then uses
+	 * store_release() to update the write_index.  Use load_acquire()
+	 * here to prevent loads of the packet data from being re-ordered
+	 * before the read of the write_index and potentially getting
+	 * stale data.
+	 */
+	write_loc = virt_load_acquire(&rbi->ring_buffer->write_index);
 
 	if (write_loc >= priv_read_loc)
 		return write_loc - priv_read_loc;
-- 
1.8.3.1

Re: [PATCH 1/1] hv: drivers: vmbus: Prevent load re-ordering when reading ring buffer

[Andrea Parri]

On Sun, Mar 27, 2022 at 08:25:10AM -0700, Michael Kelley wrote:
> When reading a packet from a host-to-guest ring buffer, there is no
> memory barrier between reading the write index (to see if there is
> a packet to read) and reading the contents of the packet. The Hyper-V
> host uses store-release when updating the write index to ensure that
> writes of the packet data are completed first. On the guest side,
> the processor can reorder and read the packet data before the write
> index, and sometimes get stale packet data. Getting such stale packet
> data has been observed in a reproducible case in a VM on ARM64.
> 
> Fix this by using virt_load_acquire() to read the write index,
> ensuring that reads of the packet data cannot be reordered
> before it. Preventing such reordering is logically correct, and
> with this change, getting stale data can no longer be reproduced.
> 
> Signed-off-by: Michael Kelley <mikelley@microsoft.com>

Reviewed-by: Andrea Parri (Microsoft) <parri.andrea@gmail.com>

Nit: subject prefix -> "Drivers: hv: vmbus:".

Thanks,
  Andrea

Re: [PATCH 1/1] hv: drivers: vmbus: Prevent load re-ordering when reading ring buffer

[Wei Liu]

On Tue, Mar 29, 2022 at 01:12:33AM +0200, Andrea Parri wrote:
> On Sun, Mar 27, 2022 at 08:25:10AM -0700, Michael Kelley wrote:
> > When reading a packet from a host-to-guest ring buffer, there is no
> > memory barrier between reading the write index (to see if there is
> > a packet to read) and reading the contents of the packet. The Hyper-V
> > host uses store-release when updating the write index to ensure that
> > writes of the packet data are completed first. On the guest side,
> > the processor can reorder and read the packet data before the write
> > index, and sometimes get stale packet data. Getting such stale packet
> > data has been observed in a reproducible case in a VM on ARM64.
> > 
> > Fix this by using virt_load_acquire() to read the write index,
> > ensuring that reads of the packet data cannot be reordered
> > before it. Preventing such reordering is logically correct, and
> > with this change, getting stale data can no longer be reproduced.
> > 
> > Signed-off-by: Michael Kelley <mikelley@microsoft.com>
> 
> Reviewed-by: Andrea Parri (Microsoft) <parri.andrea@gmail.com>
> 
> Nit: subject prefix -> "Drivers: hv: vmbus:".

Applied to hyperv-fixes. Thanks.

> 
> Thanks,
>   Andrea