Re: [PATCH] iommu/vt-d: Fix copied_tables bitmap leak on error in copy_translation_tables

[Baolu Lu]

On 7/21/26 21:26, ZhaoJinming wrote:
> The iommu->copied_tables bitmap was introduced by the IOMMU live
> update series to track which context entries have been copied from
> the previous kernel.  The allocation via bitmap_zalloc() was added
> inside copy_translation_tables(), but the error paths were not
> updated to free it:
>
>    1. When old_rt_phys is 0 (invalid root table address)
>    2. When memremap(old_rt_phys) fails
>    3. When kcalloc for ctxt_tbls fails (goto out_unmap, which only
>       unmaps old_rt without releasing the bitmap)
>
> The bitmap is only cleaned up by free_dmar_iommu(), which is
> called from the free_iommu error label in init_dmars().  However,
> when copy_translation_tables() fails, init_dmars() does not jump
> to free_iommu -- it logs the error, falls through, and continues
> with the next IOMMU.  As a result, copied_tables is leaked.
>
> Fix this by converting the two early returns to goto a new
> err_free_bitmap label, and by making out_unmap fall through to
> it so that the bitmap is always freed on any error path.  The
> success path is changed to return 0 directly instead of falling
> through to out_unmap, since copied_tables must remain allocated
> for subsequent use.
>
> Signed-off-by: ZhaoJinming <zhaojinming@xxxxxxxxxxxxx>
> ---
>   drivers/iommu/intel/iommu.c | 18 ++++++++++++------
>   1 file changed, 12 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
> index 849d06dfe1ae..25f49d29da8a 100644
> --- a/drivers/iommu/intel/iommu.c
> +++ b/drivers/iommu/intel/iommu.c
> @@ -1554,12 +1554,16 @@ static int copy_translation_tables(struct intel_iommu *iommu)
>   		return -ENOMEM;
>   
>   	old_rt_phys = rtaddr_reg & VTD_PAGE_MASK;
> -	if (!old_rt_phys)
> -		return -EINVAL;
> +	if (!old_rt_phys) {
> +		ret = -EINVAL;
> +		goto err_free_bitmap;
> +	}
>   
>   	old_rt = memremap(old_rt_phys, PAGE_SIZE, MEMREMAP_WB);
> -	if (!old_rt)
> -		return -ENOMEM;
> +	if (!old_rt) {
> +		ret = -ENOMEM;
> +		goto err_free_bitmap;
> +	}
>   
>   	/* This is too big for the stack - allocate it from slab */
>   	ctxt_table_entries = ext ? 512 : 256;
> @@ -1603,11 +1607,13 @@ static int copy_translation_tables(struct intel_iommu *iommu)
>   
>   	__iommu_flush_cache(iommu, iommu->root_entry, PAGE_SIZE);
>   
> -	ret = 0;
> +	return 0;

Returning directly here bypasses memunmap().

Sashiko reported this as well:

"
Does this code leak the old_rt memory mapping on the success path?
old_rt is mapped earlier in this function:
copy_translation_tables() {
    ...
    old_rt = memremap(old_rt_phys, PAGE_SIZE, MEMREMAP_WB);
    ...
}

By returning directly here instead of setting ret = 0 and falling
through, it looks like we bypass the memunmap(old_rt) call under the
out_unmap label, leaving the mapping active.
"

https://sashiko.dev/#/patchset/20260721132635.1436908-1-zhaojinming%40uniontech.com

Do you mind fixing this with a v2?

>   
>   out_unmap:
>   	memunmap(old_rt);
> -
> +err_free_bitmap:
> +	bitmap_free(iommu->copied_tables);
> +	iommu->copied_tables = NULL;
>   	return ret;
>   }
>   

Thanks,
baolu