[PATCH 1/2] virtio: use virtio accessor to access packed descriptor flags

[PATCH 1/2] virtio: use virtio accessor to access packed descriptor flags

[Jason Wang]

We used to access packed descriptor flags via
address_space_{write|read}_cached(). When we hit the cache, memcpy()
is used which is not an atomic operation which may lead a wrong value
is read or wrote.

So this patch switches to use virito_{stw|lduw}_phys_cached() to make
sure the aceess is atomic.

Fixes: 86044b24e865f ("virtio: basic packed virtqueue support")
Cc: qemu-stable@nongnu.org
Signed-off-by: Jason Wang <jasowang@redhat.com>
---
 hw/virtio/virtio.c | 11 ++++-------
 1 file changed, 4 insertions(+), 7 deletions(-)

diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index cc69a9b881..939bcbfeb9 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -507,11 +507,9 @@ static void vring_packed_desc_read_flags(VirtIODevice *vdev,
                                          MemoryRegionCache *cache,
                                          int i)
 {
-    address_space_read_cached(cache,
-                              i * sizeof(VRingPackedDesc) +
-                              offsetof(VRingPackedDesc, flags),
-                              flags, sizeof(*flags));
-    virtio_tswap16s(vdev, flags);
+    hwaddr off = i * sizeof(VRingPackedDesc) + offsetof(VRingPackedDesc, flags);
+
+    *flags = virtio_lduw_phys_cached(vdev, cache, off);
 }
 
 static void vring_packed_desc_read(VirtIODevice *vdev,
@@ -564,8 +562,7 @@ static void vring_packed_desc_write_flags(VirtIODevice *vdev,
 {
     hwaddr off = i * sizeof(VRingPackedDesc) + offsetof(VRingPackedDesc, flags);
 
-    virtio_tswap16s(vdev, &desc->flags);
-    address_space_write_cached(cache, off, &desc->flags, sizeof(desc->flags));
+    virtio_stw_phys_cached(vdev, cache, off, desc->flags);
     address_space_cache_invalidate(cache, off, sizeof(desc->flags));
 }
 
-- 
2.25.1

[PATCH 2/2] virtio: use virtio accessor to access packed event

[Jason Wang]

We used to access packed descriptor event and off_wrap via
address_space_{write|read}_cached(). When we hit the cache, memcpy()
is used which is not atomic which may lead a wrong value to be read or
wrote.

This patch fixes this by switching to use
virito_{stw|lduw}_phys_cached() to make sure the access is atomic.

Fixes: 683f7665679c1 ("virtio: event suppression support for packed ring")
Cc: qemu-stable@nongnu.org
Signed-off-by: Jason Wang <jasowang@redhat.com>
---
 hw/virtio/virtio.c | 13 ++++---------
 1 file changed, 4 insertions(+), 9 deletions(-)

diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index 939bcbfeb9..ea7c079fb0 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -247,13 +247,10 @@ static void vring_packed_event_read(VirtIODevice *vdev,
     hwaddr off_off = offsetof(VRingPackedDescEvent, off_wrap);
     hwaddr off_flags = offsetof(VRingPackedDescEvent, flags);
 
-    address_space_read_cached(cache, off_flags, &e->flags,
-                              sizeof(e->flags));
+    e->flags = virtio_lduw_phys_cached(vdev, cache, off_flags);
     /* Make sure flags is seen before off_wrap */
     smp_rmb();
-    address_space_read_cached(cache, off_off, &e->off_wrap,
-                              sizeof(e->off_wrap));
-    virtio_tswap16s(vdev, &e->off_wrap);
+    e->off_wrap = virtio_lduw_phys_cached(vdev, cache, off_off);
     virtio_tswap16s(vdev, &e->flags);
 }
 
@@ -263,8 +260,7 @@ static void vring_packed_off_wrap_write(VirtIODevice *vdev,
 {
     hwaddr off = offsetof(VRingPackedDescEvent, off_wrap);
 
-    virtio_tswap16s(vdev, &off_wrap);
-    address_space_write_cached(cache, off, &off_wrap, sizeof(off_wrap));
+    virtio_stw_phys_cached(vdev, cache, off, off_wrap);
     address_space_cache_invalidate(cache, off, sizeof(off_wrap));
 }
 
@@ -273,8 +269,7 @@ static void vring_packed_flags_write(VirtIODevice *vdev,
 {
     hwaddr off = offsetof(VRingPackedDescEvent, flags);
 
-    virtio_tswap16s(vdev, &flags);
-    address_space_write_cached(cache, off, &flags, sizeof(flags));
+    virtio_stw_phys_cached(vdev, cache, off, flags);
     address_space_cache_invalidate(cache, off, sizeof(flags));
 }
 
-- 
2.25.1

Re: [PATCH 2/2] virtio: use virtio accessor to access packed event

[Philippe Mathieu-Daudé]

On 11/11/21 07:38, Jason Wang wrote:
> We used to access packed descriptor event and off_wrap via
> address_space_{write|read}_cached(). When we hit the cache, memcpy()
> is used which is not atomic which may lead a wrong value to be read or
> wrote.
> 
> This patch fixes this by switching to use
> virito_{stw|lduw}_phys_cached() to make sure the access is atomic.
> 
> Fixes: 683f7665679c1 ("virtio: event suppression support for packed ring")
> Cc: qemu-stable@nongnu.org
> Signed-off-by: Jason Wang <jasowang@redhat.com>
> ---
>  hw/virtio/virtio.c | 13 ++++---------
>  1 file changed, 4 insertions(+), 9 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

No cover so asking here, what about vring_packed_desc_read()?

Re: [PATCH 1/2] virtio: use virtio accessor to access packed descriptor flags

[Philippe Mathieu-Daudé]

On 11/11/21 07:38, Jason Wang wrote:
> We used to access packed descriptor flags via
> address_space_{write|read}_cached(). When we hit the cache, memcpy()
> is used which is not an atomic operation which may lead a wrong value
> is read or wrote.
> 
> So this patch switches to use virito_{stw|lduw}_phys_cached() to make
> sure the aceess is atomic.
> 
> Fixes: 86044b24e865f ("virtio: basic packed virtqueue support")
> Cc: qemu-stable@nongnu.org
> Signed-off-by: Jason Wang <jasowang@redhat.com>
> ---
>  hw/virtio/virtio.c | 11 ++++-------
>  1 file changed, 4 insertions(+), 7 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Re: [PATCH 1/2] virtio: use virtio accessor to access packed descriptor flags

[Michael S. Tsirkin]

On Thu, Nov 11, 2021 at 02:38:53PM +0800, Jason Wang wrote:
> We used to access packed descriptor flags via
> address_space_{write|read}_cached(). When we hit the cache, memcpy()
> is used which is not an atomic operation which may lead a wrong value
> is read or wrote.

Could you clarify where's the memcpy that you see?
Thanks!

> So this patch switches to use virito_{stw|lduw}_phys_cached() to make
> sure the aceess is atomic.
> 
> Fixes: 86044b24e865f ("virtio: basic packed virtqueue support")
> Cc: qemu-stable@nongnu.org
> Signed-off-by: Jason Wang <jasowang@redhat.com>
> ---
>  hw/virtio/virtio.c | 11 ++++-------
>  1 file changed, 4 insertions(+), 7 deletions(-)
> 
> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> index cc69a9b881..939bcbfeb9 100644
> --- a/hw/virtio/virtio.c
> +++ b/hw/virtio/virtio.c
> @@ -507,11 +507,9 @@ static void vring_packed_desc_read_flags(VirtIODevice *vdev,
>                                           MemoryRegionCache *cache,
>                                           int i)
>  {
> -    address_space_read_cached(cache,
> -                              i * sizeof(VRingPackedDesc) +
> -                              offsetof(VRingPackedDesc, flags),
> -                              flags, sizeof(*flags));
> -    virtio_tswap16s(vdev, flags);
> +    hwaddr off = i * sizeof(VRingPackedDesc) + offsetof(VRingPackedDesc, flags);
> +
> +    *flags = virtio_lduw_phys_cached(vdev, cache, off);
>  }
>  
>  static void vring_packed_desc_read(VirtIODevice *vdev,
> @@ -564,8 +562,7 @@ static void vring_packed_desc_write_flags(VirtIODevice *vdev,
>  {
>      hwaddr off = i * sizeof(VRingPackedDesc) + offsetof(VRingPackedDesc, flags);
>  
> -    virtio_tswap16s(vdev, &desc->flags);
> -    address_space_write_cached(cache, off, &desc->flags, sizeof(desc->flags));
> +    virtio_stw_phys_cached(vdev, cache, off, desc->flags);
>      address_space_cache_invalidate(cache, off, sizeof(desc->flags));
>  }
>  
> -- 
> 2.25.1

Re: [PATCH 1/2] virtio: use virtio accessor to access packed descriptor flags

[Jason Wang]

On Thu, Nov 11, 2021 at 4:27 PM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Thu, Nov 11, 2021 at 02:38:53PM +0800, Jason Wang wrote:
> > We used to access packed descriptor flags via
> > address_space_{write|read}_cached(). When we hit the cache, memcpy()
> > is used which is not an atomic operation which may lead a wrong value
> > is read or wrote.
>
> Could you clarify where's the memcpy that you see?
> Thanks!

In the address_space_{write|read}_cached it self:

static inline MemTxResult
=>dress_space_write_cached(MemoryRegionCache *cache, hwaddr addr,
                           const void *buf, hwaddr len)
{
    assert(addr < cache->len && len <= cache->len - addr);
    if (likely(cache->ptr)) {
        memcpy(cache->ptr + addr, buf, len);
        return MEMTX_OK;
    } else {
        return address_space_write_cached_slow(cache, addr, buf, len);
    }
}

Thanks

>
> > So this patch switches to use virito_{stw|lduw}_phys_cached() to make
> > sure the aceess is atomic.
> >
> > Fixes: 86044b24e865f ("virtio: basic packed virtqueue support")
> > Cc: qemu-stable@nongnu.org
> > Signed-off-by: Jason Wang <jasowang@redhat.com>
> > ---
> >  hw/virtio/virtio.c | 11 ++++-------
> >  1 file changed, 4 insertions(+), 7 deletions(-)
> >
> > diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> > index cc69a9b881..939bcbfeb9 100644
> > --- a/hw/virtio/virtio.c
> > +++ b/hw/virtio/virtio.c
> > @@ -507,11 +507,9 @@ static void vring_packed_desc_read_flags(VirtIODevice *vdev,
> >                                           MemoryRegionCache *cache,
> >                                           int i)
> >  {
> > -    address_space_read_cached(cache,
> > -                              i * sizeof(VRingPackedDesc) +
> > -                              offsetof(VRingPackedDesc, flags),
> > -                              flags, sizeof(*flags));
> > -    virtio_tswap16s(vdev, flags);
> > +    hwaddr off = i * sizeof(VRingPackedDesc) + offsetof(VRingPackedDesc, flags);
> > +
> > +    *flags = virtio_lduw_phys_cached(vdev, cache, off);
> >  }
> >
> >  static void vring_packed_desc_read(VirtIODevice *vdev,
> > @@ -564,8 +562,7 @@ static void vring_packed_desc_write_flags(VirtIODevice *vdev,
> >  {
> >      hwaddr off = i * sizeof(VRingPackedDesc) + offsetof(VRingPackedDesc, flags);
> >
> > -    virtio_tswap16s(vdev, &desc->flags);
> > -    address_space_write_cached(cache, off, &desc->flags, sizeof(desc->flags));
> > +    virtio_stw_phys_cached(vdev, cache, off, desc->flags);
> >      address_space_cache_invalidate(cache, off, sizeof(desc->flags));
> >  }
> >
> > --
> > 2.25.1
>

Re: [PATCH 2/2] virtio: use virtio accessor to access packed event

[Jason Wang]

On Thu, Nov 11, 2021 at 3:51 PM Philippe Mathieu-Daudé
<philmd@redhat.com> wrote:
>
> On 11/11/21 07:38, Jason Wang wrote:
> > We used to access packed descriptor event and off_wrap via
> > address_space_{write|read}_cached(). When we hit the cache, memcpy()
> > is used which is not atomic which may lead a wrong value to be read or
> > wrote.
> >
> > This patch fixes this by switching to use
> > virito_{stw|lduw}_phys_cached() to make sure the access is atomic.
> >
> > Fixes: 683f7665679c1 ("virtio: event suppression support for packed ring")
> > Cc: qemu-stable@nongnu.org
> > Signed-off-by: Jason Wang <jasowang@redhat.com>
> > ---
> >  hw/virtio/virtio.c | 13 ++++---------
> >  1 file changed, 4 insertions(+), 9 deletions(-)
>
> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>
> No cover so asking here, what about vring_packed_desc_read()?

In that function, the vring_packed_desc_read_flags() used for reading
the flags atomically. If the flags told us the buffer is available,
there's no need read the rest of descriptor in atomic operation since
the driver guarantee that the changes of flags are visible after the
rest of the descriptor is setup.

Thanks

>

Re: [PATCH 1/2] virtio: use virtio accessor to access packed descriptor flags

[Michael S. Tsirkin]

On Fri, Nov 12, 2021 at 10:23:12AM +0800, Jason Wang wrote:
> On Thu, Nov 11, 2021 at 4:27 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> >
> > On Thu, Nov 11, 2021 at 02:38:53PM +0800, Jason Wang wrote:
> > > We used to access packed descriptor flags via
> > > address_space_{write|read}_cached(). When we hit the cache, memcpy()
> > > is used which is not an atomic operation which may lead a wrong value
> > > is read or wrote.
> >
> > Could you clarify where's the memcpy that you see?
> > Thanks!
> 
> In the address_space_{write|read}_cached it self:
> 
> static inline MemTxResult
> =>dress_space_write_cached(MemoryRegionCache *cache, hwaddr addr,
>                            const void *buf, hwaddr len)
> {
>     assert(addr < cache->len && len <= cache->len - addr);
>     if (likely(cache->ptr)) {
>         memcpy(cache->ptr + addr, buf, len);
>         return MEMTX_OK;
>     } else {
>         return address_space_write_cached_slow(cache, addr, buf, len);
>     }
> }
> 
> Thanks

But that's a copy from the cache, not from guest memory.
I don't see how it can change so I don't see why it needs
to be atomic. Was there an actual issue you observed or
is this theoretical?


> >
> > > So this patch switches to use virito_{stw|lduw}_phys_cached() to make
> > > sure the aceess is atomic.
> > >
> > > Fixes: 86044b24e865f ("virtio: basic packed virtqueue support")
> > > Cc: qemu-stable@nongnu.org
> > > Signed-off-by: Jason Wang <jasowang@redhat.com>
> > > ---
> > >  hw/virtio/virtio.c | 11 ++++-------
> > >  1 file changed, 4 insertions(+), 7 deletions(-)
> > >
> > > diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> > > index cc69a9b881..939bcbfeb9 100644
> > > --- a/hw/virtio/virtio.c
> > > +++ b/hw/virtio/virtio.c
> > > @@ -507,11 +507,9 @@ static void vring_packed_desc_read_flags(VirtIODevice *vdev,
> > >                                           MemoryRegionCache *cache,
> > >                                           int i)
> > >  {
> > > -    address_space_read_cached(cache,
> > > -                              i * sizeof(VRingPackedDesc) +
> > > -                              offsetof(VRingPackedDesc, flags),
> > > -                              flags, sizeof(*flags));
> > > -    virtio_tswap16s(vdev, flags);
> > > +    hwaddr off = i * sizeof(VRingPackedDesc) + offsetof(VRingPackedDesc, flags);
> > > +
> > > +    *flags = virtio_lduw_phys_cached(vdev, cache, off);
> > >  }
> > >
> > >  static void vring_packed_desc_read(VirtIODevice *vdev,
> > > @@ -564,8 +562,7 @@ static void vring_packed_desc_write_flags(VirtIODevice *vdev,
> > >  {
> > >      hwaddr off = i * sizeof(VRingPackedDesc) + offsetof(VRingPackedDesc, flags);
> > >
> > > -    virtio_tswap16s(vdev, &desc->flags);
> > > -    address_space_write_cached(cache, off, &desc->flags, sizeof(desc->flags));
> > > +    virtio_stw_phys_cached(vdev, cache, off, desc->flags);
> > >      address_space_cache_invalidate(cache, off, sizeof(desc->flags));
> > >  }
> > >
> > > --
> > > 2.25.1
> >

Re: [PATCH 1/2] virtio: use virtio accessor to access packed descriptor flags

[Jason Wang]

On Fri, Nov 12, 2021 at 6:04 PM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Fri, Nov 12, 2021 at 10:23:12AM +0800, Jason Wang wrote:
> > On Thu, Nov 11, 2021 at 4:27 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> > >
> > > On Thu, Nov 11, 2021 at 02:38:53PM +0800, Jason Wang wrote:
> > > > We used to access packed descriptor flags via
> > > > address_space_{write|read}_cached(). When we hit the cache, memcpy()
> > > > is used which is not an atomic operation which may lead a wrong value
> > > > is read or wrote.
> > >
> > > Could you clarify where's the memcpy that you see?
> > > Thanks!
> >
> > In the address_space_{write|read}_cached it self:
> >
> > static inline MemTxResult
> > =>dress_space_write_cached(MemoryRegionCache *cache, hwaddr addr,
> >                            const void *buf, hwaddr len)
> > {
> >     assert(addr < cache->len && len <= cache->len - addr);
> >     if (likely(cache->ptr)) {
> >         memcpy(cache->ptr + addr, buf, len);
> >         return MEMTX_OK;
> >     } else {
> >         return address_space_write_cached_slow(cache, addr, buf, len);
> >     }
> > }
> >
> > Thanks
>
> But that's a copy from the cache, not from guest memory.

I may miss something but it looks to me the cache is the cache of
address translation not the cache from guest memory:

See address_space_cache_init():

        cache->ptr = qemu_ram_ptr_length(mr->ram_block, cache->xlat, &l, true);

> I don't see how it can change so I don't see why it needs
> to be atomic. Was there an actual issue you observed or
> is this theoretical?

During code review.

Thanks

>
>
> > >
> > > > So this patch switches to use virito_{stw|lduw}_phys_cached() to make
> > > > sure the aceess is atomic.
> > > >
> > > > Fixes: 86044b24e865f ("virtio: basic packed virtqueue support")
> > > > Cc: qemu-stable@nongnu.org
> > > > Signed-off-by: Jason Wang <jasowang@redhat.com>
> > > > ---
> > > >  hw/virtio/virtio.c | 11 ++++-------
> > > >  1 file changed, 4 insertions(+), 7 deletions(-)
> > > >
> > > > diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> > > > index cc69a9b881..939bcbfeb9 100644
> > > > --- a/hw/virtio/virtio.c
> > > > +++ b/hw/virtio/virtio.c
> > > > @@ -507,11 +507,9 @@ static void vring_packed_desc_read_flags(VirtIODevice *vdev,
> > > >                                           MemoryRegionCache *cache,
> > > >                                           int i)
> > > >  {
> > > > -    address_space_read_cached(cache,
> > > > -                              i * sizeof(VRingPackedDesc) +
> > > > -                              offsetof(VRingPackedDesc, flags),
> > > > -                              flags, sizeof(*flags));
> > > > -    virtio_tswap16s(vdev, flags);
> > > > +    hwaddr off = i * sizeof(VRingPackedDesc) + offsetof(VRingPackedDesc, flags);
> > > > +
> > > > +    *flags = virtio_lduw_phys_cached(vdev, cache, off);
> > > >  }
> > > >
> > > >  static void vring_packed_desc_read(VirtIODevice *vdev,
> > > > @@ -564,8 +562,7 @@ static void vring_packed_desc_write_flags(VirtIODevice *vdev,
> > > >  {
> > > >      hwaddr off = i * sizeof(VRingPackedDesc) + offsetof(VRingPackedDesc, flags);
> > > >
> > > > -    virtio_tswap16s(vdev, &desc->flags);
> > > > -    address_space_write_cached(cache, off, &desc->flags, sizeof(desc->flags));
> > > > +    virtio_stw_phys_cached(vdev, cache, off, desc->flags);
> > > >      address_space_cache_invalidate(cache, off, sizeof(desc->flags));
> > > >  }
> > > >
> > > > --
> > > > 2.25.1
> > >
>