[PATCH 0/2] Fix lack of section mismatch warnings with LTO

[PATCH 0/2] Fix lack of section mismatch warnings with LTO

[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.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(-)


base-commit: b7b275e60bcd5f89771e865a8239325f86d9927d
-- 
2.38.1

[PATCH 1/2] padata: Do not mark padata_mt_helper() as __init

[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 __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

Re: [PATCH 0/2] Fix lack of section mismatch warnings with LTO

[Sami Tolvanen]

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

Re: [PATCH 1/2] padata: Do not mark padata_mt_helper() as __init

[Masahiro Yamada]

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

Re: [PATCH 1/2] padata: Do not mark padata_mt_helper() as __init

[Masahiro Yamada]

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

Re: [PATCH 1/2] padata: Do not mark padata_mt_helper() as __init

[Nathan Chancellor]

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

Re: [PATCH 1/2] padata: Do not mark padata_mt_helper() as __init

[Daniel Jordan]

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.

Re: [PATCH 1/2] padata: Do not mark padata_mt_helper() as __init

[Nathan Chancellor]

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