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: <Z5Q9KvugnVQv8QIO@google.com>
Date: Sat, 25 Jan 2025 01:23:54 +0000
From: Roman Gushchin <roman.gushchin@...ux.dev>
To: Peter Zijlstra <peterz@...radead.org>
Cc: linux-kernel@...r.kernel.org, linux-mm@...ck.org,
	Andrew Morton <akpm@...ux-foundation.org>,
	Jann Horn <jannh@...gle.com>, Will Deacon <will@...nel.org>,
	"Aneesh Kumar K.V" <aneesh.kumar@...nel.org>,
	Nick Piggin <npiggin@...il.com>, Hugh Dickins <hughd@...gle.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, Jan 24, 2025 at 09:22:50AM +0100, Peter Zijlstra wrote:
> On Thu, Jan 23, 2025 at 11:12:33PM +0000, Roman Gushchin wrote:
> 
> > > +static inline void tlb_free_vma(struct mmu_gather *tlb, struct vm_area_struct *vma)
> > >  {
> > >  	if (tlb->fullmm)
> > >  		return;
> > >  
> > >  	/*
> > >  	 * VM_PFNMAP is more fragile because the core mm will not track the
> > > +	 * page mapcount -- there might not be page-frames for these PFNs
> > > +	 * after all.
> > > +	 *
> > > +	 * Specifically() there is a race between munmap() and
> > > +	 * unmap_mapping_range(), where munmap() will unlink the VMA, such
> > > +	 * that unmap_mapping_range() will no longer observe the VMA and
> > > +	 * no-op, without observing the TLBI, returning prematurely.
> > > +	 *
> > > +	 * So if we're about to unlink such a VMA, and we have pending
> > > +	 * TLBI for such a vma, flush things now.
> > >  	 */
> > > +	if ((vma->vm_flags & (VM_PFNMAP|VM_MIXEDMAP)) && tlb->vma_pfn)
> > >  		tlb_flush_mmu_tlbonly(tlb);
> > 
> > Why do we need to re-check vma->vm_flags & (VM_PFNMAP|VM_MIXEDMAP) here?
> 
> No need, but an opportunity.
> 
> > In free_pgtables() we're iterating over multiple vma's. What if the first has
> > no VM_PFNMAP set, but some other do? Idk if it's even possible, but it's not
> > obvious that it's not possible either.
> 
> If we only need to flush PFN entries before unlinking PFN VMAs, then:
> 
>  - if there are no PFNs pending (vma_pfn), we don't need to flush;
>  - if no PFN vma is being freed (vm_flags), we don't need to flush.

Right, but if I understand the code correctly, more than one vma can be
freed by a single free_pgtables() invocation. Should we then check
each vma's flags in the while loop in free_pgtables()? But then
we're back to where we're now with multiple flushes.

Do I misunderstand this?

Thanks