Re: [dpdk-dev] [PATCH v1] vhost: add sanity check for resubmiting reqs in split ring

[Li Feng <fengli@smartx.com>]

On Thu, Oct 14, 2021 at 7:38 PM Maxime Coquelin
<maxime.coquelin@redhat.com> wrote:
>
>
>
> On 10/14/21 13:25, Li Feng wrote:
> > Thank you for your response.
> >
> > On Thu, Oct 14, 2021 at 4:17 PM Maxime Coquelin
> > <maxime.coquelin@redhat.com> wrote:
> >>
> >> Hi Li,
> >>
> >> Adding Jin Yu who introduced this function.
> >>
> >> On 8/27/21 07:12, Li Feng wrote:
> >>> When getting reqs from the avail ring, the id may exceed inflight
> >>> queue size. Then the dpdk will crash forever.
> >>
> >> You need to add Fixes tag and Cc stable@dpdk.org so that it can be
> >> backported.
> > OK, I will send the v2 version.
> >
> >>
> >>> Signed-off-by: Li Feng <fengli@smartx.com>
> >>> ---
> >>>    lib/vhost/vhost_user.c | 10 ++++++++--
> >>>    1 file changed, 8 insertions(+), 2 deletions(-)
> >>>
> >>> diff --git a/lib/vhost/vhost_user.c b/lib/vhost/vhost_user.c
> >>> index 29a4c9af60..f09d0f6a48 100644
> >>> --- a/lib/vhost/vhost_user.c
> >>> +++ b/lib/vhost/vhost_user.c
> >>> @@ -1823,8 +1823,14 @@ vhost_check_queue_inflights_split(struct virtio_net *dev,
> >>>        last_io = inflight_split->last_inflight_io;
> >>>
> >>>        if (inflight_split->used_idx != used->idx) {
> >>> -             inflight_split->desc[last_io].inflight = 0;
> >>> -             rte_atomic_thread_fence(__ATOMIC_SEQ_CST);
> >>> +             if (unlikely(last_io >= inflight_split->desc_num)) {
> >>> +                     VHOST_LOG_CONFIG(ERR, "last_inflight_io '%"PRIu16"' exceeds inflight "
> >>> +                             "queue size (%"PRIu16").\n", last_io,
> >>> +                             inflight_split->desc_num);
> >>
> >> If such error happens, shouldn't we return RTE_VHOST_MSG_RESULT_ERR
> >> instead of just logging an error?
> > I think ignoring the error is ok. No one could handle this error correctly.
> > At this time the guest virtio driver of this virtqueue may be in an
> > incorrect state.
>
> Not sure to understand how it can happen.
> But I see that last_io is actually vq->inflight_split->last_inflight_io,
> which is set only by rte_vhost_set_last_inflight_io_split() API.
The polluted value is from the frontend driver.
My environment occurs this issue, and a VM is hang, so I guess this
bad value comes from it.

>
> Shouldn't there be a sanity check there to ensure that last_inflight_io
> is smaller than desc_num value set by the frontend?

Yes, putting a check in rte_vhost_set_last_inflight_io_split is also ok.
I will send the v2 version that includes this.

Thanks.
>
> Returning an error is the right thing to do anyway.
OK.
>
> >>
> >>> +             } else {
> >>> +                     inflight_split->desc[last_io].inflight = 0;
> >>> +                     rte_atomic_thread_fence(__ATOMIC_SEQ_CST);
> >>> +             }
> >>>                inflight_split->used_idx = used->idx;
> >>>        }
> >>>
> >>>
> >>
> >> Regards,
> >> Maxime
> >>
> >
>