Re: [BUG] vhost-vdpa: qemu-system-s390x crashes with second virtio-net-ccw device

[Jason Wang <jasowang@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 2592 bytes --]


On 2020/7/24 下午11:34, Cornelia Huck wrote:
> On Fri, 24 Jul 2020 11:17:57 -0400
> "Michael S. Tsirkin"<mst@redhat.com>  wrote:
>
>> On Fri, Jul 24, 2020 at 04:56:27PM +0200, Cornelia Huck wrote:
>>> On Fri, 24 Jul 2020 09:30:58 -0400
>>> "Michael S. Tsirkin"<mst@redhat.com>  wrote:
>>>    
>>>> On Fri, Jul 24, 2020 at 03:27:18PM +0200, Cornelia Huck wrote:
>>>>> When I start qemu with a second virtio-net-ccw device (i.e. adding
>>>>> -device virtio-net-ccw in addition to the autogenerated device), I get
>>>>> a segfault. gdb points to
>>>>>
>>>>> #0  0x000055d6ab52681d in virtio_net_get_config (vdev=<optimized out>,
>>>>>      config=0x55d6ad9e3f80 "RT") at /home/cohuck/git/qemu/hw/net/virtio-net.c:146
>>>>> 146	    if (nc->peer->info->type == NET_CLIENT_DRIVER_VHOST_VDPA) {
>>>>>
>>>>> (backtrace doesn't go further)
>>> The core was incomplete, but running under gdb directly shows that it
>>> is just a bog-standard config space access (first for that device).
>>>
>>> The cause of the crash is that nc->peer is not set... no idea how that
>>> can happen, not that familiar with that part of QEMU. (Should the code
>>> check, or is that really something that should not happen?)
>>>
>>> What I don't understand is why it is set correctly for the first,
>>> autogenerated virtio-net-ccw device, but not for the second one, and
>>> why virtio-net-pci doesn't show these problems. The only difference
>>> between -ccw and -pci that comes to my mind here is that config space
>>> accesses for ccw are done via an asynchronous operation, so timing
>>> might be different.
>> Hopefully Jason has an idea. Could you post a full command line
>> please? Do you need a working guest to trigger this? Does this trigger
>> on an x86 host?
> Yes, it does trigger with tcg-on-x86 as well. I've been using
>
> s390x-softmmu/qemu-system-s390x -M s390-ccw-virtio,accel=tcg -cpu qemu,zpci=on
> -m 1024 -nographic -device virtio-scsi-ccw,id=scsi0,devno=fe.0.0001
> -drive file=/path/to/image,format=qcow2,if=none,id=drive-scsi0-0-0-0
> -device scsi-hd,bus=scsi0.0,channel=0,scsi-id=0,lun=0,drive=drive-scsi0-0-0-0,id=scsi0-0-0-0,bootindex=1
> -device virtio-net-ccw
>
> It seems it needs the guest actually doing something with the nics; I
> cannot reproduce the crash if I use the old advent calendar moon buggy
> image and just add a virtio-net-ccw device.
>
> (I don't think it's a problem with my local build, as I see the problem
> both on my laptop and on an LPAR.)


It looks to me we forget the check the existence of peer.

Please try the attached patch to see if it works.

Thanks


[-- Attachment #2: 0001-virtio-net-check-the-existence-of-peer-before-accesi.patch --]
[-- Type: text/x-patch, Size: 2822 bytes --]

From f6959056dcc65cbdc256c4af2b1a0eaee784c15f Mon Sep 17 00:00:00 2001
From: Jason Wang <jasowang@redhat.com>
Date: Sat, 25 Jul 2020 08:13:17 +0800
Subject: [PATCH] virtio-net: check the existence of peer before accesing its
 config

We try to get config from peer unconditionally which may lead NULL
pointer dereference. Add a check before trying to access the config.

Fixes: 108a64818e69b ("vhost-vdpa: introduce vhost-vdpa backend")
Signed-off-by: Jason Wang <jasowang@redhat.com>
---
 hw/net/virtio-net.c | 22 +++++++++++-----------
 1 file changed, 11 insertions(+), 11 deletions(-)

diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index 4895af1cbe..935b9ef5c7 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -125,6 +125,7 @@ static void virtio_net_get_config(VirtIODevice *vdev, uint8_t *config)
 {
     VirtIONet *n = VIRTIO_NET(vdev);
     struct virtio_net_config netcfg;
+    NetClientState *nc = qemu_get_queue(n->nic);
 
     int ret = 0;
     memset(&netcfg, 0 , sizeof(struct virtio_net_config));
@@ -142,13 +143,12 @@ static void virtio_net_get_config(VirtIODevice *vdev, uint8_t *config)
                  VIRTIO_NET_RSS_SUPPORTED_HASHES);
     memcpy(config, &netcfg, n->config_size);
 
-    NetClientState *nc = qemu_get_queue(n->nic);
-    if (nc->peer->info->type == NET_CLIENT_DRIVER_VHOST_VDPA) {
+    if (nc->peer && nc->peer->info->type == NET_CLIENT_DRIVER_VHOST_VDPA) {
         ret = vhost_net_get_config(get_vhost_net(nc->peer), (uint8_t *)&netcfg,
-                             n->config_size);
-    if (ret != -1) {
-        memcpy(config, &netcfg, n->config_size);
-    }
+                                   n->config_size);
+        if (ret != -1) {
+            memcpy(config, &netcfg, n->config_size);
+        }
     }
 }
 
@@ -156,6 +156,7 @@ static void virtio_net_set_config(VirtIODevice *vdev, const uint8_t *config)
 {
     VirtIONet *n = VIRTIO_NET(vdev);
     struct virtio_net_config netcfg = {};
+    NetClientState *nc = qemu_get_queue(n->nic);
 
     memcpy(&netcfg, config, n->config_size);
 
@@ -166,11 +167,10 @@ static void virtio_net_set_config(VirtIODevice *vdev, const uint8_t *config)
         qemu_format_nic_info_str(qemu_get_queue(n->nic), n->mac);
     }
 
-    NetClientState *nc = qemu_get_queue(n->nic);
-    if (nc->peer->info->type == NET_CLIENT_DRIVER_VHOST_VDPA) {
-        vhost_net_set_config(get_vhost_net(nc->peer), (uint8_t *)&netcfg,
-                               0, n->config_size,
-                        VHOST_SET_CONFIG_TYPE_MASTER);
+    if (nc->peer && nc->peer->info->type == NET_CLIENT_DRIVER_VHOST_VDPA) {
+        vhost_net_set_config(get_vhost_net(nc->peer),
+                             (uint8_t *)&netcfg, 0, n->config_size,
+                             VHOST_SET_CONFIG_TYPE_MASTER);
       }
 }
 
-- 
2.20.1