| Message ID | 20240807220513.3100483-1-song@kernel.org (mailing list archive) |
|---|---|
| Headers | show |
| Series | Fix kallsyms with CONFIG_LTO_CLANG | expand |
Hi folks, Do we have more concerns and/or suggestions with this set? If not, what would be the next step for it? Thanks, Song On Wed, Aug 7, 2024 at 3:05 PM Song Liu <song@kernel.org> wrote: > > With CONFIG_LTO_CLANG, the compiler/linker adds .llvm.<hash> suffix to > local symbols to avoid duplications. Existing scripts/kallsyms sorts > symbols without .llvm.<hash> suffix. However, this causes quite some > issues later on. Some users of kallsyms, such as livepatch, have to match > symbols exactly. > > Address this by sorting full symbols at build time, and let kallsyms > lookup APIs to match the symbols exactly. > > Changes v2 => v3: > 1. Remove the _without_suffix APIs, as kprobe will not use them. > (Masami Hiramatsu) > > v2: https://lore.kernel.org/live-patching/20240802210836.2210140-1-song@kernel.org/T/#u > > Changes v1 => v2: > 1. Update the APIs to remove all .XXX suffixes (v1 only removes .llvm.*). > 2. Rename the APIs as *_without_suffix. (Masami Hiramatsu) > 3. Fix another user from kprobe. (Masami Hiramatsu) > 4. Add tests for the new APIs in kallsyms_selftests. > > v1: https://lore.kernel.org/live-patching/20240730005433.3559731-1-song@kernel.org/T/#u > > Song Liu (2): > kallsyms: Do not cleanup .llvm.<hash> suffix before sorting symbols > kallsyms: Match symbols exactly with CONFIG_LTO_CLANG > > kernel/kallsyms.c | 55 +++++--------------------------------- > kernel/kallsyms_selftest.c | 22 +-------------- > scripts/kallsyms.c | 31 ++------------------- > scripts/link-vmlinux.sh | 4 --- > 4 files changed, 9 insertions(+), 103 deletions(-) > > -- > 2.43.5
On Mon, Aug 12, 2024 at 09:21:02AM -0700, Song Liu wrote: > Hi folks, > > Do we have more concerns and/or suggestions with this set? If not, > what would be the next step for it? I'm all for simplifying things, and this does just that, however, I'm not the one you need to convince, the folks who added the original hacks should provide their Reviewed-by / Tested-by not just for CONFIG_LTO_CLANG but also given this provides an alternative fix, don't we want to invert the order so we don't regress CONFIG_LTO_CLANG ? And shouldn't the patches also have their respective Fixes tag? Provided the commit logs are extended with Fixes and order is maintained to be able to bisect correctly: Reviewed-by: Luis Chamberlain <mcgrof@kernel.org> Luis
Hi Luis, > On Aug 12, 2024, at 9:57 AM, Luis Chamberlain <mcgrof@kernel.org> wrote: > > On Mon, Aug 12, 2024 at 09:21:02AM -0700, Song Liu wrote: >> Hi folks, >> >> Do we have more concerns and/or suggestions with this set? If not, >> what would be the next step for it? > > I'm all for simplifying things, and this does just that, however, > I'm not the one you need to convince, the folks who added the original > hacks should provide their Reviewed-by / Tested-by not just for CONFIG_LTO_CLANG > but also given this provides an alternative fix, don't we want to invert > the order so we don't regress CONFIG_LTO_CLANG ? And shouldn't the patches > also have their respective Fixes tag? kallsyms has got quite a few changes/improvements in the past few years: 1. Sami added logic to trim LTO hash in 2021 [1]; 2. Zhen added logic to sort kallsyms in 2022 [2]; 3. Yonghong changed cleanup_symbol_name() in 2023 [3]. In this set, we are undoing 1 and 3, but we keep 2. Shall we point Fixes tag to [1] or [3]? The patch won't apply to a kernel with only [1] (without [2] and [3]); while this set is not just fixing [3]. So I think it is not accurate either way. OTOH, the combination of CONFIG_LTO_CLANG and livepatching is probably not used by a lot of users, so I guess we are OK without Fixes tags? I personally don't have a strong preference either way. It is not necessary to invert the order of the two patches. Only applying one of the two patches won't cause more issues than what we have today. Thanks, Song > > Provided the commit logs are extended with Fixes and order is maintained > to be able to bisect correctly: > > Reviewed-by: Luis Chamberlain <mcgrof@kernel.org> > > Luis [1] 8b8e6b5d3b01 ("kallsyms: strip ThinLTO hashes from static functions") [2] 60443c88f3a8 ("kallsyms: Improve the performance of kallsyms_lookup_name()") [3] 8cc32a9bbf29 ("kallsyms: strip LTO-only suffixes from promoted global functions")
On Wed, 7 Aug 2024 15:05:11 -0700 Song Liu <song@kernel.org> wrote: > With CONFIG_LTO_CLANG, the compiler/linker adds .llvm.<hash> suffix to > local symbols to avoid duplications. Existing scripts/kallsyms sorts > symbols without .llvm.<hash> suffix. However, this causes quite some > issues later on. Some users of kallsyms, such as livepatch, have to match > symbols exactly. > > Address this by sorting full symbols at build time, and let kallsyms > lookup APIs to match the symbols exactly. > I've tested this series and confirmed it makes kprobes work with llvm suffixed symbols. /sys/kernel/tracing # echo "p c_start.llvm.8011538628216713357" >> kprobe_events /sys/kernel/tracing # cat kprobe_events p:kprobes/p_c_start_llvm_8011538628216713357_0 c_start.llvm.8011538628216713357 /sys/kernel/tracing # echo "p c_start" >> kprobe_events /sys/kernel/tracing # cat kprobe_events p:kprobes/p_c_start_llvm_8011538628216713357_0 c_start.llvm.8011538628216713357 p:kprobes/p_c_start_0 c_start And ftrace too. /sys/kernel/tracing # grep ^c_start available_filter_functions c_start.llvm.8011538628216713357 c_start c_start.llvm.17132674095431275852 Tested-by: Masami Hiramatsu (Google) <mhiramat@kernel.org> Reviewed-by: Masami Hiramatsu (Google) <mhiramat@kernel.org> for this series. > Changes v2 => v3: > 1. Remove the _without_suffix APIs, as kprobe will not use them. > (Masami Hiramatsu) > > v2: https://lore.kernel.org/live-patching/20240802210836.2210140-1-song@kernel.org/T/#u > > Changes v1 => v2: > 1. Update the APIs to remove all .XXX suffixes (v1 only removes .llvm.*). > 2. Rename the APIs as *_without_suffix. (Masami Hiramatsu) > 3. Fix another user from kprobe. (Masami Hiramatsu) > 4. Add tests for the new APIs in kallsyms_selftests. > > v1: https://lore.kernel.org/live-patching/20240730005433.3559731-1-song@kernel.org/T/#u > > Song Liu (2): > kallsyms: Do not cleanup .llvm.<hash> suffix before sorting symbols > kallsyms: Match symbols exactly with CONFIG_LTO_CLANG > > kernel/kallsyms.c | 55 +++++--------------------------------- > kernel/kallsyms_selftest.c | 22 +-------------- > scripts/kallsyms.c | 31 ++------------------- > scripts/link-vmlinux.sh | 4 --- > 4 files changed, 9 insertions(+), 103 deletions(-) > > -- > 2.43.5
Hi Masami, Thanks for your review and test! @Sami, could you please also review the set? @Luis, I replied to 1/2 and 2/2 with Fixes tags that I think make most sense. Please let me know if we need changes to the set or more reviews and tests. Thanks, Song > On Aug 12, 2024, at 9:29 PM, Masami Hiramatsu <mhiramat@kernel.org> wrote: > > On Wed, 7 Aug 2024 15:05:11 -0700 > Song Liu <song@kernel.org> wrote: > >> With CONFIG_LTO_CLANG, the compiler/linker adds .llvm.<hash> suffix to >> local symbols to avoid duplications. Existing scripts/kallsyms sorts >> symbols without .llvm.<hash> suffix. However, this causes quite some >> issues later on. Some users of kallsyms, such as livepatch, have to match >> symbols exactly. >> >> Address this by sorting full symbols at build time, and let kallsyms >> lookup APIs to match the symbols exactly. >> > > I've tested this series and confirmed it makes kprobes work with llvm suffixed > symbols. > > /sys/kernel/tracing # echo "p c_start.llvm.8011538628216713357" >> kprobe_events > /sys/kernel/tracing # cat kprobe_events > p:kprobes/p_c_start_llvm_8011538628216713357_0 c_start.llvm.8011538628216713357 > /sys/kernel/tracing # echo "p c_start" >> kprobe_events > /sys/kernel/tracing # cat kprobe_events > p:kprobes/p_c_start_llvm_8011538628216713357_0 c_start.llvm.8011538628216713357 > p:kprobes/p_c_start_0 c_start > > And ftrace too. > > /sys/kernel/tracing # grep ^c_start available_filter_functions > c_start.llvm.8011538628216713357 > c_start > c_start.llvm.17132674095431275852 > > Tested-by: Masami Hiramatsu (Google) <mhiramat@kernel.org> > Reviewed-by: Masami Hiramatsu (Google) <mhiramat@kernel.org> > > for this series. > >> Changes v2 => v3: >> 1. Remove the _without_suffix APIs, as kprobe will not use them. >> (Masami Hiramatsu) >> >> v2: https://lore.kernel.org/live-patching/20240802210836.2210140-1-song@kernel.org/T/#u >> >> Changes v1 => v2: >> 1. Update the APIs to remove all .XXX suffixes (v1 only removes .llvm.*). >> 2. Rename the APIs as *_without_suffix. (Masami Hiramatsu) >> 3. Fix another user from kprobe. (Masami Hiramatsu) >> 4. Add tests for the new APIs in kallsyms_selftests. >> >> v1: https://lore.kernel.org/live-patching/20240730005433.3559731-1-song@kernel.org/T/#u >> >> Song Liu (2): >> kallsyms: Do not cleanup .llvm.<hash> suffix before sorting symbols >> kallsyms: Match symbols exactly with CONFIG_LTO_CLANG >> >> kernel/kallsyms.c | 55 +++++--------------------------------- >> kernel/kallsyms_selftest.c | 22 +-------------- >> scripts/kallsyms.c | 31 ++------------------- >> scripts/link-vmlinux.sh | 4 --- >> 4 files changed, 9 insertions(+), 103 deletions(-) >> >> -- >> 2.43.5 > > > -- > Masami Hiramatsu (Google) <mhiramat@kernel.org>
Hi, On Tue, Aug 13, 2024 at 2:20 PM Song Liu <songliubraving@meta.com> wrote: > > Hi Masami, > > Thanks for your review and test! > > @Sami, could you please also review the set? As the kernel no longer uses the Clang feature combination that was the primary motivation for adding these kallsyms changes in the first place, this series looks reasonable to me. Thanks for cleaning this up! Reviewed-by: Sami Tolvanen <samitolvanen@google.com> Sami
On Wed 2024-08-07 15:05:11, Song Liu wrote: > With CONFIG_LTO_CLANG, the compiler/linker adds .llvm.<hash> suffix to > local symbols to avoid duplications. Existing scripts/kallsyms sorts > symbols without .llvm.<hash> suffix. However, this causes quite some > issues later on. Some users of kallsyms, such as livepatch, have to match > symbols exactly. > > Address this by sorting full symbols at build time, and let kallsyms > lookup APIs to match the symbols exactly. The changes look good from the livepatching POV. For both patches, feel free to use: Acked-by: Petr Mladek <pmladek@suse.com> I made a quick glance over the code changes. They look sane. But I did not check them deep enough to provide a valuable Reviewed-by ;-) Best Regards, Petr
On Mon, Aug 12, 2024 at 06:13:22PM +0000, Song Liu wrote: > Hi Luis, > > > On Aug 12, 2024, at 9:57 AM, Luis Chamberlain <mcgrof@kernel.org> wrote: > > > > On Mon, Aug 12, 2024 at 09:21:02AM -0700, Song Liu wrote: > >> Hi folks, > >> > >> Do we have more concerns and/or suggestions with this set? If not, > >> what would be the next step for it? > > > > I'm all for simplifying things, and this does just that, however, > > I'm not the one you need to convince, the folks who added the original > > hacks should provide their Reviewed-by / Tested-by not just for CONFIG_LTO_CLANG > > but also given this provides an alternative fix, don't we want to invert > > the order so we don't regress CONFIG_LTO_CLANG ? And shouldn't the patches > > also have their respective Fixes tag? > > kallsyms has got quite a few changes/improvements in the past few years: > > 1. Sami added logic to trim LTO hash in 2021 [1]; > 2. Zhen added logic to sort kallsyms in 2022 [2]; > 3. Yonghong changed cleanup_symbol_name() in 2023 [3]. > > In this set, we are undoing 1 and 3, but we keep 2. Shall we point Fixes > tag to [1] or [3]? The patch won't apply to a kernel with only [1] > (without [2] and [3]); while this set is not just fixing [3]. So I think > it is not accurate either way. OTOH, the combination of CONFIG_LTO_CLANG > and livepatching is probably not used by a lot of users, so I guess we > are OK without Fixes tags? I personally don't have a strong preference > either way. > > It is not necessary to invert the order of the two patches. Only applying > one of the two patches won't cause more issues than what we have today. Which tree should carry this series?
Hi Kees, > On Aug 15, 2024, at 9:05 AM, Kees Cook <kees@kernel.org> wrote: > > On Mon, Aug 12, 2024 at 06:13:22PM +0000, Song Liu wrote: >> Hi Luis, >> >>> On Aug 12, 2024, at 9:57 AM, Luis Chamberlain <mcgrof@kernel.org> wrote: >>> >>> On Mon, Aug 12, 2024 at 09:21:02AM -0700, Song Liu wrote: >>>> Hi folks, >>>> >>>> Do we have more concerns and/or suggestions with this set? If not, >>>> what would be the next step for it? >>> >>> I'm all for simplifying things, and this does just that, however, >>> I'm not the one you need to convince, the folks who added the original >>> hacks should provide their Reviewed-by / Tested-by not just for CONFIG_LTO_CLANG >>> but also given this provides an alternative fix, don't we want to invert >>> the order so we don't regress CONFIG_LTO_CLANG ? And shouldn't the patches >>> also have their respective Fixes tag? >> >> kallsyms has got quite a few changes/improvements in the past few years: >> >> 1. Sami added logic to trim LTO hash in 2021 [1]; >> 2. Zhen added logic to sort kallsyms in 2022 [2]; >> 3. Yonghong changed cleanup_symbol_name() in 2023 [3]. >> >> In this set, we are undoing 1 and 3, but we keep 2. Shall we point Fixes >> tag to [1] or [3]? The patch won't apply to a kernel with only [1] >> (without [2] and [3]); while this set is not just fixing [3]. So I think >> it is not accurate either way. OTOH, the combination of CONFIG_LTO_CLANG >> and livepatching is probably not used by a lot of users, so I guess we >> are OK without Fixes tags? I personally don't have a strong preference >> either way. >> >> It is not necessary to invert the order of the two patches. Only applying >> one of the two patches won't cause more issues than what we have today. > > Which tree should carry this series? I am looking through the commit log on kernel/kallsyms.c _just now_, and found you took most of recent patches for kallsyms. Could you please take this set as well? Thanks, Song
On Wed, 07 Aug 2024 15:05:11 -0700, Song Liu wrote: > With CONFIG_LTO_CLANG, the compiler/linker adds .llvm.<hash> suffix to > local symbols to avoid duplications. Existing scripts/kallsyms sorts > symbols without .llvm.<hash> suffix. However, this causes quite some > issues later on. Some users of kallsyms, such as livepatch, have to match > symbols exactly. > > Address this by sorting full symbols at build time, and let kallsyms > lookup APIs to match the symbols exactly. > > [...] Applied to for-linus/hardening, thanks! [1/2] kallsyms: Do not cleanup .llvm.<hash> suffix before sorting symbols https://git.kernel.org/kees/c/020925ce9299 [2/2] kallsyms: Match symbols exactly with CONFIG_LTO_CLANG https://git.kernel.org/kees/c/fb6a421fb615 Take care,
Hi, On Wed, 7 Aug 2024, Song Liu wrote: > With CONFIG_LTO_CLANG, the compiler/linker adds .llvm.<hash> suffix to > local symbols to avoid duplications. Existing scripts/kallsyms sorts > symbols without .llvm.<hash> suffix. However, this causes quite some > issues later on. Some users of kallsyms, such as livepatch, have to match > symbols exactly. > > Address this by sorting full symbols at build time, and let kallsyms > lookup APIs to match the symbols exactly. > > Changes v2 => v3: > 1. Remove the _without_suffix APIs, as kprobe will not use them. > (Masami Hiramatsu) > > v2: https://lore.kernel.org/live-patching/20240802210836.2210140-1-song@kernel.org/T/#u > > Changes v1 => v2: > 1. Update the APIs to remove all .XXX suffixes (v1 only removes .llvm.*). > 2. Rename the APIs as *_without_suffix. (Masami Hiramatsu) > 3. Fix another user from kprobe. (Masami Hiramatsu) > 4. Add tests for the new APIs in kallsyms_selftests. > > v1: https://lore.kernel.org/live-patching/20240730005433.3559731-1-song@kernel.org/T/#u > > Song Liu (2): > kallsyms: Do not cleanup .llvm.<hash> suffix before sorting symbols > kallsyms: Match symbols exactly with CONFIG_LTO_CLANG > > kernel/kallsyms.c | 55 +++++--------------------------------- > kernel/kallsyms_selftest.c | 22 +-------------- > scripts/kallsyms.c | 31 ++------------------- > scripts/link-vmlinux.sh | 4 --- > 4 files changed, 9 insertions(+), 103 deletions(-) I was on holiday most of August and the patch set has been merged but let me at least add Acked-by: Miroslav Benes <mbenes@suse.cz> here since I participated in the discussion at the beginning. Thank you for cleaning it up! Miroslav