| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <95a7d98c-b227-7929-b833-f6adc3b7e3ca@huaweicloud.com>
Date: Fri, 25 Aug 2023 15:19:10 +0800
From: "Leizhen (ThunderTown)" <thunder.leizhen@...weicloud.com>
To: Yonghong Song <yonghong.song@...ux.dev>,
Kees Cook <keescook@...omium.org>,
Nick Desaulniers <ndesaulniers@...gle.com>,
Petr Mladek <pmladek@...e.com>, Song Liu <song@...nel.org>,
Steven Rostedt <rostedt@...dmis.org>
Cc: Fangrui Song <maskray@...gle.com>, kernel-team@...com,
Leizhen <thunder.leizhen@...wei.com>,
linux-kernel@...r.kernel.org, llvm@...ts.linux.dev,
kernel test robot <oliver.sang@...el.com>
Subject: Re: [PATCH] kallsyms: Fix kallsyms_selftest failure
On 2023/8/25 11:46, Yonghong Song wrote:
> Kernel test robot reported a kallsyms_test failure when clang lto is
> enabled (thin or full) and CONFIG_KALLSYMS_SELFTEST is also enabled.
> I can reproduce in my local environment with the following error message
> with thin lto:
> [ 1.877897] kallsyms_selftest: Test for 1750th symbol failed: (tsc_cs_mark_unstable) addr=ffffffff81038090
> [ 1.877901] kallsyms_selftest: abort
>
> It appears that commit 8cc32a9bbf29 ("kallsyms: strip LTO-only suffixes
> from promoted global functions") caused the failure. Commit 8cc32a9bbf29
> changed cleanup_symbol_name() based on ".llvm." instead of '.' where
> ".llvm." is appended to a before-lto-optimization local symbol name.
> We need to propagate such knowledge in kallsyms_selftest.c as well.
>
> Further more, compare_symbol_name() in kallsyms.c needs change as well.
> In scripts/kallsyms.c, kallsyms_names and kallsyms_seqs_of_names are used
> to record symbol names themselves and index to symbol names respectively.
> For example:
> kallsyms_names:
> ...
> __amd_smn_rw._entry <== seq 1000
> __amd_smn_rw._entry.5 <== seq 1001
> __amd_smn_rw.llvm.<hash> <== seq 1002
> ...
>
> kallsyms_seqs_of_names are sorted based on cleanup_symbol_name() through, so
> the order in kallsyms_seqs_of_names actually has
>
> index 1000: seq 1002 <== __amd_smn_rw.llvm.<hash> (actual symbol comparison using '__amd_smn_rw')
> index 1001: seq 1000 <== __amd_smn_rw._entry
> index 1002: seq 1001 <== __amd_smn_rw._entry.5
>
> Let us say at a particular point, at index 1000, symbol '__amd_smn_rw.llvm.<hash>'
> is comparing to '__amd_smn_rw._entry' where '__amd_smn_rw._entry' is the one to
> search e.g., with function kallsyms_on_each_match_symbol(). The current implementation
> will find out '__amd_smn_rw._entry' is less than '__amd_smn_rw.llvm.<hash>' and
> then continue to search e.g., index 999 and never found a match although the actual
> index 1001 is a match.
>
> To fix this issue, let us do cleanup_symbol_name() first and then do comparison.
> In the above case, comparing '__amd_smn_rw' vs '__amd_smn_rw._entry' and
> '__amd_smn_rw._entry' being greater than '__amd_smn_rw', the next comparison will
> be > index 1000 and eventually index 1001 will be hit an a match is found.
>
> For any symbols not having '.llvm.' substr, there is no functionality change
> for compare_symbol_name().
Reviewed-by: Zhen Lei <thunder.leizhen@...wei.com>
>
> Fixes: 8cc32a9bbf29 ("kallsyms: strip LTO-only suffixes from promoted global functions")
> Reported-by: kernel test robot <oliver.sang@...el.com>
> Closes: https://lore.kernel.org/oe-lkp/202308232200.1c932a90-oliver.sang@intel.com
> Signed-off-by: Yonghong Song <yonghong.song@...ux.dev>
> ---
> kernel/kallsyms.c | 17 +++++++----------
> kernel/kallsyms_selftest.c | 23 +----------------------
> 2 files changed, 8 insertions(+), 32 deletions(-)
>
> diff --git a/kernel/kallsyms.c b/kernel/kallsyms.c
> index 016d997131d4..e12d26c10dba 100644
> --- a/kernel/kallsyms.c
> +++ b/kernel/kallsyms.c
> @@ -188,16 +188,13 @@ static bool cleanup_symbol_name(char *s)
>
> static int compare_symbol_name(const char *name, char *namebuf)
> {
> - int ret;
> -
> - ret = strcmp(name, namebuf);
> - if (!ret)
> - return ret;
> -
> - if (cleanup_symbol_name(namebuf) && !strcmp(name, namebuf))
> - return 0;
> -
> - return ret;
> + /* The kallsyms_seqs_of_names is sorted based on names after
> + * cleanup_symbol_name() (see scripts/kallsyms.c) if clang lto is enabled.
> + * To ensure correct bisection in kallsyms_lookup_names(), do
> + * cleanup_symbol_name(namebuf) before comparing name and namebuf.
> + */
> + cleanup_symbol_name(namebuf);
> + return strcmp(name, namebuf);
> }
>
> static unsigned int get_symbol_seq(int index)
> diff --git a/kernel/kallsyms_selftest.c b/kernel/kallsyms_selftest.c
> index a2e3745d15c4..e05ddc33a752 100644
> --- a/kernel/kallsyms_selftest.c
> +++ b/kernel/kallsyms_selftest.c
> @@ -196,7 +196,7 @@ static bool match_cleanup_name(const char *s, const char *name)
> if (!IS_ENABLED(CONFIG_LTO_CLANG))
> return false;
>
> - p = strchr(s, '.');
> + p = strstr(s, ".llvm.");
> if (!p)
> return false;
>
> @@ -344,27 +344,6 @@ static int test_kallsyms_basic_function(void)
> goto failed;
> }
>
> - /*
> - * The first '.' may be the initial letter, in which case the
> - * entire symbol name will be truncated to an empty string in
> - * cleanup_symbol_name(). Do not test these symbols.
> - *
> - * For example:
> - * cat /proc/kallsyms | awk '{print $3}' | grep -E "^\." | head
> - * .E_read_words
> - * .E_leading_bytes
> - * .E_trailing_bytes
> - * .E_write_words
> - * .E_copy
> - * .str.292.llvm.12122243386960820698
> - * .str.24.llvm.12122243386960820698
> - * .str.29.llvm.12122243386960820698
> - * .str.75.llvm.12122243386960820698
> - * .str.99.llvm.12122243386960820698
> - */
> - if (IS_ENABLED(CONFIG_LTO_CLANG) && !namebuf[0])
> - continue;
> -
> lookup_addr = kallsyms_lookup_name(namebuf);
>
> memset(stat, 0, sizeof(*stat));
>
--
Regards,
Zhen Lei
Powered by blists - more mailing lists