diff mbox series

[v1] vhost: add sanity check for resubmiting reqs in split ring

Message ID 20210827051241.2448098-1-fengli@smartx.com (mailing list archive)
State Superseded
Delegated to: Maxime Coquelin
Headers show
Series [v1] vhost: add sanity check for resubmiting reqs in split ring | expand

Checks

Context Check Description
ci/iol-aarch64-unit-testing success Testing PASS
ci/intel-Testing success Testing PASS
ci/iol-x86_64-compile-testing success Testing PASS
ci/iol-mellanox-Performance success Performance Testing PASS
ci/iol-x86_64-unit-testing fail Testing issues
ci/iol-aarch64-compile-testing success Testing PASS
ci/iol-intel-Performance success Performance Testing PASS
ci/iol-intel-Functional success Functional Testing PASS
ci/iol-broadcom-Functional success Functional Testing PASS
ci/iol-broadcom-Performance success Performance Testing PASS
ci/Intel-compilation success Compilation OK
ci/github-robot: build success github build: passed
ci/checkpatch success coding style OK

Commit Message

Li Feng Aug. 27, 2021, 5:12 a.m. UTC
When getting reqs from the avail ring, the id may exceed inflight
queue size. Then the dpdk will crash forever.

Signed-off-by: Li Feng <fengli@smartx.com>
---
 lib/vhost/vhost_user.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

Comments

Li Feng Sept. 1, 2021, 5:01 a.m. UTC | #1
ping...
BTW, how could I retrigger the failure CI? The failure is not about this patch.

Thanks,
Feng Li

Li Feng <fengli@smartx.com> 于2021年8月27日周五 下午1:16写道:
>
> When getting reqs from the avail ring, the id may exceed inflight
> queue size. Then the dpdk will crash forever.
>
> 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);
> +               } else {
> +                       inflight_split->desc[last_io].inflight = 0;
> +                       rte_atomic_thread_fence(__ATOMIC_SEQ_CST);
> +               }
>                 inflight_split->used_idx = used->idx;
>         }
>
> --
> 2.31.1
>

Li Feng <fengli@smartx.com> 于2021年8月27日周五 下午1:16写道:
>
> When getting reqs from the avail ring, the id may exceed inflight
> queue size. Then the dpdk will crash forever.
>
> 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);
> +               } else {
> +                       inflight_split->desc[last_io].inflight = 0;
> +                       rte_atomic_thread_fence(__ATOMIC_SEQ_CST);
> +               }
>                 inflight_split->used_idx = used->idx;
>         }
>
> --
> 2.31.1
>
Maxime Coquelin Oct. 14, 2021, 8:17 a.m. UTC | #2
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.

> 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?

> +		} else {
> +			inflight_split->desc[last_io].inflight = 0;
> +			rte_atomic_thread_fence(__ATOMIC_SEQ_CST);
> +		}
>   		inflight_split->used_idx = used->idx;
>   	}
>   
> 

Regards,
Maxime
Maxime Coquelin Oct. 14, 2021, 8:25 a.m. UTC | #3
On 10/14/21 10:17, Maxime Coquelin wrote:
> Hi Li,
> 
> Adding Jin Yu who introduced this function.

Looks like Jin Yu has left Intel, Chenbo, could you find someone from
the Intel SPDK team to look at it?

> 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.
> 
>> 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?
> 
>> +        } else {
>> +            inflight_split->desc[last_io].inflight = 0;
>> +            rte_atomic_thread_fence(__ATOMIC_SEQ_CST);
>> +        }
>>           inflight_split->used_idx = used->idx;
>>       }
>>
> 
> Regards,
> Maxime
Xia, Chenbo Oct. 14, 2021, 8:28 a.m. UTC | #4
Hi Changpeng,

> -----Original Message-----
> From: Maxime Coquelin <maxime.coquelin@redhat.com>
> Sent: Thursday, October 14, 2021 4:26 PM
> To: Li Feng <fengli@smartx.com>; Xia, Chenbo <chenbo.xia@intel.com>
> Cc: dev@dpdk.org
> Subject: Re: [PATCH v1] vhost: add sanity check for resubmiting reqs in split
> ring
> 
> 
> 
> On 10/14/21 10:17, Maxime Coquelin wrote:
> > Hi Li,
> >
> > Adding Jin Yu who introduced this function.
> 
> Looks like Jin Yu has left Intel, Chenbo, could you find someone from
> the Intel SPDK team to look at it?

Could you or your team member help check this?

Thanks,
Chenbo

> 
> > 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.
> >
> >> 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?
> >
> >> +        } else {
> >> +            inflight_split->desc[last_io].inflight = 0;
> >> +            rte_atomic_thread_fence(__ATOMIC_SEQ_CST);
> >> +        }
> >>           inflight_split->used_idx = used->idx;
> >>       }
> >>
> >
> > Regards,
> > Maxime
Li Feng Oct. 14, 2021, 11:25 a.m. UTC | #5
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.

>
> > +             } else {
> > +                     inflight_split->desc[last_io].inflight = 0;
> > +                     rte_atomic_thread_fence(__ATOMIC_SEQ_CST);
> > +             }
> >               inflight_split->used_idx = used->idx;
> >       }
> >
> >
>
> Regards,
> Maxime
>
Maxime Coquelin Oct. 14, 2021, 11:38 a.m. UTC | #6
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.

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

Returning an error is the right thing to do anyway.

>>
>>> +             } else {
>>> +                     inflight_split->desc[last_io].inflight = 0;
>>> +                     rte_atomic_thread_fence(__ATOMIC_SEQ_CST);
>>> +             }
>>>                inflight_split->used_idx = used->idx;
>>>        }
>>>
>>>
>>
>> Regards,
>> Maxime
>>
>
Li Feng Oct. 14, 2021, 11:54 a.m. UTC | #7
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
> >>
> >
>
Liu, Changpeng Oct. 18, 2021, 1:01 a.m. UTC | #8
I agree with Maxime, just add an error log doesn't help anything, there might be something wrong in other places,
I don't have the context for this issue, if this can be reproduced in SPDK, I suggest to submit an issue to SPDK first.

> -----Original Message-----
> From: Xia, Chenbo <chenbo.xia@intel.com>
> Sent: Thursday, October 14, 2021 4:28 PM
> To: Maxime Coquelin <maxime.coquelin@redhat.com>; Li Feng
> <fengli@smartx.com>; Liu, Changpeng <changpeng.liu@intel.com>
> Cc: dev@dpdk.org
> Subject: RE: [PATCH v1] vhost: add sanity check for resubmiting reqs in split ring
> 
> Hi Changpeng,
> 
> > -----Original Message-----
> > From: Maxime Coquelin <maxime.coquelin@redhat.com>
> > Sent: Thursday, October 14, 2021 4:26 PM
> > To: Li Feng <fengli@smartx.com>; Xia, Chenbo <chenbo.xia@intel.com>
> > Cc: dev@dpdk.org
> > Subject: Re: [PATCH v1] vhost: add sanity check for resubmiting reqs in split
> > ring
> >
> >
> >
> > On 10/14/21 10:17, Maxime Coquelin wrote:
> > > Hi Li,
> > >
> > > Adding Jin Yu who introduced this function.
> >
> > Looks like Jin Yu has left Intel, Chenbo, could you find someone from
> > the Intel SPDK team to look at it?
> 
> Could you or your team member help check this?
> 
> Thanks,
> Chenbo
> 
> >
> > > 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.
> > >
> > >> 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?
> > >
> > >> +        } else {
> > >> +            inflight_split->desc[last_io].inflight = 0;
> > >> +            rte_atomic_thread_fence(__ATOMIC_SEQ_CST);
> > >> +        }
> > >>           inflight_split->used_idx = used->idx;
> > >>       }
> > >>
> > >
> > > Regards,
> > > Maxime
diff mbox series

Patch

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);
+		} else {
+			inflight_split->desc[last_io].inflight = 0;
+			rte_atomic_thread_fence(__ATOMIC_SEQ_CST);
+		}
 		inflight_split->used_idx = used->idx;
 	}