virtio-net: count VIRTIO_NET_F_MAC when calculating config_len

On Mon, Apr 29, 2013 at 07:21:06PM +0300, Michael S. Tsirkin wrote:
> On Mon, Apr 29, 2013 at 06:14:41PM +0200, KONRAD Frédéric wrote:
> > On 29/04/2013 18:02, Michael S. Tsirkin wrote:
> > >On Mon, Apr 29, 2013 at 10:55:36AM -0500, Jesse Larrew wrote:
> > >>On 04/29/2013 10:29 AM, KONRAD Frédéric wrote:
> > >>>On 29/04/2013 17:14, Jesse Larrew wrote:
> > >>>>On 04/29/2013 09:55 AM, KONRAD Frédéric wrote:
> > >>>>>On 29/04/2013 16:42, Jesse Larrew wrote:
> > >>>>>>On 04/25/2013 01:59 AM, Michael S. Tsirkin wrote:
> > >>>>>>>On Thu, Apr 25, 2013 at 02:21:29PM +0800, Jason Wang wrote:
> > >>>>>>>>Commit 14f9b664 (hw/virtio-net.c: set config size using host features) tries to
> > >>>>>>>>calculate config size based on the host features. But it forgets the
> > >>>>>>>>VIRTIO_NET_F_MAC were always set for qemu later. This will lead a zero config
> > >>>>>>>>len for virtio-net device when both VIRTIO_NET_F_STATUS and VIRTIO_NET_F_MQ were
> > >>>>>>>>disabled form command line. Then qemu will crash when user tries to read the
> > >>>>>>>>config of virtio-net.
> > >>>>>>>>
> > >>>>>>>>Fix this by counting VIRTIO_NET_F_MAC and make sure the config at least contains
> > >>>>>>>>the mac address.
> > >>>>>>>>
> > >>>>>>>>Cc: Jesse Larrew <jlarrew@linux.vnet.ibm.com>
> > >>>>>>>>Signed-off-by: Jason Wang <jasowang@redhat.com>
> > >>>>>>>>---
> > >>>>>>>>    hw/net/virtio-net.c |    3 ++-
> > >>>>>>>>    1 files changed, 2 insertions(+), 1 deletions(-)
> > >>>>>>>>
> > >>>>>>>>diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> > >>>>>>>>index 70c8fce..33a70ef 100644
> > >>>>>>>>--- a/hw/net/virtio-net.c
> > >>>>>>>>+++ b/hw/net/virtio-net.c
> > >>>>>>>>@@ -1264,7 +1264,8 @@ static void virtio_net_guest_notifier_mask(VirtIODevice *vdev, int idx,
> > >>>>>>>>      void virtio_net_set_config_size(VirtIONet *n, uint32_t host_features)
> > >>>>>>>>    {
> > >>>>>>>>-    int i, config_size = 0;
> > >>>>>>>>+    /* VIRTIO_NET_F_MAC can't be disabled from qemu side */
> > >>>>>>>>+    int i, config_size = feature_sizes[0].end;
> > >>>>>>>This would be cleaner:
> > >>>>>>>      host_features |= (1 << VIRTIO_NET_F_MAC);
> > >>>>>>>
> > >>>>>>>no need for a comment then.
> > >>>>>>>
> > >>>>>>It seems to me that the real problem here is that host_features isn't
> > >>>>>>properly populated before virtio_net_set_config_size() is called. Looking
> > >>>>>>at virtio_device_init(), we can see why:
> > >>>>>>
> > >>>>>>static int virtio_device_init(DeviceState *qdev)
> > >>>>>>{
> > >>>>>>       VirtIODevice *vdev = VIRTIO_DEVICE(qdev);
> > >>>>>>       VirtioDeviceClass *k = VIRTIO_DEVICE_GET_CLASS(qdev);
> > >>>>>>       assert(k->init != NULL);
> > >>>>>>       if (k->init(vdev) < 0) {
> > >>>>>>           return -1;
> > >>>>>>       }
> > >>>>>>       virtio_bus_plug_device(vdev);
> > >>>>>>       return 0;
> > >>>>>>}
> > >>>>>>
> > >>>>>>virtio_net_set_config_size() is currently being called as part of the
> > >>>>>>k->init call, but the host_features aren't properly setup until the device
> > >>>>>>is plugged into the bus using virtio_bus_plug_device().
> > >>>>>>
> > >>>>>>After talking with mdroth, I think the proper way to fix this would be to
> > >>>>>>extend VirtioDeviceClass to include a calculate_config_size() method that
> > >>>>>>can be called at the appropriate time during device initialization like so:
> > >>>>>>
> > >>>>>>static int virtio_device_init(DeviceState *qdev)
> > >>>>>>{
> > >>>>>>       VirtIODevice *vdev = VIRTIO_DEVICE(qdev);
> > >>>>>>       VirtioDeviceClass *k = VIRTIO_DEVICE_GET_CLASS(qdev);
> > >>>>>>       assert(k->init != NULL);
> > >>>>>>       if (k->init(vdev) < 0) {
> > >>>>>>           return -1;
> > >>>>>>       }
> > >>>>>>       virtio_bus_plug_device(vdev);
> > >>>>>>+   if (k->calculate_config_size && k->calculate_config_size(vdev) < 0) {
> > >>>>>>+       return -1;
> > >>>>>>+   }
> > >>>>>>       return 0;
> > >>>>>>}
> > >>>>>>
> > >>>>>>This would ensure that host_features contains the proper data before any
> > >>>>>>devices try to make use of it to calculate the config size.
> > >>>>>Good point, I didn't saw that.
> > >>>>>
> > >>>>>but this was not the case with commit 14f9b664 no?
> > >>>>>
> > >>>>I suspect this bug was present in 14f9b664 as well. We just hadn't triggered
> > >>>>it yet. I'll confirm this afternoon.
> > >>>Ok, I think there is a problem with your patch:
> > >>>
> > >>>     virtio_init(VIRTIO_DEVICE(n), "virtio-net", VIRTIO_ID_NET,
> > >>>                                   n->config_size);
> > >>>
> > >>>is called in virtio_net_device_init (k->init).
> > >>>
> > >>>Is there a way to resize the config after that?
> > >>>
> > >>Nope. That's definitely a bug. I can send a patch to fix this today. Any
> > >>objection to the method suggested above (extending VirtioDeviceClass)?
> > >This needs more thought. All devices expected everything
> > >is setup during the init call. config size is
> > >likely not the only issue.
> > >
> > >So we need almost all of virtio_bus_plug_device before init,
> > >and then after init send the signal:
> > >
> > >     if (klass->device_plugged != NULL) {
> > >         klass->device_plugged(qbus->parent);
> > >     }
> > 
> > Seems the interesting part is in virtio_pci_device_plugged at the end:
> > 
> >     proxy->host_features |= 0x1 << VIRTIO_F_NOTIFY_ON_EMPTY;
> >     proxy->host_features |= 0x1 << VIRTIO_F_BAD_FEATURE;
> >     proxy->host_features = virtio_bus_get_vdev_features(bus,
> > proxy->host_features);
> > 
> > This is and was called after set_config_size, isn't that the issue?
> 
> Basically devices expected everything to be setup at
> the point where init is called.
> We found one bug but let's fix it properly.
> There's no reason to delay parts of setup until after init,
> if we do, we'll trip on more uses of uninitialized data.
> 
> > >
> > >
> > >>>>>>>>        for (i = 0; feature_sizes[i].flags != 0; i++) {
> > >>>>>>>>            if (host_features & feature_sizes[i].flags) {
> > >>>>>>>>                config_size = MAX(feature_sizes[i].end, config_size);
> > >>>>>>>>-- 
> > >>>>>>>>1.7.1
> > >>>>Jesse Larrew
> > >>>>Software Engineer, KVM Team
> > >>>>IBM Linux Technology Center
> > >>>>Phone: (512) 973-2052 (T/L: 363-2052)
> > >>>>jlarrew@linux.vnet.ibm.com
> > >>>>
> > >>
> > >>-- 
> > >>
> > >>Jesse Larrew
> > >>Software Engineer, KVM Team
> > >>IBM Linux Technology Center
> > >>Phone: (512) 973-2052 (T/L: 363-2052)
> > >>jlarrew@linux.vnet.ibm.com


Basically this is what I propose. Warning! compile-tested
only. (I also dropped an unused return value).


Signed-off-by: Michael S. Tsirkin <mst@redhat.com>

---

On 29/04/2013 18:30, Michael S. Tsirkin wrote:
> On Mon, Apr 29, 2013 at 07:21:06PM +0300, Michael S. Tsirkin wrote:
>> On Mon, Apr 29, 2013 at 06:14:41PM +0200, KONRAD Frédéric wrote:
>>> On 29/04/2013 18:02, Michael S. Tsirkin wrote:
>>>> On Mon, Apr 29, 2013 at 10:55:36AM -0500, Jesse Larrew wrote:
>>>>> On 04/29/2013 10:29 AM, KONRAD Frédéric wrote:
>>>>>> On 29/04/2013 17:14, Jesse Larrew wrote:
>>>>>>> On 04/29/2013 09:55 AM, KONRAD Frédéric wrote:
>>>>>>>> On 29/04/2013 16:42, Jesse Larrew wrote:
>>>>>>>>> On 04/25/2013 01:59 AM, Michael S. Tsirkin wrote:
>>>>>>>>>> On Thu, Apr 25, 2013 at 02:21:29PM +0800, Jason Wang wrote:
>>>>>>>>>>> Commit 14f9b664 (hw/virtio-net.c: set config size using host features) tries to
>>>>>>>>>>> calculate config size based on the host features. But it forgets the
>>>>>>>>>>> VIRTIO_NET_F_MAC were always set for qemu later. This will lead a zero config
>>>>>>>>>>> len for virtio-net device when both VIRTIO_NET_F_STATUS and VIRTIO_NET_F_MQ were
>>>>>>>>>>> disabled form command line. Then qemu will crash when user tries to read the
>>>>>>>>>>> config of virtio-net.
>>>>>>>>>>>
>>>>>>>>>>> Fix this by counting VIRTIO_NET_F_MAC and make sure the config at least contains
>>>>>>>>>>> the mac address.
>>>>>>>>>>>
>>>>>>>>>>> Cc: Jesse Larrew <jlarrew@linux.vnet.ibm.com>
>>>>>>>>>>> Signed-off-by: Jason Wang <jasowang@redhat.com>
>>>>>>>>>>> ---
>>>>>>>>>>>     hw/net/virtio-net.c |    3 ++-
>>>>>>>>>>>     1 files changed, 2 insertions(+), 1 deletions(-)
>>>>>>>>>>>
>>>>>>>>>>> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
>>>>>>>>>>> index 70c8fce..33a70ef 100644
>>>>>>>>>>> --- a/hw/net/virtio-net.c
>>>>>>>>>>> +++ b/hw/net/virtio-net.c
>>>>>>>>>>> @@ -1264,7 +1264,8 @@ static void virtio_net_guest_notifier_mask(VirtIODevice *vdev, int idx,
>>>>>>>>>>>       void virtio_net_set_config_size(VirtIONet *n, uint32_t host_features)
>>>>>>>>>>>     {
>>>>>>>>>>> -    int i, config_size = 0;
>>>>>>>>>>> +    /* VIRTIO_NET_F_MAC can't be disabled from qemu side */
>>>>>>>>>>> +    int i, config_size = feature_sizes[0].end;
>>>>>>>>>> This would be cleaner:
>>>>>>>>>>       host_features |= (1 << VIRTIO_NET_F_MAC);
>>>>>>>>>>
>>>>>>>>>> no need for a comment then.
>>>>>>>>>>
>>>>>>>>> It seems to me that the real problem here is that host_features isn't
>>>>>>>>> properly populated before virtio_net_set_config_size() is called. Looking
>>>>>>>>> at virtio_device_init(), we can see why:
>>>>>>>>>
>>>>>>>>> static int virtio_device_init(DeviceState *qdev)
>>>>>>>>> {
>>>>>>>>>        VirtIODevice *vdev = VIRTIO_DEVICE(qdev);
>>>>>>>>>        VirtioDeviceClass *k = VIRTIO_DEVICE_GET_CLASS(qdev);
>>>>>>>>>        assert(k->init != NULL);
>>>>>>>>>        if (k->init(vdev) < 0) {
>>>>>>>>>            return -1;
>>>>>>>>>        }
>>>>>>>>>        virtio_bus_plug_device(vdev);
>>>>>>>>>        return 0;
>>>>>>>>> }
>>>>>>>>>
>>>>>>>>> virtio_net_set_config_size() is currently being called as part of the
>>>>>>>>> k->init call, but the host_features aren't properly setup until the device
>>>>>>>>> is plugged into the bus using virtio_bus_plug_device().
>>>>>>>>>
>>>>>>>>> After talking with mdroth, I think the proper way to fix this would be to
>>>>>>>>> extend VirtioDeviceClass to include a calculate_config_size() method that
>>>>>>>>> can be called at the appropriate time during device initialization like so:
>>>>>>>>>
>>>>>>>>> static int virtio_device_init(DeviceState *qdev)
>>>>>>>>> {
>>>>>>>>>        VirtIODevice *vdev = VIRTIO_DEVICE(qdev);
>>>>>>>>>        VirtioDeviceClass *k = VIRTIO_DEVICE_GET_CLASS(qdev);
>>>>>>>>>        assert(k->init != NULL);
>>>>>>>>>        if (k->init(vdev) < 0) {
>>>>>>>>>            return -1;
>>>>>>>>>        }
>>>>>>>>>        virtio_bus_plug_device(vdev);
>>>>>>>>> +   if (k->calculate_config_size && k->calculate_config_size(vdev) < 0) {
>>>>>>>>> +       return -1;
>>>>>>>>> +   }
>>>>>>>>>        return 0;
>>>>>>>>> }
>>>>>>>>>
>>>>>>>>> This would ensure that host_features contains the proper data before any
>>>>>>>>> devices try to make use of it to calculate the config size.
>>>>>>>> Good point, I didn't saw that.
>>>>>>>>
>>>>>>>> but this was not the case with commit 14f9b664 no?
>>>>>>>>
>>>>>>> I suspect this bug was present in 14f9b664 as well. We just hadn't triggered
>>>>>>> it yet. I'll confirm this afternoon.
>>>>>> Ok, I think there is a problem with your patch:
>>>>>>
>>>>>>      virtio_init(VIRTIO_DEVICE(n), "virtio-net", VIRTIO_ID_NET,
>>>>>>                                    n->config_size);
>>>>>>
>>>>>> is called in virtio_net_device_init (k->init).
>>>>>>
>>>>>> Is there a way to resize the config after that?
>>>>>>
>>>>> Nope. That's definitely a bug. I can send a patch to fix this today. Any
>>>>> objection to the method suggested above (extending VirtioDeviceClass)?
>>>> This needs more thought. All devices expected everything
>>>> is setup during the init call. config size is
>>>> likely not the only issue.
>>>>
>>>> So we need almost all of virtio_bus_plug_device before init,
>>>> and then after init send the signal:
>>>>
>>>>      if (klass->device_plugged != NULL) {
>>>>          klass->device_plugged(qbus->parent);
>>>>      }
>>> Seems the interesting part is in virtio_pci_device_plugged at the end:
>>>
>>>      proxy->host_features |= 0x1 << VIRTIO_F_NOTIFY_ON_EMPTY;
>>>      proxy->host_features |= 0x1 << VIRTIO_F_BAD_FEATURE;
>>>      proxy->host_features = virtio_bus_get_vdev_features(bus,
>>> proxy->host_features);
>>>
>>> This is and was called after set_config_size, isn't that the issue?
>> Basically devices expected everything to be setup at
>> the point where init is called.
>> We found one bug but let's fix it properly.
>> There's no reason to delay parts of setup until after init,
>> if we do, we'll trip on more uses of uninitialized data.
>>
>>>>
>>>>>>>>>>>         for (i = 0; feature_sizes[i].flags != 0; i++) {
>>>>>>>>>>>             if (host_features & feature_sizes[i].flags) {
>>>>>>>>>>>                 config_size = MAX(feature_sizes[i].end, config_size);
>>>>>>>>>>> -- 
>>>>>>>>>>> 1.7.1
>>>>>>> Jesse Larrew
>>>>>>> Software Engineer, KVM Team
>>>>>>> IBM Linux Technology Center
>>>>>>> Phone: (512) 973-2052 (T/L: 363-2052)
>>>>>>> jlarrew@linux.vnet.ibm.com
>>>>>>>
>>>>> -- 
>>>>>
>>>>> Jesse Larrew
>>>>> Software Engineer, KVM Team
>>>>> IBM Linux Technology Center
>>>>> Phone: (512) 973-2052 (T/L: 363-2052)
>>>>> jlarrew@linux.vnet.ibm.com
>
> Basically this is what I propose. Warning! compile-tested
> only. (I also dropped an unused return value).
>
>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>
> ---
>
> diff --git a/hw/virtio/virtio-bus.c b/hw/virtio/virtio-bus.c
> index 1596a1c..c75c8ae 100644
> --- a/hw/virtio/virtio-bus.c
> +++ b/hw/virtio/virtio-bus.c
> @@ -38,7 +38,7 @@ do { printf("virtio_bus: " fmt , ## __VA_ARGS__); } while (0)
>   #endif
>   
>   /* Plug the VirtIODevice */
> -int virtio_bus_plug_device(VirtIODevice *vdev)
> +void virtio_bus_plug_device(VirtIODevice *vdev)
>   {
>       DeviceState *qdev = DEVICE(vdev);
>       BusState *qbus = BUS(qdev_get_parent_bus(qdev));
> @@ -64,12 +64,18 @@ int virtio_bus_plug_device(VirtIODevice *vdev)
>       bus->bindings.set_host_notifier = klass->set_host_notifier;
>       bus->bindings.vmstate_change = klass->vmstate_change;
>       virtio_bind_device(bus->vdev, &bus->bindings, qbus->parent);
> +}
> +
> +void virtio_bus_plugged(VirtIODevice *vdev)
> +{
> +    DeviceState *qdev = DEVICE(vdev);
> +    BusState *qbus = BUS(qdev_get_parent_bus(qdev));
> +    VirtioBusState *bus = VIRTIO_BUS(qbus);
> +    VirtioBusClass *klass = VIRTIO_BUS_GET_CLASS(bus);
>   
>       if (klass->device_plugged != NULL) {
>           klass->device_plugged(qbus->parent);
>       }
> -
> -    return 0;
>   }
>   
>   /* Reset the virtio_bus */
> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> index 1c2282c..65f69cf 100644
> --- a/hw/virtio/virtio.c
> +++ b/hw/virtio/virtio.c
> @@ -1088,11 +1088,16 @@ static int virtio_device_init(DeviceState *qdev)
>   {
>       VirtIODevice *vdev = VIRTIO_DEVICE(qdev);
>       VirtioDeviceClass *k = VIRTIO_DEVICE_GET_CLASS(qdev);
> +
> +    virtio_bus_plug_device(vdev);
> +
>       assert(k->init != NULL);
>       if (k->init(vdev) < 0) {
>           return -1;
>       }
> -    virtio_bus_plug_device(vdev);
> +
> +    virtio_bus_plugged(vdev);
> +
>       return 0;
>   }
>   
> diff --git a/include/hw/virtio/virtio-bus.h b/include/hw/virtio/virtio-bus.h
> index 311e8c7..471e55e 100644
> --- a/include/hw/virtio/virtio-bus.h
> +++ b/include/hw/virtio/virtio-bus.h
> @@ -76,7 +76,8 @@ struct VirtioBusState {
>       VirtIOBindings bindings;
>   };
>   
> -int virtio_bus_plug_device(VirtIODevice *vdev);
> +void virtio_bus_plug_device(VirtIODevice *vdev);
> +void virtio_bus_plugged(VirtIODevice *vdev);
>   void virtio_bus_reset(VirtioBusState *bus);
>   void virtio_bus_destroy_device(VirtioBusState *bus);
>   /* Get the device id of the plugged device. */
Which tree are you using? Is it up to date?