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

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

Message-ID: <6800205d86e73_71fe294e4@dwillia2-xfh.jf.intel.com.notmuch>
Date: Wed, 16 Apr 2025 14:25:49 -0700
From: Dan Williams <dan.j.williams@...el.com>
To: Naveen N Rao <naveen@...nel.org>, 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

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.

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]