linux-kernel - Re: [PATCH V2 7/8] vfio/pci: Support dynamic MSI-x

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] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 3 Apr 2023 10:31:23 -0700
From:   Reinette Chatre <reinette.chatre@...el.com>
To:     Alex Williamson <alex.williamson@...hat.com>
CC:     <jgg@...dia.com>, <yishaih@...dia.com>,
        <shameerali.kolothum.thodi@...wei.com>, <kevin.tian@...el.com>,
        <tglx@...utronix.de>, <darwi@...utronix.de>, <kvm@...r.kernel.org>,
        <dave.jiang@...el.com>, <jing2.liu@...el.com>,
        <ashok.raj@...el.com>, <fenghua.yu@...el.com>,
        <tom.zanussi@...ux.intel.com>, <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH V2 7/8] vfio/pci: Support dynamic MSI-x

Hi Alex,

On 3/31/2023 3:24 PM, Alex Williamson wrote:
> On Fri, 31 Mar 2023 10:49:16 -0700
> Reinette Chatre <reinette.chatre@...el.com> wrote:
>> On 3/30/2023 3:42 PM, Alex Williamson wrote:
>>> On Thu, 30 Mar 2023 16:40:50 -0600
>>> Alex Williamson <alex.williamson@...hat.com> wrote:
>>>   
>>>> On Tue, 28 Mar 2023 14:53:34 -0700
>>>> Reinette Chatre <reinette.chatre@...el.com> wrote:
>>>>  

...

>>>>> +		msix_map.index = vector;
>>>>> +		msix_map.virq = irq;
>>>>> +		pci_msix_free_irq(pdev, msix_map);
>>>>> +	}
>>>>> +	vfio_pci_memory_unlock_and_restore(vdev, cmd);
>>>>>  out_put_eventfd_ctx:
>>>>>  	eventfd_ctx_put(trigger);
>>>>>  out_free_name:
>>>>>  	kfree(ctx->name);
>>>>>  	ctx->name = NULL;
>>>>> +out_free_ctx:
>>>>> +	if (allow_dyn_alloc && new_ctx)
>>>>> +		vfio_irq_ctx_free(vdev, ctx, vector);
>>>>>  	return ret;
>>>>>  }
>>>>>      
>>>>
>>>> Do we really need the new_ctx test in the above cases?  Thanks,  
>>
>> new_ctx is not required for correctness but instead is used to keep
>> the code symmetric. 
>> Specifically, if the user enables MSI-X without providing triggers and
>> then later assign triggers then an error path without new_ctx would unwind
>> more than done in this function, it would free the context that
>> was allocated within vfio_msi_enable(). 
> 
> Seems like we already have that asymmetry, if a trigger is unset we'll
> free the ctx allocated by vfio_msi_enable().  Tracking which are

Apologies, but could you please elaborate on where the asymmetry is? I am
not able to see a flow in this solution where the ctx allocated by
vfio_msi_enable() is freed if the trigger is unset.

> allocated where is unnecessarily complex, how about a policy that

I do not see this as tracking where allocations are made. Instead I
see it as containing/compartmentalizing state changes with the goal of
making the code easier to understand and maintain. Specifically, new_ctx
is used so that if vfio_msi_set_vector_signal() fails, the state 
before and after vfio_msi_set_vector_signal() will be the same.

I do agree that it makes vfio_msi_set_vector_signal() more complex
and I can remove new_ctx if you find that this is unnecessary after
considering the motivations behind its use. 

> devices supporting vdev->has_dyn_msix only ever have active contexts
> allocated?  Thanks,

What do you see as an "active context"? A policy that is currently enforced
is that an allocated context always has an allocated interrupt associated
with it. I do not see how this could be expanded to also require an
enabled interrupt because interrupt enabling requires a trigger that
may not be available.

Reinette