| Message ID | 20210826172658.2116840-4-philmd@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | hw/virtio: Minor housekeeping patches | expand |
On Thu, Aug 26, 2021 at 07:26:58PM +0200, Philippe Mathieu-Daudé wrote: >Both virtqueue_packed_get_avail_bytes() and >virtqueue_split_get_avail_bytes() access the region cache, but >their caller also does. Simplify by having virtqueue_get_avail_bytes >calling both with RCU lock held, and passing the caches as argument. > >Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> >--- >RFC because I'm not sure this is safe enough It seems safe to me. While reviewing I saw that vring_get_region_caches() has /* Called within rcu_read_lock(). */ comment, but it seems to me that we call that function in places where we haven't acquired it, which shouldn't be a problem, but should we remove that comment? Thanks, Stefano >--- > hw/virtio/virtio.c | 29 ++++++++++++----------------- > 1 file changed, 12 insertions(+), 17 deletions(-) > >diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c >index 3a1f6c520cb..8237693a567 100644 >--- a/hw/virtio/virtio.c >+++ b/hw/virtio/virtio.c >@@ -984,28 +984,23 @@ static int virtqueue_split_read_next_desc(VirtIODevice *vdev, VRingDesc *desc, > return VIRTQUEUE_READ_DESC_MORE; > } > >+/* Called within rcu_read_lock(). */ > static void virtqueue_split_get_avail_bytes(VirtQueue *vq, > unsigned int *in_bytes, unsigned int *out_bytes, >- unsigned max_in_bytes, unsigned max_out_bytes) >+ unsigned max_in_bytes, unsigned max_out_bytes, >+ VRingMemoryRegionCaches *caches) > { > VirtIODevice *vdev = vq->vdev; > unsigned int max, idx; > unsigned int total_bufs, in_total, out_total; >- VRingMemoryRegionCaches *caches; > MemoryRegionCache indirect_desc_cache = MEMORY_REGION_CACHE_INVALID; > int64_t len = 0; > int rc; > >- RCU_READ_LOCK_GUARD(); >- > idx = vq->last_avail_idx; > total_bufs = in_total = out_total = 0; > > max = vq->vring.num; >- caches = vring_get_region_caches(vq); >- if (!caches) { >- goto err; >- } > > while ((rc = virtqueue_num_heads(vq, idx)) > 0) { > MemoryRegionCache *desc_cache = &caches->desc; >@@ -1124,32 +1119,28 @@ static int virtqueue_packed_read_next_desc(VirtQueue *vq, > return VIRTQUEUE_READ_DESC_MORE; > } > >+/* Called within rcu_read_lock(). */ > static void virtqueue_packed_get_avail_bytes(VirtQueue *vq, > unsigned int *in_bytes, > unsigned int *out_bytes, > unsigned max_in_bytes, >- unsigned max_out_bytes) >+ unsigned max_out_bytes, >+ VRingMemoryRegionCaches *caches) > { > VirtIODevice *vdev = vq->vdev; > unsigned int max, idx; > unsigned int total_bufs, in_total, out_total; > MemoryRegionCache *desc_cache; >- VRingMemoryRegionCaches *caches; > MemoryRegionCache indirect_desc_cache = MEMORY_REGION_CACHE_INVALID; > int64_t len = 0; > VRingPackedDesc desc; > bool wrap_counter; > >- RCU_READ_LOCK_GUARD(); > idx = vq->last_avail_idx; > wrap_counter = vq->last_avail_wrap_counter; > total_bufs = in_total = out_total = 0; > > max = vq->vring.num; >- caches = vring_get_region_caches(vq); >- if (!caches) { >- goto err; >- } > > for (;;) { > unsigned int num_bufs = total_bufs; >@@ -1250,6 +1241,8 @@ void virtqueue_get_avail_bytes(VirtQueue *vq, unsigned int *in_bytes, > uint16_t desc_size; > VRingMemoryRegionCaches *caches; > >+ RCU_READ_LOCK_GUARD(); >+ > if (unlikely(!vq->vring.desc)) { > goto err; > } >@@ -1268,10 +1261,12 @@ void virtqueue_get_avail_bytes(VirtQueue *vq, unsigned int *in_bytes, > > if (virtio_vdev_has_feature(vq->vdev, VIRTIO_F_RING_PACKED)) { > virtqueue_packed_get_avail_bytes(vq, in_bytes, out_bytes, >- max_in_bytes, max_out_bytes); >+ max_in_bytes, max_out_bytes, >+ caches); > } else { > virtqueue_split_get_avail_bytes(vq, in_bytes, out_bytes, >- max_in_bytes, max_out_bytes); >+ max_in_bytes, max_out_bytes, >+ caches); > } > > return; >-- >2.31.1 > >
On Wed, Sep 01, 2021 at 05:55:38PM +0200, Stefano Garzarella wrote: > On Thu, Aug 26, 2021 at 07:26:58PM +0200, Philippe Mathieu-Daudé wrote: > > Both virtqueue_packed_get_avail_bytes() and > > virtqueue_split_get_avail_bytes() access the region cache, but > > their caller also does. Simplify by having virtqueue_get_avail_bytes > > calling both with RCU lock held, and passing the caches as argument. > > > > Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> > > --- > > RFC because I'm not sure this is safe enough > > It seems safe to me. > > While reviewing I saw that vring_get_region_caches() has > /* Called within rcu_read_lock(). */ comment, but it seems to me that we > call that function in places where we haven't acquired it, which shouldn't > be a problem, but should we remove that comment? Do you have specific examples? That sounds worrying because the caller can't do much with the returned pointer if it was fetched outside the RCU read lock. Stefan
On Thu, Sep 02, 2021 at 01:12:33PM +0100, Stefan Hajnoczi wrote: >On Wed, Sep 01, 2021 at 05:55:38PM +0200, Stefano Garzarella wrote: >> On Thu, Aug 26, 2021 at 07:26:58PM +0200, Philippe Mathieu-Daudé wrote: >> > Both virtqueue_packed_get_avail_bytes() and >> > virtqueue_split_get_avail_bytes() access the region cache, but >> > their caller also does. Simplify by having virtqueue_get_avail_bytes >> > calling both with RCU lock held, and passing the caches as argument. >> > >> > Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> >> > --- >> > RFC because I'm not sure this is safe enough >> >> It seems safe to me. >> >> While reviewing I saw that vring_get_region_caches() has >> /* Called within rcu_read_lock(). */ comment, but it seems to me >> that we >> call that function in places where we haven't acquired it, which shouldn't >> be a problem, but should we remove that comment? > >Do you have specific examples? That sounds worrying because the caller >can't do much with the returned pointer if it was fetched outside the >RCU read lock. > One was the virtqueue_get_avail_bytes(), but Phil is fixing it in this patch. Should we fix it in a separate patch to backport to stable branches? Other suspicious places seem to be these: - virtqueue_packed_fill_desc() - virtqueue_packed_drop_all() Stefano
On Thu, Sep 02, 2021 at 03:09:54PM +0200, Stefano Garzarella wrote: > On Thu, Sep 02, 2021 at 01:12:33PM +0100, Stefan Hajnoczi wrote: > > On Wed, Sep 01, 2021 at 05:55:38PM +0200, Stefano Garzarella wrote: > > > On Thu, Aug 26, 2021 at 07:26:58PM +0200, Philippe Mathieu-Daudé wrote: > > > > Both virtqueue_packed_get_avail_bytes() and > > > > virtqueue_split_get_avail_bytes() access the region cache, but > > > > their caller also does. Simplify by having virtqueue_get_avail_bytes > > > > calling both with RCU lock held, and passing the caches as argument. > > > > > > > > Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> > > > > --- > > > > RFC because I'm not sure this is safe enough > > > > > > It seems safe to me. > > > > > > While reviewing I saw that vring_get_region_caches() has > > > /* Called within rcu_read_lock(). */ comment, but it seems to me > > > that we > > > call that function in places where we haven't acquired it, which shouldn't > > > be a problem, but should we remove that comment? > > > > Do you have specific examples? That sounds worrying because the caller > > can't do much with the returned pointer if it was fetched outside the > > RCU read lock. > > > > One was the virtqueue_get_avail_bytes(), but Phil is fixing it in this > patch. > > Should we fix it in a separate patch to backport to stable branches? > > Other suspicious places seem to be these: > - virtqueue_packed_fill_desc() This seems to be safe in practice: only hw/net/virtio-net.c:virtio_net_receive_rcu() calls it via virtqueue_flush(). virtqueue_flush() needs a doc comment indicating that it must be called under the RCU read lock though. > - virtqueue_packed_drop_all() This looks broken. Stefan
diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c index 3a1f6c520cb..8237693a567 100644 --- a/hw/virtio/virtio.c +++ b/hw/virtio/virtio.c @@ -984,28 +984,23 @@ static int virtqueue_split_read_next_desc(VirtIODevice *vdev, VRingDesc *desc, return VIRTQUEUE_READ_DESC_MORE; } +/* Called within rcu_read_lock(). */ static void virtqueue_split_get_avail_bytes(VirtQueue *vq, unsigned int *in_bytes, unsigned int *out_bytes, - unsigned max_in_bytes, unsigned max_out_bytes) + unsigned max_in_bytes, unsigned max_out_bytes, + VRingMemoryRegionCaches *caches) { VirtIODevice *vdev = vq->vdev; unsigned int max, idx; unsigned int total_bufs, in_total, out_total; - VRingMemoryRegionCaches *caches; MemoryRegionCache indirect_desc_cache = MEMORY_REGION_CACHE_INVALID; int64_t len = 0; int rc; - RCU_READ_LOCK_GUARD(); - idx = vq->last_avail_idx; total_bufs = in_total = out_total = 0; max = vq->vring.num; - caches = vring_get_region_caches(vq); - if (!caches) { - goto err; - } while ((rc = virtqueue_num_heads(vq, idx)) > 0) { MemoryRegionCache *desc_cache = &caches->desc; @@ -1124,32 +1119,28 @@ static int virtqueue_packed_read_next_desc(VirtQueue *vq, return VIRTQUEUE_READ_DESC_MORE; } +/* Called within rcu_read_lock(). */ static void virtqueue_packed_get_avail_bytes(VirtQueue *vq, unsigned int *in_bytes, unsigned int *out_bytes, unsigned max_in_bytes, - unsigned max_out_bytes) + unsigned max_out_bytes, + VRingMemoryRegionCaches *caches) { VirtIODevice *vdev = vq->vdev; unsigned int max, idx; unsigned int total_bufs, in_total, out_total; MemoryRegionCache *desc_cache; - VRingMemoryRegionCaches *caches; MemoryRegionCache indirect_desc_cache = MEMORY_REGION_CACHE_INVALID; int64_t len = 0; VRingPackedDesc desc; bool wrap_counter; - RCU_READ_LOCK_GUARD(); idx = vq->last_avail_idx; wrap_counter = vq->last_avail_wrap_counter; total_bufs = in_total = out_total = 0; max = vq->vring.num; - caches = vring_get_region_caches(vq); - if (!caches) { - goto err; - } for (;;) { unsigned int num_bufs = total_bufs; @@ -1250,6 +1241,8 @@ void virtqueue_get_avail_bytes(VirtQueue *vq, unsigned int *in_bytes, uint16_t desc_size; VRingMemoryRegionCaches *caches; + RCU_READ_LOCK_GUARD(); + if (unlikely(!vq->vring.desc)) { goto err; } @@ -1268,10 +1261,12 @@ void virtqueue_get_avail_bytes(VirtQueue *vq, unsigned int *in_bytes, if (virtio_vdev_has_feature(vq->vdev, VIRTIO_F_RING_PACKED)) { virtqueue_packed_get_avail_bytes(vq, in_bytes, out_bytes, - max_in_bytes, max_out_bytes); + max_in_bytes, max_out_bytes, + caches); } else { virtqueue_split_get_avail_bytes(vq, in_bytes, out_bytes, - max_in_bytes, max_out_bytes); + max_in_bytes, max_out_bytes, + caches); } return;
Both virtqueue_packed_get_avail_bytes() and virtqueue_split_get_avail_bytes() access the region cache, but their caller also does. Simplify by having virtqueue_get_avail_bytes calling both with RCU lock held, and passing the caches as argument. Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> --- RFC because I'm not sure this is safe enough --- hw/virtio/virtio.c | 29 ++++++++++++----------------- 1 file changed, 12 insertions(+), 17 deletions(-)