Re: [RFC PATCH 3/3] hw/virtio: Have virtqueue_get_avail_bytes() pass caches arg to callees

[Stefan Hajnoczi <stefanha@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 1771 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]