linux-kernel - Re: [PATCH] iommu/arm-smmu-v3: Optimize checking for invalid values

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

Message-ID: <93e31bd7-890b-abd9-a75b-44e86df5bb6d@arm.com>
Date:   Mon, 14 Nov 2022 10:46:47 +0000
From:   Robin Murphy <robin.murphy@....com>
To:     Harry Song <jundongsong1@...il.com>, will@...nel.org
Cc:     joro@...tes.org, linux-arm-kernel@...ts.infradead.org,
        iommu@...ts.linux.dev, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] iommu/arm-smmu-v3: Optimize checking for invalid values

On 2022-11-13 13:38, Harry Song wrote:
> Move the check of invalid value (iotlb_gather->pgsize = 0) from
> arm_smmu_iotlb_sync() to __arm_smmu_tlb_inv_range() for iotlb sync
> to make the code clearer.

How is this an optimisation? It adds a redundant check on paths that 
don't need it, and even the unmap error paths now have to do *more* work 
to figure out when there's nothing to sync. Furthermore, to me "clear 
code" means making decisions at the appropriate level of abstraction, 
not deliberately passing invalid arguments down through 3 more levels of 
callstack to eventually do nothing.

If you think that the gather->pgsize logic is hard to follow and needs 
more explanation then by all means add a comment to call out how 
iommu_iotlb_gather_init() and iommu_iotlb_gather_add_page() join it up, 
but I don't agree with this patch as it is, sorry.

Thanks,
Robin.

> Signed-off-by: Harry Song <jundongsong1@...il.com>
> ---
>   drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 5 +----
>   1 file changed, 1 insertion(+), 4 deletions(-)
> 
> diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> index 6d5df91c5..e51b9f506 100644
> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> @@ -1873,7 +1873,7 @@ static void __arm_smmu_tlb_inv_range(struct arm_smmu_cmdq_ent *cmd,
>   	size_t inv_range = granule;
>   	struct arm_smmu_cmdq_batch cmds;
>   
> -	if (!size)
> +	if (!size || !inv_range)
>   		return;
>   
>   	if (smmu->features & ARM_SMMU_FEAT_RANGE_INV) {
> @@ -2507,9 +2507,6 @@ static void arm_smmu_iotlb_sync(struct iommu_domain *domain,
>   {
>   	struct arm_smmu_domain *smmu_domain = to_smmu_domain(domain);
>   
> -	if (!gather->pgsize)
> -		return;
> -
>   	arm_smmu_tlb_inv_range_domain(gather->start,
>   				      gather->end - gather->start + 1,
>   				      gather->pgsize, true, smmu_domain);