linux-kernel - Re: [PATCH v4 15/24] iommu/io-pgtable: Remove non-strict quirk

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] [day] [month] [year] [list]

Message-ID: <1f7de684-c646-cda9-b246-3712f046f2b0@arm.com>
Date:   Tue, 24 Aug 2021 14:46:02 +0100
From:   Robin Murphy <robin.murphy@....com>
To:     Geert Uytterhoeven <geert@...ux-m68k.org>
Cc:     Joerg Roedel <joro@...tes.org>, Will Deacon <will@...nel.org>,
        Linux IOMMU <iommu@...ts.linux-foundation.org>,
        Linux ARM <linux-arm-kernel@...ts.infradead.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        suravee.suthikulpanit@....com, baolu.lu@...ux.intel.com,
        John Garry <john.garry@...wei.com>,
        Doug Anderson <dianders@...omium.org>, rajatja@...gle.com,
        chenxiang <chenxiang66@...ilicon.com>,
        Linux-Renesas <linux-renesas-soc@...r.kernel.org>
Subject: Re: [PATCH v4 15/24] iommu/io-pgtable: Remove non-strict quirk

Hi Geert,

On 2021-08-24 14:25, Geert Uytterhoeven wrote:
> Hi Robin,
> 
> On Wed, Aug 11, 2021 at 2:24 PM Robin Murphy <robin.murphy@....com> wrote:
>> IO_PGTABLE_QUIRK_NON_STRICT was never a very comfortable fit, since it's
>> not a quirk of the pagetable format itself. Now that we have a more
>> appropriate way to convey non-strict unmaps, though, this last of the
>> non-quirk quirks can also go, and with the flush queue code also now
>> enforcing its own ordering we can have a lovely cleanup all round.
>>
>> Signed-off-by: Robin Murphy <robin.murphy@....com>
> 
> Thanks for your patch, which is now commit a8e5f04458c4e496
> ("iommu/io-pgtable: Remove non-strict quirk") in iommu/next.
> 
>> --- a/drivers/iommu/io-pgtable-arm-v7s.c
>> +++ b/drivers/iommu/io-pgtable-arm-v7s.c
>> @@ -700,14 +700,7 @@ static size_t __arm_v7s_unmap(struct arm_v7s_io_pgtable *data,
>>                                                  ARM_V7S_BLOCK_SIZE(lvl + 1));
>>                                  ptep = iopte_deref(pte[i], lvl, data);
>>                                  __arm_v7s_free_table(ptep, lvl + 1, data);
>> -                       } else if (iop->cfg.quirks & IO_PGTABLE_QUIRK_NON_STRICT) {
>> -                               /*
>> -                                * Order the PTE update against queueing the IOVA, to
>> -                                * guarantee that a flush callback from a different CPU
>> -                                * has observed it before the TLBIALL can be issued.
>> -                                */
>> -                               smp_wmb();
>> -                       } else {
>> +                       } else if (!gather->queued) {
> 
> If CONFIG_IOMMU_API=n:
> 
> error: ‘struct iommu_iotlb_gather’ has no member named ‘queued’
> 
> This can be reproduced using e.g. shmobile_defconfig with
>      CONFIG_IOMMU_SUPPORT=y
>      CONFIG_IOMMU_IO_PGTABLE_ARMV7S=y
> 
> 
>>                                  io_pgtable_tlb_add_page(iop, gather, iova, blk_size);
>>                          }
>>                          iova += blk_size;
> 
>> --- a/drivers/iommu/io-pgtable-arm.c
>> +++ b/drivers/iommu/io-pgtable-arm.c
>> @@ -638,14 +638,7 @@ static size_t __arm_lpae_unmap(struct arm_lpae_io_pgtable *data,
>>                                  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 (iop->cfg.quirks & IO_PGTABLE_QUIRK_NON_STRICT) {
>> -                               /*
>> -                                * Order the PTE update against queueing the IOVA, to
>> -                                * guarantee that a flush callback from a different CPU
>> -                                * has observed it before the TLBIALL can be issued.
>> -                                */
>> -                               smp_wmb();
>> -                       } else {
>> +                       } else if (!gather->queued) {
> 
> If CONFIG_IOMMU_API=n:
> 
> error: ‘struct iommu_iotlb_gather’ has no member named ‘queued’
> 
> This can be reproduced using e.g. shmobile_defconfig with
>      CONFIG_IOMMU_SUPPORT=y
>      CONFIG_IOMMU_IO_PGTABLE_LPAE=y
> 
>>                                  io_pgtable_tlb_add_page(iop, gather, iova + i * size, size);
>>                          }
>>
> 
> Perhaps "select IOMMU_API" should be added (moved from individual
> drivers) to both IOMMU_IO_PGTABLE_ARMV7S and IOMMU_IO_PGTABLE_LPAE?
> Or iommu_iotlb_gather.queued should not be accessed here, or the
> access wrapped into a static inline helper function with a dummy for
> the CONFIG_IOMMU_API=n case?

Those (and worse) should be fixed by this patch:

https://lore.kernel.org/linux-iommu/83672ee76f6405c82845a55c148fa836f56fbbc1.1629465282.git.robin.murphy@arm.com/

which apparently hasn't made it to -next yet.

Thanks,
Robin.

> 
> Gr{oetje,eeting}s,
> 
>                          Geert
>