Fix lack of section mismatch warnings with LTO
Subsystems (3)
Nathan Chancellor
maintainercover Fix lack of section mismatch warnings with LTO
Hi all,
Vincent recently reported an issue with lack of section mismatch warnings with LTO. This is due to commit 6c730bfc894f ("modpost: handle -ffunction-sections"), which ignores all function sections for modpost.
I believe this is incorrect, as these function sections may still refer to symbols in other sections and they will ultimately be coalesced into .text by vmlinux.lds anyways.
The first patch fixes a warning that I see with allmodconfig + ThinLTO builds after applying the second patch. The second patch moves ".text.*" into TEXT_SECTIONS so that modpost audits them for mismatches.
I expect this to go via the kbuild tree with an ack from the padata maintainers.
Nathan Chancellor (2):
padata: Do not mark padata_mt_helper() as __init
modpost: Include '.text.*' in TEXT_SECTIONS
kernel/padata.c | 4 ++-- scripts/mod/modpost.c | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-)
base-commit: b7b275e60bcd5f89771e865a8239325f86d9927d
Signature · 1 line
2.38.1
Nathan Chancellor
maintainerPATCH 1/2 padata: Do not mark padata_mt_helper() as __init
When building arm64 allmodconfig + ThinLTO with clang and a proposed modpost update to account for -ffuncton-sections, the following warning appears:
WARNING: modpost: vmlinux.o: section mismatch in reference: padata_work_init (section: .text.padata_work_init) -> padata_mt_helper (section: .init.text)
WARNING: modpost: vmlinux.o: section mismatch in reference: padata_work_init (section: .text.padata_work_init) -> padata_mt_helper (section: .init.text)
In both cases, an __init function calls padata_work_init(), which is not marked __init, with padata_mt_helper(), another __init function, as a work function argument.
padata_work_init() is called from non-init paths, otherwise it could be marked __init to resolve the warning. Instead, remove __init from padata_mt_helper() to resolve the warning.
---
Cc: Steffen Klassert <[email protected]>
Cc: Daniel Jordan <[email protected]>
Cc: [email protected]
--- kernel/padata.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
kernel/padata.c +2 -2
@@ -45,7 +45,7 @@ struct padata_mt_job_state { };static void padata_free_pd (struct parallel_data *pd); -static void __init padata_mt_helper (struct work_struct *work); +static void padata_mt_helper (struct work_struct *work);static int padata_index_to_cpu (struct parallel_data *pd,int cpu_index) {
@@ -425,7 +425,7 @@ static int padata_setup_cpumasks(struct padata_instance *pinst)return err; } -static void __init padata_mt_helper (struct work_struct *w) +static void padata_mt_helper (struct work_struct *w) { struct padata_work *pw = container_of(w, struct padata_work, pw_work);struct padata_mt_job_state *ps = pw->pw_data;
Signature · 1 line
2.38.1
Re: [PATCH 1/2] padata: Do not mark padata_mt_helper() as __init
Quoted 4 lines · Nathan Chancellor
When building arm64 allmodconfig + ThinLTO with clang and a proposed modpost update to account for -ffuncton-sections, the following warning appears:
How to enable -ffuncton-sections for ARCH=arm64 ?
(in other words, how to set CONFIG_LD_DEAD_CODE_DATA_ELIMINATION ?)
In upstream, it is only possible for mips and powerpc.
./arch/mips/Kconfig:82: select HAVE_LD_DEAD_CODE_DATA_ELIMINATION ./arch/powerpc/Kconfig:237: select HAVE_LD_DEAD_CODE_DATA_ELIMINATION
Is there another proposal to add it for arm64,
or is this about a downstream kernel?
Quoted 46 lines
WARNING: modpost: vmlinux.o: section mismatch in reference:
padata_work_init(section: .text.padata_work_init) ->padata_mt_helper(section: .init.text)
WARNING: modpost: vmlinux.o: section mismatch in reference:padata_work_init(section: .text.padata_work_init) ->padata_mt_helper(section: .init.text)In both cases, an
__initfunction callspadata_work_init(), which is not marked__init, withpadata_mt_helper(), another__initfunction, as a work function argument.
padata_work_init()is called from non-init paths, otherwise it could be marked__initto resolve the warning. Instead, remove__initfrompadata_mt_helper()to resolve the warning.Signed-off-by Nathan Chancellor <[email protected]>---
Cc: Steffen Klassert <[email protected]>
Cc: Daniel Jordan <[email protected]>
Cc: [email protected]--- kernel/padata.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)kernel/padata.c +2
-3@@ -45,7 +45,7 @@ struct padata_mt_job_state { };static void padata_free_pd (struct parallel_data *pd); -static void __init padata_mt_helper (struct work_struct *work); +static void padata_mt_helper (struct work_struct *work);static int padata_index_to_cpu (struct parallel_data *pd,int cpu_index) {@@ -425,7 +425,7 @@ static int padata_setup_cpumasks(struct padata_instance *pinst)return err; } -static void __init padata_mt_helper (struct work_struct *w) +static void padata_mt_helper (struct work_struct *w) { struct padata_work *pw = container_of(w, struct padata_work, pw_work);struct padata_mt_job_state *ps = pw->pw_data; --2.38.1
Signature · 2 lines
Best Regards Masahiro Yamada
Nathan Chancellor
maintainerRe: [PATCH 1/2] padata: Do not mark padata_mt_helper() as __init
Quoted 10 lines · Masahiro Yamada
Quoted 4 lines · Nathan Chancellor
When building arm64 allmodconfig + ThinLTO with clang and a proposed modpost update to account for -ffuncton-sections, the following warning appears:
How to enable -ffuncton-sections for ARCH=arm64 ?
(in other words, how to setCONFIG_LD_DEAD_CODE_DATA_ELIMINATION?)
clang LTO implies -fdata-sections and -ffunction-sections.
$ cat foo.c
int foo(void)
{
return 0;
}$ cat bar.c extern int foo(void);
int bar(void)
{
return foo();
}$ clang -c -o foo.{o,c}
$ clang -c -o bar.{o,c}
$ ld.lld -r -o foobar {foo,bar}.o
$ llvm-readelf -s foobarSymbol table '.symtab' contains 9 entries:
Num: Value Size Type Bind Vis Ndx Name
0: 0000000000000000 0 NOTYPE LOCAL DEFAULT UND
1: 0000000000000000 0 FILE LOCAL DEFAULT ABS foo.c
2: 0000000000000000 0 SECTION LOCAL DEFAULT 1 .text
3: 0000000000000000 0 SECTION LOCAL DEFAULT 3 .eh_frame
4: 0000000000000000 0 SECTION LOCAL DEFAULT 5 .llvm_addrsig
5: 0000000000000000 0 FILE LOCAL DEFAULT ABS bar.c
6: 0000000000000000 0 SECTION LOCAL DEFAULT 2 .comment
7: 0000000000000000 8 FUNC GLOBAL DEFAULT 1 foo
8: 0000000000000010 11 FUNC GLOBAL DEFAULT 1 bar$ clang -flto -c -o foo.{o,c}
$ clang -flto -c -o bar.{o,c}
$ ld.lld -r -o foobar {foo,bar}.o
$ llvm-readelf -s foobarSymbol table '.symtab' contains 10 entries:
Num: Value Size Type Bind Vis Ndx Name
0: 0000000000000000 0 NOTYPE LOCAL DEFAULT UND
1: 0000000000000000 0 FILE LOCAL DEFAULT ABS ld-temp.o
2: 0000000000000000 0 SECTION LOCAL DEFAULT 1 .text
3: 0000000000000000 0 SECTION LOCAL DEFAULT 2 .text.foo
4: 0000000000000000 0 SECTION LOCAL DEFAULT 3 .text.bar
5: 0000000000000000 0 SECTION LOCAL DEFAULT 6 .eh_frame
6: 0000000000000000 0 SECTION LOCAL DEFAULT 8 .llvm_addrsig
7: 0000000000000000 0 SECTION LOCAL DEFAULT 5 .comment
8: 0000000000000000 8 FUNC GLOBAL DEFAULT 2 foo
9: 0000000000000000 13 FUNC GLOBAL DEFAULT 3 barQuoted 65 lines
In upstream, it is only possible for mips and powerpc.
./arch/mips/Kconfig:82: selectHAVE_LD_DEAD_CODE_DATA_ELIMINATION./arch/powerpc/Kconfig:237: selectHAVE_LD_DEAD_CODE_DATA_ELIMINATIONIs there another proposal to add it for arm64,
or is this about a downstream kernel?Quoted 46 lines
WARNING: modpost: vmlinux.o: section mismatch in reference:
padata_work_init(section: .text.padata_work_init) ->padata_mt_helper(section: .init.text)
WARNING: modpost: vmlinux.o: section mismatch in reference:padata_work_init(section: .text.padata_work_init) ->padata_mt_helper(section: .init.text)In both cases, an
__initfunction callspadata_work_init(), which is not marked__init, withpadata_mt_helper(), another__initfunction, as a work function argument.
padata_work_init()is called from non-init paths, otherwise it could be marked__initto resolve the warning. Instead, remove__initfrompadata_mt_helper()to resolve the warning.Signed-off-by Nathan Chancellor <[email protected]>---
Cc: Steffen Klassert <[email protected]>
Cc: Daniel Jordan <[email protected]>
Cc: [email protected]--- kernel/padata.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)kernel/padata.c +2
-3@@ -45,7 +45,7 @@ struct padata_mt_job_state { };static void padata_free_pd (struct parallel_data *pd); -static void __init padata_mt_helper (struct work_struct *work); +static void padata_mt_helper (struct work_struct *work);static int padata_index_to_cpu (struct parallel_data *pd,int cpu_index) {@@ -425,7 +425,7 @@ static int padata_setup_cpumasks(struct padata_instance *pinst)return err; } -static void __init padata_mt_helper (struct work_struct *w) +static void padata_mt_helper (struct work_struct *w) { struct padata_work *pw = container_of(w, struct padata_work, pw_work);struct padata_mt_job_state *ps = pw->pw_data; --2.38.1
Signature · 2 lines
Best Regards Masahiro Yamada
Re: [PATCH 1/2] padata: Do not mark padata_mt_helper() as __init
In reply to Nathan ChancellorQuoted 48 lines · Nathan Chancellor
When building arm64 allmodconfig + ThinLTO with clang and a proposed modpost update to account for -ffuncton-sections, the following warning appears:
WARNING: modpost: vmlinux.o: section mismatch in reference:
padata_work_init(section: .text.padata_work_init) ->padata_mt_helper(section: .init.text)
WARNING: modpost: vmlinux.o: section mismatch in reference:padata_work_init(section: .text.padata_work_init) ->padata_mt_helper(section: .init.text)In both cases, an
__initfunction callspadata_work_init(), which is not marked__init, withpadata_mt_helper(), another__initfunction, as a work function argument.
padata_work_init()is called from non-init paths, otherwise it could be marked__initto resolve the warning. Instead, remove__initfrompadata_mt_helper()to resolve the warning.Signed-off-by Nathan Chancellor <[email protected]>---
Cc: Steffen Klassert <[email protected]>
Cc: Daniel Jordan <[email protected]>
Cc: [email protected]--- kernel/padata.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)kernel/padata.c +2
-3@@ -45,7 +45,7 @@ struct padata_mt_job_state { };static void padata_free_pd (struct parallel_data *pd); -static void __init padata_mt_helper (struct work_struct *work); +static void padata_mt_helper (struct work_struct *work);static int padata_index_to_cpu (struct parallel_data *pd,int cpu_index) {@@ -425,7 +425,7 @@ static int padata_setup_cpumasks(struct padata_instance *pinst)return err; } -static void __init padata_mt_helper (struct work_struct *w) +static void padata_mt_helper (struct work_struct *w) { struct padata_work *pw = container_of(w, struct padata_work, pw_work);struct padata_mt_job_state *ps = pw->pw_data; --
Quoted 2 lines
2.38.1
This patch seems wrong.
padata_work_init() does not reference to padata_mt_helper()
padata_work_alloc_mt() and padata_do_multithreaded() do.
Signature · 2 lines
Best Regards Masahiro Yamada
Daniel Jordan
maintainerRe: [PATCH 1/2] padata: Do not mark padata_mt_helper() as __init
Quoted 54 lines · Masahiro Yamada
Quoted 48 lines · Nathan Chancellor
When building arm64 allmodconfig + ThinLTO with clang and a proposed modpost update to account for -ffuncton-sections, the following warning appears:
WARNING: modpost: vmlinux.o: section mismatch in reference:
padata_work_init(section: .text.padata_work_init) ->padata_mt_helper(section: .init.text)
WARNING: modpost: vmlinux.o: section mismatch in reference:padata_work_init(section: .text.padata_work_init) ->padata_mt_helper(section: .init.text)In both cases, an
__initfunction callspadata_work_init(), which is not marked__init, withpadata_mt_helper(), another__initfunction, as a work function argument.
padata_work_init()is called from non-init paths, otherwise it could be marked__initto resolve the warning. Instead, remove__initfrompadata_mt_helper()to resolve the warning.Signed-off-by Nathan Chancellor <[email protected]>---
Cc: Steffen Klassert <[email protected]>
Cc: Daniel Jordan <[email protected]>
Cc: [email protected]--- kernel/padata.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)kernel/padata.c +2
-3@@ -45,7 +45,7 @@ struct padata_mt_job_state { };static void padata_free_pd (struct parallel_data *pd); -static void __init padata_mt_helper (struct work_struct *work); +static void padata_mt_helper (struct work_struct *work);static int padata_index_to_cpu (struct parallel_data *pd,int cpu_index) {@@ -425,7 +425,7 @@ static int padata_setup_cpumasks(struct padata_instance *pinst)return err; } -static void __init padata_mt_helper (struct work_struct *w) +static void padata_mt_helper (struct work_struct *w) { struct padata_work *pw = container_of(w, struct padata_work, pw_work);struct padata_mt_job_state *ps = pw->pw_data; --Quoted 2 lines
2.38.1
This patch seems wrong.
Quoted 4 lines
padata_work_init()does not reference topadata_mt_helper()
padata_work_alloc_mt()andpadata_do_multithreaded()do.
I see LLVM optimizing padata_work_init by embedding padata_mt_helper's address in its text, which runs afoul of modpost.
I agree with Masahiro, the warning is a false positive since only __init functions ever cause the embedded address to be used.
We have __ref for situations like this. That way, padata_mt_helper can stay properly __init.
Nathan Chancellor
maintainerRe: [PATCH 1/2] padata: Do not mark padata_mt_helper() as __init
Quoted 60 lines · Daniel Jordan
Quoted 54 lines · Masahiro Yamada
Quoted 48 lines · Nathan Chancellor
When building arm64 allmodconfig + ThinLTO with clang and a proposed modpost update to account for -ffuncton-sections, the following warning appears:
WARNING: modpost: vmlinux.o: section mismatch in reference:
padata_work_init(section: .text.padata_work_init) ->padata_mt_helper(section: .init.text)
WARNING: modpost: vmlinux.o: section mismatch in reference:padata_work_init(section: .text.padata_work_init) ->padata_mt_helper(section: .init.text)In both cases, an
__initfunction callspadata_work_init(), which is not marked__init, withpadata_mt_helper(), another__initfunction, as a work function argument.
padata_work_init()is called from non-init paths, otherwise it could be marked__initto resolve the warning. Instead, remove__initfrompadata_mt_helper()to resolve the warning.Signed-off-by Nathan Chancellor <[email protected]>---
Cc: Steffen Klassert <[email protected]>
Cc: Daniel Jordan <[email protected]>
Cc: [email protected]--- kernel/padata.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)kernel/padata.c +2
-3@@ -45,7 +45,7 @@ struct padata_mt_job_state { };static void padata_free_pd (struct parallel_data *pd); -static void __init padata_mt_helper (struct work_struct *work); +static void padata_mt_helper (struct work_struct *work);static int padata_index_to_cpu (struct parallel_data *pd,int cpu_index) {@@ -425,7 +425,7 @@ static int padata_setup_cpumasks(struct padata_instance *pinst)return err; } -static void __init padata_mt_helper (struct work_struct *w) +static void padata_mt_helper (struct work_struct *w) { struct padata_work *pw = container_of(w, struct padata_work, pw_work);struct padata_mt_job_state *ps = pw->pw_data; --Quoted 2 lines
2.38.1
This patch seems wrong.
Quoted 4 lines
padata_work_init()does not reference topadata_mt_helper()
padata_work_alloc_mt()andpadata_do_multithreaded()do.
Quoted 8 lines
I see LLVM optimizing
padata_work_initby embedding padata_mt_helper's address in its text, which runs afoul of modpost.I agree with Masahiro, the warning is a false positive since only
__initfunctions ever cause the embedded address to be used.We have
__reffor situations like this. That way,padata_mt_helpercan stay properly__init.
Ah, thank you for pointing out __ref, that seems to be exactly what we want here. I will send a v2 marking padata_work_init() as __ref shortly.
Cheers,
Nathan
Nathan Chancellor
maintainerPATCH 2/2 modpost: Include '.text.*' in TEXT_SECTIONS
In reply to Nathan ChancellorCommit 6c730bfc894f ("modpost: handle -ffunction-sections") added ".text.*" to the OTHER_TEXT_SECTIONS macro to fix certain section mismatch warnings. Unfortunately, this makes it impossible for modpost to warn about section mismatchs with LTO, which implies '-ffunction-sections', as all functions are put in their own '.text.<func_name>' sections, which may still reference functions in sections they are not supposed to, such as __init.
Fix this by moving ".text.*" into TEXT_SECTIONS, so that configurations with '-ffunction-sections' will see warnings about mismatched sections.
--- scripts/mod/modpost.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
scripts/mod/modpost.c +2 -2
@@ -823,10 +823,10 @@ static void check_section(const char *modname, struct elf_info *elf,#define ALL_EXIT_SECTIONS EXIT_SECTIONS, ALL_XXXEXIT_SECTIONS#define DATA_SECTIONS ".data", ".data.rel" -#define TEXT_SECTIONS ".text", ".text.unlikely", ".sched.text", \ +#define TEXT_SECTIONS ".text", ".text.*", ".sched.text", \ ".kprobes.text", ".cpuidle.text", ".noinstr.text"#define OTHER_TEXT_SECTIONS ".ref.text", ".head.text", ".spinlock.text", \ - ".fixup", ".entry.text", ".exception.text", ".text.*", \ + ".fixup", ".entry.text", ".exception.text", \ ".coldtext", ".softirqentry.text"#define INIT_SECTIONS ".init.*"
Signature · 1 line
2.38.1
Re: [PATCH 2/2] modpost: Include '.text.*' in TEXT_SECTIONS
Quoted 8 lines · Nathan Chancellor
Commit
6c730bfc894f("modpost: handle -ffunction-sections") added ".text.*" to theOTHER_TEXT_SECTIONSmacro to fix certain section mismatch warnings. Unfortunately, this makes it impossible for modpost to warn about section mismatchs with LTO, which implies '-ffunction-sections', as all functions are put in their own '.text.<func_name>' sections, which may still reference functions in sections they are not supposed to, such as__init.
Quoted 6 lines
Fix this by moving ".text.*" into
TEXT_SECTIONS, so that configurations with '-ffunction-sections' will see warnings about mismatched sections.Reported-by Vincent Donnefort <[email protected]>Signed-off-by Nathan Chancellor <[email protected]>
Thanks!
Quoted 24 lines
--- scripts/mod/modpost.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)scripts/mod/modpost.c +2
-2@@ -823,10 +823,10 @@ static void check_section(const char *modname, struct elf_info *elf,#define ALL_EXIT_SECTIONS EXIT_SECTIONS, ALL_XXXEXIT_SECTIONS#define DATA_SECTIONS ".data", ".data.rel" -#define TEXT_SECTIONS ".text", ".text.unlikely", ".sched.text", \ +#define TEXT_SECTIONS ".text", ".text.*", ".sched.text", \ ".kprobes.text", ".cpuidle.text", ".noinstr.text"#define OTHER_TEXT_SECTIONS ".ref.text", ".head.text", ".spinlock.text", \ - ".fixup", ".entry.text", ".exception.text", ".text.*", \ + ".fixup", ".entry.text", ".exception.text", \ ".coldtext", ".softirqentry.text"#define INIT_SECTIONS ".init.*"Signature · 1 line
2.38.1
Re: [PATCH 2/2] modpost: Include '.text.*' in TEXT_SECTIONS
In reply to Nathan ChancellorFrom: Nathan Chancellor <[email protected]>
Date: Tue, 29 Nov 2022 12:01:23 -0700
Quoted 8 lines
Commit
6c730bfc894f("modpost: handle -ffunction-sections") added ".text.*" to theOTHER_TEXT_SECTIONSmacro to fix certain section mismatch warnings. Unfortunately, this makes it impossible for modpost to warn about section mismatchs with LTO, which implies '-ffunction-sections', as all functions are put in their own '.text.<func_name>' sections, which may still reference functions in sections they are not supposed to, such as__init.
Quoted 6 lines
Fix this by moving ".text.*" into
TEXT_SECTIONS, so that configurations with '-ffunction-sections' will see warnings about mismatched sections.Reported-by Vincent Donnefort <[email protected]>Signed-off-by Nathan Chancellor <[email protected]>
This revealed a couple issues in the FG-KASLR kernel. None of them are false-positive although FG-KASLR doesn't merge text.* into one section in the final vmlinux. Nice!
Quoted 3 lines
--- scripts/mod/modpost.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
[...]
Quoted 2 lines
Signature · 1 line
2.38.1
Thanks,
Olek
Quoted 23 lines · Nathan Chancellor
Hi all,
Vincent recently reported an issue with lack of section mismatch warnings with LTO. This is due to commit
6c730bfc894f("modpost: handle -ffunction-sections"), which ignores all function sections for modpost.I believe this is incorrect, as these function sections may still refer to symbols in other sections and they will ultimately be coalesced into .text by
vmlinux.ldsanyways.The first patch fixes a warning that I see with allmodconfig + ThinLTO builds after applying the second patch. The second patch moves ".text.*" into
TEXT_SECTIONSso that modpost audits them for mismatches.I expect this to go via the kbuild tree with an ack from the padata maintainers.
Quoted 7 lines
Nathan Chancellor (2):
padata: Do not markpadata_mt_helper()as__init
modpost: Include '.text.*' inTEXT_SECTIONSkernel/padata.c | 4 ++-- scripts/mod/modpost.c | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-)
These look good to me. Thanks for fixing the issue, Nathan!
Sami