Message-ID: <b38312b0-268b-ed86-a5b3-886f86ea13f5@arm.com>
Date:   Fri, 7 Apr 2017 15:46:27 +0100
From:   Robin Murphy <robin.murphy@....com>
To:     Frank Rowand <frowand.list@...il.com>,
        Sricharan R <sricharan@...eaurora.org>, will.deacon@....com,
        joro@...tes.org, lorenzo.pieralisi@....com,
        iommu@...ts.linux-foundation.org,
        linux-arm-kernel@...ts.infradead.org,
        linux-arm-msm@...r.kernel.org, m.szyprowski@...sung.com,
        bhelgaas@...gle.com, linux-pci@...r.kernel.org,
        linux-acpi@...r.kernel.org, tn@...ihalf.com, hanjun.guo@...aro.org,
        okaya@...eaurora.org, robh+dt@...nel.org,
        devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
        sudeep.holla@....com, rjw@...ysocki.net, lenb@...nel.org,
        catalin.marinas@....com, arnd@...db.de, linux-arch@...r.kernel.org,
        gregkh@...uxfoundation.org
Subject: 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 0xFFFFFFFFFFFFFFFF,
>>>> 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 <sricharan@...eaurora.org>
>>>> ---
>>>>  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 == 0xffffffffffffffffULL 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 = 0xffffffffffffffffULL,
>> but DMA_BIT_MASK(ilog2(dma_addr + size)) would be wrong though,
>> making coherent_dma_mask to be smaller 0x7fffffffffffffffULL.
> 
> Yes, that was my point.  Setting size to 0x7fffffffffffffffULL
> 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 suddenly block this
almost-entirely-orthogonal series that various other work has been
waiting on for some time now. The WIP patch I have for
arch_setup_dma_ops() already touches 3 architectures and 4 other
subsystems...

Robin.

> 
>>
>> Regards,
>>  Sricharan
>>
>>> I agree that the proper solution involves passing a mask instead
>>> of a size to arch_setup_dma_ops().
>>>
>>
> 

Powered by blists - more mailing lists