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: <s6fek3k3zsgf74yuppzckhcnud67pgfitz66n6uwkky7gvjcpc@rp4pxvie2dpb>
Date: Mon, 14 Apr 2025 23:47:19 +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 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.

> -#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?


- Naveen

>  	if (!range_is_allowed(pfn, size))
>  		return 0;
>  
> diff --git a/drivers/char/mem.c b/drivers/char/mem.c
> index 169eed162a7f..48839958b0b1 100644
> --- a/drivers/char/mem.c
> +++ b/drivers/char/mem.c
> @@ -61,29 +61,11 @@ static inline int page_is_allowed(unsigned long pfn)
>  {
>  	return devmem_is_allowed(pfn);
>  }
> -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;
> -
> -	while (cursor < to) {
> -		if (!devmem_is_allowed(pfn))
> -			return 0;
> -		cursor += PAGE_SIZE;
> -		pfn++;
> -	}
> -	return 1;
> -}
>  #else
>  static inline int page_is_allowed(unsigned long pfn)
>  {
>  	return 1;
>  }
> -static inline int range_is_allowed(unsigned long pfn, unsigned long size)
> -{
> -	return 1;
> -}
>  #endif
>  
>  static inline bool should_stop_iteration(void)
> diff --git a/include/linux/io.h b/include/linux/io.h
> index 6a6bc4d46d0a..0642c7ee41db 100644
> --- a/include/linux/io.h
> +++ b/include/linux/io.h
> @@ -183,4 +183,25 @@ static inline void arch_io_free_memtype_wc(resource_size_t base,
>  int devm_arch_io_reserve_memtype_wc(struct device *dev, resource_size_t start,
>  				    resource_size_t size);
>  
> +#ifdef CONFIG_STRICT_DEVMEM
> +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;
> +
> +	while (cursor < to) {
> +		if (!devmem_is_allowed(pfn))
> +			return 0;
> +		cursor += PAGE_SIZE;
> +		pfn++;
> +	}
> +	return 1;
> +}
> +#else
> +static inline int range_is_allowed(unsigned long pfn, unsigned long size)
> +{
> +	return 1;
> +}
> +#endif
>  #endif /* _LINUX_IO_H */
>