[v3,0/2] vhost fixes for OVS SIGSEGV in PMD

  This is an alternative, more general fix compared with PATCH v1,
and fixes style issues in v2.

The series fixes a segmentation fault in the OVS PMD thread when
resynchronizing with QEMU after the guest application has been killed
with SIGKILL (patch 1/2),

The segmentation fault can be caused by the guest DPDK application,
which is able this way to crash the OVS process on the host,
see the backtrace in patch 1/2.

Patch 2/2 is an additional improvement in the current error handling.

---
Changes from v2: fix warnings from checkpatch.
---

Changes from v1:

* patch 1/2: instead of only fixing virtio_dev_tx_split, put the check
  for nr_vec == 0 inside desc_to_mbuf and mbuf_to_desc, so that in no
  case they attempt to read and dereference addresses from the buf_vec[]
  array when it does not contain any valid elements.

---

For your review and comments,

Claudio

Claudio Fontana (2):
  vhost: check for nr_vec == 0 in desc_to_mbuf, mbuf_to_desc
  vhost: improve error handling in desc_to_mbuf

 lib/vhost/virtio_net.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)
  

On Tue,  2 Aug 2022 02:49:36 +0200
Claudio Fontana <cfontana@suse.de> wrote:

> This is an alternative, more general fix compared with PATCH v1,
> and fixes style issues in v2.
> 
> The series fixes a segmentation fault in the OVS PMD thread when
> resynchronizing with QEMU after the guest application has been killed
> with SIGKILL (patch 1/2),
> 
> The segmentation fault can be caused by the guest DPDK application,
> which is able this way to crash the OVS process on the host,
> see the backtrace in patch 1/2.
> 
> Patch 2/2 is an additional improvement in the current error handling.

Checking for NULL and 0 is good on host side.
But guest should probably not be sending such a useless request?
  

On 8/2/22 03:40, Stephen Hemminger wrote:
> On Tue,  2 Aug 2022 02:49:36 +0200
> Claudio Fontana <cfontana@suse.de> wrote:
> 
>> This is an alternative, more general fix compared with PATCH v1,
>> and fixes style issues in v2.
>>
>> The series fixes a segmentation fault in the OVS PMD thread when
>> resynchronizing with QEMU after the guest application has been killed
>> with SIGKILL (patch 1/2),
>>
>> The segmentation fault can be caused by the guest DPDK application,
>> which is able this way to crash the OVS process on the host,
>> see the backtrace in patch 1/2.
>>
>> Patch 2/2 is an additional improvement in the current error handling.
> 
> Checking for NULL and 0 is good on host side.
> But guest should probably not be sending such a useless request?


Right, I focused on hardening the host side, as that is what the customer required.

This happens specifically when the guest application goes away abruptly and has no chance to signal anything (SIGKILL),
and at restart issues a virtio reset on the device, which in qemu causes also a (actually two) virtio_net set_status, which attempt to stop the queues (twice).

DPDK seems to think at that point that it needs to drain the queue, and tries to process MAX_PKT_BURST buffers
("about to dequeue 32 buffers"),

then calls fill_vec_buf_split and gets absolutely nothing.

I think this should also address the reports in this thread:

https://inbox.dpdk.org/dev/SA1PR08MB713373B0D19329C38C7527BB839A9@SA1PR08MB7133.namprd08.prod.outlook.com/

in addition to my specific customer request,

Thanks,

Claudio
  

On 8/2/22 19:20, Claudio Fontana wrote:
> On 8/2/22 03:40, Stephen Hemminger wrote:
>> On Tue,  2 Aug 2022 02:49:36 +0200
>> Claudio Fontana <cfontana@suse.de> wrote:
>>
>>> This is an alternative, more general fix compared with PATCH v1,
>>> and fixes style issues in v2.
>>>
>>> The series fixes a segmentation fault in the OVS PMD thread when
>>> resynchronizing with QEMU after the guest application has been killed
>>> with SIGKILL (patch 1/2),
>>>
>>> The segmentation fault can be caused by the guest DPDK application,
>>> which is able this way to crash the OVS process on the host,
>>> see the backtrace in patch 1/2.
>>>
>>> Patch 2/2 is an additional improvement in the current error handling.
>>
>> Checking for NULL and 0 is good on host side.
>> But guest should probably not be sending such a useless request?
> 
> 
> Right, I focused on hardening the host side, as that is what the customer required.
> 
> This happens specifically when the guest application goes away abruptly and has no chance to signal anything (SIGKILL),
> and at restart issues a virtio reset on the device, which in qemu causes also a (actually two) virtio_net set_status, which attempt to stop the queues (twice).
> 
> DPDK seems to think at that point that it needs to drain the queue, and tries to process MAX_PKT_BURST buffers
> ("about to dequeue 32 buffers"),
> 
> then calls fill_vec_buf_split and gets absolutely nothing.
> 
> I think this should also address the reports in this thread:
> 
> https://inbox.dpdk.org/dev/SA1PR08MB713373B0D19329C38C7527BB839A9@SA1PR08MB7133.namprd08.prod.outlook.com/
> 
> in addition to my specific customer request,
> 
> Thanks,
> 
> Claudio

anything more required from my side? Do you need a respin without the "Tested-by" tag?

Thanks,

Claudio
  

A weekly ping on this one,

any chance to get this fix for a guest-triggered host crash included?

Thanks,

Claudio

On 8/2/22 02:49, Claudio Fontana wrote:
> This is an alternative, more general fix compared with PATCH v1,
> and fixes style issues in v2.
> 
> The series fixes a segmentation fault in the OVS PMD thread when
> resynchronizing with QEMU after the guest application has been killed
> with SIGKILL (patch 1/2),
> 
> The segmentation fault can be caused by the guest DPDK application,
> which is able this way to crash the OVS process on the host,
> see the backtrace in patch 1/2.
> 
> Patch 2/2 is an additional improvement in the current error handling.
> 
> ---
> Changes from v2: fix warnings from checkpatch.
> ---
> 
> Changes from v1:
> 
> * patch 1/2: instead of only fixing virtio_dev_tx_split, put the check
>   for nr_vec == 0 inside desc_to_mbuf and mbuf_to_desc, so that in no
>   case they attempt to read and dereference addresses from the buf_vec[]
>   array when it does not contain any valid elements.
> 
> ---
> 
> For your review and comments,
> 
> Claudio
> 
> Claudio Fontana (2):
>   vhost: check for nr_vec == 0 in desc_to_mbuf, mbuf_to_desc
>   vhost: improve error handling in desc_to_mbuf
> 
>  lib/vhost/virtio_net.c | 16 ++++++++++++----
>  1 file changed, 12 insertions(+), 4 deletions(-)
>