Re: [PATCH 10/13] powerpc: Define KMSAN metadata address ranges for vmalloc and ioremap

From: Nicholas Miehlbradt <nicholas@linux.ibm.com>
To: Christophe Leroy <christophe.leroy@csgroup.eu>,
	"glider@google.com" <glider@google.com>,
	"elver@google.com" <elver@google.com>,
	"dvyukov@google.com" <dvyukov@google.com>,
	"akpm@linux-foundation.org" <akpm@linux-foundation.org>,
	"mpe@ellerman.id.au" <mpe@ellerman.id.au>,
	"npiggin@gmail.com" <npiggin@gmail.com>
Cc: "linux-mm@kvack.org" <linux-mm@kvack.org>,
	"linuxppc-dev@lists.ozlabs.org" <linuxppc-dev@lists.ozlabs.org>,
	"iii@linux.ibm.com" <iii@linux.ibm.com>,
	"kasan-dev@googlegroups.com" <kasan-dev@googlegroups.com>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH 10/13] powerpc: Define KMSAN metadata address ranges for vmalloc and ioremap
Date: Wed, 10 Jan 2024 14:54:06 +1100	[thread overview]
Message-ID: <74ad4d5f-7bf7-484c-9386-07945f0c6c5d@linux.ibm.com> (raw)
In-Reply-To: <d24c430a-bde5-4432-8550-57de33cb203c@csgroup.eu>


On 14/12/2023 8:17 pm, Christophe Leroy wrote:
> 
> 
> Le 14/12/2023 à 06:55, Nicholas Miehlbradt a écrit :
>> Splits the vmalloc region into four. The first quarter is the new
>> vmalloc region, the second is used to store shadow metadata and the
>> third is used to store origin metadata. The fourth quarter is unused.
>>
>> Do the same for the ioremap region.
>>
>> Module data is stored in the vmalloc region so alias the modules
>> metadata addresses to the respective vmalloc metadata addresses. Define
>> MODULES_VADDR and MODULES_END to the start and end of the vmalloc
>> region.
>>
>> Since MODULES_VADDR was previously only defined on ppc32 targets checks
>> for if this macro is defined need to be updated to include
>> defined(CONFIG_PPC32).
> 
> Why ?
> 
> In your case MODULES_VADDR is above PAGE_OFFSET so there should be no
> difference.
> 
> Christophe
> 
On 64 bit builds the BUILD_BUG always triggers since MODULES_VADDR 
expands to __vmalloc_start which is defined in a different translation 
unit. I can restrict the #ifdef CONFIG_PPC32 to just around the 
BUILD_BUG since as you pointed out there is no difference otherwise.
>>
>> Signed-off-by: Nicholas Miehlbradt <nicholas@linux.ibm.com>
>> ---
>>    arch/powerpc/include/asm/book3s/64/pgtable.h | 42 ++++++++++++++++++++
>>    arch/powerpc/kernel/module.c                 |  2 +-
>>    2 files changed, 43 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/powerpc/include/asm/book3s/64/pgtable.h b/arch/powerpc/include/asm/book3s/64/pgtable.h
>> index cb77eddca54b..b3a02b8d96e3 100644
>> --- a/arch/powerpc/include/asm/book3s/64/pgtable.h
>> +++ b/arch/powerpc/include/asm/book3s/64/pgtable.h
>> @@ -249,7 +249,38 @@ enum pgtable_index {
>>    extern unsigned long __vmalloc_start;
>>    extern unsigned long __vmalloc_end;
>>    #define VMALLOC_START	__vmalloc_start
>> +
>> +#ifndef CONFIG_KMSAN
>>    #define VMALLOC_END	__vmalloc_end
>> +#else
>> +/*
>> + * In KMSAN builds vmalloc area is four times smaller, and the remaining 3/4
>> + * are used to keep the metadata for virtual pages. The memory formerly
>> + * belonging to vmalloc area is now laid out as follows:
>> + *
>> + * 1st quarter: VMALLOC_START to VMALLOC_END - new vmalloc area
>> + * 2nd quarter: KMSAN_VMALLOC_SHADOW_START to
>> + *              KMSAN_VMALLOC_SHADOW_START+VMALLOC_LEN - vmalloc area shadow
>> + * 3rd quarter: KMSAN_VMALLOC_ORIGIN_START to
>> + *              KMSAN_VMALLOC_ORIGIN_START+VMALLOC_LEN - vmalloc area origins
>> + * 4th quarter: unused
>> + */
>> +#define VMALLOC_LEN ((__vmalloc_end - __vmalloc_start) >> 2)
>> +#define VMALLOC_END (VMALLOC_START + VMALLOC_LEN)
>> +
>> +#define KMSAN_VMALLOC_SHADOW_START VMALLOC_END
>> +#define KMSAN_VMALLOC_ORIGIN_START (VMALLOC_END + VMALLOC_LEN)
>> +
>> +/*
>> + * Module metadata is stored in the corresponding vmalloc metadata regions
>> + */
>> +#define KMSAN_MODULES_SHADOW_START	KMSAN_VMALLOC_SHADOW_START
>> +#define KMSAN_MODULES_ORIGIN_START	KMSAN_VMALLOC_ORIGIN_START
>> +#endif /* CONFIG_KMSAN */
>> +
>> +#define MODULES_VADDR VMALLOC_START
>> +#define MODULES_END VMALLOC_END
>> +#define MODULES_LEN		(MODULES_END - MODULES_VADDR)
>>    
>>    static inline unsigned int ioremap_max_order(void)
>>    {
>> @@ -264,7 +295,18 @@ extern unsigned long __kernel_io_start;
>>    extern unsigned long __kernel_io_end;
>>    #define KERN_VIRT_START __kernel_virt_start
>>    #define KERN_IO_START  __kernel_io_start
>> +#ifndef CONFIG_KMSAN
>>    #define KERN_IO_END __kernel_io_end
>> +#else
>> +/*
>> + * In KMSAN builds IO space is 4 times smaller, the remaining space is used to
>> + * store metadata. See comment for vmalloc regions above.
>> + */
>> +#define KERN_IO_LEN             ((__kernel_io_end - __kernel_io_start) >> 2)
>> +#define KERN_IO_END             (KERN_IO_START + KERN_IO_LEN)
>> +#define KERN_IO_SHADOW_START    KERN_IO_END
>> +#define KERN_IO_ORIGIN_START    (KERN_IO_SHADOW_START + KERN_IO_LEN)
>> +#endif /* !CONFIG_KMSAN */
>>    
>>    extern struct page *vmemmap;
>>    extern unsigned long pci_io_base;
>> diff --git a/arch/powerpc/kernel/module.c b/arch/powerpc/kernel/module.c
>> index f6d6ae0a1692..5043b959ad4d 100644
>> --- a/arch/powerpc/kernel/module.c
>> +++ b/arch/powerpc/kernel/module.c
>> @@ -107,7 +107,7 @@ __module_alloc(unsigned long size, unsigned long start, unsigned long end, bool
>>    
>>    void *module_alloc(unsigned long size)
>>    {
>> -#ifdef MODULES_VADDR
>> +#if defined(MODULES_VADDR) && defined(CONFIG_PPC32)
>>    	unsigned long limit = (unsigned long)_etext - SZ_32M;
>>    	void *ptr = NULL;
>>