linux-kernel - Re: [PATCH] mm/x86/pat: Only untrack the pfn range if unmap region

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <ZpfxC7LSEMLjt2o4@x1n>
Date: Wed, 17 Jul 2024 12:27:55 -0400
From: Peter Xu <peterx@...hat.com>
To: David Hildenbrand <david@...hat.com>
Cc: David Wang <00107082@....com>, linux-kernel@...r.kernel.org,
	linux-mm@...ck.org, Andrew Morton <akpm@...ux-foundation.org>,
	Alex Williamson <alex.williamson@...hat.com>,
	Jason Gunthorpe <jgg@...dia.com>, Al Viro <viro@...iv.linux.org.uk>,
	Dave Hansen <dave.hansen@...ux.intel.com>,
	Andy Lutomirski <luto@...nel.org>,
	Peter Zijlstra <peterz@...radead.org>,
	Thomas Gleixner <tglx@...utronix.de>,
	Ingo Molnar <mingo@...hat.com>, Borislav Petkov <bp@...en8.de>,
	"Kirill A . Shutemov" <kirill@...temov.name>, x86@...nel.org,
	Yan Zhao <yan.y.zhao@...el.com>, Kevin Tian <kevin.tian@...el.com>,
	Pei Li <peili.dev@...il.com>, Bert Karwatzki <spasswolf@....de>,
	Sergey Senozhatsky <senozhatsky@...omium.org>
Subject: Re: [PATCH] mm/x86/pat: Only untrack the pfn range if unmap region

On Wed, Jul 17, 2024 at 04:14:15PM +0200, David Hildenbrand wrote:
> [catching up on mails]
> 
> > > indicates that file truncation seems to end up messing with a PFNMAP mapping
> > > that has PAT set. That is ... weird. I would have thought that PFNMAP would
> > > never really happen with file truncation.
> > > 
> > > Does this only happen with an OOT driver, that seems to do weird truncate
> > > stuff on files that have a PFNMAP mapping?
> > > 
> > > [1]
> > > https://lore.kernel.org/all/3879ee72-84de-4d2a-93a8-c0b3dc3f0a4c@redhat.com/
> > 
> > Ohhh.. I guess this will also stop working in VFIO, but I think it's fine
> > for now because as Yan pointed out VFIO PCI doesn't register those regions
> > now so VM_PAT is not yet set..
> 
> Interesting, I was assuming that VFIO might be relying on that.
> 
> > 
> > And one thing I said wrong in the previous reply to Yan is, obviously
> > memtype_check_insert() can work with >1 owners as long as the memtype
> > matches.. and that's how fork() works where VM_PAT needs to be duplicated.
> > But this whole thing is a bit confusing to me..  As I think it also means
> > when fork the track_pfn_copy() will call memtype_kernel_map_sync one more
> > time even if we're 100% sure the pgprot will be the same for the kernel
> > mappings..
> 
> I consider the VM_PAT code quite ugly and I wish we could just get rid of it
> (especially, the automatic "entire VMA covered" handling thingy).

Yep, I agree.

> 
> > 
> > I wonder whether there's some way that untrack pfn framework doesn't need
> > to rely on the pgtable to fetch the pfn, because VFIO MMIO region
> > protection will also do that in the near future, AFAICT.  The pgprot part
> > should be easy there to fetch: get_pat_info() should fallback to vma's
> > pgprot if no mapping found; the only outlier should be CoW pages in
> > reality.  The pfn is the real issue so far, so that either track_pfn_copy()
> > or untrack_pfn() may need to know the pfn to untrack, even if it only has
> > the vma information.
> 
> I had a prototype to store that information per VMA to avoid the page table
> lookup. VMA splitting was a bit "added complication", but I got it to work.
> (maybe I can still find it if there is demand)
> 
> The downside was having to consume more memory for all VMAs in the system
> simply (even if only 8 byte) because a handful of VMAs in the system could
> be VM_PAT. I decided that's not what we want. I managed to not consume
> memory in some configurations, but not in all, so I discarded that approach.
> 
> I did not explore storing that information in some auxiliary datastructure.

One idea to avoid that is to let driver opt-in for such information, e.g. a
hook in vm_operations_struct to fetch base pfn for a vma map.  But that
will involve any driver to provide that information, e.g. for David Wang's
case IIUC it's at least an OOT driver, so nothing to fix it from an
upstream patch with that solution (while it should work for VFIO).

> 
> IMHO the whole VM_PAT model is weird:
> 
> 1) mmap()
> 2) remap_pfn_range(): if it covers the whole VMA apply some magic
>    reservation.
> 3) munmap(): we unmap *all* PFNs and, therefore, clean up VM_PAT
> 
> (VMA splitting make the whole model weirder, but it works, because we never
> merge these VMAs)
> 
> This model cannot properly work if we get partial page table zapping via
> truncation/MADV_DONTNEED or similar things after 2). And likely we also
> shouldn't be doing it that way. We should forbid any partial unmappings in
> that model, just like we already disallow MADV_DONTNEED as you note.
> 
> As you mention in your other comment, maybe relevant/all? caller should just
> manage the PAT side independently. So maybe we can move to a different
> model.

Any elaboration of what's the new model you're describing?

Thanks,

-- 
Peter Xu