| 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
| ||
|
Message-ID: <d5272f42-f3e3-b2a8-428e-bd7815cf7518@csgroup.eu>
Date: Mon, 22 Aug 2022 06:25:47 +0000
From: Christophe Leroy <christophe.leroy@...roup.eu>
To: Baoquan He <bhe@...hat.com>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
CC: "linux-mm@...ck.org" <linux-mm@...ck.org>,
"akpm@...ux-foundation.org" <akpm@...ux-foundation.org>,
"hch@...radead.org" <hch@...radead.org>,
"agordeev@...ux.ibm.com" <agordeev@...ux.ibm.com>,
"wangkefeng.wang@...wei.com" <wangkefeng.wang@...wei.com>,
"linux-arm-kernel@...ts.infradead.org"
<linux-arm-kernel@...ts.infradead.org>
Subject: Re: [PATCH v2 01/11] mm/ioremap: change the return value of
io[re|un]map_allowed and rename
Le 20/08/2022 à 02:31, Baoquan He a écrit :
> In some architectures, there are ARCH specifici io address mapping
> handling when calling ioremap() or ioremap_prot(), e.g, arc, ia64,
> openrisc, s390, sh.
>
> In oder to convert them to take GENERIC_IOREMAP method, we need change
> the return value of hook ioremap_allowed() and iounmap_allowed().
> Meanwhile, rename them to arch_ioremap() and arch_iounmap() to reflect
> their current behaviour.
Please don't just say you need to change the return value. Explain why.
And why does it need a name change ? The new name suggests that what was
simply a check function becomes now a function doing the job. Is that
the intention ?
>
> ===
> arch_ioremap() return a bool,
It is not a bool. A bool is either true or false.
> - IS_ERR means return an error
> - NULL means continue to remap
> - a non-NULL, non-IS_ERR pointer is returned directly
> arch_iounmap() return a bool,
Same here, not a bool either.
> - 0 means continue to vunmap
> - error code means skip vunmap and return directly
>
> This is taken from Kefeng's below old patch. Christoph suggested the
> return value because he foresaw the doablity of converting to take
> GENERIC_IOREMAP on more architectures.
> - [PATCH v3 4/6] mm: ioremap: Add arch_ioremap/iounmap()
> - https://lore.kernel.org/all/20220519082552.117736-5-wangkefeng.wang@huawei.com/T/#u
>
> While at it, the invocation of arch_ioremap() need be moved to the
> beginning of ioremap_prot() because architectures like sh, openrisc,
> ia64, need do the ARCH specific io address mapping on the original
> physical address. And in the later patch, the address fix up code
> in arch_ioremap() also need be done on the original addre on some
> architectures.
>
> This is preparation for later patch, no functionality change.
No functionnal change, really ?
>
> Signed-off-by: Baoquan He <bhe@...hat.com>
> ---
> arch/arm64/include/asm/io.h | 4 ++--
> arch/arm64/mm/ioremap.c | 15 ++++++++++-----
> include/asm-generic/io.h | 29 +++++++++++++++--------------
> mm/ioremap.c | 12 ++++++++----
> 4 files changed, 35 insertions(+), 25 deletions(-)
>
> diff --git a/arch/arm64/include/asm/io.h b/arch/arm64/include/asm/io.h
> index 877495a0fd0c..dd7e1c2dc86c 100644
> --- a/arch/arm64/include/asm/io.h
> +++ b/arch/arm64/include/asm/io.h
> @@ -139,8 +139,8 @@ extern void __memset_io(volatile void __iomem *, int, size_t);
> * I/O memory mapping functions.
> */
>
> -bool ioremap_allowed(phys_addr_t phys_addr, size_t size, unsigned long prot);
> -#define ioremap_allowed ioremap_allowed
> +void __iomem *arch_ioremap(phys_addr_t phys_addr, size_t size, unsigned long prot);
> +#define arch_ioremap arch_ioremap
>
> #define _PAGE_IOREMAP PROT_DEVICE_nGnRE
>
> diff --git a/arch/arm64/mm/ioremap.c b/arch/arm64/mm/ioremap.c
> index c5af103d4ad4..b0f4cea86f0e 100644
> --- a/arch/arm64/mm/ioremap.c
> +++ b/arch/arm64/mm/ioremap.c
> @@ -3,19 +3,24 @@
> #include <linux/mm.h>
> #include <linux/io.h>
>
> -bool ioremap_allowed(phys_addr_t phys_addr, size_t size, unsigned long prot)
> +void __iomem *arch_ioremap(phys_addr_t phys_addr, size_t size, unsigned long prot)
> {
> - unsigned long last_addr = phys_addr + size - 1;
> + unsigned long last_addr, offset;
> +
> + offset = phys_addr & (~PAGE_MASK);
> + phys_addr -= offset;
> + size = PAGE_ALIGN(size + offset);
> + last_addr = phys_addr + size - 1;
>
> /* Don't allow outside PHYS_MASK */
> if (last_addr & ~PHYS_MASK)
> - return false;
> + return IOMEM_ERR_PTR(-EINVAL);
>
> /* Don't allow RAM to be mapped. */
> if (WARN_ON(pfn_is_map_memory(__phys_to_pfn(phys_addr))))
> - return false;
> + return IOMEM_ERR_PTR(-EINVAL);
>
> - return true;
> + return NULL;
> }
>
> /*
> diff --git a/include/asm-generic/io.h b/include/asm-generic/io.h
> index a68f8fbf423b..7b6bfb62ef80 100644
> --- a/include/asm-generic/io.h
> +++ b/include/asm-generic/io.h
> @@ -1049,27 +1049,28 @@ static inline void iounmap(volatile void __iomem *addr)
>
> /*
> * Arch code can implement the following two hooks when using GENERIC_IOREMAP
> - * ioremap_allowed() return a bool,
> - * - true means continue to remap
> - * - false means skip remap and return directly
> - * iounmap_allowed() return a bool,
> - * - true means continue to vunmap
> - * - false means skip vunmap and return directly
> + * arch_ioremap() return a bool,
> + * - IS_ERR means return an error
> + * - NULL means continue to remap
> + * - a non-NULL, non-IS_ERR pointer is returned directly
> + * arch_iounmap() return a bool,
> + * - 0 means continue to vunmap
> + * - error code means skip vunmap and return directly
> */
> -#ifndef ioremap_allowed
> -#define ioremap_allowed ioremap_allowed
> -static inline bool ioremap_allowed(phys_addr_t phys_addr, size_t size,
> +#ifndef arch_ioremap
> +#define arch_ioremap arch_ioremap
> +static inline void __iomem *arch_ioremap(phys_addr_t phys_addr, size_t size,
> unsigned long prot)
> {
> - return true;
> + return NULL;
> }
> #endif
>
> -#ifndef iounmap_allowed
> -#define iounmap_allowed iounmap_allowed
> -static inline bool iounmap_allowed(void *addr)
> +#ifndef arch_iounmap
> +#define arch_iounmap arch_iounmap
> +static inline int arch_iounmap(void __iomem *addr)
> {
> - return true;
> + return 0;
> }
> #endif
>
> diff --git a/mm/ioremap.c b/mm/ioremap.c
> index 8652426282cc..99fde69becc7 100644
> --- a/mm/ioremap.c
> +++ b/mm/ioremap.c
> @@ -17,6 +17,13 @@ void __iomem *ioremap_prot(phys_addr_t phys_addr, size_t size,
> unsigned long offset, vaddr;
> phys_addr_t last_addr;
> struct vm_struct *area;
> + void __iomem *ioaddr;
> +
> + ioaddr = arch_ioremap(phys_addr, size, prot);
> + if (IS_ERR(ioaddr))
> + return NULL;
> + else if (ioaddr)
> + return ioaddr;
>
> /* Disallow wrap-around or zero size */
> last_addr = phys_addr + size - 1;
> @@ -28,9 +35,6 @@ void __iomem *ioremap_prot(phys_addr_t phys_addr, size_t size,
> phys_addr -= offset;
> size = PAGE_ALIGN(size + offset);
>
> - if (!ioremap_allowed(phys_addr, size, prot))
> - return NULL;
> -
> area = get_vm_area_caller(size, VM_IOREMAP,
> __builtin_return_address(0));
> if (!area)
> @@ -52,7 +56,7 @@ void iounmap(volatile void __iomem *addr)
> {
> void *vaddr = (void *)((unsigned long)addr & PAGE_MASK);
>
> - if (!iounmap_allowed(vaddr))
> + if (arch_iounmap((void __iomem *)addr))
> return;
>
> if (is_vmalloc_addr(vaddr))
Powered by blists - more mailing lists