| Message ID | dd36936c3d99582a623c8f01345f618ed4c036dd.1612884525.git.andreyknvl@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [mm] arm64: kasan: fix MTE symbols exports | expand |
On Tue, Feb 09, 2021 at 04:32:30PM +0100, Andrey Konovalov wrote: > diff --git a/arch/arm64/kernel/mte.c b/arch/arm64/kernel/mte.c > index a66c2806fc4d..788ef0c3a25e 100644 > --- a/arch/arm64/kernel/mte.c > +++ b/arch/arm64/kernel/mte.c > @@ -113,13 +113,17 @@ void mte_enable_kernel(void) > sysreg_clear_set(sctlr_el1, SCTLR_ELx_TCF_MASK, SCTLR_ELx_TCF_SYNC); > isb(); > } > +#if IS_ENABLED(CONFIG_KASAN_KUNIT_TEST) > EXPORT_SYMBOL_GPL(mte_enable_kernel); > +#endif > > void mte_set_report_once(bool state) > { > WRITE_ONCE(report_fault_once, state); > } > +#if IS_ENABLED(CONFIG_KASAN_KUNIT_TEST) > EXPORT_SYMBOL_GPL(mte_set_report_once); > +#endif Do we actually care about exporting them when KASAN_KUNIT_TEST=n? It looks weird to have these #ifdefs in the arch code. Either the arch-kasan API requires these symbols to be exported to modules or not. I'm not keen on such kasan internals trickling down into the arch code. If you don't want to export them in the KASAN_KUNIT_TEST=n case, add a wrapper in the kasan built-in code (e.g. kasan_test_enable_tagging, kasan_test_set_report_once) and conditionally compile them based on KASAN_KUNIT_TEST.
On Tue, 9 Feb 2021 17:02:56 +0000 Catalin Marinas <catalin.marinas@arm.com> wrote: > On Tue, Feb 09, 2021 at 04:32:30PM +0100, Andrey Konovalov wrote: > > diff --git a/arch/arm64/kernel/mte.c b/arch/arm64/kernel/mte.c > > index a66c2806fc4d..788ef0c3a25e 100644 > > --- a/arch/arm64/kernel/mte.c > > +++ b/arch/arm64/kernel/mte.c > > @@ -113,13 +113,17 @@ void mte_enable_kernel(void) > > sysreg_clear_set(sctlr_el1, SCTLR_ELx_TCF_MASK, SCTLR_ELx_TCF_SYNC); > > isb(); > > } > > +#if IS_ENABLED(CONFIG_KASAN_KUNIT_TEST) > > EXPORT_SYMBOL_GPL(mte_enable_kernel); > > +#endif > > > > void mte_set_report_once(bool state) > > { > > WRITE_ONCE(report_fault_once, state); > > } > > +#if IS_ENABLED(CONFIG_KASAN_KUNIT_TEST) > > EXPORT_SYMBOL_GPL(mte_set_report_once); > > +#endif > > Do we actually care about exporting them when KASAN_KUNIT_TEST=n? It > looks weird to have these #ifdefs in the arch code. Either the > arch-kasan API requires these symbols to be exported to modules or not. > I'm not keen on such kasan internals trickling down into the arch code. > > If you don't want to export them in the KASAN_KUNIT_TEST=n case, add a > wrapper in the kasan built-in code (e.g. kasan_test_enable_tagging, > kasan_test_set_report_once) and conditionally compile them based on > KASAN_KUNIT_TEST. In other words, the patch's changelog was poor! It told us what the patch does (which is often obvious from the code) but it failed to explain why the patch does what it does. The same goes for code comments, folks - please explain "why it does this" rather than "what it does".
On Tue, Feb 9, 2021 at 7:45 PM Andrew Morton <akpm@linux-foundation.org> wrote: > > On Tue, 9 Feb 2021 17:02:56 +0000 Catalin Marinas <catalin.marinas@arm.com> wrote: > > > On Tue, Feb 09, 2021 at 04:32:30PM +0100, Andrey Konovalov wrote: > > > diff --git a/arch/arm64/kernel/mte.c b/arch/arm64/kernel/mte.c > > > index a66c2806fc4d..788ef0c3a25e 100644 > > > --- a/arch/arm64/kernel/mte.c > > > +++ b/arch/arm64/kernel/mte.c > > > @@ -113,13 +113,17 @@ void mte_enable_kernel(void) > > > sysreg_clear_set(sctlr_el1, SCTLR_ELx_TCF_MASK, SCTLR_ELx_TCF_SYNC); > > > isb(); > > > } > > > +#if IS_ENABLED(CONFIG_KASAN_KUNIT_TEST) > > > EXPORT_SYMBOL_GPL(mte_enable_kernel); > > > +#endif > > > > > > void mte_set_report_once(bool state) > > > { > > > WRITE_ONCE(report_fault_once, state); > > > } > > > +#if IS_ENABLED(CONFIG_KASAN_KUNIT_TEST) > > > EXPORT_SYMBOL_GPL(mte_set_report_once); > > > +#endif > > > > Do we actually care about exporting them when KASAN_KUNIT_TEST=n? It > > looks weird to have these #ifdefs in the arch code. Either the > > arch-kasan API requires these symbols to be exported to modules or not. > > I'm not keen on such kasan internals trickling down into the arch code. Understood. > > If you don't want to export them in the KASAN_KUNIT_TEST=n case, add a > > wrapper in the kasan built-in code (e.g. kasan_test_enable_tagging, > > kasan_test_set_report_once) and conditionally compile them based on > > KASAN_KUNIT_TEST. This might be a better approach indeed. > In other words, the patch's changelog was poor! It told us what the > patch does (which is often obvious from the code) but it failed to > explain why the patch does what it does. > > The same goes for code comments, folks - please explain "why it does > this" rather than "what it does". I'm sorry, Andrew. Could you please drop the "arm64: kasan: export MTE symbols for KASAN tests" patch from the mm tree (but keep the rest of that series)? I'll post a separate patch with a fix. Thanks!
diff --git a/arch/arm64/kernel/mte.c b/arch/arm64/kernel/mte.c index a66c2806fc4d..788ef0c3a25e 100644 --- a/arch/arm64/kernel/mte.c +++ b/arch/arm64/kernel/mte.c @@ -113,13 +113,17 @@ void mte_enable_kernel(void) sysreg_clear_set(sctlr_el1, SCTLR_ELx_TCF_MASK, SCTLR_ELx_TCF_SYNC); isb(); } +#if IS_ENABLED(CONFIG_KASAN_KUNIT_TEST) EXPORT_SYMBOL_GPL(mte_enable_kernel); +#endif void mte_set_report_once(bool state) { WRITE_ONCE(report_fault_once, state); } +#if IS_ENABLED(CONFIG_KASAN_KUNIT_TEST) EXPORT_SYMBOL_GPL(mte_set_report_once); +#endif bool mte_report_once(void) {
Only export MTE symbols when KASAN-KUnit tests are enabled. Signed-off-by: Andrey Konovalov <andreyknvl@google.com> --- Andrew, please squash this into: "arm64: kasan: export MTE symbols for KASAN tests" --- arch/arm64/kernel/mte.c | 4 ++++ 1 file changed, 4 insertions(+)