| Message ID | 20231214055539.9420-1-nicholas@linux.ibm.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | kmsan: Enable on powerpc | expand |
Le 14/12/2023 à 06:55, Nicholas Miehlbradt a écrit : > All elements of bounce_buffer are eventually read and passed to the > hypervisor so it should probably be fully initialized. should or shall ? > > Signed-off-by: Nicholas Miehlbradt <nicholas@linux.ibm.com> Should be a Fixed: tag ? > --- > drivers/tty/hvc/hvc_vio.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/tty/hvc/hvc_vio.c b/drivers/tty/hvc/hvc_vio.c > index 736b230f5ec0..1e88bfcdde20 100644 > --- a/drivers/tty/hvc/hvc_vio.c > +++ b/drivers/tty/hvc/hvc_vio.c > @@ -227,7 +227,7 @@ static const struct hv_ops hvterm_hvsi_ops = { > static void udbg_hvc_putc(char c) > { > int count = -1; > - unsigned char bounce_buffer[16]; > + unsigned char bounce_buffer[16] = { 0 }; Why 16 while we have a count of 1 in the call to hvterm_raw_put_chars() ? > > if (!hvterm_privs[0]) > return;
Le 14/12/2023 à 06:55, Nicholas Miehlbradt a écrit : > Word sized accesses may read uninitialized data when optimizing loads. > Disable this optimization when KMSAN is enabled to prevent false > positives. > > Signed-off-by: Nicholas Miehlbradt <nicholas@linux.ibm.com> > --- > arch/powerpc/Kconfig | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig > index 6f105ee4f3cf..e33e3250c478 100644 > --- a/arch/powerpc/Kconfig > +++ b/arch/powerpc/Kconfig > @@ -182,7 +182,7 @@ config PPC > select BUILDTIME_TABLE_SORT > select CLONE_BACKWARDS > select CPUMASK_OFFSTACK if NR_CPUS >= 8192 > - select DCACHE_WORD_ACCESS if PPC64 && CPU_LITTLE_ENDIAN > + select DCACHE_WORD_ACCESS if PPC64 && CPU_LITTLE_ENDIAN && !KMSAN > select DMA_OPS_BYPASS if PPC64 > select DMA_OPS if PPC64 > select DYNAMIC_FTRACE if FUNCTION_TRACER Seems like all archs do this. Maybe a better approach would be to define a HAVE_DCACHE_WORD_ACCESS that is selected by arches, and then the core part select DCACHE_WORD_ACCESS when HAVE_DCACHE_WORD_ACCESS && !KMSAN Christophe
Christophe Leroy <christophe.leroy@csgroup.eu> writes: > Le 14/12/2023 à 06:55, Nicholas Miehlbradt a écrit : >> All elements of bounce_buffer are eventually read and passed to the >> hypervisor so it should probably be fully initialized. > > should or shall ? > >> >> Signed-off-by: Nicholas Miehlbradt <nicholas@linux.ibm.com> > > Should be a Fixed: tag ? > >> --- >> drivers/tty/hvc/hvc_vio.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/drivers/tty/hvc/hvc_vio.c b/drivers/tty/hvc/hvc_vio.c >> index 736b230f5ec0..1e88bfcdde20 100644 >> --- a/drivers/tty/hvc/hvc_vio.c >> +++ b/drivers/tty/hvc/hvc_vio.c >> @@ -227,7 +227,7 @@ static const struct hv_ops hvterm_hvsi_ops = { >> static void udbg_hvc_putc(char c) >> { >> int count = -1; >> - unsigned char bounce_buffer[16]; >> + unsigned char bounce_buffer[16] = { 0 }; > > Why 16 while we have a count of 1 in the call to hvterm_raw_put_chars() ? Because hvterm_raw_put_chars() calls hvc_put_chars() which requires a 16 byte buffer, because it passes the buffer directly to firmware which expects a 16 byte buffer. It's a pretty horrible calling convention, but I guess it's to avoid needing another bounce buffer inside hvc_put_chars(). We should probably do the change below, to at least document the interface better. cheers diff --git a/arch/powerpc/include/asm/hvconsole.h b/arch/powerpc/include/asm/hvconsole.h index ccb2034506f0..0ee7ed019e23 100644 --- a/arch/powerpc/include/asm/hvconsole.h +++ b/arch/powerpc/include/asm/hvconsole.h @@ -22,7 +22,7 @@ * parm is included to conform to put_chars() function pointer template */ extern int hvc_get_chars(uint32_t vtermno, char *buf, int count); -extern int hvc_put_chars(uint32_t vtermno, const char *buf, int count); +extern int hvc_put_chars(uint32_t vtermno, const char buf[16], int count); /* Provided by HVC VIO */ void hvc_vio_init_early(void); diff --git a/arch/powerpc/platforms/pseries/hvconsole.c b/arch/powerpc/platforms/pseries/hvconsole.c index 1ac52963e08b..c40a82e49d59 100644 --- a/arch/powerpc/platforms/pseries/hvconsole.c +++ b/arch/powerpc/platforms/pseries/hvconsole.c @@ -52,7 +52,7 @@ EXPORT_SYMBOL(hvc_get_chars); * firmware. Must be at least 16 bytes, even if count is less than 16. * @count: Send this number of characters. */ -int hvc_put_chars(uint32_t vtermno, const char *buf, int count) +int hvc_put_chars(uint32_t vtermno, const char buf[16], int count) { unsigned long *lbuf = (unsigned long *) buf; long ret; diff --git a/drivers/tty/hvc/hvc_vio.c b/drivers/tty/hvc/hvc_vio.c index 736b230f5ec0..011b239a7e52 100644 --- a/drivers/tty/hvc/hvc_vio.c +++ b/drivers/tty/hvc/hvc_vio.c @@ -115,7 +115,7 @@ static int hvterm_raw_get_chars(uint32_t vtermno, char *buf, int count) * you are sending fewer chars. * @count: number of chars to send. */ -static int hvterm_raw_put_chars(uint32_t vtermno, const char *buf, int count) +static int hvterm_raw_put_chars(uint32_t vtermno, const char buf[16], int count) { struct hvterm_priv *pv = hvterm_privs[vtermno];
Le 14/12/2023 à 06:55, Nicholas Miehlbradt a écrit : > kmsan_handle_dma is required by virtio drivers. Export kmsan_handle_dma > so that the drivers can be compiled as modules. > > Signed-off-by: Nicholas Miehlbradt <nicholas@linux.ibm.com> > --- > mm/kmsan/hooks.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/mm/kmsan/hooks.c b/mm/kmsan/hooks.c > index 7a30274b893c..3532d9275ca5 100644 > --- a/mm/kmsan/hooks.c > +++ b/mm/kmsan/hooks.c > @@ -358,6 +358,7 @@ void kmsan_handle_dma(struct page *page, size_t offset, size_t size, > size -= to_go; > } > } > +EXPORT_SYMBOL(kmsan_handle_dma); virtio is GPL and all exports inside virtio are EXPORT_SYMBOL_GPL(). Should this one be _GPL as well ? > > void kmsan_handle_dma_sg(struct scatterlist *sg, int nents, > enum dma_data_direction dir)
Le 14/12/2023 à 06:55, Nicholas Miehlbradt a écrit : > This series provides the minimal support for Kernal Memory Sanitizer on > powerpc pseries le guests. Kernal Memory Sanitizer is a tool which detects > uses of uninitialized memory. Currently KMSAN is clang only. > > The clang support for powerpc has not yet been merged, the pull request can > be found here [1]. As clang doesn't support it yet, it is probably prematurate to merge that in the kernel. I have open https://github.com/linuxppc/issues/issues/475 to follow through In the meantime I flag this series as "change requested" for a revisit it when time comes Christophe > > In addition to this series, there are a number of changes required in > generic kmsan code. These changes are already on mailing lists as part of > the series implementing KMSAN for s390 [2]. This series is intended to be > rebased on top of the s390 series. > > In addition, I found a bug in the rtc driver used on powerpc. I have sent > a fix to this in a seperate series [3]. > > With this series and the two series mentioned above, I can successfully > boot pseries le defconfig without KMSAN warnings. I have not tested other > powerpc platforms. > > [1] https://github.com/llvm/llvm-project/pull/73611 > [2] https://lore.kernel.org/linux-mm/20231121220155.1217090-1-iii@linux.ibm.com/ > [3] https://lore.kernel.org/linux-rtc/20231129073647.2624497-1-nicholas@linux.ibm.com/ > > Nicholas Miehlbradt (13): > kmsan: Export kmsan_handle_dma > hvc: Fix use of uninitialized array in udbg_hvc_putc > powerpc: Disable KMSAN santitization for prom_init, vdso and purgatory > powerpc: Disable CONFIG_DCACHE_WORD_ACCESS when KMSAN is enabled > powerpc: Unpoison buffers populated by hcalls > powerpc/pseries/nvram: Unpoison buffer populated by rtas_call > powerpc/kprobes: Unpoison instruction in kprobe struct > powerpc: Unpoison pt_regs > powerpc: Disable KMSAN checks on functions which walk the stack > powerpc: Define KMSAN metadata address ranges for vmalloc and ioremap > powerpc: Implement architecture specific KMSAN interface > powerpc/string: Add KMSAN support > powerpc: Enable KMSAN on powerpc > > arch/powerpc/Kconfig | 3 +- > arch/powerpc/include/asm/book3s/64/pgtable.h | 42 +++++++++++++++ > arch/powerpc/include/asm/interrupt.h | 2 + > arch/powerpc/include/asm/kmsan.h | 51 +++++++++++++++++++ > arch/powerpc/include/asm/string.h | 18 ++++++- > arch/powerpc/kernel/Makefile | 2 + > arch/powerpc/kernel/irq_64.c | 2 + > arch/powerpc/kernel/kprobes.c | 2 + > arch/powerpc/kernel/module.c | 2 +- > arch/powerpc/kernel/process.c | 6 +-- > arch/powerpc/kernel/stacktrace.c | 10 ++-- > arch/powerpc/kernel/vdso/Makefile | 1 + > arch/powerpc/lib/Makefile | 2 + > arch/powerpc/lib/mem_64.S | 5 +- > arch/powerpc/lib/memcpy_64.S | 2 + > arch/powerpc/perf/callchain.c | 2 +- > arch/powerpc/platforms/pseries/hvconsole.c | 2 + > arch/powerpc/platforms/pseries/nvram.c | 4 ++ > arch/powerpc/purgatory/Makefile | 1 + > arch/powerpc/sysdev/xive/spapr.c | 3 ++ > drivers/tty/hvc/hvc_vio.c | 2 +- > mm/kmsan/hooks.c | 1 + > .../selftests/powerpc/copyloops/asm/kmsan.h | 0 > .../powerpc/copyloops/linux/export.h | 1 + > 24 files changed, 152 insertions(+), 14 deletions(-) > create mode 100644 arch/powerpc/include/asm/kmsan.h > create mode 100644 tools/testing/selftests/powerpc/copyloops/asm/kmsan.h >