Re: [PATCH] iommu: Up front sanity check in the arm_lpae_map

[Robin Murphy]

On 2020-12-05 08:29, Keqian Zhu wrote:
> ... then we have more chance to detect wrong code logic.

I don't follow that justification - it's still the same check with the 
same outcome, so how does moving it have any effect on the chance to 
detect errors?

AFAICS the only difference it would make is to make some errors *less* 
obvious - if a sufficiently broken caller passes an empty prot value 
alongside an invalid size or already-mapped address, this will now 
quietly hide the warnings from the more serious condition(s).

Yes, it will bail out a bit faster in the specific case where the prot 
value is the only thing wrong, but since when do we optimise for 
fundamentally incorrect API usage?

Robin.

> Signed-off-by: Keqian Zhu <zhukeqian1@xxxxxxxxxx>
> ---
>   drivers/iommu/io-pgtable-arm.c | 8 ++++----
>   1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/iommu/io-pgtable-arm.c b/drivers/iommu/io-pgtable-arm.c
> index a7a9bc08dcd1..8ade72adab31 100644
> --- a/drivers/iommu/io-pgtable-arm.c
> +++ b/drivers/iommu/io-pgtable-arm.c
> @@ -444,10 +444,6 @@ static int arm_lpae_map(struct io_pgtable_ops *ops, unsigned long iova,
>   	arm_lpae_iopte prot;
>   	long iaext = (s64)iova >> cfg->ias;
>   
> -	/* If no access, then nothing to do */
> -	if (!(iommu_prot & (IOMMU_READ | IOMMU_WRITE)))
> -		return 0;
> -
>   	if (WARN_ON(!size || (size & cfg->pgsize_bitmap) != size))
>   		return -EINVAL;
>   
> @@ -456,6 +452,10 @@ static int arm_lpae_map(struct io_pgtable_ops *ops, unsigned long iova,
>   	if (WARN_ON(iaext || paddr >> cfg->oas))
>   		return -ERANGE;
>   
> +	/* If no access, then nothing to do */
> +	if (!(iommu_prot & (IOMMU_READ | IOMMU_WRITE)))
> +		return 0;
> +
>   	prot = arm_lpae_prot_to_pte(data, iommu_prot);
>   	ret = __arm_lpae_map(data, iova, paddr, size, prot, lvl, ptep, gfp);
>   	/*