* [PATCH] iommu: Up front sanity check in the arm_lpae_map
@ 2020-12-05 8:29 ` Keqian Zhu
0 siblings, 0 replies; 24+ messages in thread
From: Keqian Zhu @ 2020-12-05 8:29 UTC (permalink / raw)
To: linux-kernel, linux-arm-kernel, iommu
Cc: Suzuki K Poulose, Marc Zyngier, Joerg Roedel, jiangkunkun,
Robin Murphy, Sean Christopherson, Alexios Zavras, Mark Brown,
James Morse, Catalin Marinas, Keqian Zhu, wanghaibin.wang,
Thomas Gleixner, Will Deacon, Andrew Morton, Julien Thierry
... then we have more chance to detect wrong code logic.
Signed-off-by: Keqian Zhu <zhukeqian1@huawei.com>
---
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);
/*
--
2.23.0
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 24+ messages in thread
* [PATCH] iommu: Up front sanity check in the arm_lpae_map
@ 2020-12-05 8:29 ` Keqian Zhu
0 siblings, 0 replies; 24+ messages in thread
From: Keqian Zhu @ 2020-12-05 8:29 UTC (permalink / raw)
To: linux-kernel, linux-arm-kernel, iommu
Cc: Suzuki K Poulose, Marc Zyngier, jiangkunkun, Robin Murphy,
Sean Christopherson, Alexios Zavras, Mark Brown, James Morse,
Catalin Marinas, Keqian Zhu, wanghaibin.wang, Thomas Gleixner,
Will Deacon, Andrew Morton, Julien Thierry
... then we have more chance to detect wrong code logic.
Signed-off-by: Keqian Zhu <zhukeqian1@huawei.com>
---
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);
/*
--
2.23.0
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
^ permalink raw reply related [flat|nested] 24+ messages in thread
* Re: [PATCH] iommu: Up front sanity check in the arm_lpae_map
2020-12-05 8:29 ` Keqian Zhu
(?)
@ 2020-12-07 10:59 ` Will Deacon
-1 siblings, 0 replies; 24+ messages in thread
From: Will Deacon @ 2020-12-07 10:59 UTC (permalink / raw)
To: Keqian Zhu
Cc: linux-kernel, linux-arm-kernel, iommu, Marc Zyngier,
Robin Murphy, Joerg Roedel, Catalin Marinas, James Morse,
Suzuki K Poulose, Sean Christopherson, Julien Thierry,
Mark Brown, Thomas Gleixner, Andrew Morton, Alexios Zavras,
wanghaibin.wang, jiangkunkun
On Sat, Dec 05, 2020 at 04:29:57PM +0800, Keqian Zhu wrote:
> ... then we have more chance to detect wrong code logic.
This could do with being a bit more explicit. Something like:
Although handling a mapping request with no permissions is a
trivial no-op, defer the early return until after the size/range
checks so that we are consistent with other mapping requests.
> Signed-off-by: Keqian Zhu <zhukeqian1@huawei.com>
> ---
> 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;
This looks sensible to me, but please can you make the same change for
io-pgtable-arm-v7s.c so that the behaviour is consistent across the two
formats?
Thanks,
Will
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] iommu: Up front sanity check in the arm_lpae_map
@ 2020-12-07 10:59 ` Will Deacon
0 siblings, 0 replies; 24+ messages in thread
From: Will Deacon @ 2020-12-07 10:59 UTC (permalink / raw)
To: Keqian Zhu
Cc: Suzuki K Poulose, Marc Zyngier, Joerg Roedel, jiangkunkun,
linux-kernel, Sean Christopherson, Alexios Zavras, iommu,
Mark Brown, James Morse, Julien Thierry, Catalin Marinas,
wanghaibin.wang, Thomas Gleixner, Robin Murphy, Andrew Morton,
linux-arm-kernel
On Sat, Dec 05, 2020 at 04:29:57PM +0800, Keqian Zhu wrote:
> ... then we have more chance to detect wrong code logic.
This could do with being a bit more explicit. Something like:
Although handling a mapping request with no permissions is a
trivial no-op, defer the early return until after the size/range
checks so that we are consistent with other mapping requests.
> Signed-off-by: Keqian Zhu <zhukeqian1@huawei.com>
> ---
> 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;
This looks sensible to me, but please can you make the same change for
io-pgtable-arm-v7s.c so that the behaviour is consistent across the two
formats?
Thanks,
Will
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] iommu: Up front sanity check in the arm_lpae_map
@ 2020-12-07 10:59 ` Will Deacon
0 siblings, 0 replies; 24+ messages in thread
From: Will Deacon @ 2020-12-07 10:59 UTC (permalink / raw)
To: Keqian Zhu
Cc: Suzuki K Poulose, Marc Zyngier, jiangkunkun, linux-kernel,
Sean Christopherson, Alexios Zavras, iommu, Mark Brown,
James Morse, Julien Thierry, Catalin Marinas, wanghaibin.wang,
Thomas Gleixner, Robin Murphy, Andrew Morton, linux-arm-kernel
On Sat, Dec 05, 2020 at 04:29:57PM +0800, Keqian Zhu wrote:
> ... then we have more chance to detect wrong code logic.
This could do with being a bit more explicit. Something like:
Although handling a mapping request with no permissions is a
trivial no-op, defer the early return until after the size/range
checks so that we are consistent with other mapping requests.
> Signed-off-by: Keqian Zhu <zhukeqian1@huawei.com>
> ---
> 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;
This looks sensible to me, but please can you make the same change for
io-pgtable-arm-v7s.c so that the behaviour is consistent across the two
formats?
Thanks,
Will
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] iommu: Up front sanity check in the arm_lpae_map
2020-12-07 10:59 ` Will Deacon
(?)
@ 2020-12-07 11:37 ` zhukeqian
-1 siblings, 0 replies; 24+ messages in thread
From: zhukeqian @ 2020-12-07 11:37 UTC (permalink / raw)
To: Will Deacon
Cc: linux-kernel, linux-arm-kernel, iommu, Marc Zyngier,
Robin Murphy, Joerg Roedel, Catalin Marinas, James Morse,
Suzuki K Poulose, Sean Christopherson, Julien Thierry,
Mark Brown, Thomas Gleixner, Andrew Morton, Alexios Zavras,
wanghaibin.wang, jiangkunkun
Hi Will,
On 2020/12/7 18:59, Will Deacon wrote:
> On Sat, Dec 05, 2020 at 04:29:57PM +0800, Keqian Zhu wrote:
>> ... then we have more chance to detect wrong code logic.
>
> This could do with being a bit more explicit. Something like:
>
> Although handling a mapping request with no permissions is a
> trivial no-op, defer the early return until after the size/range
> checks so that we are consistent with other mapping requests.
This looks well, thanks.
>
>> Signed-off-by: Keqian Zhu <zhukeqian1@huawei.com>
>> ---
>> 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;
>
> This looks sensible to me, but please can you make the same change for
> io-pgtable-arm-v7s.c so that the behaviour is consistent across the two
> formats?
>
OK. I can do it right now.
Thanks,
Keqian
> Thanks,
>
> Will
> .
>
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] iommu: Up front sanity check in the arm_lpae_map
@ 2020-12-07 11:37 ` zhukeqian
0 siblings, 0 replies; 24+ messages in thread
From: zhukeqian @ 2020-12-07 11:37 UTC (permalink / raw)
To: Will Deacon
Cc: Suzuki K Poulose, Marc Zyngier, Joerg Roedel, jiangkunkun,
linux-kernel, Sean Christopherson, Alexios Zavras, iommu,
Mark Brown, James Morse, Julien Thierry, Catalin Marinas,
wanghaibin.wang, Thomas Gleixner, Robin Murphy, Andrew Morton,
linux-arm-kernel
Hi Will,
On 2020/12/7 18:59, Will Deacon wrote:
> On Sat, Dec 05, 2020 at 04:29:57PM +0800, Keqian Zhu wrote:
>> ... then we have more chance to detect wrong code logic.
>
> This could do with being a bit more explicit. Something like:
>
> Although handling a mapping request with no permissions is a
> trivial no-op, defer the early return until after the size/range
> checks so that we are consistent with other mapping requests.
This looks well, thanks.
>
>> Signed-off-by: Keqian Zhu <zhukeqian1@huawei.com>
>> ---
>> 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;
>
> This looks sensible to me, but please can you make the same change for
> io-pgtable-arm-v7s.c so that the behaviour is consistent across the two
> formats?
>
OK. I can do it right now.
Thanks,
Keqian
> Thanks,
>
> Will
> .
>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] iommu: Up front sanity check in the arm_lpae_map
@ 2020-12-07 11:37 ` zhukeqian
0 siblings, 0 replies; 24+ messages in thread
From: zhukeqian @ 2020-12-07 11:37 UTC (permalink / raw)
To: Will Deacon
Cc: Suzuki K Poulose, Marc Zyngier, jiangkunkun, linux-kernel,
Sean Christopherson, Alexios Zavras, iommu, Mark Brown,
James Morse, Julien Thierry, Catalin Marinas, wanghaibin.wang,
Thomas Gleixner, Robin Murphy, Andrew Morton, linux-arm-kernel
Hi Will,
On 2020/12/7 18:59, Will Deacon wrote:
> On Sat, Dec 05, 2020 at 04:29:57PM +0800, Keqian Zhu wrote:
>> ... then we have more chance to detect wrong code logic.
>
> This could do with being a bit more explicit. Something like:
>
> Although handling a mapping request with no permissions is a
> trivial no-op, defer the early return until after the size/range
> checks so that we are consistent with other mapping requests.
This looks well, thanks.
>
>> Signed-off-by: Keqian Zhu <zhukeqian1@huawei.com>
>> ---
>> 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;
>
> This looks sensible to me, but please can you make the same change for
> io-pgtable-arm-v7s.c so that the behaviour is consistent across the two
> formats?
>
OK. I can do it right now.
Thanks,
Keqian
> Thanks,
>
> Will
> .
>
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] iommu: Up front sanity check in the arm_lpae_map
2020-12-05 8:29 ` Keqian Zhu
(?)
@ 2020-12-07 12:01 ` Robin Murphy
-1 siblings, 0 replies; 24+ messages in thread
From: Robin Murphy @ 2020-12-07 12:01 UTC (permalink / raw)
To: Keqian Zhu, linux-kernel, linux-arm-kernel, iommu
Cc: Will Deacon, Marc Zyngier, Joerg Roedel, Catalin Marinas,
James Morse, Suzuki K Poulose, Sean Christopherson,
Julien Thierry, Mark Brown, Thomas Gleixner, Andrew Morton,
Alexios Zavras, wanghaibin.wang, jiangkunkun
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@huawei.com>
> ---
> 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);
> /*
>
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] iommu: Up front sanity check in the arm_lpae_map
@ 2020-12-07 12:01 ` Robin Murphy
0 siblings, 0 replies; 24+ messages in thread
From: Robin Murphy @ 2020-12-07 12:01 UTC (permalink / raw)
To: Keqian Zhu, linux-kernel, linux-arm-kernel, iommu
Cc: Suzuki K Poulose, Marc Zyngier, Joerg Roedel, jiangkunkun,
Sean Christopherson, Alexios Zavras, Mark Brown, James Morse,
Catalin Marinas, wanghaibin.wang, Thomas Gleixner, Will Deacon,
Andrew Morton, Julien Thierry
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@huawei.com>
> ---
> 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);
> /*
>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] iommu: Up front sanity check in the arm_lpae_map
@ 2020-12-07 12:01 ` Robin Murphy
0 siblings, 0 replies; 24+ messages in thread
From: Robin Murphy @ 2020-12-07 12:01 UTC (permalink / raw)
To: Keqian Zhu, linux-kernel, linux-arm-kernel, iommu
Cc: Suzuki K Poulose, Marc Zyngier, jiangkunkun, Sean Christopherson,
Alexios Zavras, Mark Brown, James Morse, Catalin Marinas,
wanghaibin.wang, Thomas Gleixner, Will Deacon, Andrew Morton,
Julien Thierry
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@huawei.com>
> ---
> 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);
> /*
>
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] iommu: Up front sanity check in the arm_lpae_map
2020-12-07 12:01 ` Robin Murphy
(?)
@ 2020-12-07 12:05 ` Will Deacon
-1 siblings, 0 replies; 24+ messages in thread
From: Will Deacon @ 2020-12-07 12:05 UTC (permalink / raw)
To: Robin Murphy
Cc: Keqian Zhu, linux-kernel, linux-arm-kernel, iommu, Marc Zyngier,
Joerg Roedel, Catalin Marinas, James Morse, Suzuki K Poulose,
Sean Christopherson, Julien Thierry, Mark Brown, Thomas Gleixner,
Andrew Morton, Alexios Zavras, wanghaibin.wang, jiangkunkun
On Mon, Dec 07, 2020 at 12:01:09PM +0000, Robin Murphy wrote:
> 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?
I thought it was the other way round -- doesn't this patch move the "empty
prot" check later, so we have a chance to check the size and addresses
first?
Will
> > Signed-off-by: Keqian Zhu <zhukeqian1@huawei.com>
> > ---
> > 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);
> > /*
> >
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] iommu: Up front sanity check in the arm_lpae_map
@ 2020-12-07 12:05 ` Will Deacon
0 siblings, 0 replies; 24+ messages in thread
From: Will Deacon @ 2020-12-07 12:05 UTC (permalink / raw)
To: Robin Murphy
Cc: Suzuki K Poulose, Marc Zyngier, Joerg Roedel, jiangkunkun,
linux-kernel, Sean Christopherson, Alexios Zavras, iommu,
Mark Brown, James Morse, Julien Thierry, Catalin Marinas,
wanghaibin.wang, Thomas Gleixner, Keqian Zhu, Andrew Morton,
linux-arm-kernel
On Mon, Dec 07, 2020 at 12:01:09PM +0000, Robin Murphy wrote:
> 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?
I thought it was the other way round -- doesn't this patch move the "empty
prot" check later, so we have a chance to check the size and addresses
first?
Will
> > Signed-off-by: Keqian Zhu <zhukeqian1@huawei.com>
> > ---
> > 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);
> > /*
> >
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] iommu: Up front sanity check in the arm_lpae_map
@ 2020-12-07 12:05 ` Will Deacon
0 siblings, 0 replies; 24+ messages in thread
From: Will Deacon @ 2020-12-07 12:05 UTC (permalink / raw)
To: Robin Murphy
Cc: Suzuki K Poulose, Marc Zyngier, jiangkunkun, linux-kernel,
Sean Christopherson, Alexios Zavras, iommu, Mark Brown,
James Morse, Julien Thierry, Catalin Marinas, wanghaibin.wang,
Thomas Gleixner, Keqian Zhu, Andrew Morton, linux-arm-kernel
On Mon, Dec 07, 2020 at 12:01:09PM +0000, Robin Murphy wrote:
> 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?
I thought it was the other way round -- doesn't this patch move the "empty
prot" check later, so we have a chance to check the size and addresses
first?
Will
> > Signed-off-by: Keqian Zhu <zhukeqian1@huawei.com>
> > ---
> > 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);
> > /*
> >
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] iommu: Up front sanity check in the arm_lpae_map
2020-12-07 12:05 ` Will Deacon
(?)
@ 2020-12-07 12:15 ` zhukeqian
-1 siblings, 0 replies; 24+ messages in thread
From: zhukeqian @ 2020-12-07 12:15 UTC (permalink / raw)
To: Will Deacon, Robin Murphy
Cc: linux-kernel, linux-arm-kernel, iommu, Marc Zyngier,
Joerg Roedel, Catalin Marinas, James Morse, Suzuki K Poulose,
Sean Christopherson, Julien Thierry, Mark Brown, Thomas Gleixner,
Andrew Morton, Alexios Zavras, wanghaibin.wang, jiangkunkun
Hi,
On 2020/12/7 20:05, Will Deacon wrote:
> On Mon, Dec 07, 2020 at 12:01:09PM +0000, Robin Murphy wrote:
>> 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?
>
> I thought it was the other way round -- doesn't this patch move the "empty
> prot" check later, so we have a chance to check the size and addresses
> first?
Yes, this is my original idea.
For that we treat iommu_prot with no permission as success at early start, defer
this early return can expose hidden errors.
Thanks,
Keqian
>
> Will
>
>>> Signed-off-by: Keqian Zhu <zhukeqian1@huawei.com>
>>> ---
>>> 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);
>>> /*
>>>
> .
>
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] iommu: Up front sanity check in the arm_lpae_map
@ 2020-12-07 12:15 ` zhukeqian
0 siblings, 0 replies; 24+ messages in thread
From: zhukeqian @ 2020-12-07 12:15 UTC (permalink / raw)
To: Will Deacon, Robin Murphy
Cc: Suzuki K Poulose, Marc Zyngier, Joerg Roedel, jiangkunkun,
linux-kernel, Sean Christopherson, Alexios Zavras, iommu,
Mark Brown, James Morse, Julien Thierry, Catalin Marinas,
wanghaibin.wang, Thomas Gleixner, Andrew Morton,
linux-arm-kernel
Hi,
On 2020/12/7 20:05, Will Deacon wrote:
> On Mon, Dec 07, 2020 at 12:01:09PM +0000, Robin Murphy wrote:
>> 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?
>
> I thought it was the other way round -- doesn't this patch move the "empty
> prot" check later, so we have a chance to check the size and addresses
> first?
Yes, this is my original idea.
For that we treat iommu_prot with no permission as success at early start, defer
this early return can expose hidden errors.
Thanks,
Keqian
>
> Will
>
>>> Signed-off-by: Keqian Zhu <zhukeqian1@huawei.com>
>>> ---
>>> 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);
>>> /*
>>>
> .
>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] iommu: Up front sanity check in the arm_lpae_map
@ 2020-12-07 12:15 ` zhukeqian
0 siblings, 0 replies; 24+ messages in thread
From: zhukeqian @ 2020-12-07 12:15 UTC (permalink / raw)
To: Will Deacon, Robin Murphy
Cc: Suzuki K Poulose, Marc Zyngier, jiangkunkun, linux-kernel,
Sean Christopherson, Alexios Zavras, iommu, Mark Brown,
James Morse, Julien Thierry, Catalin Marinas, wanghaibin.wang,
Thomas Gleixner, Andrew Morton, linux-arm-kernel
Hi,
On 2020/12/7 20:05, Will Deacon wrote:
> On Mon, Dec 07, 2020 at 12:01:09PM +0000, Robin Murphy wrote:
>> 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?
>
> I thought it was the other way round -- doesn't this patch move the "empty
> prot" check later, so we have a chance to check the size and addresses
> first?
Yes, this is my original idea.
For that we treat iommu_prot with no permission as success at early start, defer
this early return can expose hidden errors.
Thanks,
Keqian
>
> Will
>
>>> Signed-off-by: Keqian Zhu <zhukeqian1@huawei.com>
>>> ---
>>> 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);
>>> /*
>>>
> .
>
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] iommu: Up front sanity check in the arm_lpae_map
2020-12-07 12:15 ` zhukeqian
(?)
@ 2020-12-07 12:46 ` Robin Murphy
-1 siblings, 0 replies; 24+ messages in thread
From: Robin Murphy @ 2020-12-07 12:46 UTC (permalink / raw)
To: zhukeqian, Will Deacon
Cc: Suzuki K Poulose, Marc Zyngier, jiangkunkun, linux-kernel,
Sean Christopherson, Alexios Zavras, iommu, Mark Brown,
James Morse, Julien Thierry, Catalin Marinas, wanghaibin.wang,
Thomas Gleixner, Andrew Morton, linux-arm-kernel
On 2020-12-07 12:15, zhukeqian wrote:
> Hi,
>
> On 2020/12/7 20:05, Will Deacon wrote:
>> On Mon, Dec 07, 2020 at 12:01:09PM +0000, Robin Murphy wrote:
>>> 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?
>>
>> I thought it was the other way round -- doesn't this patch move the "empty
>> prot" check later, so we have a chance to check the size and addresses
>> first?
>
> Yes, this is my original idea.
> For that we treat iommu_prot with no permission as success at early start, defer
> this early return can expose hidden errors.
...oh dear, my apologies. I've just had a week off and apparently in
that time I lost the ability to read :(
I was somehow convinced that the existing check happened at the point
where we go to install the PTE, and this patch was moving it earlier.
Looking at the whole code in context now I see I got it completely
backwards. Sorry for being an idiot.
I guess that only goes to show that a more descriptive commit message
would definitely be a good thing :)
Robin.
>
> Thanks,
> Keqian
>>
>> Will
>>
>>>> Signed-off-by: Keqian Zhu <zhukeqian1@huawei.com>
>>>> ---
>>>> 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);
>>>> /*
>>>>
>> .
>>
> _______________________________________________
> iommu mailing list
> iommu@lists.linux-foundation.org
> https://lists.linuxfoundation.org/mailman/listinfo/iommu
>
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] iommu: Up front sanity check in the arm_lpae_map
@ 2020-12-07 12:46 ` Robin Murphy
0 siblings, 0 replies; 24+ messages in thread
From: Robin Murphy @ 2020-12-07 12:46 UTC (permalink / raw)
To: zhukeqian, Will Deacon
Cc: Suzuki K Poulose, Marc Zyngier, jiangkunkun, linux-kernel,
Sean Christopherson, Alexios Zavras, iommu, Mark Brown,
James Morse, linux-arm-kernel, Catalin Marinas, wanghaibin.wang,
Thomas Gleixner, Andrew Morton, Julien Thierry
On 2020-12-07 12:15, zhukeqian wrote:
> Hi,
>
> On 2020/12/7 20:05, Will Deacon wrote:
>> On Mon, Dec 07, 2020 at 12:01:09PM +0000, Robin Murphy wrote:
>>> 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?
>>
>> I thought it was the other way round -- doesn't this patch move the "empty
>> prot" check later, so we have a chance to check the size and addresses
>> first?
>
> Yes, this is my original idea.
> For that we treat iommu_prot with no permission as success at early start, defer
> this early return can expose hidden errors.
...oh dear, my apologies. I've just had a week off and apparently in
that time I lost the ability to read :(
I was somehow convinced that the existing check happened at the point
where we go to install the PTE, and this patch was moving it earlier.
Looking at the whole code in context now I see I got it completely
backwards. Sorry for being an idiot.
I guess that only goes to show that a more descriptive commit message
would definitely be a good thing :)
Robin.
>
> Thanks,
> Keqian
>>
>> Will
>>
>>>> Signed-off-by: Keqian Zhu <zhukeqian1@huawei.com>
>>>> ---
>>>> 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);
>>>> /*
>>>>
>> .
>>
> _______________________________________________
> iommu mailing list
> iommu@lists.linux-foundation.org
> https://lists.linuxfoundation.org/mailman/listinfo/iommu
>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] iommu: Up front sanity check in the arm_lpae_map
@ 2020-12-07 12:46 ` Robin Murphy
0 siblings, 0 replies; 24+ messages in thread
From: Robin Murphy @ 2020-12-07 12:46 UTC (permalink / raw)
To: zhukeqian, Will Deacon
Cc: Suzuki K Poulose, Marc Zyngier, jiangkunkun, linux-kernel,
Sean Christopherson, Alexios Zavras, iommu, Mark Brown,
James Morse, linux-arm-kernel, Catalin Marinas, wanghaibin.wang,
Thomas Gleixner, Andrew Morton, Julien Thierry
On 2020-12-07 12:15, zhukeqian wrote:
> Hi,
>
> On 2020/12/7 20:05, Will Deacon wrote:
>> On Mon, Dec 07, 2020 at 12:01:09PM +0000, Robin Murphy wrote:
>>> 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?
>>
>> I thought it was the other way round -- doesn't this patch move the "empty
>> prot" check later, so we have a chance to check the size and addresses
>> first?
>
> Yes, this is my original idea.
> For that we treat iommu_prot with no permission as success at early start, defer
> this early return can expose hidden errors.
...oh dear, my apologies. I've just had a week off and apparently in
that time I lost the ability to read :(
I was somehow convinced that the existing check happened at the point
where we go to install the PTE, and this patch was moving it earlier.
Looking at the whole code in context now I see I got it completely
backwards. Sorry for being an idiot.
I guess that only goes to show that a more descriptive commit message
would definitely be a good thing :)
Robin.
>
> Thanks,
> Keqian
>>
>> Will
>>
>>>> Signed-off-by: Keqian Zhu <zhukeqian1@huawei.com>
>>>> ---
>>>> 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);
>>>> /*
>>>>
>> .
>>
> _______________________________________________
> iommu mailing list
> iommu@lists.linux-foundation.org
> https://lists.linuxfoundation.org/mailman/listinfo/iommu
>
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] iommu: Up front sanity check in the arm_lpae_map
2020-12-07 12:46 ` Robin Murphy
(?)
@ 2020-12-07 13:36 ` zhukeqian
-1 siblings, 0 replies; 24+ messages in thread
From: zhukeqian @ 2020-12-07 13:36 UTC (permalink / raw)
To: Robin Murphy, Will Deacon
Cc: Suzuki K Poulose, Marc Zyngier, jiangkunkun, linux-kernel,
Sean Christopherson, Alexios Zavras, iommu, Mark Brown,
James Morse, Julien Thierry, Catalin Marinas, wanghaibin.wang,
Thomas Gleixner, Andrew Morton, linux-arm-kernel
Hi Robin,
On 2020/12/7 20:46, Robin Murphy wrote:
> On 2020-12-07 12:15, zhukeqian wrote:
>> Hi,
>>
>> On 2020/12/7 20:05, Will Deacon wrote:
>>> On Mon, Dec 07, 2020 at 12:01:09PM +0000, Robin Murphy wrote:
>>>> 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?
>>>
>>> I thought it was the other way round -- doesn't this patch move the "empty
>>> prot" check later, so we have a chance to check the size and addresses
>>> first?
>>
>> Yes, this is my original idea.
>> For that we treat iommu_prot with no permission as success at early start, defer
>> this early return can expose hidden errors.
>
> ...oh dear, my apologies. I've just had a week off and apparently in that time I lost the ability to read :(
>
> I was somehow convinced that the existing check happened at the point where we go to install the PTE, and this patch was moving it earlier. Looking at the whole code in context now I see I got it completely backwards. Sorry for being an idiot.
>
I see :-) I should make a more descriptive commit message.
> I guess that only goes to show that a more descriptive commit message would definitely be a good thing :)
>
I have adapted Will's suggestion and sent v2, please check whether it is OK to you?
Cheers,
Keqian
[...]
>> _______________________________________________
>> iommu mailing list
>> iommu@lists.linux-foundation.org
>> https://lists.linuxfoundation.org/mailman/listinfo/iommu
>>
> .
>
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] iommu: Up front sanity check in the arm_lpae_map
@ 2020-12-07 13:36 ` zhukeqian
0 siblings, 0 replies; 24+ messages in thread
From: zhukeqian @ 2020-12-07 13:36 UTC (permalink / raw)
To: Robin Murphy, Will Deacon
Cc: Suzuki K Poulose, Marc Zyngier, jiangkunkun, linux-kernel,
Sean Christopherson, Alexios Zavras, iommu, Mark Brown,
James Morse, linux-arm-kernel, Catalin Marinas, wanghaibin.wang,
Thomas Gleixner, Andrew Morton, Julien Thierry
Hi Robin,
On 2020/12/7 20:46, Robin Murphy wrote:
> On 2020-12-07 12:15, zhukeqian wrote:
>> Hi,
>>
>> On 2020/12/7 20:05, Will Deacon wrote:
>>> On Mon, Dec 07, 2020 at 12:01:09PM +0000, Robin Murphy wrote:
>>>> 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?
>>>
>>> I thought it was the other way round -- doesn't this patch move the "empty
>>> prot" check later, so we have a chance to check the size and addresses
>>> first?
>>
>> Yes, this is my original idea.
>> For that we treat iommu_prot with no permission as success at early start, defer
>> this early return can expose hidden errors.
>
> ...oh dear, my apologies. I've just had a week off and apparently in that time I lost the ability to read :(
>
> I was somehow convinced that the existing check happened at the point where we go to install the PTE, and this patch was moving it earlier. Looking at the whole code in context now I see I got it completely backwards. Sorry for being an idiot.
>
I see :-) I should make a more descriptive commit message.
> I guess that only goes to show that a more descriptive commit message would definitely be a good thing :)
>
I have adapted Will's suggestion and sent v2, please check whether it is OK to you?
Cheers,
Keqian
[...]
>> _______________________________________________
>> iommu mailing list
>> iommu@lists.linux-foundation.org
>> https://lists.linuxfoundation.org/mailman/listinfo/iommu
>>
> .
>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH] iommu: Up front sanity check in the arm_lpae_map
@ 2020-12-07 13:36 ` zhukeqian
0 siblings, 0 replies; 24+ messages in thread
From: zhukeqian @ 2020-12-07 13:36 UTC (permalink / raw)
To: Robin Murphy, Will Deacon
Cc: Suzuki K Poulose, Marc Zyngier, jiangkunkun, linux-kernel,
Sean Christopherson, Alexios Zavras, iommu, Mark Brown,
James Morse, linux-arm-kernel, Catalin Marinas, wanghaibin.wang,
Thomas Gleixner, Andrew Morton, Julien Thierry
Hi Robin,
On 2020/12/7 20:46, Robin Murphy wrote:
> On 2020-12-07 12:15, zhukeqian wrote:
>> Hi,
>>
>> On 2020/12/7 20:05, Will Deacon wrote:
>>> On Mon, Dec 07, 2020 at 12:01:09PM +0000, Robin Murphy wrote:
>>>> 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?
>>>
>>> I thought it was the other way round -- doesn't this patch move the "empty
>>> prot" check later, so we have a chance to check the size and addresses
>>> first?
>>
>> Yes, this is my original idea.
>> For that we treat iommu_prot with no permission as success at early start, defer
>> this early return can expose hidden errors.
>
> ...oh dear, my apologies. I've just had a week off and apparently in that time I lost the ability to read :(
>
> I was somehow convinced that the existing check happened at the point where we go to install the PTE, and this patch was moving it earlier. Looking at the whole code in context now I see I got it completely backwards. Sorry for being an idiot.
>
I see :-) I should make a more descriptive commit message.
> I guess that only goes to show that a more descriptive commit message would definitely be a good thing :)
>
I have adapted Will's suggestion and sent v2, please check whether it is OK to you?
Cheers,
Keqian
[...]
>> _______________________________________________
>> iommu mailing list
>> iommu@lists.linux-foundation.org
>> https://lists.linuxfoundation.org/mailman/listinfo/iommu
>>
> .
>
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
^ permalink raw reply [flat|nested] 24+ messages in thread