Virtualization - May 2022 - [PATCH] vringh: Fix maximum number check for indirect descriptors

On Thu, May 5, 2022 at 4:06 PM Yongji Xie <xieyongji at bytedance.com>
wrote:
>
> On Thu, May 5, 2022 at 3:47 PM Jason Wang <jasowang at redhat.com>
wrote:
> >
> > On Wed, May 4, 2022 at 4:12 PM Xie Yongji <xieyongji at
bytedance.com> wrote:
> > >
> > > We should use size of descriptor chain to check the maximum
> > > number of consumed descriptors in indirect case.
> >
> > AFAIK, it's a guard for loop descriptors.
> >
>
> Yes, but for indirect descriptors, we know the size of the descriptor
> chain. Should we use it to test loop condition rather than using
> virtqueue size?
Yes.
>
> > > And the
> > > statistical counts should also be reset to zero each time
> > > we get an indirect descriptor.
> >
> > What might happen if we don't have this patch?
> >
>
> Then we can't handle the case that one request includes multiple
> indirect descriptors. Although I never see this kind of case now, the
> spec doesn't forbid it.
It looks to me we need to introduce dedicated counters for indirect
descriptors instead of trying to use a single counter?

(All evils came from the move_to_indirect()/return_from_indierct()
logic, vhost have dedicated helper to deal with indirect descriptors -
get_indirect()).

Thanks

>
> > >
> > > Fixes: f87d0fbb5798 ("vringh: host-side implementation of
virtio rings.")
> > > Signed-off-by: Xie Yongji <xieyongji at bytedance.com>
> > > Signed-off-by: Fam Zheng <fam.zheng at bytedance.com>
> > > ---
> > >  drivers/vhost/vringh.c | 4 +++-
> > >  1 file changed, 3 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/vhost/vringh.c b/drivers/vhost/vringh.c
> > > index 14e2043d7685..c1810b77a05e 100644
> > > --- a/drivers/vhost/vringh.c
> > > +++ b/drivers/vhost/vringh.c
> > > @@ -344,12 +344,13 @@ __vringh_iov(struct vringh *vrh, u16 i,
> > >                         addr = (void *)(long)(a + range.offset);
> > >                         err = move_to_indirect(vrh, &up_next,
&i, addr, &desc,
> > >                                                &descs,
&desc_max);
> > > +                       count = 0;
> >
> > Then it looks to me we can detect a loop indirect descriptor chain?
> >
>
> I think so.
>
> Thanks,
> Yongji
>