netdev - Re: [PATCH] ifcvf: move IRQ request/free to status change handlers

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]

Message-ID: <47713210-e9d9-d185-6e2e-433e2c436de9@redhat.com>
Date:   Tue, 12 May 2020 13:51:25 +0800
From:   Jason Wang <jasowang@...hat.com>
To:     "Zhu, Lingshan" <lingshan.zhu@...el.com>, mst@...hat.com,
        kvm@...r.kernel.org, virtualization@...ts.linux-foundation.org,
        linux-kernel@...r.kernel.org, netdev@...r.kernel.org
Cc:     lulu@...hat.com, dan.daly@...el.com, cunming.liang@...el.com
Subject: Re: [PATCH] ifcvf: move IRQ request/free to status change handlers


On 2020/5/12 上午11:38, Jason Wang wrote:
>>>>
>>>>   static int ifcvf_start_datapath(void *private)
>>>>   {
>>>>       struct ifcvf_hw *vf = ifcvf_private_to_vf(private);
>>>> @@ -118,9 +172,12 @@ static void ifcvf_vdpa_set_status(struct 
>>>> vdpa_device *vdpa_dev, u8 status)
>>>>   {
>>>>       struct ifcvf_adapter *adapter;
>>>>       struct ifcvf_hw *vf;
>>>> +    u8 status_old;
>>>> +    int ret;
>>>>         vf  = vdpa_to_vf(vdpa_dev);
>>>>       adapter = dev_get_drvdata(vdpa_dev->dev.parent);
>>>> +    status_old = ifcvf_get_status(vf);
>>>>         if (status == 0) {
>>>>           ifcvf_stop_datapath(adapter);
>>>> @@ -128,7 +185,22 @@ static void ifcvf_vdpa_set_status(struct 
>>>> vdpa_device *vdpa_dev, u8 status)
>>>>           return;
>>>>       }
>>>>   -    if (status & VIRTIO_CONFIG_S_DRIVER_OK) {
>>>> +    if ((status_old & VIRTIO_CONFIG_S_DRIVER_OK) &&
>>>> +        !(status & VIRTIO_CONFIG_S_DRIVER_OK)) {
>>>> +        ifcvf_stop_datapath(adapter);
>>>> +        ifcvf_free_irq(adapter, IFCVF_MAX_QUEUE_PAIRS * 2);
>>>> +    }
>>>> +
>>>> +    if ((status & VIRTIO_CONFIG_S_DRIVER_OK) &&
>>>> +        !(status_old & VIRTIO_CONFIG_S_DRIVER_OK)) {
>>>> +        ret = ifcvf_request_irq(adapter);
>>>> +        if (ret) {
>>>> +            status = ifcvf_get_status(vf);
>>>> +            status |= VIRTIO_CONFIG_S_FAILED;
>>>> +            ifcvf_set_status(vf, status);
>>>> +            return;
>>>> +        }
>>>> +
>>>
>>>
>>> Have a hard though on the logic here.
>>>
>>> This depends on the status setting from guest or userspace. Which 
>>> means it can not deal with e.g when qemu or userspace is crashed? Do 
>>> we need to care this or it's a over engineering?
>>>
>>> Thanks
>> If qemu crash, I guess users may re-run qmeu / re-initialize the 
>> device, according to the spec, there should be a reset routine.
>> This code piece handles status change on DRIVER_OK flipping. I am not 
>> sure I get your point, mind to give more hints?
>
>
> The problem is if we don't launch new qemu instance, the interrupt 
> will be still there?


Ok, we reset on vhost_vdpa_release() so the following is suspicious:

With the patch, we do:

static void ifcvf_vdpa_set_status(struct vdpa_device *vdpa_dev, u8 status)
{
         struct ifcvf_adapter *adapter;
         struct ifcvf_hw *vf;
         u8 status_old;
         int ret;

         vf  = vdpa_to_vf(vdpa_dev);
         adapter = dev_get_drvdata(vdpa_dev->dev.parent);
         status_old = ifcvf_get_status(vf);

         if (status == 0) {
                 ifcvf_stop_datapath(adapter);
                 ifcvf_reset_vring(adapter);
                 return;
         }

         if ((status_old & VIRTIO_CONFIG_S_DRIVER_OK) &&
             !(status & VIRTIO_CONFIG_S_DRIVER_OK)) {
                 ifcvf_stop_datapath(adapter);
                 ifcvf_free_irq(adapter, IFCVF_MAX_QUEUE_PAIRS * 2);
         }

...

So the handling of status == 0 looks wrong.

The OK -> !OK check should already cover the datapath stopping and irq 
stuffs.

We only need to deal with vring reset and only need to do it after we 
stop the datapath/irq stuffs.

Thanks



>
> Thanks