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.
Cc: Steffen Klassert <[email protected]>
Cc: Daniel Jordan <[email protected]>
Cc: [email protected]
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
--
2.38.1
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.
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(-)
diff --git a/kernel/padata.c b/kernel/padata.c
index e5819bb8bd1d..c2271d7e446d 100644
--- a/kernel/padata.c
+++ b/kernel/padata.c
@@ -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
On Tue, Nov 29, 2022 at 11:02 AM Nathan Chancellor <[email protected]> wrote:
>
>
> 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.
>
> Cc: Steffen Klassert <[email protected]>
> Cc: Daniel Jordan <[email protected]>
> Cc: [email protected]
>
> 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(-)
These look good to me. Thanks for fixing the issue, Nathan!
Reviewed-by: Sami Tolvanen <[email protected]>
Sami
On Wed, Nov 30, 2022 at 4:02 AM Nathan Chancellor <[email protected]> wrote:
>
> 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.
>
> 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(-)
>
> diff --git a/kernel/padata.c b/kernel/padata.c
> index e5819bb8bd1d..c2271d7e446d 100644
> --- a/kernel/padata.c
> +++ b/kernel/padata.c
> @@ -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
>
This patch seems wrong.
padata_work_init() does not reference to padata_mt_helper()
padata_work_alloc_mt() and padata_do_multithreaded() do.
--
Best Regards
Masahiro Yamada
On Thu, Dec 01, 2022 at 07:20:47AM +0900, Masahiro Yamada wrote:
> On Wed, Nov 30, 2022 at 4:02 AM Nathan Chancellor <[email protected]> wrote:
> >
> > 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 ?)
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 foobar
Symbol 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 foobar
Symbol 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 bar
> 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?
>
>
>
>
>
> >
> > 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.
> >
> > 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(-)
> >
> > diff --git a/kernel/padata.c b/kernel/padata.c
> > index e5819bb8bd1d..c2271d7e446d 100644
> > --- a/kernel/padata.c
> > +++ b/kernel/padata.c
> > @@ -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
> >
>
>
> --
> Best Regards
> Masahiro Yamada
On Thu, Dec 01, 2022 at 07:35:59AM +0900, Masahiro Yamada wrote:
> On Wed, Nov 30, 2022 at 4:02 AM Nathan Chancellor <[email protected]> wrote:
> >
> > 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.
> >
> > 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(-)
> >
> > diff --git a/kernel/padata.c b/kernel/padata.c
> > index e5819bb8bd1d..c2271d7e446d 100644
> > --- a/kernel/padata.c
> > +++ b/kernel/padata.c
> > @@ -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
> >
>
> This patch seems wrong.
>
> padata_work_init() does not reference to padata_mt_helper()
>
>
> padata_work_alloc_mt() and padata_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.
On Tue, Dec 06, 2022 at 03:15:26PM -0500, Daniel Jordan wrote:
> On Thu, Dec 01, 2022 at 07:35:59AM +0900, Masahiro Yamada wrote:
> > On Wed, Nov 30, 2022 at 4:02 AM Nathan Chancellor <[email protected]> wrote:
> > >
> > > 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.
> > >
> > > 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(-)
> > >
> > > diff --git a/kernel/padata.c b/kernel/padata.c
> > > index e5819bb8bd1d..c2271d7e446d 100644
> > > --- a/kernel/padata.c
> > > +++ b/kernel/padata.c
> > > @@ -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
> > >
> >
> > This patch seems wrong.
> >
> > padata_work_init() does not reference to padata_mt_helper()
> >
> >
> > padata_work_alloc_mt() and padata_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.
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