Hello,
On Fri, 7 Feb 2025 at 22:54, Rae Moar <[email protected]> wrote:
>
> On Sun, Feb 2, 2025 at 2:24 PM Sergio González Collado
> <[email protected]> wrote:
> >
> > The longest length of a symbol (KSYM_NAME_LEN) was increased to 512
> > in the reference [1]. This patch adds kunit test suite to check the longest
> > symbol length. These tests verify that the longest symbol length defined
> > is supported.
> >
> > This test can also help other efforts for longer symbol length,
> > like [2].
> >
> > The test suite defines one symbol with the longest possible length.
> >
> > The first test verify that functions with names of the created
> > symbol, can be called or not.
> >
> > The second test, verify that the symbols are created (or
> > not) in the kernel symbol table.
> >
> > [1] https://lore.kernel.org/lkml/[email protected]/
> > [2] https://lore.kernel.org/lkml/[email protected]/
> >
> > Tested-by: Martin Rodriguez Reboredo <[email protected]>
> > Reviewed-by: Shuah Khan <[email protected]>
> > Reviewed-by: Rae Moar <[email protected]>
> > Signed-off-by: Sergio González Collado <[email protected]>
> > Link: https://github.com/Rust-for-Linux/linux/issues/504
>
> Hello!
>
> Thanks for fixing the typo and this new version! This patch still does
> not apply cleanly in the Makefile for me. Have you rebased it on the
> kselftest/kunit branch? I also have a few more questions that I just
> noticed.
>
> Thanks!
> -Rae
>
> > ---
> > V7 -> V8: typo fixed & rebased
> > ---
> > V6 -> V7: rebased
> > ---
> > V5 -> V6: remove tests with symbols of length KSYM_NAME_LEN+1
> > ---
> > V4 -> V5: fixed typo, added improved description
> > ---
> > V3 -> V4: add x86 mantainers, add new reference.
> > ---
> > V2 -> V3: updated base and added MODULE_DESCRIPTION() and MODULE_AUTHOR()
> > ---
> > V1 -> V2: corrected CI tests. Added fix proposed at [3]
> >
> > [3]
> > https://lore.kernel.org/lkml/y9es4ukl%[email protected]/T/#m3ef0e12bb834d01ed1ebdcae12ef5f2add342077
> >
> > The test execution should result in something like:
> > ```
> > [20:04:35] =============== longest-symbol (4 subtests) ================
> > [20:04:35] [PASSED] test_longest_symbol
> > [20:04:35] [PASSED] test_longest_symbol_kallsyms
> > [20:04:35] ================= [PASSED] longest-symbol ==================
> > [20:04:35] ============================================================
> > [20:04:35] Testing complete. Ran 4 tests: passed: 4
> > ```
> > ---
> > arch/x86/tools/insn_decoder_test.c | 3 +-
> > lib/Kconfig.debug | 9 ++++
> > lib/Makefile | 2 +
> > lib/longest_symbol_kunit.c | 84 ++++++++++++++++++++++++++++++
> > 4 files changed, 97 insertions(+), 1 deletion(-)
> > create mode 100644 lib/longest_symbol_kunit.c
> >
> > diff --git a/arch/x86/tools/insn_decoder_test.c
> > b/arch/x86/tools/insn_decoder_test.c
> > index 472540aeabc2..6c2986d2ad11 100644
> > --- a/arch/x86/tools/insn_decoder_test.c
> > +++ b/arch/x86/tools/insn_decoder_test.c
> > @@ -10,6 +10,7 @@
> > #include <assert.h>
> > #include <unistd.h>
> > #include <stdarg.h>
> > +#include <linux/kallsyms.h>
> >
> > #define unlikely(cond) (cond)
> >
> > @@ -106,7 +107,7 @@ static void parse_args(int argc, char **argv)
> > }
> > }
> >
> > -#define BUFSIZE 256
> > +#define BUFSIZE (256 + KSYM_NAME_LEN)
>
> I'm not too familiar with this test. I believe this would potentially
> make a symbol with a length that exceeds the KSYM_NAME_LEN. What is
> the intention for this line?
That will define how much space to write a symbol. I'm also not
familiar with that test, but I know had to be fixed:
https://lore.kernel.org/lkml/y9es4ukl%[email protected]/T/#m3ef0e12bb834d01ed1ebdcae12ef5f2add342077
>
> >
> > int main(int argc, char **argv)
> > {
> > diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
> > index 1af972a92d06..62d43aa9e8f0 100644
> > --- a/lib/Kconfig.debug
> > +++ b/lib/Kconfig.debug
> > @@ -2838,6 +2838,15 @@ config FORTIFY_KUNIT_TEST
> > by the str*() and mem*() family of functions. For testing runtime
> > traps of FORTIFY_SOURCE, see LKDTM's "FORTIFY_*" tests.
> >
> > +config LONGEST_SYM_KUNIT_TEST
> > + tristate "Test the longest symbol possible" if !KUNIT_ALL_TESTS
> > + depends on KUNIT && KPROBES
> > + default KUNIT_ALL_TESTS
> > + help
> > + Tests the longest symbol possible
> > +
> > + If unsure, say N.
> > +
> > config HW_BREAKPOINT_KUNIT_TEST
> > bool "Test hw_breakpoint constraints accounting" if !KUNIT_ALL_TESTS
> > depends on HAVE_HW_BREAKPOINT
> > diff --git a/lib/Makefile b/lib/Makefile
> > index d5cfc7afbbb8..e8fec9defec2 100644
> > --- a/lib/Makefile
> > +++ b/lib/Makefile
> > @@ -393,6 +393,8 @@ obj-$(CONFIG_FORTIFY_KUNIT_TEST) += fortify_kunit.o
> > obj-$(CONFIG_CRC_KUNIT_TEST) += crc_kunit.o
> > obj-$(CONFIG_SIPHASH_KUNIT_TEST) += siphash_kunit.o
> > obj-$(CONFIG_USERCOPY_KUNIT_TEST) += usercopy_kunit.o
> > +obj-$(CONFIG_LONGEST_SYM_KUNIT_TEST) += longest_symbol_kunit.o
> > +CFLAGS_longest_symbol_kunit.o += $(call cc-disable-warning,
> > missing-prototypes)
> >
> > obj-$(CONFIG_GENERIC_LIB_DEVMEM_IS_ALLOWED) += devmem_is_allowed.o
>
> These are the lines that are causing the patch to not apply cleanly.
> The change list that applies cleanly for me is:
>
> obj-$(CONFIG_FORTIFY_KUNIT_TEST) += fortify_kunit.o
> obj-$(CONFIG_SIPHASH_KUNIT_TEST) += siphash_kunit.o
> obj-$(CONFIG_USERCOPY_KUNIT_TEST) += usercopy_kunit.o
> obj-$(CONFIG_CRC16_KUNIT_TEST) += crc16_kunit.o
> +obj-$(CONFIG_LONGEST_SYM_KUNIT_TEST) += longest_symbol_kunit.o
> +CFLAGS_longest_symbol_kunit.o += $(call cc-disable-warning,
> missing-prototypes)
>
> obj-$(CONFIG_GENERIC_LIB_DEVMEM_IS_ALLOWED) += devmem_is_allowed.o
>
I will create a new rebased version of the patch.
> >
> > diff --git a/lib/longest_symbol_kunit.c b/lib/longest_symbol_kunit.c
> > new file mode 100644
> > index 000000000000..2a2dd1151097
> > --- /dev/null
> > +++ b/lib/longest_symbol_kunit.c
> > @@ -0,0 +1,84 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Test the longest symbol length. Execute with:
> > + * ./tools/testing/kunit/kunit.py run longest-symbol
> > + * --arch=x86_64 --kconfig_add CONFIG_KPROBES=y --kconfig_add
> > CONFIG_MODULES=y
> > + * --kconfig_add CONFIG_RETPOLINE=n --kconfig_add CONFIG_CFI_CLANG=n
> > + * --kconfig_add CONFIG_MITIGATION_RETPOLINE=n
> > + */
> > +
> > +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
>
> I don't believe you use this macro. Could probably be deleted.
>
This macro should work out of the box by means of the pr_*() macros:
https://docs.kernel.org/core-api/printk-basics.html#c.pr_fmt
>
> > +
> > +#include <kunit/test.h>
> > +#include <linux/stringify.h>
> > +#include <linux/kprobes.h>
> > +#include <linux/kallsyms.h>
> > +
> > +#define DI(name) s##name##name
> > +#define DDI(name) DI(n##name##name)
> > +#define DDDI(name) DDI(n##name##name)
> > +#define DDDDI(name) DDDI(n##name##name)
> > +#define DDDDDI(name) DDDDI(n##name##name)
> > +
> > +#define PLUS1(name) __PASTE(name, e)
>
> I don't think you use this anymore with the new changes. Can probably
> be deleted.
>
Indeed, I totally overlooked that.
>
> > +
> > +/*Generate a symbol whose name length is 511 */
> > +#define LONGEST_SYM_NAME DDDDDI(g1h2i3j4k5l6m7n)
> > +
> > +#define RETURN_LONGEST_SYM 0xAAAAA
> > +
> > +noinline int LONGEST_SYM_NAME(void);
> > +noinline int LONGEST_SYM_NAME(void)
> > +{
> > + return RETURN_LONGEST_SYM;
> > +}
> > +
> > +_Static_assert(sizeof(__stringify(LONGEST_SYM_NAME)) == KSYM_NAME_LEN,
> > +"Incorrect symbol length found. Expected KSYM_NAME_LEN: "
> > +__stringify(KSYM_NAME) ", but found: "
> > +__stringify(sizeof(LONGEST_SYM_NAME)));
>
> Should this error return __stringify(KSYM_NAME_LEN) instead of
> __stringify(KSYM_NAME) to give the maximum length?
>
It is done on purpose to show what the actual symbol is. Under some
conditions the names can incorporate
prefix symbols, for example:
https://lore.kernel.org/lkml/[email protected]/,
if this happens it will be easier to spot what is going on.
>
> Also, I get an error because the length of LONGEST_SYM_NAME is 512.
> The error is produced by this code:
> https://elixir.bootlin.com/linux/v6.13.1/source/scripts/kallsyms.c#L152
> and alerts if the symbol length is >= to KSYM_NAME_LEN. That is fine
> as long as that is the intention of this test to produce a warning. Or
> should this warning change to be "> KSYM_NAME_LEN" if usage of symbols
> that are the maximum length is allowed?
>
Indeed that is expected, as the limit is reached. I think it may not
be really needed to update the condition to "> KSYM_NAME_LEN".
Thanks a lot for your remarks.
Cheers!
