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: <20250127095331.GB16742@noisy.programming.kicks-ass.net>
Date: Mon, 27 Jan 2025 10:53:31 +0100
From: Peter Zijlstra <peterz@...radead.org>
To: Hugh Dickins <hughd@...gle.com>
Cc: 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 Sun, Jan 26, 2025 at 06:34:48PM -0800, Hugh Dickins wrote:

> > 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.

Ah, I had not seen v3 yet.

> > 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;

Yeah, I suppose it doesn't happen. But I figured why assume stuff.

> 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).

Duh, yeah, so tlb_free_vma() wants to be inside the vma loop of
free_pgtables().

> 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().

I prefer a function over the in-line thing such that mmu-gather is more
or less self contained.

So my concern is/was maintainability of all this; tlb_flush_mmu_pfnmap()
tells me nothing about when this function should be called. Otoh
tlb_free_vma() tell me this should be called when we're freeing VMAs --
much harder to misplace etc. If we have hooks placed at, and named
after, natural events in the lifetime of things, placement is 'obvious'.

Another possible name might be tlb_free_pgtables(), indicating we're
about to start freeing pagetables -- but it would need to assert
!tlb->freed_tables, and I'm not sure this is a constraint worth
imposing. It would bring pain if someone wanted to mix freeing pages and
page-tables.

And we already have vma based hooks, so I much prefer adding one more of
those. This is about funky VMAs after all.