linux-kernel - Re: [PATCH] Revert "iommu/io-pgtable-arm: Optimise non-coherent unmap"

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <4dbdd1ee-fedd-4b97-86ba-ffe6af8d0473@arm.com>
Date: Thu, 5 Sep 2024 14:32:46 +0100
From: Robin Murphy <robin.murphy@....com>
To: Rob Clark <robdclark@...il.com>, iommu@...ts.linux.dev
Cc: linux-arm-msm@...r.kernel.org, freedreno@...ts.freedesktop.org,
 Ashish Mhetre <amhetre@...dia.com>, Rob Clark <robdclark@...omium.org>,
 Will Deacon <will@...nel.org>, Joerg Roedel <joro@...tes.org>,
 "moderated list:ARM SMMU DRIVERS" <linux-arm-kernel@...ts.infradead.org>,
 open list <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] Revert "iommu/io-pgtable-arm: Optimise non-coherent
 unmap"

On 05/09/2024 2:24 pm, Robin Murphy wrote:
> On 05/09/2024 1:49 pm, Rob Clark wrote:
>> From: Rob Clark <robdclark@...omium.org>
>>
>> This reverts commit 85b715a334583488ad7fbd3001fe6fd617b7d4c0.
>>
>> It was causing gpu smmu faults on x1e80100.
>>
>> I _think_ what is causing this is the change in ordering of
>> __arm_lpae_clear_pte() (dma_sync_single_for_device() on the pgtable
>> memory) and io_pgtable_tlb_flush_walk().
> 
> As I just commented, how do you believe the order of operations between:

...or at least I *thought* I'd just commented, but Thunderbird is now 
denying any knowledge of the reply to your other mail I swore I sent a 
couple of hours ago :/

Apologies for any confusion,
Robin.

> 
>      __arm_lpae_clear_pte();
>       if (!iopte_leaf()) {
>           io_pgtable_tlb_flush_walk();
> 
> and:
> 
>       if (!iopte_leaf()) {
>          __arm_lpae_clear_pte();
>           io_pgtable_tlb_flush_walk();
> 
> fundamentally differs?
> 
> I'm not saying there couldn't be some subtle bug in the implementation 
> which we've all missed, but I still can't see an issue with the intended 
> logic.
> 
>>  I'm not entirely sure how
>> this patch is supposed to work correctly in the face of other
>> concurrent translations (to buffers unrelated to the one being
>> unmapped(), because after the io_pgtable_tlb_flush_walk() we can have
>> stale data read back into the tlb.
> 
> Read back from where? The ex-table PTE which was already set to zero 
> before tlb_flush_walk was called?
> 
> And isn't the hilariously overcomplicated TBU driver supposed to be 
> telling you exactly what happened here? Otherwise I'm going to continue 
> to seriously question the purpose of shoehorning that upstream at all...
> 
> Thanks,
> Robin.
> 
>> Signed-off-by: Rob Clark <robdclark@...omium.org>
>> ---
>>   drivers/iommu/io-pgtable-arm.c | 31 ++++++++++++++-----------------
>>   1 file changed, 14 insertions(+), 17 deletions(-)
>>
>> diff --git a/drivers/iommu/io-pgtable-arm.c 
>> b/drivers/iommu/io-pgtable-arm.c
>> index 16e51528772d..85261baa3a04 100644
>> --- a/drivers/iommu/io-pgtable-arm.c
>> +++ b/drivers/iommu/io-pgtable-arm.c
>> @@ -274,13 +274,13 @@ static void __arm_lpae_sync_pte(arm_lpae_iopte 
>> *ptep, int num_entries,
>>                      sizeof(*ptep) * num_entries, DMA_TO_DEVICE);
>>   }
>> -static void __arm_lpae_clear_pte(arm_lpae_iopte *ptep, struct 
>> io_pgtable_cfg *cfg, int num_entries)
>> +static void __arm_lpae_clear_pte(arm_lpae_iopte *ptep, struct 
>> io_pgtable_cfg *cfg)
>>   {
>> -    for (int i = 0; i < num_entries; i++)
>> -        ptep[i] = 0;
>> -    if (!cfg->coherent_walk && num_entries)
>> -        __arm_lpae_sync_pte(ptep, num_entries, cfg);
>> +    *ptep = 0;
>> +
>> +    if (!cfg->coherent_walk)
>> +        __arm_lpae_sync_pte(ptep, 1, cfg);
>>   }
>>   static size_t __arm_lpae_unmap(struct arm_lpae_io_pgtable *data,
>> @@ -653,28 +653,25 @@ static size_t __arm_lpae_unmap(struct 
>> arm_lpae_io_pgtable *data,
>>           max_entries = ARM_LPAE_PTES_PER_TABLE(data) - unmap_idx_start;
>>           num_entries = min_t(int, pgcount, max_entries);
>> -        /* Find and handle non-leaf entries */
>> -        for (i = 0; i < num_entries; i++) {
>> -            pte = READ_ONCE(ptep[i]);
>> +        while (i < num_entries) {
>> +            pte = READ_ONCE(*ptep);
>>               if (WARN_ON(!pte))
>>                   break;
>> -            if (!iopte_leaf(pte, lvl, iop->fmt)) {
>> -                __arm_lpae_clear_pte(&ptep[i], &iop->cfg, 1);
>> +            __arm_lpae_clear_pte(ptep, &iop->cfg);
>> +            if (!iopte_leaf(pte, lvl, iop->fmt)) {
>>                   /* Also flush any partial walks */
>>                   io_pgtable_tlb_flush_walk(iop, iova + i * size, size,
>>                                 ARM_LPAE_GRANULE(data));
>>                   __arm_lpae_free_pgtable(data, lvl + 1, 
>> iopte_deref(pte, data));
>> +            } else if (!iommu_iotlb_gather_queued(gather)) {
>> +                io_pgtable_tlb_add_page(iop, gather, iova + i * size, 
>> size);
>>               }
>> -        }
>> -        /* Clear the remaining entries */
>> -        __arm_lpae_clear_pte(ptep, &iop->cfg, i);
>> -
>> -        if (gather && !iommu_iotlb_gather_queued(gather))
>> -            for (int j = 0; j < i; j++)
>> -                io_pgtable_tlb_add_page(iop, gather, iova + j * size, 
>> size);
>> +            ptep++;
>> +            i++;
>> +        }
>>           return i * size;
>>       } else if (iopte_leaf(pte, lvl, iop->fmt)) {
>