| Message ID | 20231213233605.661251-29-iii@linux.ibm.com (mailing list archive) |
|---|---|
| State | Handled Elsewhere |
| Headers | show |
| Series | kmsan: Enable on s390 | expand |
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 <glider@google.com> > Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com> > --- > 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 <agordeev@linux.ibm.com> Thanks!
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 <glider@google.com> > Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com> > --- > 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 <hca@linux.ibm.com>
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!
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.
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 |