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é <phi...@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
signature.asc
Description: PGP signature
