linux-kernel - Re: [PATCH 2/2] arm64: Notify on pte permission upgrades

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: <87ilc9qkuc.fsf@nvidia.com>
Date:   Wed, 31 May 2023 12:46:06 +1000
From:   Alistair Popple <apopple@...dia.com>
To:     Jason Gunthorpe <jgg@...dia.com>
Cc:     Sean Christopherson <seanjc@...gle.com>,
        Robin Murphy <robin.murphy@....com>,
        Andrew Morton <akpm@...ux-foundation.org>, will@...nel.org,
        catalin.marinas@....com, linux-mm@...ck.org,
        linux-kernel@...r.kernel.org, nicolinc@...dia.com,
        linux-arm-kernel@...ts.infradead.org, kvm@...r.kernel.org,
        John Hubbard <jhubbard@...dia.com>, zhi.wang.linux@...il.com
Subject: Re: [PATCH 2/2] arm64: Notify on pte permission upgrades

Jason Gunthorpe <jgg@...dia.com> writes:

> On Wed, May 31, 2023 at 10:30:48AM +1000, Alistair Popple wrote:
>
>> So I'd rather keep the invalidate in ptep_set_access_flags(). Would
>> renaming invalidate_range() to invalidate_arch_secondary_tlb() along
>> with clearing up the documentation make that more acceptable, at least
>> in the short term?
>
> Then we need to go through removing kvm first I think.

Why? I don't think we need to hold up a fix for something that is an
issue today so we can rework a fix for an unrelated problem. Strongly
agree the API/interface/documentation could be better but neither this
nor the KVM fix are abusing the API based on how it's currently
documented IMHO. So I think improving the API is a separate problem.
Happy to help with that, but don't see why it has to happen first given
KVM usage was acceptable and still presumably works even though its
implementation isn't something we like now.

>> And maybe rename invalidate_range() and/or invalidate_range_{start,end}() to make
>> it super obvious that they are intended for two different purposes?  E.g. instead
>> of invalidate_range(), something like invalidate_secondary_tlbs().
>
> Yeah, I think I would call it invalidate_arch_secondary_tlb() and
> document it as being an arch specific set of invalidations that match
> the architected TLB maintenance requrements. And maybe we can check it
> more carefully to make it be called in less places. Like I'm not sure
> it is right to call it from invalidate_range_end under this new
> definition..

I will look at this in more depth, but this comment reminded me there is
already an issue with calling .invalidate_range() from
invalidate_range_end(). We have seen slow downs when unmapping unused
ranges because unmap_vmas() will call .invalidate_range() via
.invalidate_range_end() flooding the SMMU with invalidates even though
zap_pte_range() skipped it because the PTEs were pte_none.

- Alistair