[PATCH] dma-mapping: benchmark: check the validity of dma mask bits

[PATCH] dma-mapping: benchmark: check the validity of dma mask bits

[Barry Song]

While dma_mask_bits is larger than 64, the bahvaiour is undefined. On the
other hand, dma_mask_bits which is smaller than 20 (1MB) makes no sense
in real hardware.

Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
Signed-off-by: Barry Song <song.bao.hua@hisilicon.com>
---
 kernel/dma/map_benchmark.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/kernel/dma/map_benchmark.c b/kernel/dma/map_benchmark.c
index b1496e744c68..19f661692073 100644
--- a/kernel/dma/map_benchmark.c
+++ b/kernel/dma/map_benchmark.c
@@ -214,6 +214,12 @@ static long map_benchmark_ioctl(struct file *file, unsigned int cmd,
 			return -EINVAL;
 		}
 
+		if (map->bparam.dma_bits < 20 ||
+		    map->bparam.dma_bits > 64) {
+			pr_err("invalid dma_bits\n");
+			return -EINVAL;
+		}
+
 		if (map->bparam.node != NUMA_NO_NODE &&
 		    !node_possible(map->bparam.node)) {
 			pr_err("invalid numa node\n");
-- 
2.25.1

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH] dma-mapping: benchmark: check the validity of dma mask bits

[Robin Murphy]

On 2020-12-12 10:18, Barry Song wrote:
> While dma_mask_bits is larger than 64, the bahvaiour is undefined. On the
> other hand, dma_mask_bits which is smaller than 20 (1MB) makes no sense
> in real hardware.
> 
> Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
> Signed-off-by: Barry Song <song.bao.hua@hisilicon.com>
> ---
>   kernel/dma/map_benchmark.c | 6 ++++++
>   1 file changed, 6 insertions(+)
> 
> diff --git a/kernel/dma/map_benchmark.c b/kernel/dma/map_benchmark.c
> index b1496e744c68..19f661692073 100644
> --- a/kernel/dma/map_benchmark.c
> +++ b/kernel/dma/map_benchmark.c
> @@ -214,6 +214,12 @@ static long map_benchmark_ioctl(struct file *file, unsigned int cmd,
>   			return -EINVAL;
>   		}
>   
> +		if (map->bparam.dma_bits < 20 ||

FWIW I don't think we need to bother with a lower limit here - it's 
unsigned, and a pointlessly small value will fail gracefully when we 
come to actually set the mask anyway. We only need to protect kernel 
code from going wrong, not userspace from being stupid to its own detriment.

Robin.

> +		    map->bparam.dma_bits > 64) {
> +			pr_err("invalid dma_bits\n");
> +			return -EINVAL;
> +		}
> +
>   		if (map->bparam.node != NUMA_NO_NODE &&
>   		    !node_possible(map->bparam.node)) {
>   			pr_err("invalid numa node\n");
> 
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

RE: [PATCH] dma-mapping: benchmark: check the validity of dma mask bits

[Song Bao Hua (Barry Song)]

> -----Original Message-----
> From: Robin Murphy [mailto:robin.murphy@arm.com]
> Sent: Saturday, December 19, 2020 7:10 AM
> To: Song Bao Hua (Barry Song) <song.bao.hua@hisilicon.com>; hch@lst.de;
> m.szyprowski@samsung.com
> Cc: iommu@lists.linux-foundation.org; Linuxarm <linuxarm@huawei.com>; Dan
> Carpenter <dan.carpenter@oracle.com>
> Subject: Re: [PATCH] dma-mapping: benchmark: check the validity of dma mask
> bits
> 
> On 2020-12-12 10:18, Barry Song wrote:
> > While dma_mask_bits is larger than 64, the bahvaiour is undefined. On the
> > other hand, dma_mask_bits which is smaller than 20 (1MB) makes no sense
> > in real hardware.
> >
> > Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
> > Signed-off-by: Barry Song <song.bao.hua@hisilicon.com>
> > ---
> >   kernel/dma/map_benchmark.c | 6 ++++++
> >   1 file changed, 6 insertions(+)
> >
> > diff --git a/kernel/dma/map_benchmark.c b/kernel/dma/map_benchmark.c
> > index b1496e744c68..19f661692073 100644
> > --- a/kernel/dma/map_benchmark.c
> > +++ b/kernel/dma/map_benchmark.c
> > @@ -214,6 +214,12 @@ static long map_benchmark_ioctl(struct file *file,
> unsigned int cmd,
> >   			return -EINVAL;
> >   		}
> >
> > +		if (map->bparam.dma_bits < 20 ||
> 
> FWIW I don't think we need to bother with a lower limit here - it's
> unsigned, and a pointlessly small value will fail gracefully when we
> come to actually set the mask anyway. We only need to protect kernel
> code from going wrong, not userspace from being stupid to its own detriment.

I am not sure if kernel driver can reject small dma mask bit if drivers
don't handle it properly.
As a month ago, when I was debugging dma map benchmark, I set a value
less than 32 to devices behind arm-smmu-v3, it could always succeed.
But dma_map_single() was always failing.
At that time, I didn't debug this issue. Not sure the latest status of
iommu driver.

drivers/iommu/intel/iommu.c used to have a dma_supported() to reject
small dma_mask:
static const struct dma_map_ops bounce_dma_ops = {
	...
	.dma_supported		= dma_direct_supported,
};


> 
> Robin.
> 
> > +		    map->bparam.dma_bits > 64) {
> > +			pr_err("invalid dma_bits\n");
> > +			return -EINVAL;
> > +		}
> > +
> >   		if (map->bparam.node != NUMA_NO_NODE &&
> >   		    !node_possible(map->bparam.node)) {
> >   			pr_err("invalid numa node\n");
> >

Thanks
Barry

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH] dma-mapping: benchmark: check the validity of dma mask bits

[Robin Murphy]

On 2020-12-19 03:15, Song Bao Hua (Barry Song) wrote:
> 
> 
>> -----Original Message-----
>> From: Robin Murphy [mailto:robin.murphy@arm.com]
>> Sent: Saturday, December 19, 2020 7:10 AM
>> To: Song Bao Hua (Barry Song) <song.bao.hua@hisilicon.com>; hch@lst.de;
>> m.szyprowski@samsung.com
>> Cc: iommu@lists.linux-foundation.org; Linuxarm <linuxarm@huawei.com>; Dan
>> Carpenter <dan.carpenter@oracle.com>
>> Subject: Re: [PATCH] dma-mapping: benchmark: check the validity of dma mask
>> bits
>>
>> On 2020-12-12 10:18, Barry Song wrote:
>>> While dma_mask_bits is larger than 64, the bahvaiour is undefined. On the
>>> other hand, dma_mask_bits which is smaller than 20 (1MB) makes no sense
>>> in real hardware.
>>>
>>> Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
>>> Signed-off-by: Barry Song <song.bao.hua@hisilicon.com>
>>> ---
>>>    kernel/dma/map_benchmark.c | 6 ++++++
>>>    1 file changed, 6 insertions(+)
>>>
>>> diff --git a/kernel/dma/map_benchmark.c b/kernel/dma/map_benchmark.c
>>> index b1496e744c68..19f661692073 100644
>>> --- a/kernel/dma/map_benchmark.c
>>> +++ b/kernel/dma/map_benchmark.c
>>> @@ -214,6 +214,12 @@ static long map_benchmark_ioctl(struct file *file,
>> unsigned int cmd,
>>>    			return -EINVAL;
>>>    		}
>>>
>>> +		if (map->bparam.dma_bits < 20 ||
>>
>> FWIW I don't think we need to bother with a lower limit here - it's
>> unsigned, and a pointlessly small value will fail gracefully when we
>> come to actually set the mask anyway. We only need to protect kernel
>> code from going wrong, not userspace from being stupid to its own detriment.
> 
> I am not sure if kernel driver can reject small dma mask bit if drivers
> don't handle it properly.
> As a month ago, when I was debugging dma map benchmark, I set a value
> less than 32 to devices behind arm-smmu-v3, it could always succeed.
> But dma_map_single() was always failing.
> At that time, I didn't debug this issue. Not sure the latest status of
> iommu driver.

FWIW, dma-direct should reject a mask if it doesn't cover at least the 
whole of ZONE_DMA; iommu-dma does allow anything, but that's because in 
principle it can make any mask down to PAGE_SIZE (or possibly even lower 
depending on the IOMMU) work. It's just that in that case the driver is 
liable to fill up the usable address space really really quickly :)

(I suppose technically it should be checking that masks at least cover 
more than the reserved PFN at IOVA 0, but meh...)

Either way, it still has little bearing on the benchmark itself. Say the 
user successfully sets an "acceptable" 21-bit DMA mask, but with 64K 
pages and >32 threads - the dma_map operations are still likely to start 
failing, and that failure is handled anyway, so why bother having an 
arbitrary and meaningless limit that only serves to make some unworkable 
cases fail slightly differently to others?

Anyway, this doesn't really matter - I see the patch is in -next already 
- it's just one of those things I can't help calling out on principle :)

Robin.
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu