* Re: [PATCH] kallsyms: Fix kallsyms_selftest failure
2023-08-25 3:46 [PATCH] kallsyms: Fix kallsyms_selftest failure Yonghong Song
@ 2023-08-25 6:53 ` Song Liu
2023-08-25 7:19 ` Leizhen (ThunderTown)
` (2 subsequent siblings)
3 siblings, 0 replies; 9+ messages in thread
From: Song Liu @ 2023-08-25 6:53 UTC (permalink / raw)
To: Yonghong Song
Cc: Kees Cook, Nick Desaulniers, Petr Mladek, Song Liu,
Steven Rostedt, Fangrui Song, Kernel Team, Leizhen, LKML,
llvm@lists.linux.dev, kernel test robot
> On Aug 24, 2023, at 8:46 PM, Yonghong Song <yonghong.song@linux.dev> 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().
>
> Fixes: 8cc32a9bbf29 ("kallsyms: strip LTO-only suffixes from promoted global functions")
> Reported-by: kernel test robot <oliver.sang@intel.com>
> Closes: https://lore.kernel.org/oe-lkp/202308232200.1c932a90-oliver.sang@intel.com
> Signed-off-by: Yonghong Song <yonghong.song@linux.dev>
Reviewed-by: Song Liu <song@kernel.org>
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH] kallsyms: Fix kallsyms_selftest failure
2023-08-25 3:46 [PATCH] kallsyms: Fix kallsyms_selftest failure Yonghong Song
2023-08-25 6:53 ` Song Liu
@ 2023-08-25 7:19 ` Leizhen (ThunderTown)
2023-09-21 10:33 ` Petr Mladek
2023-08-25 17:51 ` Nick Desaulniers
2023-08-25 19:57 ` Kees Cook
3 siblings, 1 reply; 9+ messages in thread
From: Leizhen (ThunderTown) @ 2023-08-25 7:19 UTC (permalink / raw)
To: Yonghong Song, Kees Cook, Nick Desaulniers, Petr Mladek, Song Liu,
Steven Rostedt
Cc: Fangrui Song, kernel-team, Leizhen, linux-kernel, llvm,
kernel test robot
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@huawei.com>
>
> Fixes: 8cc32a9bbf29 ("kallsyms: strip LTO-only suffixes from promoted global functions")
> Reported-by: kernel test robot <oliver.sang@intel.com>
> Closes: https://lore.kernel.org/oe-lkp/202308232200.1c932a90-oliver.sang@intel.com
> Signed-off-by: Yonghong Song <yonghong.song@linux.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
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH] kallsyms: Fix kallsyms_selftest failure
2023-08-25 7:19 ` Leizhen (ThunderTown)
@ 2023-09-21 10:33 ` Petr Mladek
2023-09-21 12:15 ` Leizhen (ThunderTown)
0 siblings, 1 reply; 9+ messages in thread
From: Petr Mladek @ 2023-09-21 10:33 UTC (permalink / raw)
To: Leizhen (ThunderTown)
Cc: Yonghong Song, Kees Cook, Nick Desaulniers, Song Liu,
Steven Rostedt, Fangrui Song, kernel-team, Leizhen, linux-kernel,
llvm, kernel test robot, live-patching
Adding live-patching list into Cc.
On Fri 2023-08-25 15:19:10, Leizhen (ThunderTown) wrote:
> 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@huawei.com>
>
> >
> > Fixes: 8cc32a9bbf29 ("kallsyms: strip LTO-only suffixes from promoted global functions")
> > Reported-by: kernel test robot <oliver.sang@intel.com>
> > Closes: https://lore.kernel.org/oe-lkp/202308232200.1c932a90-oliver.sang@intel.com
> > Signed-off-by: Yonghong Song <yonghong.song@linux.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);
> > }
Hmm, I think that this is not the right fix.
The problem is that compare_symbol_name() does not longer allow
to match the full name of the extra .llwm. symbols.
I think that the problem is that the problem is that the symbols
are sorted using cleanup_symbol_name(). They should be sorted
by using the full name.
Note that the original compare_symbol_name() returned return value
when comparing the non-stripped name. It will work correctly when
the non-stripped names are sorted.
I believe that the correct fix is:
diff --git a/scripts/kallsyms.c b/scripts/kallsyms.c
index 653b92f6d4c8..da1f8ae68999 100644
--- a/scripts/kallsyms.c
+++ b/scripts/kallsyms.c
@@ -339,25 +339,6 @@ static int symbol_absolute(const struct sym_entry *s)
return s->percpu_absolute;
}
-static void cleanup_symbol_name(char *s)
-{
- char *p;
-
- /*
- * ASCII[.] = 2e
- * ASCII[0-9] = 30,39
- * ASCII[A-Z] = 41,5a
- * ASCII[_] = 5f
- * ASCII[a-z] = 61,7a
- *
- * As above, replacing the first '.' in ".llvm." with '\0' does not
- * affect the main sorting, but it helps us with subsorting.
- */
- p = strstr(s, ".llvm.");
- if (p)
- *p = '\0';
-}
-
static int compare_names(const void *a, const void *b)
{
int ret;
@@ -533,10 +514,6 @@ static void write_src(void)
printf("\n");
}
- if (lto_clang)
- for (i = 0; i < table_cnt; i++)
- cleanup_symbol_name((char *)table[i]->sym);
-
sort_symbols_by_name();
output_label("kallsyms_seqs_of_names");
for (i = 0; i < table_cnt; i++)
Unfortunately, I could not check it easily because I do not have any
experience with building kernel with C-lang.
Anyway, what do you think, please?
Best Regards,
Petr
^ permalink raw reply related [flat|nested] 9+ messages in thread* Re: [PATCH] kallsyms: Fix kallsyms_selftest failure
2023-09-21 10:33 ` Petr Mladek
@ 2023-09-21 12:15 ` Leizhen (ThunderTown)
0 siblings, 0 replies; 9+ messages in thread
From: Leizhen (ThunderTown) @ 2023-09-21 12:15 UTC (permalink / raw)
To: Petr Mladek
Cc: Yonghong Song, Kees Cook, Nick Desaulniers, Song Liu,
Steven Rostedt, Fangrui Song, kernel-team, Leizhen, linux-kernel,
llvm, kernel test robot, live-patching
On 2023/9/21 18:33, Petr Mladek wrote:
> Adding live-patching list into Cc.
>
> On Fri 2023-08-25 15:19:10, Leizhen (ThunderTown) wrote:
>> 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@huawei.com>
>>
>>>
>>> Fixes: 8cc32a9bbf29 ("kallsyms: strip LTO-only suffixes from promoted global functions")
>>> Reported-by: kernel test robot <oliver.sang@intel.com>
>>> Closes: https://lore.kernel.org/oe-lkp/202308232200.1c932a90-oliver.sang@intel.com
>>> Signed-off-by: Yonghong Song <yonghong.song@linux.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);
>>> }
>
> Hmm, I think that this is not the right fix.
>
> The problem is that compare_symbol_name() does not longer allow
> to match the full name of the extra .llwm. symbols.
>
> I think that the problem is that the problem is that the symbols
> are sorted using cleanup_symbol_name(). They should be sorted
> by using the full name.
>
> Note that the original compare_symbol_name() returned return value
> when comparing the non-stripped name. It will work correctly when
> the non-stripped names are sorted.
>
> I believe that the correct fix is:
>
> diff --git a/scripts/kallsyms.c b/scripts/kallsyms.c
> index 653b92f6d4c8..da1f8ae68999 100644
> --- a/scripts/kallsyms.c
> +++ b/scripts/kallsyms.c
> @@ -339,25 +339,6 @@ static int symbol_absolute(const struct sym_entry *s)
> return s->percpu_absolute;
> }
>
> -static void cleanup_symbol_name(char *s)
> -{
> - char *p;
> -
> - /*
> - * ASCII[.] = 2e
> - * ASCII[0-9] = 30,39
> - * ASCII[A-Z] = 41,5a
> - * ASCII[_] = 5f
> - * ASCII[a-z] = 61,7a
> - *
> - * As above, replacing the first '.' in ".llvm." with '\0' does not
> - * affect the main sorting, but it helps us with subsorting.
> - */
> - p = strstr(s, ".llvm.");
> - if (p)
> - *p = '\0';
> -}
> -
> static int compare_names(const void *a, const void *b)
> {
> int ret;
> @@ -533,10 +514,6 @@ static void write_src(void)
> printf("\n");
> }
>
> - if (lto_clang)
> - for (i = 0; i < table_cnt; i++)
> - cleanup_symbol_name((char *)table[i]->sym);
> -
> sort_symbols_by_name();
> output_label("kallsyms_seqs_of_names");
> for (i = 0; i < table_cnt; i++)
>
>
> Unfortunately, I could not check it easily because I do not have any
> experience with building kernel with C-lang.
make CC=clang distclean defconfig
make CC=clang -j64 2>err.txt
make LLVM=1 LLVM_IAS=1 distclean defconfig
scripts/config -e LTO_CLANG_THIN
scripts/config -e KALLSYMS_SELFTEST
make LLVM=1 LLVM_IAS=1 -j64
>
> Anyway, what do you think, please?
I'm busy these days. There's a lot of things before the holiday.
I'll think about it over the holidays.
>
> Best Regards,
> Petr
> .
>
--
Regards,
Zhen Lei
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] kallsyms: Fix kallsyms_selftest failure
2023-08-25 3:46 [PATCH] kallsyms: Fix kallsyms_selftest failure Yonghong Song
2023-08-25 6:53 ` Song Liu
2023-08-25 7:19 ` Leizhen (ThunderTown)
@ 2023-08-25 17:51 ` Nick Desaulniers
2023-08-25 19:59 ` Kees Cook
2023-08-25 19:57 ` Kees Cook
3 siblings, 1 reply; 9+ messages in thread
From: Nick Desaulniers @ 2023-08-25 17:51 UTC (permalink / raw)
To: Yonghong Song
Cc: Kees Cook, Petr Mladek, Song Liu, Steven Rostedt, Fangrui Song,
kernel-team, Leizhen, linux-kernel, llvm, kernel test robot
On Thu, Aug 24, 2023 at 8:49 PM Yonghong Song <yonghong.song@linux.dev> wrote:
>
> 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);
Hi Yonghong,
Thanks for your work on this patch.
So if this change is removing the last place where the return value of
cleanup_symbol_name is checked, then perhaps this commit should
additionally change the function signature of cleanup_symbol_name to
have `void` return type.
--
Thanks,
~Nick Desaulniers
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH] kallsyms: Fix kallsyms_selftest failure
2023-08-25 17:51 ` Nick Desaulniers
@ 2023-08-25 19:59 ` Kees Cook
2023-08-25 20:04 ` Yonghong Song
0 siblings, 1 reply; 9+ messages in thread
From: Kees Cook @ 2023-08-25 19:59 UTC (permalink / raw)
To: Nick Desaulniers
Cc: Yonghong Song, Petr Mladek, Song Liu, Steven Rostedt,
Fangrui Song, kernel-team, Leizhen, linux-kernel, llvm,
kernel test robot
On Fri, Aug 25, 2023 at 10:51:58AM -0700, Nick Desaulniers wrote:
> On Thu, Aug 24, 2023 at 8:49 PM Yonghong Song <yonghong.song@linux.dev> wrote:
> >
> > 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);
>
> Hi Yonghong,
> Thanks for your work on this patch.
> So if this change is removing the last place where the return value of
> cleanup_symbol_name is checked, then perhaps this commit should
> additionally change the function signature of cleanup_symbol_name to
> have `void` return type.
I've landed this in -next as-is just because I want to make sure the bug
gets fixed ASAP, so if this gets adjusted, I can just include that
change on top.
--
Kees Cook
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH] kallsyms: Fix kallsyms_selftest failure
2023-08-25 19:59 ` Kees Cook
@ 2023-08-25 20:04 ` Yonghong Song
0 siblings, 0 replies; 9+ messages in thread
From: Yonghong Song @ 2023-08-25 20:04 UTC (permalink / raw)
To: Kees Cook, Nick Desaulniers
Cc: Petr Mladek, Song Liu, Steven Rostedt, Fangrui Song, kernel-team,
Leizhen, linux-kernel, llvm, kernel test robot
On 8/25/23 12:59 PM, Kees Cook wrote:
> On Fri, Aug 25, 2023 at 10:51:58AM -0700, Nick Desaulniers wrote:
>> On Thu, Aug 24, 2023 at 8:49 PM Yonghong Song <yonghong.song@linux.dev> wrote:
>>>
>>> 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);
>>
>> Hi Yonghong,
>> Thanks for your work on this patch.
>> So if this change is removing the last place where the return value of
>> cleanup_symbol_name is checked, then perhaps this commit should
>> additionally change the function signature of cleanup_symbol_name to
>> have `void` return type.
>
> I've landed this in -next as-is just because I want to make sure the bug
> gets fixed ASAP, so if this gets adjusted, I can just include that
> change on top.
Thanks, Kees! I can provide a followup soon.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] kallsyms: Fix kallsyms_selftest failure
2023-08-25 3:46 [PATCH] kallsyms: Fix kallsyms_selftest failure Yonghong Song
` (2 preceding siblings ...)
2023-08-25 17:51 ` Nick Desaulniers
@ 2023-08-25 19:57 ` Kees Cook
3 siblings, 0 replies; 9+ messages in thread
From: Kees Cook @ 2023-08-25 19:57 UTC (permalink / raw)
To: Nick Desaulniers, Petr Mladek, Song Liu, Steven Rostedt,
Yonghong Song
Cc: Kees Cook, Fangrui Song, kernel-team, Leizhen, linux-kernel, llvm,
kernel test robot
On Thu, 24 Aug 2023 20:46:59 -0700, 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
>
> [...]
Applied to for-next/hardening, thanks!
[1/1] kallsyms: Fix kallsyms_selftest failure
https://git.kernel.org/kees/c/33f0467fe069
Take care,
--
Kees Cook
^ permalink raw reply [flat|nested] 9+ messages in thread