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

[Stefano Garzarella]

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