* [PATCH 1/2] padata: Do not mark padata_mt_helper() as __init
2022-11-29 19:01 [PATCH 0/2] Fix lack of section mismatch warnings with LTO Nathan Chancellor
@ 2022-11-29 19:01 ` Nathan Chancellor
2022-11-30 22:20 ` Masahiro Yamada
2022-11-30 22:35 ` Masahiro Yamada
2022-11-29 19:01 ` [PATCH 2/2] modpost: Include '.text.*' in TEXT_SECTIONS Nathan Chancellor
2022-11-29 19:21 ` [PATCH 0/2] Fix lack of section mismatch warnings with LTO Sami Tolvanen
2 siblings, 2 replies; 11+ messages in thread
From: Nathan Chancellor @ 2022-11-29 19:01 UTC (permalink / raw)
To: Masahiro Yamada
Cc: Nick Desaulniers, Tom Rix, Nicolas Schier, Sami Tolvanen,
Vincent Donnefort, linux-kbuild, linux-kernel, llvm, patches,
Nathan Chancellor, Steffen Klassert, Daniel Jordan, linux-crypto
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 <nathan@kernel.org>
---
Cc: Steffen Klassert <steffen.klassert@secunet.com>
Cc: Daniel Jordan <daniel.m.jordan@oracle.com>
Cc: linux-crypto@vger.kernel.org
---
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
^ permalink raw reply related [flat|nested] 11+ messages in thread* Re: [PATCH 1/2] padata: Do not mark padata_mt_helper() as __init
2022-11-29 19:01 ` [PATCH 1/2] padata: Do not mark padata_mt_helper() as __init Nathan Chancellor
@ 2022-11-30 22:20 ` Masahiro Yamada
2022-11-30 22:37 ` Nathan Chancellor
2022-11-30 22:35 ` Masahiro Yamada
1 sibling, 1 reply; 11+ messages in thread
From: Masahiro Yamada @ 2022-11-30 22:20 UTC (permalink / raw)
To: Nathan Chancellor
Cc: Nick Desaulniers, Tom Rix, Nicolas Schier, Sami Tolvanen,
Vincent Donnefort, linux-kbuild, linux-kernel, llvm, patches,
Steffen Klassert, Daniel Jordan, linux-crypto
On Wed, Nov 30, 2022 at 4:02 AM Nathan Chancellor <nathan@kernel.org> 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 ?)
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 <nathan@kernel.org>
> ---
> Cc: Steffen Klassert <steffen.klassert@secunet.com>
> Cc: Daniel Jordan <daniel.m.jordan@oracle.com>
> Cc: linux-crypto@vger.kernel.org
> ---
> 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
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH 1/2] padata: Do not mark padata_mt_helper() as __init
2022-11-30 22:20 ` Masahiro Yamada
@ 2022-11-30 22:37 ` Nathan Chancellor
0 siblings, 0 replies; 11+ messages in thread
From: Nathan Chancellor @ 2022-11-30 22:37 UTC (permalink / raw)
To: Masahiro Yamada
Cc: Nick Desaulniers, Tom Rix, Nicolas Schier, Sami Tolvanen,
Vincent Donnefort, linux-kbuild, linux-kernel, llvm, patches,
Steffen Klassert, Daniel Jordan, linux-crypto
On Thu, Dec 01, 2022 at 07:20:47AM +0900, Masahiro Yamada wrote:
> On Wed, Nov 30, 2022 at 4:02 AM Nathan Chancellor <nathan@kernel.org> 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 <nathan@kernel.org>
> > ---
> > Cc: Steffen Klassert <steffen.klassert@secunet.com>
> > Cc: Daniel Jordan <daniel.m.jordan@oracle.com>
> > Cc: linux-crypto@vger.kernel.org
> > ---
> > 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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2] padata: Do not mark padata_mt_helper() as __init
2022-11-29 19:01 ` [PATCH 1/2] padata: Do not mark padata_mt_helper() as __init Nathan Chancellor
2022-11-30 22:20 ` Masahiro Yamada
@ 2022-11-30 22:35 ` Masahiro Yamada
2022-12-06 20:15 ` Daniel Jordan
1 sibling, 1 reply; 11+ messages in thread
From: Masahiro Yamada @ 2022-11-30 22:35 UTC (permalink / raw)
To: Nathan Chancellor
Cc: Nick Desaulniers, Tom Rix, Nicolas Schier, Sami Tolvanen,
Vincent Donnefort, linux-kbuild, linux-kernel, llvm, patches,
Steffen Klassert, Daniel Jordan, linux-crypto
On Wed, Nov 30, 2022 at 4:02 AM Nathan Chancellor <nathan@kernel.org> 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 <nathan@kernel.org>
> ---
> Cc: Steffen Klassert <steffen.klassert@secunet.com>
> Cc: Daniel Jordan <daniel.m.jordan@oracle.com>
> Cc: linux-crypto@vger.kernel.org
> ---
> 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
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH 1/2] padata: Do not mark padata_mt_helper() as __init
2022-11-30 22:35 ` Masahiro Yamada
@ 2022-12-06 20:15 ` Daniel Jordan
2022-12-07 18:58 ` Nathan Chancellor
0 siblings, 1 reply; 11+ messages in thread
From: Daniel Jordan @ 2022-12-06 20:15 UTC (permalink / raw)
To: Masahiro Yamada
Cc: Nathan Chancellor, Nick Desaulniers, Tom Rix, Nicolas Schier,
Sami Tolvanen, Vincent Donnefort, linux-kbuild, linux-kernel,
llvm, patches, Steffen Klassert, linux-crypto
On Thu, Dec 01, 2022 at 07:35:59AM +0900, Masahiro Yamada wrote:
> On Wed, Nov 30, 2022 at 4:02 AM Nathan Chancellor <nathan@kernel.org> 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 <nathan@kernel.org>
> > ---
> > Cc: Steffen Klassert <steffen.klassert@secunet.com>
> > Cc: Daniel Jordan <daniel.m.jordan@oracle.com>
> > Cc: linux-crypto@vger.kernel.org
> > ---
> > 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.
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH 1/2] padata: Do not mark padata_mt_helper() as __init
2022-12-06 20:15 ` Daniel Jordan
@ 2022-12-07 18:58 ` Nathan Chancellor
0 siblings, 0 replies; 11+ messages in thread
From: Nathan Chancellor @ 2022-12-07 18:58 UTC (permalink / raw)
To: Daniel Jordan
Cc: Masahiro Yamada, Nick Desaulniers, Tom Rix, Nicolas Schier,
Sami Tolvanen, Vincent Donnefort, linux-kbuild, linux-kernel,
llvm, patches, Steffen Klassert, linux-crypto
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 <nathan@kernel.org> 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 <nathan@kernel.org>
> > > ---
> > > Cc: Steffen Klassert <steffen.klassert@secunet.com>
> > > Cc: Daniel Jordan <daniel.m.jordan@oracle.com>
> > > Cc: linux-crypto@vger.kernel.org
> > > ---
> > > 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
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 2/2] modpost: Include '.text.*' in TEXT_SECTIONS
2022-11-29 19:01 [PATCH 0/2] Fix lack of section mismatch warnings with LTO Nathan Chancellor
2022-11-29 19:01 ` [PATCH 1/2] padata: Do not mark padata_mt_helper() as __init Nathan Chancellor
@ 2022-11-29 19:01 ` Nathan Chancellor
2022-11-30 12:23 ` Vincent Donnefort
2022-12-02 14:44 ` Alexander Lobakin
2022-11-29 19:21 ` [PATCH 0/2] Fix lack of section mismatch warnings with LTO Sami Tolvanen
2 siblings, 2 replies; 11+ messages in thread
From: Nathan Chancellor @ 2022-11-29 19:01 UTC (permalink / raw)
To: Masahiro Yamada
Cc: Nick Desaulniers, Tom Rix, Nicolas Schier, Sami Tolvanen,
Vincent Donnefort, linux-kbuild, linux-kernel, llvm, patches,
Nathan Chancellor
Commit 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.
Link: https://lore.kernel.org/Y39kI3MOtVI5BAnV@google.com/
Reported-by: Vincent Donnefort <vdonnefort@google.com>
Signed-off-by: Nathan Chancellor <nathan@kernel.org>
---
scripts/mod/modpost.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
index 2c80da0220c3..c861beabc128 100644
--- a/scripts/mod/modpost.c
+++ b/scripts/mod/modpost.c
@@ -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.*"
--
2.38.1
^ permalink raw reply related [flat|nested] 11+ messages in thread* Re: [PATCH 2/2] modpost: Include '.text.*' in TEXT_SECTIONS
2022-11-29 19:01 ` [PATCH 2/2] modpost: Include '.text.*' in TEXT_SECTIONS Nathan Chancellor
@ 2022-11-30 12:23 ` Vincent Donnefort
2022-12-02 14:44 ` Alexander Lobakin
1 sibling, 0 replies; 11+ messages in thread
From: Vincent Donnefort @ 2022-11-30 12:23 UTC (permalink / raw)
To: Nathan Chancellor
Cc: Masahiro Yamada, Nick Desaulniers, Tom Rix, Nicolas Schier,
Sami Tolvanen, linux-kbuild, linux-kernel, llvm, patches
On Tue, Nov 29, 2022 at 12:01:23PM -0700, Nathan Chancellor wrote:
> Commit 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.
>
> Link: https://lore.kernel.org/Y39kI3MOtVI5BAnV@google.com/
> Reported-by: Vincent Donnefort <vdonnefort@google.com>
> Signed-off-by: Nathan Chancellor <nathan@kernel.org>
Thanks!
Tested-by: Vincent Donnefort <vdonnefort@google.com>
> ---
> scripts/mod/modpost.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
> index 2c80da0220c3..c861beabc128 100644
> --- a/scripts/mod/modpost.c
> +++ b/scripts/mod/modpost.c
> @@ -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.*"
> --
> 2.38.1
>
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH 2/2] modpost: Include '.text.*' in TEXT_SECTIONS
2022-11-29 19:01 ` [PATCH 2/2] modpost: Include '.text.*' in TEXT_SECTIONS Nathan Chancellor
2022-11-30 12:23 ` Vincent Donnefort
@ 2022-12-02 14:44 ` Alexander Lobakin
1 sibling, 0 replies; 11+ messages in thread
From: Alexander Lobakin @ 2022-12-02 14:44 UTC (permalink / raw)
To: Nathan Chancellor
Cc: Alexander Lobakin, Masahiro Yamada, Nick Desaulniers, Tom Rix,
Nicolas Schier, Sami Tolvanen, Vincent Donnefort, linux-kbuild,
linux-kernel, llvm, patches
From: Nathan Chancellor <nathan@kernel.org>
Date: Tue, 29 Nov 2022 12:01:23 -0700
> Commit 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.
>
> Link: https://lore.kernel.org/Y39kI3MOtVI5BAnV@google.com/
> Reported-by: Vincent Donnefort <vdonnefort@google.com>
> Signed-off-by: Nathan Chancellor <nathan@kernel.org>
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!
Reviewed-and-tested-by: Alexander Lobakin <alexandr.lobakin@intel.com>
> ---
> scripts/mod/modpost.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
[...]
> --
> 2.38.1
Thanks,
Olek
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 0/2] Fix lack of section mismatch warnings with LTO
2022-11-29 19:01 [PATCH 0/2] Fix lack of section mismatch warnings with LTO Nathan Chancellor
2022-11-29 19:01 ` [PATCH 1/2] padata: Do not mark padata_mt_helper() as __init Nathan Chancellor
2022-11-29 19:01 ` [PATCH 2/2] modpost: Include '.text.*' in TEXT_SECTIONS Nathan Chancellor
@ 2022-11-29 19:21 ` Sami Tolvanen
2 siblings, 0 replies; 11+ messages in thread
From: Sami Tolvanen @ 2022-11-29 19:21 UTC (permalink / raw)
To: Nathan Chancellor
Cc: Masahiro Yamada, Nick Desaulniers, Tom Rix, Nicolas Schier,
Vincent Donnefort, linux-kbuild, linux-kernel, llvm, patches,
Steffen Klassert, Daniel Jordan, linux-crypto
On Tue, Nov 29, 2022 at 11:02 AM Nathan Chancellor <nathan@kernel.org> 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 <steffen.klassert@secunet.com>
> Cc: Daniel Jordan <daniel.m.jordan@oracle.com>
> Cc: linux-crypto@vger.kernel.org
>
> 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 <samitolvanen@google.com>
Sami
^ permalink raw reply [flat|nested] 11+ messages in thread