| Message ID | 20210728190254.3921642-3-hca@linux.ibm.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | s390: add kfence support | expand |
On 28.07.21 21:02, Heiko Carstens wrote: > From: Sven Schnelle <svens@linux.ibm.com> > > s390 only reports the page address during a translation fault. > To make the kfence unit tests pass, add a function that might > be implemented by architectures to mask out address bits. FWIW, the s390 hardware does indeed only provide the page address for page faults. We had to do the same trick for other software, e.g. see valgrind https://sourceware.org/git/?p=valgrind.git;a=blob;f=coregrind/m_signals.c;h=b45afe59923245352ac17fdd1eeeb5e220f912be;hb=HEAD#l2702 > > Signed-off-by: Sven Schnelle <svens@linux.ibm.com> > Signed-off-by: Heiko Carstens <hca@linux.ibm.com> > --- > mm/kfence/kfence_test.c | 13 ++++++++++++- > 1 file changed, 12 insertions(+), 1 deletion(-) > > diff --git a/mm/kfence/kfence_test.c b/mm/kfence/kfence_test.c > index 942cbc16ad26..eb6307c199ea 100644 > --- a/mm/kfence/kfence_test.c > +++ b/mm/kfence/kfence_test.c > @@ -23,8 +23,15 @@ > #include <linux/tracepoint.h> > #include <trace/events/printk.h> > > +#include <asm/kfence.h> > + > #include "kfence.h" > > +/* May be overridden by <asm/kfence.h>. */ > +#ifndef arch_kfence_test_address > +#define arch_kfence_test_address(addr) (addr) > +#endif > + > /* Report as observed from console. */ > static struct { > spinlock_t lock; > @@ -82,6 +89,7 @@ static const char *get_access_type(const struct expect_report *r) > /* Check observed report matches information in @r. */ > static bool report_matches(const struct expect_report *r) > { > + unsigned long addr = (unsigned long)r->addr; > bool ret = false; > unsigned long flags; > typeof(observed.lines) expect; > @@ -131,22 +139,25 @@ static bool report_matches(const struct expect_report *r) > switch (r->type) { > case KFENCE_ERROR_OOB: > cur += scnprintf(cur, end - cur, "Out-of-bounds %s at", get_access_type(r)); > + addr = arch_kfence_test_address(addr); > break; > case KFENCE_ERROR_UAF: > cur += scnprintf(cur, end - cur, "Use-after-free %s at", get_access_type(r)); > + addr = arch_kfence_test_address(addr); > break; > case KFENCE_ERROR_CORRUPTION: > cur += scnprintf(cur, end - cur, "Corrupted memory at"); > break; > case KFENCE_ERROR_INVALID: > cur += scnprintf(cur, end - cur, "Invalid %s at", get_access_type(r)); > + addr = arch_kfence_test_address(addr); > break; > case KFENCE_ERROR_INVALID_FREE: > cur += scnprintf(cur, end - cur, "Invalid free of"); > break; > } > > - cur += scnprintf(cur, end - cur, " 0x%p", (void *)r->addr); > + cur += scnprintf(cur, end - cur, " 0x%p", (void *)addr); > > spin_lock_irqsave(&observed.lock, flags); > if (!report_available()) >
On Wed, Jul 28, 2021 at 09:02PM +0200, Heiko Carstens wrote: > From: Sven Schnelle <svens@linux.ibm.com> > > s390 only reports the page address during a translation fault. > To make the kfence unit tests pass, add a function that might > be implemented by architectures to mask out address bits. > > Signed-off-by: Sven Schnelle <svens@linux.ibm.com> > Signed-off-by: Heiko Carstens <hca@linux.ibm.com> I noticed this breaks on x86 if CONFIG_KFENCE_KUNIT_TEST=m, because x86 conditionally declares some asm functions if !MODULE. I think the below is the simplest to fix, and if you agree, please carry it as a patch in this series before this patch. With the below, you can add to this patch: Reviewed-by: Marco Elver <elver@google.com> Thanks, -- Marco ------ >8 ------ From: Marco Elver <elver@google.com> Date: Wed, 28 Jul 2021 21:57:41 +0200 Subject: [PATCH] kfence, x86: only define helpers if !MODULE x86's <asm/tlbflush.h> only declares non-module accessible functions (such as flush_tlb_one_kernel) if !MODULE. In preparation of including <asm/kfence.h> from the KFENCE test module, only define the helpers if !MODULE to avoid breaking the build with CONFIG_KFENCE_KUNIT_TEST=m. Signed-off-by: Marco Elver <elver@google.com> --- arch/x86/include/asm/kfence.h | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/arch/x86/include/asm/kfence.h b/arch/x86/include/asm/kfence.h index 05b48b33baf0..ff5c7134a37a 100644 --- a/arch/x86/include/asm/kfence.h +++ b/arch/x86/include/asm/kfence.h @@ -8,6 +8,8 @@ #ifndef _ASM_X86_KFENCE_H #define _ASM_X86_KFENCE_H +#ifndef MODULE + #include <linux/bug.h> #include <linux/kfence.h> @@ -66,4 +68,6 @@ static inline bool kfence_protect_page(unsigned long addr, bool protect) return true; } +#endif /* !MODULE */ + #endif /* _ASM_X86_KFENCE_H */
On Thu, Jul 29, 2021 at 09:48:58AM +0200, Marco Elver wrote: > On Wed, Jul 28, 2021 at 09:02PM +0200, Heiko Carstens wrote: > > From: Sven Schnelle <svens@linux.ibm.com> > > > > s390 only reports the page address during a translation fault. > > To make the kfence unit tests pass, add a function that might > > be implemented by architectures to mask out address bits. > > > > Signed-off-by: Sven Schnelle <svens@linux.ibm.com> > > Signed-off-by: Heiko Carstens <hca@linux.ibm.com> > > I noticed this breaks on x86 if CONFIG_KFENCE_KUNIT_TEST=m, because x86 > conditionally declares some asm functions if !MODULE. > > I think the below is the simplest to fix, and if you agree, please carry > it as a patch in this series before this patch. Will do. > With the below, you can add to this patch: > > Reviewed-by: Marco Elver <elver@google.com> Done - Thank you! I silently assume this means also you have no objections if we carry this via the s390 tree for upstreaming.
On Thu, 29 Jul 2021 at 14:25, Heiko Carstens <hca@linux.ibm.com> wrote: > On Thu, Jul 29, 2021 at 09:48:58AM +0200, Marco Elver wrote: > > On Wed, Jul 28, 2021 at 09:02PM +0200, Heiko Carstens wrote: > > > From: Sven Schnelle <svens@linux.ibm.com> > > > > > > s390 only reports the page address during a translation fault. > > > To make the kfence unit tests pass, add a function that might > > > be implemented by architectures to mask out address bits. > > > > > > Signed-off-by: Sven Schnelle <svens@linux.ibm.com> > > > Signed-off-by: Heiko Carstens <hca@linux.ibm.com> > > > > I noticed this breaks on x86 if CONFIG_KFENCE_KUNIT_TEST=m, because x86 > > conditionally declares some asm functions if !MODULE. > > > > I think the below is the simplest to fix, and if you agree, please carry > > it as a patch in this series before this patch. > > Will do. > > > With the below, you can add to this patch: > > > > Reviewed-by: Marco Elver <elver@google.com> > > Done - Thank you! I silently assume this means also you have no > objections if we carry this via the s390 tree for upstreaming. I think that's reasonable. I'm not aware of any conflicts, nor am I expecting any for the upcoming cycle. Thanks, -- Marco
On Wed, Jul 28, 2021 at 9:03 PM Heiko Carstens <hca@linux.ibm.com> wrote: > > From: Sven Schnelle <svens@linux.ibm.com> > > s390 only reports the page address during a translation fault. > To make the kfence unit tests pass, add a function that might > be implemented by architectures to mask out address bits. > > Signed-off-by: Sven Schnelle <svens@linux.ibm.com> > Signed-off-by: Heiko Carstens <hca@linux.ibm.com> > --- > mm/kfence/kfence_test.c | 13 ++++++++++++- > 1 file changed, 12 insertions(+), 1 deletion(-) > > diff --git a/mm/kfence/kfence_test.c b/mm/kfence/kfence_test.c > index 942cbc16ad26..eb6307c199ea 100644 > --- a/mm/kfence/kfence_test.c > +++ b/mm/kfence/kfence_test.c > @@ -23,8 +23,15 @@ > #include <linux/tracepoint.h> > #include <trace/events/printk.h> > > +#include <asm/kfence.h> > + > #include "kfence.h" > > +/* May be overridden by <asm/kfence.h>. */ > +#ifndef arch_kfence_test_address > +#define arch_kfence_test_address(addr) (addr) > +#endif > + > /* Report as observed from console. */ > static struct { > spinlock_t lock; > @@ -82,6 +89,7 @@ static const char *get_access_type(const struct expect_report *r) > /* Check observed report matches information in @r. */ > static bool report_matches(const struct expect_report *r) > { > + unsigned long addr = (unsigned long)r->addr; > bool ret = false; > unsigned long flags; > typeof(observed.lines) expect; > @@ -131,22 +139,25 @@ static bool report_matches(const struct expect_report *r) > switch (r->type) { > case KFENCE_ERROR_OOB: > cur += scnprintf(cur, end - cur, "Out-of-bounds %s at", get_access_type(r)); > + addr = arch_kfence_test_address(addr); Can we normalize addr once before (or after) this switch? > break; > case KFENCE_ERROR_UAF: > cur += scnprintf(cur, end - cur, "Use-after-free %s at", get_access_type(r)); > + addr = arch_kfence_test_address(addr); > break; > case KFENCE_ERROR_CORRUPTION: > cur += scnprintf(cur, end - cur, "Corrupted memory at"); > break; > case KFENCE_ERROR_INVALID: > cur += scnprintf(cur, end - cur, "Invalid %s at", get_access_type(r)); > + addr = arch_kfence_test_address(addr); > break; > case KFENCE_ERROR_INVALID_FREE: > cur += scnprintf(cur, end - cur, "Invalid free of"); > break; > } > > - cur += scnprintf(cur, end - cur, " 0x%p", (void *)r->addr); > + cur += scnprintf(cur, end - cur, " 0x%p", (void *)addr); > > spin_lock_irqsave(&observed.lock, flags); > if (!report_available()) > -- > 2.25.1 >
Alexander Potapenko <glider@google.com> writes: > On Wed, Jul 28, 2021 at 9:03 PM Heiko Carstens <hca@linux.ibm.com> wrote: >> >> From: Sven Schnelle <svens@linux.ibm.com> >> >> s390 only reports the page address during a translation fault. >> To make the kfence unit tests pass, add a function that might >> be implemented by architectures to mask out address bits. >> >> Signed-off-by: Sven Schnelle <svens@linux.ibm.com> >> Signed-off-by: Heiko Carstens <hca@linux.ibm.com> >> --- >> mm/kfence/kfence_test.c | 13 ++++++++++++- >> 1 file changed, 12 insertions(+), 1 deletion(-) >> >> diff --git a/mm/kfence/kfence_test.c b/mm/kfence/kfence_test.c >> index 942cbc16ad26..eb6307c199ea 100644 >> --- a/mm/kfence/kfence_test.c >> +++ b/mm/kfence/kfence_test.c >> @@ -23,8 +23,15 @@ >> #include <linux/tracepoint.h> >> #include <trace/events/printk.h> >> >> +#include <asm/kfence.h> >> + >> #include "kfence.h" >> >> +/* May be overridden by <asm/kfence.h>. */ >> +#ifndef arch_kfence_test_address >> +#define arch_kfence_test_address(addr) (addr) >> +#endif >> + >> /* Report as observed from console. */ >> static struct { >> spinlock_t lock; >> @@ -82,6 +89,7 @@ static const char *get_access_type(const struct expect_report *r) >> /* Check observed report matches information in @r. */ >> static bool report_matches(const struct expect_report *r) >> { >> + unsigned long addr = (unsigned long)r->addr; >> bool ret = false; >> unsigned long flags; >> typeof(observed.lines) expect; >> @@ -131,22 +139,25 @@ static bool report_matches(const struct expect_report *r) >> switch (r->type) { >> case KFENCE_ERROR_OOB: >> cur += scnprintf(cur, end - cur, "Out-of-bounds %s at", get_access_type(r)); >> + addr = arch_kfence_test_address(addr); > > Can we normalize addr once before (or after) this switch? > I don't think so. When reporing corrupted memory or an invalid free the address is not generated by hardware but kfence itself, and therefore we would strip valid bits. >> break; >> case KFENCE_ERROR_UAF: >> cur += scnprintf(cur, end - cur, "Use-after-free %s at", get_access_type(r)); >> + addr = arch_kfence_test_address(addr); >> break; >> case KFENCE_ERROR_CORRUPTION: >> cur += scnprintf(cur, end - cur, "Corrupted memory at"); >> break; >> case KFENCE_ERROR_INVALID: >> cur += scnprintf(cur, end - cur, "Invalid %s at", get_access_type(r)); >> + addr = arch_kfence_test_address(addr); >> break; >> case KFENCE_ERROR_INVALID_FREE: >> cur += scnprintf(cur, end - cur, "Invalid free of"); >> break; >> } >> >> - cur += scnprintf(cur, end - cur, " 0x%p", (void *)r->addr); >> + cur += scnprintf(cur, end - cur, " 0x%p", (void *)addr); >> >> spin_lock_irqsave(&observed.lock, flags); >> if (!report_available()) >> -- >> 2.25.1 >>
On Thu, Jul 29, 2021 at 3:47 PM Sven Schnelle <svens@linux.ibm.com> wrote: > > Alexander Potapenko <glider@google.com> writes: > > > On Wed, Jul 28, 2021 at 9:03 PM Heiko Carstens <hca@linux.ibm.com> wrote: > >> > >> From: Sven Schnelle <svens@linux.ibm.com> > >> > >> s390 only reports the page address during a translation fault. > >> To make the kfence unit tests pass, add a function that might > >> be implemented by architectures to mask out address bits. > >> > >> Signed-off-by: Sven Schnelle <svens@linux.ibm.com> > >> Signed-off-by: Heiko Carstens <hca@linux.ibm.com> > >> --- > >> mm/kfence/kfence_test.c | 13 ++++++++++++- > >> 1 file changed, 12 insertions(+), 1 deletion(-) > >> > >> diff --git a/mm/kfence/kfence_test.c b/mm/kfence/kfence_test.c > >> index 942cbc16ad26..eb6307c199ea 100644 > >> --- a/mm/kfence/kfence_test.c > >> +++ b/mm/kfence/kfence_test.c > >> @@ -23,8 +23,15 @@ > >> #include <linux/tracepoint.h> > >> #include <trace/events/printk.h> > >> > >> +#include <asm/kfence.h> > >> + > >> #include "kfence.h" > >> > >> +/* May be overridden by <asm/kfence.h>. */ > >> +#ifndef arch_kfence_test_address > >> +#define arch_kfence_test_address(addr) (addr) > >> +#endif > >> + > >> /* Report as observed from console. */ > >> static struct { > >> spinlock_t lock; > >> @@ -82,6 +89,7 @@ static const char *get_access_type(const struct expect_report *r) > >> /* Check observed report matches information in @r. */ > >> static bool report_matches(const struct expect_report *r) > >> { > >> + unsigned long addr = (unsigned long)r->addr; > >> bool ret = false; > >> unsigned long flags; > >> typeof(observed.lines) expect; > >> @@ -131,22 +139,25 @@ static bool report_matches(const struct expect_report *r) > >> switch (r->type) { > >> case KFENCE_ERROR_OOB: > >> cur += scnprintf(cur, end - cur, "Out-of-bounds %s at", get_access_type(r)); > >> + addr = arch_kfence_test_address(addr); > > > > Can we normalize addr once before (or after) this switch? > > > > I don't think so. When reporing corrupted memory or an invalid free the > address is not generated by hardware but kfence itself, and therefore we > would strip valid bits. Ah, sorry, I missed that.
diff --git a/mm/kfence/kfence_test.c b/mm/kfence/kfence_test.c index 942cbc16ad26..eb6307c199ea 100644 --- a/mm/kfence/kfence_test.c +++ b/mm/kfence/kfence_test.c @@ -23,8 +23,15 @@ #include <linux/tracepoint.h> #include <trace/events/printk.h> +#include <asm/kfence.h> + #include "kfence.h" +/* May be overridden by <asm/kfence.h>. */ +#ifndef arch_kfence_test_address +#define arch_kfence_test_address(addr) (addr) +#endif + /* Report as observed from console. */ static struct { spinlock_t lock; @@ -82,6 +89,7 @@ static const char *get_access_type(const struct expect_report *r) /* Check observed report matches information in @r. */ static bool report_matches(const struct expect_report *r) { + unsigned long addr = (unsigned long)r->addr; bool ret = false; unsigned long flags; typeof(observed.lines) expect; @@ -131,22 +139,25 @@ static bool report_matches(const struct expect_report *r) switch (r->type) { case KFENCE_ERROR_OOB: cur += scnprintf(cur, end - cur, "Out-of-bounds %s at", get_access_type(r)); + addr = arch_kfence_test_address(addr); break; case KFENCE_ERROR_UAF: cur += scnprintf(cur, end - cur, "Use-after-free %s at", get_access_type(r)); + addr = arch_kfence_test_address(addr); break; case KFENCE_ERROR_CORRUPTION: cur += scnprintf(cur, end - cur, "Corrupted memory at"); break; case KFENCE_ERROR_INVALID: cur += scnprintf(cur, end - cur, "Invalid %s at", get_access_type(r)); + addr = arch_kfence_test_address(addr); break; case KFENCE_ERROR_INVALID_FREE: cur += scnprintf(cur, end - cur, "Invalid free of"); break; } - cur += scnprintf(cur, end - cur, " 0x%p", (void *)r->addr); + cur += scnprintf(cur, end - cur, " 0x%p", (void *)addr); spin_lock_irqsave(&observed.lock, flags); if (!report_available())