Re: [PATCH v3 28/34] s390/mm: Define KMSAN metadata for vmalloc and modules
On Thu, Jan 04, 2024 at 11:03:42AM +0100, Alexander Gordeev wrote: > On Tue, Jan 02, 2024 at 04:05:31PM +0100, Heiko Carstens wrote: > Hi Heiko, > ... > > > @@ -253,9 +253,17 @@ static unsigned long setup_kernel_memory_layout(void) > > > MODULES_END = round_down(__abs_lowcore, _SEGMENT_SIZE); > > > MODULES_VADDR = MODULES_END - MODULES_LEN; > > > VMALLOC_END = MODULES_VADDR; > > > +#ifdef CONFIG_KMSAN > > > + VMALLOC_END -= MODULES_LEN * 2; > > > +#endif > > > > > > /* allow vmalloc area to occupy up to about 1/2 of the rest virtual > > > space left */ > > > vmalloc_size = min(vmalloc_size, round_down(VMALLOC_END / 2, > > > _REGION3_SIZE)); > > > +#ifdef CONFIG_KMSAN > > > + /* take 2/3 of vmalloc area for KMSAN shadow and origins */ > > > + vmalloc_size = round_down(vmalloc_size / 3, _REGION3_SIZE); > > > + VMALLOC_END -= vmalloc_size * 2; > > > +#endif > > > > Please use > > > > if (IS_ENABLED(CONFIG_KMSAN)) > > > > above, since this way we get more compile time checks. > > This way we will get a mixture of CONFIG_KASAN and CONFIG_KMSAN > #ifdef vs IS_ENABLED() checks within one function. I guess, we > would rather address it with a separate cleanup? I don't think so, since you can't convert the CONFIG_KASAN ifdef to IS_ENABLED() here: it won't compile. But IS_ENABLED(CONFIG_KMSAN) should work. I highly prefer IS_ENABLED() over ifdef since it allows for better compile time checks, and you won't be surprised by code that doesn't compile if you just change a config option. We've seen that way too often.
Re: [PATCH v3 28/34] s390/mm: Define KMSAN metadata for vmalloc and modules
On Tue, Jan 02, 2024 at 04:05:31PM +0100, Heiko Carstens wrote: Hi Heiko, ... > > @@ -253,9 +253,17 @@ static unsigned long setup_kernel_memory_layout(void) > > MODULES_END = round_down(__abs_lowcore, _SEGMENT_SIZE); > > MODULES_VADDR = MODULES_END - MODULES_LEN; > > VMALLOC_END = MODULES_VADDR; > > +#ifdef CONFIG_KMSAN > > + VMALLOC_END -= MODULES_LEN * 2; > > +#endif > > > > /* allow vmalloc area to occupy up to about 1/2 of the rest virtual > > space left */ > > vmalloc_size = min(vmalloc_size, round_down(VMALLOC_END / 2, > > _REGION3_SIZE)); > > +#ifdef CONFIG_KMSAN > > + /* take 2/3 of vmalloc area for KMSAN shadow and origins */ > > + vmalloc_size = round_down(vmalloc_size / 3, _REGION3_SIZE); > > + VMALLOC_END -= vmalloc_size * 2; > > +#endif > > Please use > > if (IS_ENABLED(CONFIG_KMSAN)) > > above, since this way we get more compile time checks. This way we will get a mixture of CONFIG_KASAN and CONFIG_KMSAN #ifdef vs IS_ENABLED() checks within one function. I guess, we would rather address it with a separate cleanup? Thanks!
Re: [PATCH v3 28/34] s390/mm: Define KMSAN metadata for vmalloc and modules
On Thu, Dec 14, 2023 at 12:24:48AM +0100, Ilya Leoshkevich wrote: > The pages for the KMSAN metadata associated with most kernel mappings > are taken from memblock by the common code. However, vmalloc and module > metadata needs to be defined by the architectures. > > Be a little bit more careful than x86: allocate exactly MODULES_LEN > for the module shadow and origins, and then take 2/3 of vmalloc for > the vmalloc shadow and origins. This ensures that users passing small > vmalloc= values on the command line do not cause module metadata > collisions. > > Reviewed-by: Alexander Potapenko > Signed-off-by: Ilya Leoshkevich > --- > arch/s390/boot/startup.c| 8 > arch/s390/include/asm/pgtable.h | 10 ++ > 2 files changed, 18 insertions(+) > > diff --git a/arch/s390/boot/startup.c b/arch/s390/boot/startup.c > index 8104e0e3d188..e37e7ffda430 100644 > --- a/arch/s390/boot/startup.c > +++ b/arch/s390/boot/startup.c > @@ -253,9 +253,17 @@ static unsigned long setup_kernel_memory_layout(void) > MODULES_END = round_down(__abs_lowcore, _SEGMENT_SIZE); > MODULES_VADDR = MODULES_END - MODULES_LEN; > VMALLOC_END = MODULES_VADDR; > +#ifdef CONFIG_KMSAN > + VMALLOC_END -= MODULES_LEN * 2; > +#endif > > /* allow vmalloc area to occupy up to about 1/2 of the rest virtual > space left */ > vmalloc_size = min(vmalloc_size, round_down(VMALLOC_END / 2, > _REGION3_SIZE)); > +#ifdef CONFIG_KMSAN > + /* take 2/3 of vmalloc area for KMSAN shadow and origins */ > + vmalloc_size = round_down(vmalloc_size / 3, _REGION3_SIZE); > + VMALLOC_END -= vmalloc_size * 2; > +#endif Please use if (IS_ENABLED(CONFIG_KMSAN)) above, since this way we get more compile time checks. > +#ifdef CONFIG_KMSAN > +#define KMSAN_VMALLOC_SIZE (VMALLOC_END - VMALLOC_START) > +#define KMSAN_VMALLOC_SHADOW_START VMALLOC_END > +#define KMSAN_VMALLOC_ORIGIN_START (KMSAN_VMALLOC_SHADOW_START + \ > + KMSAN_VMALLOC_SIZE) > +#define KMSAN_MODULES_SHADOW_START (KMSAN_VMALLOC_ORIGIN_START + \ > + KMSAN_VMALLOC_SIZE) Long single lines for these, please :) With that, and Alexander Gordeev's comments addressed: Acked-by: Heiko Carstens
Re: [PATCH v3 28/34] s390/mm: Define KMSAN metadata for vmalloc and modules
On Thu, Dec 14, 2023 at 12:24:48AM +0100, Ilya Leoshkevich wrote:
> The pages for the KMSAN metadata associated with most kernel mappings
> are taken from memblock by the common code. However, vmalloc and module
> metadata needs to be defined by the architectures.
>
> Be a little bit more careful than x86: allocate exactly MODULES_LEN
> for the module shadow and origins, and then take 2/3 of vmalloc for
> the vmalloc shadow and origins. This ensures that users passing small
> vmalloc= values on the command line do not cause module metadata
> collisions.
>
> Reviewed-by: Alexander Potapenko
> Signed-off-by: Ilya Leoshkevich
> ---
> arch/s390/boot/startup.c| 8
> arch/s390/include/asm/pgtable.h | 10 ++
> 2 files changed, 18 insertions(+)
>
> diff --git a/arch/s390/boot/startup.c b/arch/s390/boot/startup.c
> index 8104e0e3d188..e37e7ffda430 100644
> --- a/arch/s390/boot/startup.c
> +++ b/arch/s390/boot/startup.c
> @@ -253,9 +253,17 @@ static unsigned long setup_kernel_memory_layout(void)
> MODULES_END = round_down(__abs_lowcore, _SEGMENT_SIZE);
> MODULES_VADDR = MODULES_END - MODULES_LEN;
> VMALLOC_END = MODULES_VADDR;
> +#ifdef CONFIG_KMSAN
> + VMALLOC_END -= MODULES_LEN * 2;
> +#endif
>
> /* allow vmalloc area to occupy up to about 1/2 of the rest virtual
> space left */
> vmalloc_size = min(vmalloc_size, round_down(VMALLOC_END / 2,
> _REGION3_SIZE));
Since commit 2a65c6e1ad06 ("s390/boot: always align vmalloc area on segment
boundary")
vmalloc_size is aligned on _SEGMENT_SIZE boundary.
> +#ifdef CONFIG_KMSAN
> + /* take 2/3 of vmalloc area for KMSAN shadow and origins */
> + vmalloc_size = round_down(vmalloc_size / 3, _REGION3_SIZE);
And thus, the alignment here should be _SEGMENT_SIZE as well.
> + VMALLOC_END -= vmalloc_size * 2;
> +#endif
> VMALLOC_START = VMALLOC_END - vmalloc_size;
>
> /* split remaining virtual space between 1:1 mapping & vmemmap array */
...
With the above fixup:
Acked-by: Alexander Gordeev
Thanks!
[PATCH v3 28/34] s390/mm: Define KMSAN metadata for vmalloc and modules
The pages for the KMSAN metadata associated with most kernel mappings are taken from memblock by the common code. However, vmalloc and module metadata needs to be defined by the architectures. Be a little bit more careful than x86: allocate exactly MODULES_LEN for the module shadow and origins, and then take 2/3 of vmalloc for the vmalloc shadow and origins. This ensures that users passing small vmalloc= values on the command line do not cause module metadata collisions. Reviewed-by: Alexander Potapenko Signed-off-by: Ilya Leoshkevich --- arch/s390/boot/startup.c| 8 arch/s390/include/asm/pgtable.h | 10 ++ 2 files changed, 18 insertions(+) diff --git a/arch/s390/boot/startup.c b/arch/s390/boot/startup.c index 8104e0e3d188..e37e7ffda430 100644 --- a/arch/s390/boot/startup.c +++ b/arch/s390/boot/startup.c @@ -253,9 +253,17 @@ static unsigned long setup_kernel_memory_layout(void) MODULES_END = round_down(__abs_lowcore, _SEGMENT_SIZE); MODULES_VADDR = MODULES_END - MODULES_LEN; VMALLOC_END = MODULES_VADDR; +#ifdef CONFIG_KMSAN + VMALLOC_END -= MODULES_LEN * 2; +#endif /* allow vmalloc area to occupy up to about 1/2 of the rest virtual space left */ vmalloc_size = min(vmalloc_size, round_down(VMALLOC_END / 2, _REGION3_SIZE)); +#ifdef CONFIG_KMSAN + /* take 2/3 of vmalloc area for KMSAN shadow and origins */ + vmalloc_size = round_down(vmalloc_size / 3, _REGION3_SIZE); + VMALLOC_END -= vmalloc_size * 2; +#endif VMALLOC_START = VMALLOC_END - vmalloc_size; /* split remaining virtual space between 1:1 mapping & vmemmap array */ diff --git a/arch/s390/include/asm/pgtable.h b/arch/s390/include/asm/pgtable.h index 601e87fa8a9a..d764abeb9e6d 100644 --- a/arch/s390/include/asm/pgtable.h +++ b/arch/s390/include/asm/pgtable.h @@ -107,6 +107,16 @@ static inline int is_module_addr(void *addr) return 1; } +#ifdef CONFIG_KMSAN +#define KMSAN_VMALLOC_SIZE (VMALLOC_END - VMALLOC_START) +#define KMSAN_VMALLOC_SHADOW_START VMALLOC_END +#define KMSAN_VMALLOC_ORIGIN_START (KMSAN_VMALLOC_SHADOW_START + \ + KMSAN_VMALLOC_SIZE) +#define KMSAN_MODULES_SHADOW_START (KMSAN_VMALLOC_ORIGIN_START + \ + KMSAN_VMALLOC_SIZE) +#define KMSAN_MODULES_ORIGIN_START (KMSAN_MODULES_SHADOW_START + MODULES_LEN) +#endif + /* * A 64 bit pagetable entry of S390 has following format: * |PFRA |0IPC| OS | -- 2.43.0
