linux-kernel - Re: [PATCH v2] mmu_gather: move tlb flush for VM_PFNMAP/VM_MIXEDMAP vmas into free

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: <d01c2d60-3901-1f66-770f-e9d12bfd89b5@google.com>
Date: Sun, 26 Jan 2025 18:34:48 -0800 (PST)
From: Hugh Dickins <hughd@...gle.com>
To: Peter Zijlstra <peterz@...radead.org>
cc: Hugh Dickins <hughd@...gle.com>, Jann Horn <jannh@...gle.com>, 
    Roman Gushchin <roman.gushchin@...ux.dev>, linux-kernel@...r.kernel.org, 
    linux-mm@...ck.org, Andrew Morton <akpm@...ux-foundation.org>, 
    Will Deacon <will@...nel.org>, 
    "Aneesh Kumar K.V" <aneesh.kumar@...nel.org>, 
    Nick Piggin <npiggin@...il.com>, linux-arch@...r.kernel.org
Subject: Re: [PATCH v2] mmu_gather: move tlb flush for VM_PFNMAP/VM_MIXEDMAP
 vmas into free_pgtables()

On Fri, 24 Jan 2025, Peter Zijlstra wrote:
> On Thu, Jan 23, 2025 at 08:42:36PM -0800, Hugh Dickins wrote:
> > The changelog of commit b67fbebd4cf9 ("mmu_gather: Force tlb-flush
> > VM_PFNMAP vmas") has not helped me either.  Nor could I locate any
> > discussion (Jann, Linus, Peter, Will?) that led up to it.
> 
> Hmm, that was probably on security -- I should have those mails around
> somewhere, I'll see if I can dig them up.

That was very helpful, thank you: I've gone through a lot of confusion,
but feeling more confident about it all today.

> 
> > To me, Peter's patch looks much like yours, except wth different
> > names and comments, plus the "vma" error you point out below.
> 
> Yes, 3 differences:
> 
>  - naming;
>  - the extra check;
>  - the vma_pfn clearing condition.
> 
> Under the assumption that this is all about those PFNs, the argument
> (as also outlined in the email to Roman just now) is that you only need
> to flush if both: you have pending TLBI for PFN and are indeed about to
> unlink a PFN vma.
> 
> If we've flushed the relevant PFNs earlier, for whatever reason,
> batching, or the arch has !MERGE_VMAS or whatever, then we do not need
> to flush again. So clearing vma_pfn in __tlb_reset_range() is the right
> place.

Yes, Roman moved to clearing vma_pfn in __tlb_reset_range() in his v3:
we are all in agreement on that.

> 
> Similarly, if we don't ever actually free/unlink the PFN vma, we also
> don't care.

I cannot think of a case in which we arrive at free_pgtables(), but do not
unlink the vma(s) which caused vma_pfn to be set.  If there is such a case,
it's not worth optimizing for; and wrong to check just the first vma in the
list (don't look only at the stable commit 895428ee124a which Roman cited -
it had to be fixed by 891f03f688de afterwards).

Personally, I prefer code inline in free_pgtables() which shows what's
going on, as Roman did in v1, rather than struggling to devise a
self-explanatory function name for something over there in tlb.h.

But I may be in a minority on that, and his tlb_flush_mmu_pfnmap()
is much more to the point than tlb_free_vma().

Hugh