Re: [PATCH 1/2] iommu: free dev->iommu when iommu_fwspec_init() cannot allocate the fwspec

[Tal Zussman]

On 11/2/26 11:51 AM, Robin Murphy wrote:
> On 31/10/2026 8:33 pm, Tal Zussman wrote:
>> iommu_fwspec_init() allocates dev->iommu with dev_iommu_get() and then
>> the fwspec. If the fwspec allocation fails it returns -ENOMEM but leaves
>> dev->iommu allocated, and it leaks.
>> 
>> kmemleak reports it when iommufd_fail_nth injects the allocation
>> failure:
>> 
>>    unreferenced object 0xffff88811c138b00 (size 192):
>>      comm "iommufd_fail_nt", pid 783
>>      backtrace:
>>        __kmalloc_cache_noprof
>>        dev_iommu_get
>>        iommu_fwspec_init
>>        iommu_mock_device_add
>>        iommufd_test
>>        iommufd_fops_ioctl
>> 
>> Free dev->iommu on failure if this call allocated it. iommu_init_device()
>> allocates it before calling into the driver and frees it on its own
>> error path, so leave it alone in that case.
>> 
>> Fixes: 72acd9df18f1 ("iommu: Move iommu_fwspec to struct dev_iommu")
> 
> No, the bug is clearly the early return in 2a918911ed3d ("iommufd:
> Register iommufd mock devices with fwspec"), so fix it there please.
> iommu_fwspec_init() has always been designed to be called in conjunction
> with iommu_probe_device() which handles cleanup appropriately. All of
> this mock_device hackery not using the proper probe paths is enough of a
> pain as it is...
> 
> Thanks,
> Robin.
> 

Thanks for the review!
I'll change this for v2.

- Tal

>> Signed-off-by: Tal Zussman <tz2294@xxxxxxxxxxxx>
>> ---
>>   drivers/iommu/iommu.c | 6 +++++-
>>   1 file changed, 5 insertions(+), 1 deletion(-)
>> 
>> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
>> index cd1bca7ede9a..69be8819e05d 100644
>> --- a/drivers/iommu/iommu.c
>> +++ b/drivers/iommu/iommu.c
>> @@ -3074,6 +3074,7 @@ int iommu_fwspec_init(struct device *dev, struct fwnode_handle *iommu_fwnode)
>>   {
>>   	const struct iommu_device *iommu = iommu_from_fwnode(iommu_fwnode);
>>   	struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev);
>> +	bool new_iommu = !dev->iommu;
>>   
>>   	if (!iommu)
>>   		return driver_deferred_probe_check_state(dev);
>> @@ -3088,8 +3089,11 @@ int iommu_fwspec_init(struct device *dev, struct fwnode_handle *iommu_fwnode)
>>   
>>   	/* Preallocate for the overwhelmingly common case of 1 ID */
>>   	fwspec = kzalloc_flex(*fwspec, ids, 1);
>> -	if (!fwspec)
>> +	if (!fwspec) {
>> +		if (new_iommu)
>> +			dev_iommu_free(dev);
>>   		return -ENOMEM;
>> +	}
>>   
>>   	fwnode_handle_get(iommu_fwnode);
>>   	fwspec->iommu_fwnode = iommu_fwnode;
>> 
>