* [RFC] minor kfence patches
@ 2021-05-14 9:21 Sven Schnelle
2021-05-14 9:21 ` [PATCH 1/2] kfence: add function to mask address bits Sven Schnelle
` (2 more replies)
0 siblings, 3 replies; 10+ messages in thread
From: Sven Schnelle @ 2021-05-14 9:21 UTC (permalink / raw)
To: Marco Elver; +Cc: linux-kernel
i'm currently looking into adding support for KFENCE to the s390
architecture. So far everything is straightforward, and i get the
kfence testsuite to pass, which is good! :)
One minor thing i encountered is that for a translation exception,
s390 only reports the page address, but not the complete address. I
worked around that by adding a function to kfence which allows to mask
out certain bits during unit testing. I wonder whether that should be a
weak function that can be implemented by architectures if required, some
kconfig option, or some other way?
The other thing is that s390 (and some other architectures) has different
address spaces for kernel and user space, so the decision whether an
address belongs to user or kernel space cannot be made by just looking
at the address. I added a small if (user_mode(regs)) check to
kfence_handle_page_fault(). But this could of also be done in the
architecture specific code.
What do you think?
Thanks,
Sven
^ permalink raw reply [flat|nested] 10+ messages in thread* [PATCH 1/2] kfence: add function to mask address bits 2021-05-14 9:21 [RFC] minor kfence patches Sven Schnelle @ 2021-05-14 9:21 ` Sven Schnelle 2021-05-14 10:54 ` Marco Elver 2021-05-14 9:21 ` [PATCH 2/2] kfence: only handle kernel mode faults Sven Schnelle 2021-05-14 10:56 ` [RFC] minor kfence patches Marco Elver 2 siblings, 1 reply; 10+ messages in thread From: Sven Schnelle @ 2021-05-14 9:21 UTC (permalink / raw) To: Marco Elver; +Cc: linux-kernel, Sven Schnelle 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> --- include/linux/kfence.h | 1 + mm/kfence/core.c | 5 +++++ mm/kfence/kfence_test.c | 6 +++++- 3 files changed, 11 insertions(+), 1 deletion(-) diff --git a/include/linux/kfence.h b/include/linux/kfence.h index a70d1ea03532..2e15f4c4ee95 100644 --- a/include/linux/kfence.h +++ b/include/linux/kfence.h @@ -199,6 +199,7 @@ static __always_inline __must_check bool kfence_free(void *addr) * present, so that the kernel can proceed. */ bool __must_check kfence_handle_page_fault(unsigned long addr, bool is_write, struct pt_regs *regs); +unsigned long kfence_arch_mask_addr(unsigned long addr); #else /* CONFIG_KFENCE */ diff --git a/mm/kfence/core.c b/mm/kfence/core.c index e18fbbd5d9b4..bc15e3cb71d5 100644 --- a/mm/kfence/core.c +++ b/mm/kfence/core.c @@ -50,6 +50,11 @@ static unsigned long kfence_sample_interval __read_mostly = CONFIG_KFENCE_SAMPLE #endif #define MODULE_PARAM_PREFIX "kfence." +unsigned long __weak kfence_arch_mask_addr(unsigned long addr) +{ + return addr; +} + static int param_set_sample_interval(const char *val, const struct kernel_param *kp) { unsigned long num; diff --git a/mm/kfence/kfence_test.c b/mm/kfence/kfence_test.c index 4acf4251ee04..9ec572991014 100644 --- a/mm/kfence/kfence_test.c +++ b/mm/kfence/kfence_test.c @@ -82,6 +82,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 +132,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 = kfence_arch_mask_addr(addr); break; case KFENCE_ERROR_UAF: cur += scnprintf(cur, end - cur, "Use-after-free %s at", get_access_type(r)); + addr = kfence_arch_mask_addr(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 = kfence_arch_mask_addr(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 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] kfence: add function to mask address bits 2021-05-14 9:21 ` [PATCH 1/2] kfence: add function to mask address bits Sven Schnelle @ 2021-05-14 10:54 ` Marco Elver 2021-05-14 11:03 ` Sven Schnelle 0 siblings, 1 reply; 10+ messages in thread From: Marco Elver @ 2021-05-14 10:54 UTC (permalink / raw) To: Sven Schnelle; +Cc: LKML, kasan-dev, Alexander Potapenko Thanks for trying to get KFENCE on s390. On Fri, 14 May 2021 at 11:22, Sven Schnelle <svens@linux.ibm.com> wrote: > > 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. The point of the test is to test the expected behaviour. And s390 certainly isn't behaving as we'd expect, because we really ought to see the precise address to facilitate debugging. Granted, by default KFENCE prints hashed pointers, but with no_hash_pointers we still want to see the precise address. Is there any way to make s390 give us precise addresses? Of course if you say this deviation is reasonable, see my suggestions below. > Signed-off-by: Sven Schnelle <svens@linux.ibm.com> > --- > include/linux/kfence.h | 1 + > mm/kfence/core.c | 5 +++++ > mm/kfence/kfence_test.c | 6 +++++- > 3 files changed, 11 insertions(+), 1 deletion(-) > > diff --git a/include/linux/kfence.h b/include/linux/kfence.h > index a70d1ea03532..2e15f4c4ee95 100644 > --- a/include/linux/kfence.h > +++ b/include/linux/kfence.h > @@ -199,6 +199,7 @@ static __always_inline __must_check bool kfence_free(void *addr) > * present, so that the kernel can proceed. > */ > bool __must_check kfence_handle_page_fault(unsigned long addr, bool is_write, struct pt_regs *regs); > +unsigned long kfence_arch_mask_addr(unsigned long addr); I think this should not be part of the public interface, as commented below. > #else /* CONFIG_KFENCE */ > > diff --git a/mm/kfence/core.c b/mm/kfence/core.c > index e18fbbd5d9b4..bc15e3cb71d5 100644 > --- a/mm/kfence/core.c > +++ b/mm/kfence/core.c > @@ -50,6 +50,11 @@ static unsigned long kfence_sample_interval __read_mostly = CONFIG_KFENCE_SAMPLE > #endif > #define MODULE_PARAM_PREFIX "kfence." > > +unsigned long __weak kfence_arch_mask_addr(unsigned long addr) > +{ > + return addr; > +} I don't think this belongs here, because it's test-specific, furthermore if possible we'd like to put all arch-specific code into <asm/kfence.h> (whether or not your arch will have 'static inline' functions only, like x86 and arm64, or not is up to you). Because I don't see this function being terribly complex, also let's just make it a macro. Then in kfence_test.c, we can have: #ifndef kfence_test_mask_address #define kfence_test_mask_address(addr) (addr) #endif and then have it include <asm/kfence.h>. And in your <asm/kfence.h> you can simply say: #define kfence_test_mask_address(addr) (.........) It also avoids having to export kfence_test_mask_address, because kfence_test can be built as a module. > static int param_set_sample_interval(const char *val, const struct kernel_param *kp) > { > unsigned long num; > diff --git a/mm/kfence/kfence_test.c b/mm/kfence/kfence_test.c > index 4acf4251ee04..9ec572991014 100644 > --- a/mm/kfence/kfence_test.c > +++ b/mm/kfence/kfence_test.c > @@ -82,6 +82,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 +132,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 = kfence_arch_mask_addr(addr); > break; > case KFENCE_ERROR_UAF: > cur += scnprintf(cur, end - cur, "Use-after-free %s at", get_access_type(r)); > + addr = kfence_arch_mask_addr(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 = kfence_arch_mask_addr(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); The rest here looks reasonable if you think there's no way to get s390 to give us precise addresses. Thanks, -- Marco ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] kfence: add function to mask address bits 2021-05-14 10:54 ` Marco Elver @ 2021-05-14 11:03 ` Sven Schnelle 2021-05-14 11:23 ` Marco Elver 0 siblings, 1 reply; 10+ messages in thread From: Sven Schnelle @ 2021-05-14 11:03 UTC (permalink / raw) To: Marco Elver; +Cc: LKML, kasan-dev, Alexander Potapenko Marco Elver <elver@google.com> writes: >> diff --git a/mm/kfence/core.c b/mm/kfence/core.c >> index e18fbbd5d9b4..bc15e3cb71d5 100644 >> --- a/mm/kfence/core.c >> +++ b/mm/kfence/core.c >> @@ -50,6 +50,11 @@ static unsigned long kfence_sample_interval __read_mostly = CONFIG_KFENCE_SAMPLE >> #endif >> #define MODULE_PARAM_PREFIX "kfence." >> >> +unsigned long __weak kfence_arch_mask_addr(unsigned long addr) >> +{ >> + return addr; >> +} > > I don't think this belongs here, because it's test-specific, > furthermore if possible we'd like to put all arch-specific code into > <asm/kfence.h> (whether or not your arch will have 'static inline' > functions only, like x86 and arm64, or not is up to you). > > Because I don't see this function being terribly complex, also let's > just make it a macro. > > Then in kfence_test.c, we can have: > > #ifndef kfence_test_mask_address > #define kfence_test_mask_address(addr) (addr) > #endif > > and then have it include <asm/kfence.h>. And in your <asm/kfence.h> > you can simply say: > > #define kfence_test_mask_address(addr) (.........) > > It also avoids having to export kfence_test_mask_address, because > kfence_test can be built as a module. Ok, i'll change my patch accordingly. Thanks! Sven ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] kfence: add function to mask address bits 2021-05-14 11:03 ` Sven Schnelle @ 2021-05-14 11:23 ` Marco Elver 0 siblings, 0 replies; 10+ messages in thread From: Marco Elver @ 2021-05-14 11:23 UTC (permalink / raw) To: Sven Schnelle; +Cc: LKML, kasan-dev, Alexander Potapenko On Fri, 14 May 2021 at 13:03, Sven Schnelle <svens@linux.ibm.com> wrote: > > Marco Elver <elver@google.com> writes: > > >> diff --git a/mm/kfence/core.c b/mm/kfence/core.c > >> index e18fbbd5d9b4..bc15e3cb71d5 100644 > >> --- a/mm/kfence/core.c > >> +++ b/mm/kfence/core.c > >> @@ -50,6 +50,11 @@ static unsigned long kfence_sample_interval __read_mostly = CONFIG_KFENCE_SAMPLE > >> #endif > >> #define MODULE_PARAM_PREFIX "kfence." > >> > >> +unsigned long __weak kfence_arch_mask_addr(unsigned long addr) > >> +{ > >> + return addr; > >> +} > > > > I don't think this belongs here, because it's test-specific, > > furthermore if possible we'd like to put all arch-specific code into > > <asm/kfence.h> (whether or not your arch will have 'static inline' > > functions only, like x86 and arm64, or not is up to you). > > > > Because I don't see this function being terribly complex, also let's > > just make it a macro. > > > > Then in kfence_test.c, we can have: > > > > #ifndef kfence_test_mask_address > > #define kfence_test_mask_address(addr) (addr) > > #endif > > > > and then have it include <asm/kfence.h>. And in your <asm/kfence.h> > > you can simply say: > > > > #define kfence_test_mask_address(addr) (.........) > > > > It also avoids having to export kfence_test_mask_address, because > > kfence_test can be built as a module. > > Ok, i'll change my patch accordingly. Thanks! Sounds good. Also please add a brief comment on top of the "kfence_test_mask_address" part in kfence_test, like "/* May be overridden by <asm/kfence.h>. */" -- we have something similar in mm/kfence/report.c. Also, I think we want to call the macro "arch_kfence_test_address" -- the "mask" part is very much arch-dependent, and might not even be a mask on some other weird hypothetical architecture. Thanks, -- Marco ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 2/2] kfence: only handle kernel mode faults 2021-05-14 9:21 [RFC] minor kfence patches Sven Schnelle 2021-05-14 9:21 ` [PATCH 1/2] kfence: add function to mask address bits Sven Schnelle @ 2021-05-14 9:21 ` Sven Schnelle 2021-05-14 10:52 ` Marco Elver 2021-05-14 10:56 ` [RFC] minor kfence patches Marco Elver 2 siblings, 1 reply; 10+ messages in thread From: Sven Schnelle @ 2021-05-14 9:21 UTC (permalink / raw) To: Marco Elver; +Cc: linux-kernel, Sven Schnelle Signed-off-by: Sven Schnelle <svens@linux.ibm.com> --- mm/kfence/core.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/mm/kfence/core.c b/mm/kfence/core.c index bc15e3cb71d5..161df492750c 100644 --- a/mm/kfence/core.c +++ b/mm/kfence/core.c @@ -813,6 +813,9 @@ bool kfence_handle_page_fault(unsigned long addr, bool is_write, struct pt_regs enum kfence_error_type error_type; unsigned long flags; + if (user_mode(regs)) + return false; + if (!is_kfence_address((void *)addr)) return false; -- 2.25.1 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH 2/2] kfence: only handle kernel mode faults 2021-05-14 9:21 ` [PATCH 2/2] kfence: only handle kernel mode faults Sven Schnelle @ 2021-05-14 10:52 ` Marco Elver 2021-05-14 10:55 ` Sven Schnelle 0 siblings, 1 reply; 10+ messages in thread From: Marco Elver @ 2021-05-14 10:52 UTC (permalink / raw) To: Sven Schnelle; +Cc: LKML On Fri, 14 May 2021 at 11:22, Sven Schnelle <svens@linux.ibm.com> wrote: > > Signed-off-by: Sven Schnelle <svens@linux.ibm.com> > --- > mm/kfence/core.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/mm/kfence/core.c b/mm/kfence/core.c > index bc15e3cb71d5..161df492750c 100644 > --- a/mm/kfence/core.c > +++ b/mm/kfence/core.c > @@ -813,6 +813,9 @@ bool kfence_handle_page_fault(unsigned long addr, bool is_write, struct pt_regs > enum kfence_error_type error_type; > unsigned long flags; > > + if (user_mode(regs)) > + return false; > + I don't think it's required on all architectures, correct? If so, I think this should be part of the arch-specific code, i.e. just do "if (user_mode(regs) && kfence_handle_page_fault(...))" or similar. Because otherwise we'll wonder in future why we ever needed this, and e.g. determine it's useless and remove it again. ;-) Either that, or a comment. But I'd prefer to just keep it in the arch-specific code if required, because it seems to be the exception rather than the norm. Thanks, -- Marco ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 2/2] kfence: only handle kernel mode faults 2021-05-14 10:52 ` Marco Elver @ 2021-05-14 10:55 ` Sven Schnelle 2021-05-14 10:59 ` Marco Elver 0 siblings, 1 reply; 10+ messages in thread From: Sven Schnelle @ 2021-05-14 10:55 UTC (permalink / raw) To: Marco Elver; +Cc: LKML Marco Elver <elver@google.com> writes: > On Fri, 14 May 2021 at 11:22, Sven Schnelle <svens@linux.ibm.com> wrote: >> >> Signed-off-by: Sven Schnelle <svens@linux.ibm.com> >> --- >> mm/kfence/core.c | 3 +++ >> 1 file changed, 3 insertions(+) >> >> diff --git a/mm/kfence/core.c b/mm/kfence/core.c >> index bc15e3cb71d5..161df492750c 100644 >> --- a/mm/kfence/core.c >> +++ b/mm/kfence/core.c >> @@ -813,6 +813,9 @@ bool kfence_handle_page_fault(unsigned long addr, bool is_write, struct pt_regs >> enum kfence_error_type error_type; >> unsigned long flags; >> >> + if (user_mode(regs)) >> + return false; >> + > > I don't think it's required on all architectures, correct? If so, I > think this should be part of the arch-specific code, i.e. just do "if > (user_mode(regs) && kfence_handle_page_fault(...))" or similar. > Because otherwise we'll wonder in future why we ever needed this, and > e.g. determine it's useless and remove it again. ;-) Either that, or a > comment. But I'd prefer to just keep it in the arch-specific code if > required, because it seems to be the exception rather than the norm. Ok, that's fine, i add it to our code then. Thanks Sven ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 2/2] kfence: only handle kernel mode faults 2021-05-14 10:55 ` Sven Schnelle @ 2021-05-14 10:59 ` Marco Elver 0 siblings, 0 replies; 10+ messages in thread From: Marco Elver @ 2021-05-14 10:59 UTC (permalink / raw) To: Sven Schnelle; +Cc: LKML On Fri, 14 May 2021 at 12:55, Sven Schnelle <svens@linux.ibm.com> wrote: > > Marco Elver <elver@google.com> writes: > > > On Fri, 14 May 2021 at 11:22, Sven Schnelle <svens@linux.ibm.com> wrote: > >> > >> Signed-off-by: Sven Schnelle <svens@linux.ibm.com> > >> --- > >> mm/kfence/core.c | 3 +++ > >> 1 file changed, 3 insertions(+) > >> > >> diff --git a/mm/kfence/core.c b/mm/kfence/core.c > >> index bc15e3cb71d5..161df492750c 100644 > >> --- a/mm/kfence/core.c > >> +++ b/mm/kfence/core.c > >> @@ -813,6 +813,9 @@ bool kfence_handle_page_fault(unsigned long addr, bool is_write, struct pt_regs > >> enum kfence_error_type error_type; > >> unsigned long flags; > >> > >> + if (user_mode(regs)) > >> + return false; > >> + > > > > I don't think it's required on all architectures, correct? If so, I > > think this should be part of the arch-specific code, i.e. just do "if > > (user_mode(regs) && kfence_handle_page_fault(...))" or similar. Ah, this should have obviously been "if (!user_mode(regs) && kfence_handle_page_fault(...))", but I think you would have caught that anyway. ;-) > > Because otherwise we'll wonder in future why we ever needed this, and > > e.g. determine it's useless and remove it again. ;-) Either that, or a > > comment. But I'd prefer to just keep it in the arch-specific code if > > required, because it seems to be the exception rather than the norm. > > Ok, that's fine, i add it to our code then. Sounds good. Thanks, -- Marco > Thanks > Sven ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [RFC] minor kfence patches 2021-05-14 9:21 [RFC] minor kfence patches Sven Schnelle 2021-05-14 9:21 ` [PATCH 1/2] kfence: add function to mask address bits Sven Schnelle 2021-05-14 9:21 ` [PATCH 2/2] kfence: only handle kernel mode faults Sven Schnelle @ 2021-05-14 10:56 ` Marco Elver 2 siblings, 0 replies; 10+ messages in thread From: Marco Elver @ 2021-05-14 10:56 UTC (permalink / raw) To: Sven Schnelle, Alexander Potapenko, kasan-dev; +Cc: LKML On Fri, 14 May 2021 at 11:21, Sven Schnelle <svens@linux.ibm.com> wrote: > > i'm currently looking into adding support for KFENCE to the s390 > architecture. So far everything is straightforward, and i get the > kfence testsuite to pass, which is good! :) Nice to see KFENCE being added to more architectures. > One minor thing i encountered is that for a translation exception, > s390 only reports the page address, but not the complete address. I > worked around that by adding a function to kfence which allows to mask > out certain bits during unit testing. I wonder whether that should be a > weak function that can be implemented by architectures if required, some > kconfig option, or some other way? I've commented on the other patches. Thanks, -- Marco > The other thing is that s390 (and some other architectures) has different > address spaces for kernel and user space, so the decision whether an > address belongs to user or kernel space cannot be made by just looking > at the address. I added a small if (user_mode(regs)) check to > kfence_handle_page_fault(). But this could of also be done in the > architecture specific code. > > What do you think? > > Thanks, > Sven > > ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2021-05-14 11:23 UTC | newest] Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-05-14 9:21 [RFC] minor kfence patches Sven Schnelle 2021-05-14 9:21 ` [PATCH 1/2] kfence: add function to mask address bits Sven Schnelle 2021-05-14 10:54 ` Marco Elver 2021-05-14 11:03 ` Sven Schnelle 2021-05-14 11:23 ` Marco Elver 2021-05-14 9:21 ` [PATCH 2/2] kfence: only handle kernel mode faults Sven Schnelle 2021-05-14 10:52 ` Marco Elver 2021-05-14 10:55 ` Sven Schnelle 2021-05-14 10:59 ` Marco Elver 2021-05-14 10:56 ` [RFC] minor kfence patches Marco Elver
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).