netdev - Re: [PATCH net-next] virtio_net: implement VIRTIO_CONFIG_S_NEEDS

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Date:   Fri, 29 Dec 2017 11:10:34 +0800
From:   Jason Wang <jasowang@...hat.com>
To:     Willem de Bruijn <willemdebruijn.kernel@...il.com>,
        "Michael S. Tsirkin" <mst@...hat.com>
Cc:     Network Development <netdev@...r.kernel.org>,
        David Miller <davem@...emloft.net>,
        virtualization@...ts.linux-foundation.org,
        Willem de Bruijn <willemb@...gle.com>
Subject: Re: [PATCH net-next] virtio_net: implement
 VIRTIO_CONFIG_S_NEEDS_RESET



On 2017年12月29日 03:11, Willem de Bruijn wrote:
> On Mon, Oct 16, 2017 at 11:44 PM, Michael S. Tsirkin <mst@...hat.com> wrote:
>> On Tue, Oct 17, 2017 at 11:05:07AM +0800, Jason Wang wrote:
>>>
>>> On 2017年10月17日 06:34, Willem de Bruijn wrote:
>>>> On Mon, Oct 16, 2017 at 12:38 PM, Michael S. Tsirkin <mst@...hat.com> wrote:
>>>>> On Mon, Oct 16, 2017 at 12:04:57PM -0400, Willem de Bruijn wrote:
>>>>>> On Mon, Oct 16, 2017 at 11:31 AM, Michael S. Tsirkin <mst@...hat.com> wrote:
>>>>>>> On Mon, Oct 16, 2017 at 11:03:18AM -0400, Willem de Bruijn wrote:
>>>>>>>>>> +static int virtnet_reset(struct virtnet_info *vi)
>>>>>>>>>> +{
>>>>>>>>>> +     struct virtio_device *dev = vi->vdev;
>>>>>>>>>> +     int ret;
>>>>>>>>>> +
>>>>>>>>>> +     virtio_config_disable(dev);
>>>>>>>>>> +     dev->failed = dev->config->get_status(dev) & VIRTIO_CONFIG_S_FAILED;
>>>>>>>>>> +     virtnet_freeze_down(dev, true);
>>>>>>>>>> +     remove_vq_common(vi);
>>>>>>>>>> +
>>>>>>>>>> +     virtio_add_status(dev, VIRTIO_CONFIG_S_ACKNOWLEDGE);
>>>>>>>>>> +     virtio_add_status(dev, VIRTIO_CONFIG_S_DRIVER);
>>>>>>>>>> +
>>>>>>>>>> +     ret = virtio_finalize_features(dev);
>>>>>>>>>> +     if (ret)
>>>>>>>>>> +             goto err;
>>>>>>>>>> +
>>>>>>>>>> +     ret = virtnet_restore_up(dev);
>>>>>>>>>> +     if (ret)
>>>>>>>>>> +             goto err;
>>>>>>>>>> +
>>>>>>>>>> +     ret = virtnet_set_queues(vi, vi->curr_queue_pairs);
>>>>>>>>>> +     if (ret)
>>>>>>>>>> +             goto err;
>>>>>>>>>> +
>>>>>>>>>> +     virtio_add_status(dev, VIRTIO_CONFIG_S_DRIVER_OK);
>>>>>>>>>> +     virtio_config_enable(dev);
>>>>>>>>>> +     return 0;
>>>>>>>>>> +
>>>>>>>>>> +err:
>>>>>>>>>> +     virtio_add_status(dev, VIRTIO_CONFIG_S_FAILED);
>>>>>>>>>> +     return ret;
>>>>>>>>>> +}
>>>>>>>>>> +
>>>>>>>>>>    static int virtnet_set_guest_offloads(struct virtnet_info *vi, u64 offloads)
>>>>>>>>>>    {
>>>>>>>>>>         struct scatterlist sg;
>>>>>>>>> I have a question here though. How do things like MAC address
>>>>>>>>> get restored?
>>>>>>>>>
>>>>>>>>> What about the rx mode?
>>>>>>>>>
>>>>>>>>> vlans?
>>>>>>>> The function as is releases and reinitializes only ring state.
>>>>>>>> Device configuration such as mac and vlan persist across
>>>>>>>> the reset.
>>>>>>> What gave you this impression? Take a look at e.g. this
>>>>>>> code in qemu:
>>>>>>>
>>>>>>> static void virtio_net_reset(VirtIODevice *vdev)
>>>>>>> {
>>>>>>>       VirtIONet *n = VIRTIO_NET(vdev);
>>>>>>>
>>>>>>>       /* Reset back to compatibility mode */
>>>>>>>       n->promisc = 1;
>>>>>>>       n->allmulti = 0;
>>>>>>>       n->alluni = 0;
>>>>>>>       n->nomulti = 0;
>>>>>>>       n->nouni = 0;
>>>>>>>       n->nobcast = 0;
>>>>>>>       /* multiqueue is disabled by default */
>>>>>>>       n->curr_queues = 1;
>>>>>>>       timer_del(n->announce_timer);
>>>>>>>       n->announce_counter = 0;
>>>>>>>       n->status &= ~VIRTIO_NET_S_ANNOUNCE;
>>>>>>>
>>>>>>>       /* Flush any MAC and VLAN filter table state */
>>>>>>>       n->mac_table.in_use = 0;
>>>>>>>       n->mac_table.first_multi = 0;
>>>>>>>       n->mac_table.multi_overflow = 0;
>>>>>>>       n->mac_table.uni_overflow = 0;
>>>>>>>       memset(n->mac_table.macs, 0, MAC_TABLE_ENTRIES * ETH_ALEN);
>>>>>>>       memcpy(&n->mac[0], &n->nic->conf->macaddr, sizeof(n->mac));
>>>>>>>       qemu_format_nic_info_str(qemu_get_queue(n->nic), n->mac);
>>>>>>>       memset(n->vlans, 0, MAX_VLAN >> 3);
>>>>>>> }
>>>>>>>
>>>>>>> So device seems to lose all state, you have to re-program it.
>>>>>> Oh, indeed! The guest does not reset its state, so it might
>>>>>> be out of sync with the host after the operation. Was this not
>>>>>> an issue when previously resetting in the context of xdp?
>>>>> I suspect it was broken back then, too.
>>>> Okay. I guess that in principle this is all programmable through
>>>> virtnet_set_rx_mode, virtnet_vlan_rx_add_vid, etc. But it's a
>>>> lot more complex than just restoring virtnet_reset. Will need to
>>>> be careful about concurrency issues at the least. Similar to the
>>>> ones you point out below.
>>>>
>>> The problem has been pointed out during developing virtio-net XDP. But it
>>> may not be a big issue since vhost_net ignores all kinds of the filters now.
>>>
>>> Thanks
>> It might not keep doing that in the future though.
>> And virtio-net in userspace doesn't ignore the filters.
> How about the guest honor the request only if no state has been
> offloaded to the host?
>
> This is the common case for vhost_net, and not expected to change
> soon.

FYI, I'm implementing to use tun eBPF filter for virtio-net. So 
recovering filter should be considered.

Thanks

>
> Even when it does, we have a graceful degradation strategy. Guest
> revert state prior to reset and reapply. Though for the time being,
> solving this only in the case without state offload would be solve my
> use case.