linux-kernel - Re: [PATCH v2 06/12] iommu: Make dev->fault

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]

Message-ID: <621683c0-ad01-22ff-dbf9-4edb27f42640@linux.intel.com>
Date:   Fri, 11 Aug 2023 09:43:08 +0800
From:   Baolu Lu <baolu.lu@...ux.intel.com>
To:     Jason Gunthorpe <jgg@...pe.ca>
Cc:     baolu.lu@...ux.intel.com, Joerg Roedel <joro@...tes.org>,
        Will Deacon <will@...nel.org>,
        Robin Murphy <robin.murphy@....com>,
        Kevin Tian <kevin.tian@...el.com>,
        Jean-Philippe Brucker <jean-philippe@...aro.org>,
        Nicolin Chen <nicolinc@...dia.com>,
        Yi Liu <yi.l.liu@...el.com>,
        Jacob Pan <jacob.jun.pan@...ux.intel.com>,
        iommu@...ts.linux.dev, kvm@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 06/12] iommu: Make dev->fault_param static

On 2023/8/11 2:46, Jason Gunthorpe wrote:
> On Thu, Aug 10, 2023 at 03:20:07PM -0300, Jason Gunthorpe wrote:
>> On Thu, Jul 27, 2023 at 01:48:31PM +0800, Lu Baolu wrote:
>>> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
>>> index 4ba3bb692993..3e4ff984aa85 100644
>>> --- a/drivers/iommu/iommu.c
>>> +++ b/drivers/iommu/iommu.c
>>> @@ -302,7 +302,15 @@ static int dev_iommu_get(struct device *dev)
>>>   		return -ENOMEM;
>>>   
>>>   	mutex_init(&param->lock);
>>> +	param->fault_param = kzalloc(sizeof(*param->fault_param), GFP_KERNEL);
>>> +	if (!param->fault_param) {
>>> +		kfree(param);
>>> +		return -ENOMEM;
>>> +	}
>>> +	mutex_init(&param->fault_param->lock);
>>> +	INIT_LIST_HEAD(&param->fault_param->faults);
>>>   	dev->iommu = param;
>> This allocation seems pointless?
>>
>> If we always allocate the fault param then just don't make it a
>> pointer in the first place.
>>
>> The appeal of allocation would be to save a few bytes in the common
>> case that the driver doesn't support faulting.
>>
>> Which means the driver needs to make some call to enable faulting for
>> a device. In this case I'd continue to lazy free on release like this
>> patch does.
> For instance allocate the fault_param in iopf_queue_add_device() which
> is the only thing that needs it.
> 
> Actually probably just merge struct iopf_device_param and
> iommu_fault_param ?
> 
> When you call iopf_queue_add_device() it enables page faulting mode,
> does 1 additional allocation for all additional required per-device
> memory and thats it.

Agreed.

I originally kept the iommu_fault_param structure because I thought it
could also be used to store temporary data for unrecoverable faults,
just like the iopf_device_param structure is used for iopf. However, I
am not sure whether we actually need any temporary data for
unrecoverable fault forwarding, which doesn't require any response.

So, it's better to do like you suggested. It's cleaner and simpler.

Best regards,
baolu