Re: [PATCH V10 06/12] of: device: Fix overflow of coherent_dma_mask
On 08/04/17 00:13, Frank Rowand wrote:
> On 04/07/17 07:46, Robin Murphy wrote:
>> On 06/04/17 20:34, Frank Rowand wrote:
>>> On 04/06/17 04:01, Sricharan R wrote:
Hi Frank,
On 4/6/2017 12:31 PM, Frank Rowand wrote:
> On 04/04/17 03:18, Sricharan R wrote:
>> Size of the dma-range is calculated as coherent_dma_mask + 1
>> and passed to arch_setup_dma_ops further. It overflows when
>> the coherent_dma_mask is set for full 64 bits 0x,
>> resulting in size getting passed as 0 wrongly. Fix this by
>> passsing in max(mask, mask + 1). Note that in this case
>> when the mask is set to full 64bits, we will be passing the mask
>> itself to arch_setup_dma_ops instead of the size. The real fix
>> for this should be to make arch_setup_dma_ops receive the
>> mask and handle it, to be done in the future.
>>
>> Signed-off-by: Sricharan R
>> ---
>> drivers/of/device.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/of/device.c b/drivers/of/device.c
>> index c17c19d..c2ae6bb 100644
>> --- a/drivers/of/device.c
>> +++ b/drivers/of/device.c
>> @@ -107,7 +107,7 @@ void of_dma_configure(struct device *dev, struct
>> device_node *np)
>> ret = of_dma_get_range(np, &dma_addr, &paddr, &size);
>> if (ret < 0) {
>> dma_addr = offset = 0;
>> -size = dev->coherent_dma_mask + 1;
>> +size = max(dev->coherent_dma_mask, dev->coherent_dma_mask + 1);
>> } else {
>> offset = PFN_DOWN(paddr - dma_addr);
>> dev_dbg(dev, "dma_pfn_offset(%#08lx)\n", offset);
>>
>
> NACK.
>
> Passing an invalid size to arch_setup_dma_ops() is only part of the
> problem.
> size is also used in of_dma_configure() before calling
> arch_setup_dma_ops():
>
> dev->coherent_dma_mask = min(dev->coherent_dma_mask,
> DMA_BIT_MASK(ilog2(dma_addr +
> size)));
> *dev->dma_mask = min((*dev->dma_mask),
> DMA_BIT_MASK(ilog2(dma_addr + size)));
>
> which would be incorrect for size == 0xULL when
> dma_addr != 0. So the proposed fix really is not papering over
> the base problem very well.
>
Ok, but with your fix for of_dma_get_range and the above fix,
dma_addr will be '0' when size = 0xULL,
but DMA_BIT_MASK(ilog2(dma_addr + size)) would be wrong though,
making coherent_dma_mask to be smaller 0x7fffULL.
>>>
>>> Yes, that was my point. Setting size to 0x7fffULL
>>> affects several places. Another potential location (based only
>>> on the function header comment, not from reading the code) is
>>> iommu_dma_init_domain(). The header comment says:
>>>
>>> * @base and @size should be exact multiples of IOMMU page granularity to
>>> * avoid rounding surprises.
>>
>> That is really only referring to the fact that some of the work done
>> therein involves truncation to PFNs, so anyone passing in non-exact
>> values expecting them to round a particular way may get things off by a
>> page one way or the other. It's not going to have much practical
>> significance for real devices (in particular since size is used more as
>> a sanity check than any kind of actual limit there).
>>
>>> I have not read enough context to really understand of_dma_configure(), but
>>> it seems there is yet another issue in how the error return case from
>>> of_dma_get_range() is handled (with the existing code, as well as if
>>> my patch gets accepted). An error return value can mean _either_
>>> there is no dma-ranges property _or_ "an other problem occurred". Should
>>> the "an other problem occurred" case be handled by defaulting size to
>>> a value based on dev->coherent_dma_mask (the current case) or should the
>>> attempt to set up the DMA configuration just fail?
>>
>> There is indeed a lot wrong with of_dma_configure() and
>> arch_setup_dma_ops(), but fixing those is beyond the scope of this
>> series. This is just working around a latent bug in the one specific
>> case where a value is *not* derived from DT. Any DT which worked before
>> still works; any DT which made of_dma_configure() go wrong before still
>> makes of_dma_configure() go wrong exactly the same.
>>
>> Whilst it's not ideal, since a DMA mask basically represents the maximum
>> size of address that that particular device can be given, I can't see it
>> making any practical difference for a full 64-bit DMA mask to be trimmed
>> down to 63 bits upon re-probing - no system is likely to have that many
>> physical address bits anyway, and I don't think any IOMMUs support that
>> large an IOVA space either, so as long as it's still big enough to cover
>> "everything", it'll be OK.
>>
>> Of course, whether DMA_BIT_MASK(ilog2(dma_addr + size)) is the righ
Re: [PATCH V10 06/12] of: device: Fix overflow of coherent_dma_mask
On 04/07/17 07:46, Robin Murphy wrote:
> On 06/04/17 20:34, Frank Rowand wrote:
>> On 04/06/17 04:01, Sricharan R wrote:
>>> Hi Frank,
>>>
>>> On 4/6/2017 12:31 PM, Frank Rowand wrote:
On 04/04/17 03:18, Sricharan R wrote:
> Size of the dma-range is calculated as coherent_dma_mask + 1
> and passed to arch_setup_dma_ops further. It overflows when
> the coherent_dma_mask is set for full 64 bits 0x,
> resulting in size getting passed as 0 wrongly. Fix this by
> passsing in max(mask, mask + 1). Note that in this case
> when the mask is set to full 64bits, we will be passing the mask
> itself to arch_setup_dma_ops instead of the size. The real fix
> for this should be to make arch_setup_dma_ops receive the
> mask and handle it, to be done in the future.
>
> Signed-off-by: Sricharan R
> ---
> drivers/of/device.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/of/device.c b/drivers/of/device.c
> index c17c19d..c2ae6bb 100644
> --- a/drivers/of/device.c
> +++ b/drivers/of/device.c
> @@ -107,7 +107,7 @@ void of_dma_configure(struct device *dev, struct
> device_node *np)
> ret = of_dma_get_range(np, &dma_addr, &paddr, &size);
> if (ret < 0) {
> dma_addr = offset = 0;
> -size = dev->coherent_dma_mask + 1;
> +size = max(dev->coherent_dma_mask, dev->coherent_dma_mask + 1);
> } else {
> offset = PFN_DOWN(paddr - dma_addr);
> dev_dbg(dev, "dma_pfn_offset(%#08lx)\n", offset);
>
NACK.
Passing an invalid size to arch_setup_dma_ops() is only part of the
problem.
size is also used in of_dma_configure() before calling
arch_setup_dma_ops():
dev->coherent_dma_mask = min(dev->coherent_dma_mask,
DMA_BIT_MASK(ilog2(dma_addr + size)));
*dev->dma_mask = min((*dev->dma_mask),
DMA_BIT_MASK(ilog2(dma_addr + size)));
which would be incorrect for size == 0xULL when
dma_addr != 0. So the proposed fix really is not papering over
the base problem very well.
>>>
>>> Ok, but with your fix for of_dma_get_range and the above fix,
>>> dma_addr will be '0' when size = 0xULL,
>>> but DMA_BIT_MASK(ilog2(dma_addr + size)) would be wrong though,
>>> making coherent_dma_mask to be smaller 0x7fffULL.
>>
>> Yes, that was my point. Setting size to 0x7fffULL
>> affects several places. Another potential location (based only
>> on the function header comment, not from reading the code) is
>> iommu_dma_init_domain(). The header comment says:
>>
>> * @base and @size should be exact multiples of IOMMU page granularity to
>> * avoid rounding surprises.
>
> That is really only referring to the fact that some of the work done
> therein involves truncation to PFNs, so anyone passing in non-exact
> values expecting them to round a particular way may get things off by a
> page one way or the other. It's not going to have much practical
> significance for real devices (in particular since size is used more as
> a sanity check than any kind of actual limit there).
>
>> I have not read enough context to really understand of_dma_configure(), but
>> it seems there is yet another issue in how the error return case from
>> of_dma_get_range() is handled (with the existing code, as well as if
>> my patch gets accepted). An error return value can mean _either_
>> there is no dma-ranges property _or_ "an other problem occurred". Should
>> the "an other problem occurred" case be handled by defaulting size to
>> a value based on dev->coherent_dma_mask (the current case) or should the
>> attempt to set up the DMA configuration just fail?
>
> There is indeed a lot wrong with of_dma_configure() and
> arch_setup_dma_ops(), but fixing those is beyond the scope of this
> series. This is just working around a latent bug in the one specific
> case where a value is *not* derived from DT. Any DT which worked before
> still works; any DT which made of_dma_configure() go wrong before still
> makes of_dma_configure() go wrong exactly the same.
>
> Whilst it's not ideal, since a DMA mask basically represents the maximum
> size of address that that particular device can be given, I can't see it
> making any practical difference for a full 64-bit DMA mask to be trimmed
> down to 63 bits upon re-probing - no system is likely to have that many
> physical address bits anyway, and I don't think any IOMMUs support that
> large an IOVA space either, so as long as it's still big enough to cover
> "everything", it'll be OK.
>
> Of course, whether DMA_BIT_MASK(ilog2(dma_addr + size)) is the right
> thing to do in the first place is yet another matter, as there are
> plenty of cases where it results in something which can't reach the
>
Re: [PATCH V10 06/12] of: device: Fix overflow of coherent_dma_mask
On 04/06/17 00:01, Frank Rowand wrote:
> On 04/04/17 03:18, Sricharan R wrote:
>> Size of the dma-range is calculated as coherent_dma_mask + 1
>> and passed to arch_setup_dma_ops further. It overflows when
>> the coherent_dma_mask is set for full 64 bits 0x,
>> resulting in size getting passed as 0 wrongly. Fix this by
>> passsing in max(mask, mask + 1). Note that in this case
>> when the mask is set to full 64bits, we will be passing the mask
>> itself to arch_setup_dma_ops instead of the size. The real fix
>> for this should be to make arch_setup_dma_ops receive the
>> mask and handle it, to be done in the future.
>>
>> Signed-off-by: Sricharan R
>> ---
>> drivers/of/device.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/of/device.c b/drivers/of/device.c
>> index c17c19d..c2ae6bb 100644
>> --- a/drivers/of/device.c
>> +++ b/drivers/of/device.c
>> @@ -107,7 +107,7 @@ void of_dma_configure(struct device *dev, struct
>> device_node *np)
>> ret = of_dma_get_range(np, &dma_addr, &paddr, &size);
>> if (ret < 0) {
>> dma_addr = offset = 0;
>> -size = dev->coherent_dma_mask + 1;
>> +size = max(dev->coherent_dma_mask, dev->coherent_dma_mask + 1);
NACK withdrawn below.
However, I would prefer a change to this line for readability. Using max()
results
in the correct result, but obscures the reason behind the algorithm, where the
intent is to avoid an overflow. How about something like:
size = (dev->coherent_dma_mask == 0xULL)
? 0xULL : dev->coherent_dma_mask + 1;
>> } else {
>> offset = PFN_DOWN(paddr - dma_addr);
>> dev_dbg(dev, "dma_pfn_offset(%#08lx)\n", offset);
>>
>
> NACK.
>
> Passing an invalid size to arch_setup_dma_ops() is only part of the problem.
> size is also used in of_dma_configure() before calling arch_setup_dma_ops():
>
> dev->coherent_dma_mask = min(dev->coherent_dma_mask,
> DMA_BIT_MASK(ilog2(dma_addr + size)));
> *dev->dma_mask = min((*dev->dma_mask),
> DMA_BIT_MASK(ilog2(dma_addr + size)));
>
> which would be incorrect for size == 0xULL when
> dma_addr != 0. So the proposed fix really is not papering over
^ This is the flaw in my objection. When in the
(ret < 0) path, dma_addr is set to zero. So my worry about dma_addr != 0
is baseless.
I withdraw my NACK because my analysis was flawed.
-Frank
> the base problem very well.
>
> I agree that the proper solution involves passing a mask instead
> of a size to arch_setup_dma_ops().
>
> -Frank
>
Re: [PATCH V10 06/12] of: device: Fix overflow of coherent_dma_mask
On 06/04/17 20:34, Frank Rowand wrote:
> On 04/06/17 04:01, Sricharan R wrote:
>> Hi Frank,
>>
>> On 4/6/2017 12:31 PM, Frank Rowand wrote:
>>> On 04/04/17 03:18, Sricharan R wrote:
Size of the dma-range is calculated as coherent_dma_mask + 1
and passed to arch_setup_dma_ops further. It overflows when
the coherent_dma_mask is set for full 64 bits 0x,
resulting in size getting passed as 0 wrongly. Fix this by
passsing in max(mask, mask + 1). Note that in this case
when the mask is set to full 64bits, we will be passing the mask
itself to arch_setup_dma_ops instead of the size. The real fix
for this should be to make arch_setup_dma_ops receive the
mask and handle it, to be done in the future.
Signed-off-by: Sricharan R
---
drivers/of/device.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/of/device.c b/drivers/of/device.c
index c17c19d..c2ae6bb 100644
--- a/drivers/of/device.c
+++ b/drivers/of/device.c
@@ -107,7 +107,7 @@ void of_dma_configure(struct device *dev, struct
device_node *np)
ret = of_dma_get_range(np, &dma_addr, &paddr, &size);
if (ret < 0) {
dma_addr = offset = 0;
-size = dev->coherent_dma_mask + 1;
+size = max(dev->coherent_dma_mask, dev->coherent_dma_mask + 1);
} else {
offset = PFN_DOWN(paddr - dma_addr);
dev_dbg(dev, "dma_pfn_offset(%#08lx)\n", offset);
>>>
>>> NACK.
>>>
>>> Passing an invalid size to arch_setup_dma_ops() is only part of the problem.
>>> size is also used in of_dma_configure() before calling arch_setup_dma_ops():
>>>
>>> dev->coherent_dma_mask = min(dev->coherent_dma_mask,
>>> DMA_BIT_MASK(ilog2(dma_addr + size)));
>>> *dev->dma_mask = min((*dev->dma_mask),
>>> DMA_BIT_MASK(ilog2(dma_addr + size)));
>>>
>>> which would be incorrect for size == 0xULL when
>>> dma_addr != 0. So the proposed fix really is not papering over
>>> the base problem very well.
>>>
>>
>> Ok, but with your fix for of_dma_get_range and the above fix,
>> dma_addr will be '0' when size = 0xULL,
>> but DMA_BIT_MASK(ilog2(dma_addr + size)) would be wrong though,
>> making coherent_dma_mask to be smaller 0x7fffULL.
>
> Yes, that was my point. Setting size to 0x7fffULL
> affects several places. Another potential location (based only
> on the function header comment, not from reading the code) is
> iommu_dma_init_domain(). The header comment says:
>
> * @base and @size should be exact multiples of IOMMU page granularity to
> * avoid rounding surprises.
That is really only referring to the fact that some of the work done
therein involves truncation to PFNs, so anyone passing in non-exact
values expecting them to round a particular way may get things off by a
page one way or the other. It's not going to have much practical
significance for real devices (in particular since size is used more as
a sanity check than any kind of actual limit there).
> I have not read enough context to really understand of_dma_configure(), but
> it seems there is yet another issue in how the error return case from
> of_dma_get_range() is handled (with the existing code, as well as if
> my patch gets accepted). An error return value can mean _either_
> there is no dma-ranges property _or_ "an other problem occurred". Should
> the "an other problem occurred" case be handled by defaulting size to
> a value based on dev->coherent_dma_mask (the current case) or should the
> attempt to set up the DMA configuration just fail?
There is indeed a lot wrong with of_dma_configure() and
arch_setup_dma_ops(), but fixing those is beyond the scope of this
series. This is just working around a latent bug in the one specific
case where a value is *not* derived from DT. Any DT which worked before
still works; any DT which made of_dma_configure() go wrong before still
makes of_dma_configure() go wrong exactly the same.
Whilst it's not ideal, since a DMA mask basically represents the maximum
size of address that that particular device can be given, I can't see it
making any practical difference for a full 64-bit DMA mask to be trimmed
down to 63 bits upon re-probing - no system is likely to have that many
physical address bits anyway, and I don't think any IOMMUs support that
large an IOVA space either, so as long as it's still big enough to cover
"everything", it'll be OK.
Of course, whether DMA_BIT_MASK(ilog2(dma_addr + size)) is the right
thing to do in the first place is yet another matter, as there are
plenty of cases where it results in something which can't reach the
given range at all, but again, this isn't the place. Much as I'm keen to
get the behaviour of of_dma_configure() sorted out properly, it doesn't
seem reasonable that that should
Re: [PATCH V10 06/12] of: device: Fix overflow of coherent_dma_mask
Hi Frank,
On 4/7/2017 1:04 AM, Frank Rowand wrote:
On 04/06/17 04:01, Sricharan R wrote:
Hi Frank,
On 4/6/2017 12:31 PM, Frank Rowand wrote:
On 04/04/17 03:18, Sricharan R wrote:
Size of the dma-range is calculated as coherent_dma_mask + 1
and passed to arch_setup_dma_ops further. It overflows when
the coherent_dma_mask is set for full 64 bits 0x,
resulting in size getting passed as 0 wrongly. Fix this by
passsing in max(mask, mask + 1). Note that in this case
when the mask is set to full 64bits, we will be passing the mask
itself to arch_setup_dma_ops instead of the size. The real fix
for this should be to make arch_setup_dma_ops receive the
mask and handle it, to be done in the future.
Signed-off-by: Sricharan R
---
drivers/of/device.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/of/device.c b/drivers/of/device.c
index c17c19d..c2ae6bb 100644
--- a/drivers/of/device.c
+++ b/drivers/of/device.c
@@ -107,7 +107,7 @@ void of_dma_configure(struct device *dev, struct
device_node *np)
ret = of_dma_get_range(np, &dma_addr, &paddr, &size);
if (ret < 0) {
dma_addr = offset = 0;
-size = dev->coherent_dma_mask + 1;
+size = max(dev->coherent_dma_mask, dev->coherent_dma_mask + 1);
} else {
offset = PFN_DOWN(paddr - dma_addr);
dev_dbg(dev, "dma_pfn_offset(%#08lx)\n", offset);
NACK.
Passing an invalid size to arch_setup_dma_ops() is only part of the problem.
size is also used in of_dma_configure() before calling arch_setup_dma_ops():
dev->coherent_dma_mask = min(dev->coherent_dma_mask,
DMA_BIT_MASK(ilog2(dma_addr + size)));
*dev->dma_mask = min((*dev->dma_mask),
DMA_BIT_MASK(ilog2(dma_addr + size)));
which would be incorrect for size == 0xULL when
dma_addr != 0. So the proposed fix really is not papering over
the base problem very well.
Ok, but with your fix for of_dma_get_range and the above fix,
dma_addr will be '0' when size = 0xULL,
but DMA_BIT_MASK(ilog2(dma_addr + size)) would be wrong though,
making coherent_dma_mask to be smaller 0x7fffULL.
Yes, that was my point. Setting size to 0x7fffULL
affects several places. Another potential location (based only
on the function header comment, not from reading the code) is
iommu_dma_init_domain(). The header comment says:
* @base and @size should be exact multiples of IOMMU page granularity to
* avoid rounding surprises.
ok, this is the same problem that should get solved when arch_setup_dma_ops
is prepared to take mask instead of size. It would still work as said above
with a smaller mask than specified.
I have not read enough context to really understand of_dma_configure(), but
it seems there is yet another issue in how the error return case from
of_dma_get_range() is handled (with the existing code, as well as if
my patch gets accepted). An error return value can mean _either_
there is no dma-ranges property _or_ "an other problem occurred". Should
the "an other problem occurred" case be handled by defaulting size to
a value based on dev->coherent_dma_mask (the current case) or should the
attempt to set up the DMA configuration just fail?
The handling of return error value looks like separate item, but looks
like its correct with what is there now. (ie) when of_dma_get_range fails
either because 'dma-ranges property' populated in DT (or) because of some
erroneous DT setting, better to set the mask which the driver has specified
and ignore DT. So the above patch just corrects a mistake in that path.
Regards,
Sricharan
--
"QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of
Code Aurora Forum, hosted by The Linux Foundation
Re: [PATCH V10 06/12] of: device: Fix overflow of coherent_dma_mask
On 04/06/17 04:01, Sricharan R wrote:
> Hi Frank,
>
> On 4/6/2017 12:31 PM, Frank Rowand wrote:
>> On 04/04/17 03:18, Sricharan R wrote:
>>> Size of the dma-range is calculated as coherent_dma_mask + 1
>>> and passed to arch_setup_dma_ops further. It overflows when
>>> the coherent_dma_mask is set for full 64 bits 0x,
>>> resulting in size getting passed as 0 wrongly. Fix this by
>>> passsing in max(mask, mask + 1). Note that in this case
>>> when the mask is set to full 64bits, we will be passing the mask
>>> itself to arch_setup_dma_ops instead of the size. The real fix
>>> for this should be to make arch_setup_dma_ops receive the
>>> mask and handle it, to be done in the future.
>>>
>>> Signed-off-by: Sricharan R
>>> ---
>>> drivers/of/device.c | 2 +-
>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/of/device.c b/drivers/of/device.c
>>> index c17c19d..c2ae6bb 100644
>>> --- a/drivers/of/device.c
>>> +++ b/drivers/of/device.c
>>> @@ -107,7 +107,7 @@ void of_dma_configure(struct device *dev, struct
>>> device_node *np)
>>> ret = of_dma_get_range(np, &dma_addr, &paddr, &size);
>>> if (ret < 0) {
>>> dma_addr = offset = 0;
>>> -size = dev->coherent_dma_mask + 1;
>>> +size = max(dev->coherent_dma_mask, dev->coherent_dma_mask + 1);
>>> } else {
>>> offset = PFN_DOWN(paddr - dma_addr);
>>> dev_dbg(dev, "dma_pfn_offset(%#08lx)\n", offset);
>>>
>>
>> NACK.
>>
>> Passing an invalid size to arch_setup_dma_ops() is only part of the problem.
>> size is also used in of_dma_configure() before calling arch_setup_dma_ops():
>>
>> dev->coherent_dma_mask = min(dev->coherent_dma_mask,
>> DMA_BIT_MASK(ilog2(dma_addr + size)));
>> *dev->dma_mask = min((*dev->dma_mask),
>> DMA_BIT_MASK(ilog2(dma_addr + size)));
>>
>> which would be incorrect for size == 0xULL when
>> dma_addr != 0. So the proposed fix really is not papering over
>> the base problem very well.
>>
>
> Ok, but with your fix for of_dma_get_range and the above fix,
> dma_addr will be '0' when size = 0xULL,
> but DMA_BIT_MASK(ilog2(dma_addr + size)) would be wrong though,
> making coherent_dma_mask to be smaller 0x7fffULL.
Yes, that was my point. Setting size to 0x7fffULL
affects several places. Another potential location (based only
on the function header comment, not from reading the code) is
iommu_dma_init_domain(). The header comment says:
* @base and @size should be exact multiples of IOMMU page granularity to
* avoid rounding surprises.
I have not read enough context to really understand of_dma_configure(), but
it seems there is yet another issue in how the error return case from
of_dma_get_range() is handled (with the existing code, as well as if
my patch gets accepted). An error return value can mean _either_
there is no dma-ranges property _or_ "an other problem occurred". Should
the "an other problem occurred" case be handled by defaulting size to
a value based on dev->coherent_dma_mask (the current case) or should the
attempt to set up the DMA configuration just fail?
>
> Regards,
> Sricharan
>
>> I agree that the proper solution involves passing a mask instead
>> of a size to arch_setup_dma_ops().
>>
>
Re: [PATCH V10 06/12] of: device: Fix overflow of coherent_dma_mask
On 04/06/17 03:24, Robin Murphy wrote:
> On 06/04/17 08:01, Frank Rowand wrote:
>> On 04/04/17 03:18, Sricharan R wrote:
>>> Size of the dma-range is calculated as coherent_dma_mask + 1
>>> and passed to arch_setup_dma_ops further. It overflows when
>>> the coherent_dma_mask is set for full 64 bits 0x,
>>> resulting in size getting passed as 0 wrongly. Fix this by
>>> passsing in max(mask, mask + 1). Note that in this case
>>> when the mask is set to full 64bits, we will be passing the mask
>>> itself to arch_setup_dma_ops instead of the size. The real fix
>>> for this should be to make arch_setup_dma_ops receive the
>>> mask and handle it, to be done in the future.
>>>
>>> Signed-off-by: Sricharan R
>>> ---
>>> drivers/of/device.c | 2 +-
>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/of/device.c b/drivers/of/device.c
>>> index c17c19d..c2ae6bb 100644
>>> --- a/drivers/of/device.c
>>> +++ b/drivers/of/device.c
>>> @@ -107,7 +107,7 @@ void of_dma_configure(struct device *dev, struct
>>> device_node *np)
>>> ret = of_dma_get_range(np, &dma_addr, &paddr, &size);
>>> if (ret < 0) {
>>> dma_addr = offset = 0;
>>> - size = dev->coherent_dma_mask + 1;
>>> + size = max(dev->coherent_dma_mask, dev->coherent_dma_mask + 1);
>>> } else {
>>> offset = PFN_DOWN(paddr - dma_addr);
>>> dev_dbg(dev, "dma_pfn_offset(%#08lx)\n", offset);
>>>
>>
>> NACK.
>>
>> Passing an invalid size to arch_setup_dma_ops() is only part of the problem.
>> size is also used in of_dma_configure() before calling arch_setup_dma_ops():
>>
>> dev->coherent_dma_mask = min(dev->coherent_dma_mask,
>> DMA_BIT_MASK(ilog2(dma_addr + size)));
>> *dev->dma_mask = min((*dev->dma_mask),
>> DMA_BIT_MASK(ilog2(dma_addr + size)));
>>
>> which would be incorrect for size == 0xULL when
>> dma_addr != 0. So the proposed fix really is not papering over
>> the base problem very well.
>
> I'm not sure I agree there. Granted, there exist many more problematic
> aspects than are dealt with here (I've got more patches cooking to sort
> out some of the other issues we have with dma-ranges), but considering
> size specifically:
>
> - It is not possible to explicitly specify a range with a size of 2^64
> in DT. If someone does specify a size of 0, they've done a silly thing
> and should not be surprised that it ends badly.
>
> - It *is* perfectly legitimate for bus code (or a previous device
> driver, once we start coming here at probe time) to have set a device's
> DMA mask to 0xULL. If this code then blindly overflows
> and infers an invalid size of 0 from that, breaking things in the
> process, that is this code's fault alone. It just so happens that
> nothing managed to trigger the latent problem until patch #7 here shakes
> up the callsites.
The existing code that uses size does not appear capable of dealing with
the case of DMA mask of 0xULL since 2^64 does not fit
into size.
The code affected by the DMA mask is not within my area of knowledge, so
take the following with a grain of salt. If a DMA mask of
0xULL is provided, would the code still work without error
(though with reduced capability) if the mask was changed to
0xefffULL? I would guess that the location to do so would
be where dev->coherent_dma_mask is set, or some other location that
is not of_dma_configure(). This would just be a temporary workaround.
> Yes, wacky impossible base + size combinations in DT were a theoretical
> problem before, and remain a theoretical problem, but also fall into the
> "how did you ever expect this to work?" category. There's certainly
> plenty more we can do to improve the DT parsing/validation, but that
> still doesn't apply to this path where the information is *not* coming
> from the DT at all.
>
>> I agree that the proper solution involves passing a mask instead
>> of a size to arch_setup_dma_ops().
>
> Having started writing that patch too, I can tell you it's a big bugger
> touching multiple architectures and fixing up various drivers doing
> stupid things, hence why I'm happy with this point fix being the lesser
> of two evils in terms of not holding up this mostly-orthogonal series.
>
> Robin.
>
>>
>> -Frank
>>
>
>
Re: [PATCH V10 06/12] of: device: Fix overflow of coherent_dma_mask
On 06/04/17 14:56, Rob Herring wrote:
> On Thu, Apr 6, 2017 at 5:24 AM, Robin Murphy wrote:
>> On 06/04/17 08:01, Frank Rowand wrote:
>>> On 04/04/17 03:18, Sricharan R wrote:
Size of the dma-range is calculated as coherent_dma_mask + 1
and passed to arch_setup_dma_ops further. It overflows when
the coherent_dma_mask is set for full 64 bits 0x,
resulting in size getting passed as 0 wrongly. Fix this by
passsing in max(mask, mask + 1). Note that in this case
when the mask is set to full 64bits, we will be passing the mask
itself to arch_setup_dma_ops instead of the size. The real fix
for this should be to make arch_setup_dma_ops receive the
mask and handle it, to be done in the future.
Signed-off-by: Sricharan R
---
drivers/of/device.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/of/device.c b/drivers/of/device.c
index c17c19d..c2ae6bb 100644
--- a/drivers/of/device.c
+++ b/drivers/of/device.c
@@ -107,7 +107,7 @@ void of_dma_configure(struct device *dev, struct
device_node *np)
ret = of_dma_get_range(np, &dma_addr, &paddr, &size);
if (ret < 0) {
dma_addr = offset = 0;
-size = dev->coherent_dma_mask + 1;
+size = max(dev->coherent_dma_mask, dev->coherent_dma_mask +
1);
} else {
offset = PFN_DOWN(paddr - dma_addr);
dev_dbg(dev, "dma_pfn_offset(%#08lx)\n", offset);
>>>
>>> NACK.
>>>
>>> Passing an invalid size to arch_setup_dma_ops() is only part of the problem.
>>> size is also used in of_dma_configure() before calling arch_setup_dma_ops():
>>>
>>> dev->coherent_dma_mask = min(dev->coherent_dma_mask,
>>> DMA_BIT_MASK(ilog2(dma_addr + size)));
>>> *dev->dma_mask = min((*dev->dma_mask),
>>> DMA_BIT_MASK(ilog2(dma_addr + size)));
>>>
>>> which would be incorrect for size == 0xULL when
>>> dma_addr != 0. So the proposed fix really is not papering over
>>> the base problem very well.
>>
>> I'm not sure I agree there. Granted, there exist many more problematic
>> aspects than are dealt with here (I've got more patches cooking to sort
>> out some of the other issues we have with dma-ranges), but considering
>> size specifically:
>>
>> - It is not possible to explicitly specify a range with a size of 2^64
>> in DT. If someone does specify a size of 0, they've done a silly thing
>> and should not be surprised that it ends badly.
>
> And because of this, we allow ~0 (both 32 and 64 bit) in DT dma-ranges
> and fix these up as 2^32 and 2^64 sizes.
...which is what Frank's patch gets rid of.
Robin.
>
> Rob
>
Re: [PATCH V10 06/12] of: device: Fix overflow of coherent_dma_mask
On Thu, Apr 6, 2017 at 5:24 AM, Robin Murphy wrote:
> On 06/04/17 08:01, Frank Rowand wrote:
>> On 04/04/17 03:18, Sricharan R wrote:
>>> Size of the dma-range is calculated as coherent_dma_mask + 1
>>> and passed to arch_setup_dma_ops further. It overflows when
>>> the coherent_dma_mask is set for full 64 bits 0x,
>>> resulting in size getting passed as 0 wrongly. Fix this by
>>> passsing in max(mask, mask + 1). Note that in this case
>>> when the mask is set to full 64bits, we will be passing the mask
>>> itself to arch_setup_dma_ops instead of the size. The real fix
>>> for this should be to make arch_setup_dma_ops receive the
>>> mask and handle it, to be done in the future.
>>>
>>> Signed-off-by: Sricharan R
>>> ---
>>> drivers/of/device.c | 2 +-
>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/of/device.c b/drivers/of/device.c
>>> index c17c19d..c2ae6bb 100644
>>> --- a/drivers/of/device.c
>>> +++ b/drivers/of/device.c
>>> @@ -107,7 +107,7 @@ void of_dma_configure(struct device *dev, struct
>>> device_node *np)
>>> ret = of_dma_get_range(np, &dma_addr, &paddr, &size);
>>> if (ret < 0) {
>>> dma_addr = offset = 0;
>>> -size = dev->coherent_dma_mask + 1;
>>> +size = max(dev->coherent_dma_mask, dev->coherent_dma_mask + 1);
>>> } else {
>>> offset = PFN_DOWN(paddr - dma_addr);
>>> dev_dbg(dev, "dma_pfn_offset(%#08lx)\n", offset);
>>>
>>
>> NACK.
>>
>> Passing an invalid size to arch_setup_dma_ops() is only part of the problem.
>> size is also used in of_dma_configure() before calling arch_setup_dma_ops():
>>
>> dev->coherent_dma_mask = min(dev->coherent_dma_mask,
>> DMA_BIT_MASK(ilog2(dma_addr + size)));
>> *dev->dma_mask = min((*dev->dma_mask),
>> DMA_BIT_MASK(ilog2(dma_addr + size)));
>>
>> which would be incorrect for size == 0xULL when
>> dma_addr != 0. So the proposed fix really is not papering over
>> the base problem very well.
>
> I'm not sure I agree there. Granted, there exist many more problematic
> aspects than are dealt with here (I've got more patches cooking to sort
> out some of the other issues we have with dma-ranges), but considering
> size specifically:
>
> - It is not possible to explicitly specify a range with a size of 2^64
> in DT. If someone does specify a size of 0, they've done a silly thing
> and should not be surprised that it ends badly.
And because of this, we allow ~0 (both 32 and 64 bit) in DT dma-ranges
and fix these up as 2^32 and 2^64 sizes.
Rob
Re: [PATCH V10 06/12] of: device: Fix overflow of coherent_dma_mask
Hi Frank,
On 4/6/2017 12:31 PM, Frank Rowand wrote:
On 04/04/17 03:18, Sricharan R wrote:
Size of the dma-range is calculated as coherent_dma_mask + 1
and passed to arch_setup_dma_ops further. It overflows when
the coherent_dma_mask is set for full 64 bits 0x,
resulting in size getting passed as 0 wrongly. Fix this by
passsing in max(mask, mask + 1). Note that in this case
when the mask is set to full 64bits, we will be passing the mask
itself to arch_setup_dma_ops instead of the size. The real fix
for this should be to make arch_setup_dma_ops receive the
mask and handle it, to be done in the future.
Signed-off-by: Sricharan R
---
drivers/of/device.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/of/device.c b/drivers/of/device.c
index c17c19d..c2ae6bb 100644
--- a/drivers/of/device.c
+++ b/drivers/of/device.c
@@ -107,7 +107,7 @@ void of_dma_configure(struct device *dev, struct
device_node *np)
ret = of_dma_get_range(np, &dma_addr, &paddr, &size);
if (ret < 0) {
dma_addr = offset = 0;
- size = dev->coherent_dma_mask + 1;
+ size = max(dev->coherent_dma_mask, dev->coherent_dma_mask + 1);
} else {
offset = PFN_DOWN(paddr - dma_addr);
dev_dbg(dev, "dma_pfn_offset(%#08lx)\n", offset);
NACK.
Passing an invalid size to arch_setup_dma_ops() is only part of the problem.
size is also used in of_dma_configure() before calling arch_setup_dma_ops():
dev->coherent_dma_mask = min(dev->coherent_dma_mask,
DMA_BIT_MASK(ilog2(dma_addr + size)));
*dev->dma_mask = min((*dev->dma_mask),
DMA_BIT_MASK(ilog2(dma_addr + size)));
which would be incorrect for size == 0xULL when
dma_addr != 0. So the proposed fix really is not papering over
the base problem very well.
Ok, but with your fix for of_dma_get_range and the above fix,
dma_addr will be '0' when size = 0xULL,
but DMA_BIT_MASK(ilog2(dma_addr + size)) would be wrong though,
making coherent_dma_mask to be smaller 0x7fffULL.
Regards,
Sricharan
I agree that the proper solution involves passing a mask instead
of a size to arch_setup_dma_ops().
--
"QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of
Code Aurora Forum, hosted by The Linux Foundation
Re: [PATCH V10 06/12] of: device: Fix overflow of coherent_dma_mask
On 06/04/17 08:01, Frank Rowand wrote:
> On 04/04/17 03:18, Sricharan R wrote:
>> Size of the dma-range is calculated as coherent_dma_mask + 1
>> and passed to arch_setup_dma_ops further. It overflows when
>> the coherent_dma_mask is set for full 64 bits 0x,
>> resulting in size getting passed as 0 wrongly. Fix this by
>> passsing in max(mask, mask + 1). Note that in this case
>> when the mask is set to full 64bits, we will be passing the mask
>> itself to arch_setup_dma_ops instead of the size. The real fix
>> for this should be to make arch_setup_dma_ops receive the
>> mask and handle it, to be done in the future.
>>
>> Signed-off-by: Sricharan R
>> ---
>> drivers/of/device.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/of/device.c b/drivers/of/device.c
>> index c17c19d..c2ae6bb 100644
>> --- a/drivers/of/device.c
>> +++ b/drivers/of/device.c
>> @@ -107,7 +107,7 @@ void of_dma_configure(struct device *dev, struct
>> device_node *np)
>> ret = of_dma_get_range(np, &dma_addr, &paddr, &size);
>> if (ret < 0) {
>> dma_addr = offset = 0;
>> -size = dev->coherent_dma_mask + 1;
>> +size = max(dev->coherent_dma_mask, dev->coherent_dma_mask + 1);
>> } else {
>> offset = PFN_DOWN(paddr - dma_addr);
>> dev_dbg(dev, "dma_pfn_offset(%#08lx)\n", offset);
>>
>
> NACK.
>
> Passing an invalid size to arch_setup_dma_ops() is only part of the problem.
> size is also used in of_dma_configure() before calling arch_setup_dma_ops():
>
> dev->coherent_dma_mask = min(dev->coherent_dma_mask,
> DMA_BIT_MASK(ilog2(dma_addr + size)));
> *dev->dma_mask = min((*dev->dma_mask),
> DMA_BIT_MASK(ilog2(dma_addr + size)));
>
> which would be incorrect for size == 0xULL when
> dma_addr != 0. So the proposed fix really is not papering over
> the base problem very well.
I'm not sure I agree there. Granted, there exist many more problematic
aspects than are dealt with here (I've got more patches cooking to sort
out some of the other issues we have with dma-ranges), but considering
size specifically:
- It is not possible to explicitly specify a range with a size of 2^64
in DT. If someone does specify a size of 0, they've done a silly thing
and should not be surprised that it ends badly.
- It *is* perfectly legitimate for bus code (or a previous device
driver, once we start coming here at probe time) to have set a device's
DMA mask to 0xULL. If this code then blindly overflows
and infers an invalid size of 0 from that, breaking things in the
process, that is this code's fault alone. It just so happens that
nothing managed to trigger the latent problem until patch #7 here shakes
up the callsites.
Yes, wacky impossible base + size combinations in DT were a theoretical
problem before, and remain a theoretical problem, but also fall into the
"how did you ever expect this to work?" category. There's certainly
plenty more we can do to improve the DT parsing/validation, but that
still doesn't apply to this path where the information is *not* coming
from the DT at all.
> I agree that the proper solution involves passing a mask instead
> of a size to arch_setup_dma_ops().
Having started writing that patch too, I can tell you it's a big bugger
touching multiple architectures and fixing up various drivers doing
stupid things, hence why I'm happy with this point fix being the lesser
of two evils in terms of not holding up this mostly-orthogonal series.
Robin.
>
> -Frank
>
Re: [PATCH V10 06/12] of: device: Fix overflow of coherent_dma_mask
On 04/04/17 03:18, Sricharan R wrote:
> Size of the dma-range is calculated as coherent_dma_mask + 1
> and passed to arch_setup_dma_ops further. It overflows when
> the coherent_dma_mask is set for full 64 bits 0x,
> resulting in size getting passed as 0 wrongly. Fix this by
> passsing in max(mask, mask + 1). Note that in this case
> when the mask is set to full 64bits, we will be passing the mask
> itself to arch_setup_dma_ops instead of the size. The real fix
> for this should be to make arch_setup_dma_ops receive the
> mask and handle it, to be done in the future.
>
> Signed-off-by: Sricharan R
> ---
> drivers/of/device.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/of/device.c b/drivers/of/device.c
> index c17c19d..c2ae6bb 100644
> --- a/drivers/of/device.c
> +++ b/drivers/of/device.c
> @@ -107,7 +107,7 @@ void of_dma_configure(struct device *dev, struct
> device_node *np)
> ret = of_dma_get_range(np, &dma_addr, &paddr, &size);
> if (ret < 0) {
> dma_addr = offset = 0;
> - size = dev->coherent_dma_mask + 1;
> + size = max(dev->coherent_dma_mask, dev->coherent_dma_mask + 1);
> } else {
> offset = PFN_DOWN(paddr - dma_addr);
> dev_dbg(dev, "dma_pfn_offset(%#08lx)\n", offset);
>
NACK.
Passing an invalid size to arch_setup_dma_ops() is only part of the problem.
size is also used in of_dma_configure() before calling arch_setup_dma_ops():
dev->coherent_dma_mask = min(dev->coherent_dma_mask,
DMA_BIT_MASK(ilog2(dma_addr + size)));
*dev->dma_mask = min((*dev->dma_mask),
DMA_BIT_MASK(ilog2(dma_addr + size)));
which would be incorrect for size == 0xULL when
dma_addr != 0. So the proposed fix really is not papering over
the base problem very well.
I agree that the proper solution involves passing a mask instead
of a size to arch_setup_dma_ops().
-Frank
Re: [PATCH V10 06/12] of: device: Fix overflow of coherent_dma_mask
On 04/04/17 11:18, Sricharan R wrote:
> Size of the dma-range is calculated as coherent_dma_mask + 1
> and passed to arch_setup_dma_ops further. It overflows when
> the coherent_dma_mask is set for full 64 bits 0x,
> resulting in size getting passed as 0 wrongly. Fix this by
> passsing in max(mask, mask + 1). Note that in this case
> when the mask is set to full 64bits, we will be passing the mask
> itself to arch_setup_dma_ops instead of the size. The real fix
> for this should be to make arch_setup_dma_ops receive the
> mask and handle it, to be done in the future.
Agreed; in the meantime, this fix is neat and robust enough.
Reviewed-by: Robin Murphy
> Signed-off-by: Sricharan R
> ---
> drivers/of/device.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/of/device.c b/drivers/of/device.c
> index c17c19d..c2ae6bb 100644
> --- a/drivers/of/device.c
> +++ b/drivers/of/device.c
> @@ -107,7 +107,7 @@ void of_dma_configure(struct device *dev, struct
> device_node *np)
> ret = of_dma_get_range(np, &dma_addr, &paddr, &size);
> if (ret < 0) {
> dma_addr = offset = 0;
> - size = dev->coherent_dma_mask + 1;
> + size = max(dev->coherent_dma_mask, dev->coherent_dma_mask + 1);
> } else {
> offset = PFN_DOWN(paddr - dma_addr);
> dev_dbg(dev, "dma_pfn_offset(%#08lx)\n", offset);
>
[PATCH V10 06/12] of: device: Fix overflow of coherent_dma_mask
Size of the dma-range is calculated as coherent_dma_mask + 1
and passed to arch_setup_dma_ops further. It overflows when
the coherent_dma_mask is set for full 64 bits 0x,
resulting in size getting passed as 0 wrongly. Fix this by
passsing in max(mask, mask + 1). Note that in this case
when the mask is set to full 64bits, we will be passing the mask
itself to arch_setup_dma_ops instead of the size. The real fix
for this should be to make arch_setup_dma_ops receive the
mask and handle it, to be done in the future.
Signed-off-by: Sricharan R
---
drivers/of/device.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/of/device.c b/drivers/of/device.c
index c17c19d..c2ae6bb 100644
--- a/drivers/of/device.c
+++ b/drivers/of/device.c
@@ -107,7 +107,7 @@ void of_dma_configure(struct device *dev, struct
device_node *np)
ret = of_dma_get_range(np, &dma_addr, &paddr, &size);
if (ret < 0) {
dma_addr = offset = 0;
- size = dev->coherent_dma_mask + 1;
+ size = max(dev->coherent_dma_mask, dev->coherent_dma_mask + 1);
} else {
offset = PFN_DOWN(paddr - dma_addr);
dev_dbg(dev, "dma_pfn_offset(%#08lx)\n", offset);
--
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of
Code Aurora Forum, hosted by The Linux Foundation
