Use the __free(kfree) attribute with kzalloc() to automatically handle
the freeing of the allocated struct iommu_domain_info on error or early
exit paths, eliminating the need for explicit kfree() calls in error
handling branches.
Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
---
drivers/iommu/intel/iommu.c | 29 ++++++++---------------------
1 file changed, 8 insertions(+), 21 deletions(-)
diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
index 3a9ea0ad2cd3..12382c85495f 100644
--- a/drivers/iommu/intel/iommu.c
+++ b/drivers/iommu/intel/iommu.c
@@ -1337,13 +1337,14 @@ static bool first_level_by_default(struct intel_iommu *iommu)
int domain_attach_iommu(struct dmar_domain *domain, struct intel_iommu *iommu)
{
- struct iommu_domain_info *info, *curr;
- int num, ret = -ENOSPC;
+ struct iommu_domain_info *curr;
+ int num;
if (domain->domain.type == IOMMU_DOMAIN_SVA)
return 0;
- info = kzalloc(sizeof(*info), GFP_KERNEL);
+ struct iommu_domain_info *info __free(kfree) =
+ kzalloc(sizeof(*info), GFP_KERNEL);
if (!info)
return -ENOMEM;
@@ -1351,34 +1352,20 @@ int domain_attach_iommu(struct dmar_domain *domain, struct intel_iommu *iommu)
curr = xa_load(&domain->iommu_array, iommu->seq_id);
if (curr) {
curr->refcnt++;
- kfree(info);
return 0;
}
num = ida_alloc_range(&iommu->domain_ida, FLPT_DEFAULT_DID + 1,
cap_ndoms(iommu->cap) - 1, GFP_KERNEL);
- if (num < 0) {
- pr_err("%s: No free domain ids\n", iommu->name);
- goto err_unlock;
- }
+ if (num < 0)
+ return num;
info->refcnt = 1;
info->did = num;
info->iommu = iommu;
- curr = xa_cmpxchg(&domain->iommu_array, iommu->seq_id,
- NULL, info, GFP_KERNEL);
- if (curr) {
- ret = xa_err(curr) ? : -EBUSY;
- goto err_clear;
- }
- return 0;
-
-err_clear:
- ida_free(&iommu->domain_ida, info->did);
-err_unlock:
- kfree(info);
- return ret;
+ return xa_err(xa_store(&domain->iommu_array, iommu->seq_id,
+ no_free_ptr(info), GFP_KERNEL));
}
void domain_detach_iommu(struct dmar_domain *domain, struct intel_iommu *iommu)
--
2.43.0Lu Baolu wrote:
> Use the __free(kfree) attribute with kzalloc() to automatically handle
> the freeing of the allocated struct iommu_domain_info on error or early
> exit paths, eliminating the need for explicit kfree() calls in error
> handling branches.
>
> Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
> ---
> drivers/iommu/intel/iommu.c | 29 ++++++++---------------------
> 1 file changed, 8 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
> index 3a9ea0ad2cd3..12382c85495f 100644
> --- a/drivers/iommu/intel/iommu.c
> +++ b/drivers/iommu/intel/iommu.c
> @@ -1337,13 +1337,14 @@ static bool first_level_by_default(struct intel_iommu *iommu)
>
> int domain_attach_iommu(struct dmar_domain *domain, struct intel_iommu *iommu)
> {
> - struct iommu_domain_info *info, *curr;
> - int num, ret = -ENOSPC;
> + struct iommu_domain_info *curr;
> + int num;
>
> if (domain->domain.type == IOMMU_DOMAIN_SVA)
> return 0;
>
> - info = kzalloc(sizeof(*info), GFP_KERNEL);
> + struct iommu_domain_info *info __free(kfree) =
> + kzalloc(sizeof(*info), GFP_KERNEL);
> if (!info)
> return -ENOMEM;
>
> @@ -1351,34 +1352,20 @@ int domain_attach_iommu(struct dmar_domain *domain, struct intel_iommu *iommu)
[..]
> -err_clear:
> - ida_free(&iommu->domain_ida, info->did);
> -err_unlock:
> - kfree(info);
> - return ret;
> + return xa_err(xa_store(&domain->iommu_array, iommu->seq_id,
> + no_free_ptr(info), GFP_KERNEL));
> }
This pattern looks like it wants a "xa_store_or_{reset,kfree}()" helper that
handles both canceling a scope based cleanup and taking responsibility for
error-exit-freeing @info in one statement.
I.e. this is similar to a:
"return devm_add_action_or_reset(..., no_free_ptr(obj), ...)"
...pattern.
On 4/26/25 02:49, Dan Williams wrote:
> Lu Baolu wrote:
>> Use the __free(kfree) attribute with kzalloc() to automatically handle
>> the freeing of the allocated struct iommu_domain_info on error or early
>> exit paths, eliminating the need for explicit kfree() calls in error
>> handling branches.
>>
>> Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
>> ---
>> drivers/iommu/intel/iommu.c | 29 ++++++++---------------------
>> 1 file changed, 8 insertions(+), 21 deletions(-)
>>
>> diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
>> index 3a9ea0ad2cd3..12382c85495f 100644
>> --- a/drivers/iommu/intel/iommu.c
>> +++ b/drivers/iommu/intel/iommu.c
>> @@ -1337,13 +1337,14 @@ static bool first_level_by_default(struct intel_iommu *iommu)
>>
>> int domain_attach_iommu(struct dmar_domain *domain, struct intel_iommu *iommu)
>> {
>> - struct iommu_domain_info *info, *curr;
>> - int num, ret = -ENOSPC;
>> + struct iommu_domain_info *curr;
>> + int num;
>>
>> if (domain->domain.type == IOMMU_DOMAIN_SVA)
>> return 0;
>>
>> - info = kzalloc(sizeof(*info), GFP_KERNEL);
>> + struct iommu_domain_info *info __free(kfree) =
>> + kzalloc(sizeof(*info), GFP_KERNEL);
>> if (!info)
>> return -ENOMEM;
>>
>> @@ -1351,34 +1352,20 @@ int domain_attach_iommu(struct dmar_domain *domain, struct intel_iommu *iommu)
> [..]
>> -err_clear:
>> - ida_free(&iommu->domain_ida, info->did);
>> -err_unlock:
>> - kfree(info);
>> - return ret;
>> + return xa_err(xa_store(&domain->iommu_array, iommu->seq_id,
>> + no_free_ptr(info), GFP_KERNEL));
>> }
>
> This pattern looks like it wants a "xa_store_or_{reset,kfree}()" helper that
> handles both canceling a scope based cleanup and taking responsibility for
> error-exit-freeing @info in one statement.
>
> I.e. this is similar to a:
>
> "return devm_add_action_or_reset(..., no_free_ptr(obj), ...)"
>
> ...pattern.
>
Yes. Perhaps adding a xa_store variant would be beneficial in all
places that require this pattern.
Something like this?
diff --git a/include/linux/xarray.h b/include/linux/xarray.h
index 78eede109b1a..efbdff7ebda4 100644
--- a/include/linux/xarray.h
+++ b/include/linux/xarray.h
@@ -626,6 +626,35 @@ static inline void *xa_store_irq(struct xarray *xa,
unsigned long index,
return curr;
}
+/**
+ * xa_store_or_kfree() - Store this entry in the XArray.
+ * @xa: XArray.
+ * @index: Index into array.
+ * @entry: New entry.
+ * @gfp: Memory allocation flags.
+ *
+ * This function is like calling xa_store() except it kfrees the new
+ * entry if an error happened.
+ *
+ * Context: Process context. Any context. Takes and releases the xa_lock.
+ * May sleep if the @gfp flags permit.
+ * Return: The old entry at this index or xa_err() if an error happened.
+ */
+static inline void *xa_store_or_kfree(struct xarray *xa, unsigned long
index,
+ void *entry, gfp_t gfp)
+{
+ void *curr;
+
+ xa_lock(xa);
+ curr = __xa_store(xa, index, entry, gfp);
+ xa_unlock(xa);
+
+ if (xa_err(curr))
+ kfree(entry);
+
+ return curr;
+}
+
/**
* xa_erase_bh() - Erase this entry from the XArray.
* @xa: XArray.
Thanks,
baolu
> From: Lu Baolu <baolu.lu@linux.intel.com>
> Sent: Wednesday, April 23, 2025 11:10 AM
>
> num = ida_alloc_range(&iommu->domain_ida, FLPT_DEFAULT_DID +
> 1,
> cap_ndoms(iommu->cap) - 1, GFP_KERNEL);
> - if (num < 0) {
> - pr_err("%s: No free domain ids\n", iommu->name);
this error message could be kept.
> - goto err_unlock;
> - }
> + if (num < 0)
> + return num;
>
> info->refcnt = 1;
> info->did = num;
> info->iommu = iommu;
> - curr = xa_cmpxchg(&domain->iommu_array, iommu->seq_id,
> - NULL, info, GFP_KERNEL);
> - if (curr) {
> - ret = xa_err(curr) ? : -EBUSY;
> - goto err_clear;
> - }
>
> - return 0;
> -
> -err_clear:
> - ida_free(&iommu->domain_ida, info->did);
> -err_unlock:
> - kfree(info);
> - return ret;
> + return xa_err(xa_store(&domain->iommu_array, iommu->seq_id,
> + no_free_ptr(info), GFP_KERNEL));
> }
no_free_ptr() should be used before successful return. Here xa_store()
could return error but at that point no auto free as no_free_ptr() already
changes 'info' to NULL. then memory leak.
On 4/24/2025 3:46 PM, Tian, Kevin wrote:
>> From: Lu Baolu <baolu.lu@linux.intel.com>
>> Sent: Wednesday, April 23, 2025 11:10 AM
>>
>> num = ida_alloc_range(&iommu->domain_ida, FLPT_DEFAULT_DID +
>> 1,
>> cap_ndoms(iommu->cap) - 1, GFP_KERNEL);
>> - if (num < 0) {
>> - pr_err("%s: No free domain ids\n", iommu->name);
>
> this error message could be kept.
Okay.
>
>> - goto err_unlock;
>> - }
>> + if (num < 0)
>> + return num;
>>
>> info->refcnt = 1;
>> info->did = num;
>> info->iommu = iommu;
>> - curr = xa_cmpxchg(&domain->iommu_array, iommu->seq_id,
>> - NULL, info, GFP_KERNEL);
>> - if (curr) {
>> - ret = xa_err(curr) ? : -EBUSY;
>> - goto err_clear;
>> - }
>>
>> - return 0;
>> -
>> -err_clear:
>> - ida_free(&iommu->domain_ida, info->did);
>> -err_unlock:
>> - kfree(info);
>> - return ret;
>> + return xa_err(xa_store(&domain->iommu_array, iommu->seq_id,
>> + no_free_ptr(info), GFP_KERNEL));
>> }
>
> no_free_ptr() should be used before successful return. Here xa_store()
> could return error but at that point no auto free as no_free_ptr() already
> changes 'info' to NULL. then memory leak.
Hmm, I've considered this. My thought was that xa_store() failure only
occurs due to the system running out of memory, and the Linux kernel
can't recover from it. In that case, the system is already broken;
hence, handling the failure case here doesn't make things better.
Thanks,
baolu
On Thu, Apr 24, 2025 at 05:22:48PM +0800, Baolu Lu wrote: > > > -err_clear: > > > - ida_free(&iommu->domain_ida, info->did); > > > -err_unlock: > > > - kfree(info); > > > - return ret; > > > + return xa_err(xa_store(&domain->iommu_array, iommu->seq_id, > > > + no_free_ptr(info), GFP_KERNEL)); > > > } > > > > no_free_ptr() should be used before successful return. Here xa_store() > > could return error but at that point no auto free as no_free_ptr() already > > changes 'info' to NULL. then memory leak. > > Hmm, I've considered this. My thought was that xa_store() failure only > occurs due to the system running out of memory, and the Linux kernel > can't recover from it. In that case, the system is already broken; > hence, handling the failure case here doesn't make things better. That's not the kernel pattern, you are supposed to unwind correctly in those failures I think you should not use cleanup.h for something complicated like this.. Jason
On 4/24/2025 9:37 PM, Jason Gunthorpe wrote: > On Thu, Apr 24, 2025 at 05:22:48PM +0800, Baolu Lu wrote: > >>>> -err_clear: >>>> - ida_free(&iommu->domain_ida, info->did); >>>> -err_unlock: >>>> - kfree(info); >>>> - return ret; >>>> + return xa_err(xa_store(&domain->iommu_array, iommu->seq_id, >>>> + no_free_ptr(info), GFP_KERNEL)); >>>> } >>> no_free_ptr() should be used before successful return. Here xa_store() >>> could return error but at that point no auto free as no_free_ptr() already >>> changes 'info' to NULL. then memory leak. >> Hmm, I've considered this. My thought was that xa_store() failure only >> occurs due to the system running out of memory, and the Linux kernel >> can't recover from it. In that case, the system is already broken; >> hence, handling the failure case here doesn't make things better. > That's not the kernel pattern, you are supposed to unwind correctly in > those failures > > I think you should not use cleanup.h for something complicated like > this.. Okay, so let me drop this patch. Thanks, baolu
© 2016 - 2026 Red Hat, Inc.