linux-kernel - Re: [PATCH v2 1/3] x86/devmem: Remove duplicate range_is

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: <ucdjz2mytjsndtkoroadd7r7grsi4hwpqd47v752zwo6rn5bg7@a7pq6ah4tdai>
Date: Thu, 17 Apr 2025 12:58:22 +0530
From: Naveen N Rao <naveen@...nel.org>
To: Dan Williams <dan.j.williams@...el.com>
Cc: dave.hansen@...ux.intel.com, Ingo Molnar <mingo@...nel.org>, 
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 1/3] x86/devmem: Remove duplicate range_is_allowed()
 definition

On Wed, Apr 16, 2025 at 02:25:49PM -0700, Dan Williams wrote:
> Naveen N Rao wrote:
> > On Thu, Apr 10, 2025 at 06:22:23PM -0700, Dan Williams wrote:
> > > It looks like x86 has a local re-implementation of range_is_allowed()
> > > just to add a pat_enabled() check for the strong symbol override of
> > > phys_mem_access_prot_allowed() from drivers/char/mem.c.
> > > 
> > > In preparation for updating range_is_allowed() logic, arrange for there
> > > to be only one shared instance of "range_is_allowed()" in the kernel by
> > > moving a common helper to include/linux/io.h.
> > > 
> > > Cc: Dave Hansen <dave.hansen@...ux.intel.com>
> > > Cc: Ingo Molnar <mingo@...nel.org>
> > > Signed-off-by: Dan Williams <dan.j.williams@...el.com>
> > > ---
> > >  arch/x86/mm/pat/memtype.c |   31 ++++---------------------------
> > >  drivers/char/mem.c        |   18 ------------------
> > >  include/linux/io.h        |   21 +++++++++++++++++++++
> > >  3 files changed, 25 insertions(+), 45 deletions(-)
> > > 
> > > diff --git a/arch/x86/mm/pat/memtype.c b/arch/x86/mm/pat/memtype.c
> > > index 72d8cbc61158..c97b6598f187 100644
> > > --- a/arch/x86/mm/pat/memtype.c
> > > +++ b/arch/x86/mm/pat/memtype.c
> > > @@ -38,6 +38,7 @@
> > >  #include <linux/kernel.h>
> > >  #include <linux/pfn_t.h>
> > >  #include <linux/slab.h>
> > > +#include <linux/io.h>
> > >  #include <linux/mm.h>
> > >  #include <linux/highmem.h>
> > >  #include <linux/fs.h>
> > > @@ -773,38 +774,14 @@ pgprot_t phys_mem_access_prot(struct file *file, unsigned long pfn,
> > >  	return vma_prot;
> > >  }
> > >  
> > > -#ifdef CONFIG_STRICT_DEVMEM
> > > -/* This check is done in drivers/char/mem.c in case of STRICT_DEVMEM */
> > > -static inline int range_is_allowed(unsigned long pfn, unsigned long size)
> > > -{
> > > -	return 1;
> > > -}
> > 
> > It looks like no checks were done here if CONFIG_STRICT_DEVMEM was set, 
> > so this patch changes that.
> 
> Yes, but this still matches the historical intent, and the historical
> intent is a tad messy.
> 
> The pat_enabled check was originally added as a *bypass* of additional
> logic in phys_mem_access_prot_allowed() [1] to validate that /dev/mem was
> establishing compatible mappings of "System-RAM" via /dev/mem. This
> patch maintains that expectation that phys_mem_access_prot_allowed()
> returns immediately when there is no potential cache conflict.

Thanks for the background, that makes sense.

Do we also no longer need the devmem_is_allowed() checks in pat.c if PAT 
is enabled and !CONFIG_STRICT_DEVMEM?

> 
> However, the point is moot in current code because [2] and [3] removed
> all cache type validation from phys_mem_access_prot_allowed() in favor
> track_pfn_remap().
> 
> According to:
> Commit 9e41bff2708e ("x86: fix /dev/mem mmap breakage when PAT is disabled") [1]
> Commit 1886297ce0c8 ("x86/mm/pat: Fix BUG_ON() in mmap_mem() on QEMU/i386") [2]
> Commit 0c3c8a18361a ("x86, PAT: Remove duplicate memtype reserve in devmem mmap") [3]
> 
> > > -#else
> > > -/* This check is needed to avoid cache aliasing when PAT is enabled */
> > > -static inline int range_is_allowed(unsigned long pfn, unsigned long size)
> > > -{
> > > -	u64 from = ((u64)pfn) << PAGE_SHIFT;
> > > -	u64 to = from + size;
> > > -	u64 cursor = from;
> > > -
> > > -	if (!pat_enabled())
> > > -		return 1;
> > > -
> > > -	while (cursor < to) {
> > > -		if (!devmem_is_allowed(pfn))
> > > -			return 0;
> > > -		cursor += PAGE_SIZE;
> > > -		pfn++;
> > > -	}
> > > -	return 1;
> > > -}
> > > -#endif /* CONFIG_STRICT_DEVMEM */
> > > -
> > >  int phys_mem_access_prot_allowed(struct file *file, unsigned long pfn,
> > >  				unsigned long size, pgprot_t *vma_prot)
> > >  {
> > >  	enum page_cache_mode pcm = _PAGE_CACHE_MODE_WB;
> > >  
> > > +	if (!pat_enabled())
> > > +		return 1;
> > > +
> > 
> > Shouldn't this test for pat_enabled() (perhaps only if 
> > CONFIG_STRICT_DEVMEM is set) and continue with the rest of the function 
> > otherwise?
> 
> No because, per above, the check is here to short-circuit the rest of
> phys_mem_access_prot_allowed() when PAT is disabled.
> 
> I will add some notes to the changelog to save the next person from
> needing to find the history here.
> 
> I found it interesting that Venki suggested that the duplicated
> "range_is_allowed()" be cleaned up back in 2008 [4], so this is a
> cleanup 17 years (almost to the day) in the making:
> 
> Commit 0124cecfc85a ("x86, PAT: disable /dev/mem mmap RAM with PAT") [4]

Indeed!


Thanks,
Naveen