Hello,
On Fri, 7 Feb 2025 at 22:54, Rae Moar rmoar@google.com wrote:
On Sun, Feb 2, 2025 at 2:24 PM Sergio González Collado sergio.collado@gmail.com 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/20220802015052.10452-6-ojeda@kernel.org/ [2] https://lore.kernel.org/lkml/20240605032120.3179157-1-song@kernel.org/
Tested-by: Martin Rodriguez Reboredo yakoyoku@gmail.com Reviewed-by: Shuah Khan skhan@linuxfoundation.org Reviewed-by: Rae Moar rmoar@google.com Signed-off-by: Sergio González Collado sergio.collado@gmail.com 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%2F+DtvAVS@gmail.com/T/#m3ef0e12bb834d0...
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%2F+DtvAVS@gmail.com/T/#m3ef0e12bb834d0...
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_TESTSdepends on KUNIT && KPROBESdefault KUNIT_ALL_TESTShelpTests the longest symbol possibleIf 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/20221028194453.526899822@infradead.org/, 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!