Re: upgrade the orphan section warning to a hard link error

Re: upgrade the orphan section warning to a hard link error

[Nathan Chancellor]

Hi Xin,

On Sun, Oct 16, 2022 at 06:28:27AM +0000, Li, Xin3 wrote:
> Arch maintainers,
> 
> We plan to upgrade the orphan section warning to a hard link error on x86.
> And I found the most recent related commit 59612b24f78a0
> ("kbuild: Hoist '--orphan-handling' into Kconfig") from by Nathan Chancellor
> has the following statement in the help section:
>  
> +config ARCH_WANT_LD_ORPHAN_WARN
> +       bool
> +       help
> +         An arch should select this symbol once all linker sections are explicitly
> +         included, size-asserted, or discarded in the linker scripts. This is
> +         important because we never want expected sections to be placed heuristically
> +         by the linker, since the locations of such sections can change between linker
> +         versions.
> +

+ Kees, who did the heavy lifting to enable '--orphan-handling=warn' for
arm64 and x86, and the ClangBuiltLinux and linux-kbuild mailing lists.
Unfortunately, for some reason, I do not see the original posting on
lore but I left the full message intact for further discussion.

> It looks to me that it actually suggests a link error rather than a warning,
> so the question is, should we do the upgrade on all architectures with
> the orphan section warning?

It might be interesting to turn orphan sections into an error if
CONFIG_WERROR is set. Perhaps something like the following (FYI, not
even compile tested)?

diff --git a/Makefile b/Makefile
index 0837445110fc..485f47fc2c07 100644
--- a/Makefile
+++ b/Makefile
@@ -1119,7 +1119,7 @@ endif
 # We never want expected sections to be placed heuristically by the
 # linker. All sections should be explicitly named in the linker script.
 ifdef CONFIG_LD_ORPHAN_WARN
-LDFLAGS_vmlinux += --orphan-handling=warn
+LDFLAGS_vmlinux += --orphan-handling=$(if $(CONFIG_WERROR),error,warn)
 endif
 
 # Align the bit size of userspace programs with the kernel

Outright turning the warning into an error with no escape hatch might be
too aggressive, as we have had these warnings triggered by new compiler
generated sections, such as in commit 848378812e40 ("vmlinux.lds.h:
Handle clang's module.{c,d}tor sections"). Unconditionally breaking the
build in these situations is unfortunate but the warnings do need to be
dealt with so I think having it error by default with the ability to
opt-out is probably worth doing. I do not have a strong opinion though.

> BTW, the following architectures enable orphan section warning,
> arm/arm64/hexagon/loongarch/mips/	powerpc/x86,
> while all other architectures just ignore it.

Right, every architecture should eventually select
CONFIG_ARCH_WANT_LD_ORPHAN_WARN so that they get this warning as well.
It is just making architecture maintainers aware of it so they can look
into it.

Cheers,
Nathan

Re: upgrade the orphan section warning to a hard link error

[Kees Cook]

On Mon, Oct 17, 2022 at 11:26:47AM -0700, Nathan Chancellor wrote:
> It might be interesting to turn orphan sections into an error if
> CONFIG_WERROR is set. Perhaps something like the following (FYI, not
> even compile tested)?
> 
> diff --git a/Makefile b/Makefile
> index 0837445110fc..485f47fc2c07 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -1119,7 +1119,7 @@ endif
>  # We never want expected sections to be placed heuristically by the
>  # linker. All sections should be explicitly named in the linker script.
>  ifdef CONFIG_LD_ORPHAN_WARN
> -LDFLAGS_vmlinux += --orphan-handling=warn
> +LDFLAGS_vmlinux += --orphan-handling=$(if $(CONFIG_WERROR),error,warn)
>  endif

Yes, this is much preferred.

> Outright turning the warning into an error with no escape hatch might be
> too aggressive, as we have had these warnings triggered by new compiler
> generated sections, such as in commit 848378812e40 ("vmlinux.lds.h:
> Handle clang's module.{c,d}tor sections"). Unconditionally breaking the
> build in these situations is unfortunate but the warnings do need to be
> dealt with so I think having it error by default with the ability to
> opt-out is probably worth doing. I do not have a strong opinion though.

Correct; the mandate from Linus (disregarding his addition of
CONFIG_WERROR for all*config builds), is that we should avoid breaking
builds. It wrecks bisection, it causes problems across compiler versions,
etc.

So, yes, only on CONFIG_WERROR=y.

-- 
Kees Cook

Re: upgrade the orphan section warning to a hard link error

[Nathan Chancellor]

On Mon, Oct 17, 2022 at 12:32:39PM -0700, Kees Cook wrote:
> On Mon, Oct 17, 2022 at 11:26:47AM -0700, Nathan Chancellor wrote:
> > It might be interesting to turn orphan sections into an error if
> > CONFIG_WERROR is set. Perhaps something like the following (FYI, not
> > even compile tested)?
> > 
> > diff --git a/Makefile b/Makefile
> > index 0837445110fc..485f47fc2c07 100644
> > --- a/Makefile
> > +++ b/Makefile
> > @@ -1119,7 +1119,7 @@ endif
> >  # We never want expected sections to be placed heuristically by the
> >  # linker. All sections should be explicitly named in the linker script.
> >  ifdef CONFIG_LD_ORPHAN_WARN
> > -LDFLAGS_vmlinux += --orphan-handling=warn
> > +LDFLAGS_vmlinux += --orphan-handling=$(if $(CONFIG_WERROR),error,warn)
> >  endif
> 
> Yes, this is much preferred.
> 
> > Outright turning the warning into an error with no escape hatch might be
> > too aggressive, as we have had these warnings triggered by new compiler
> > generated sections, such as in commit 848378812e40 ("vmlinux.lds.h:
> > Handle clang's module.{c,d}tor sections"). Unconditionally breaking the
> > build in these situations is unfortunate but the warnings do need to be
> > dealt with so I think having it error by default with the ability to
> > opt-out is probably worth doing. I do not have a strong opinion though.
> 
> Correct; the mandate from Linus (disregarding his addition of
> CONFIG_WERROR for all*config builds), is that we should avoid breaking
> builds. It wrecks bisection, it causes problems across compiler versions,
> etc.
> 
> So, yes, only on CONFIG_WERROR=y.

We would probably want to alter the text of CONFIG_WERROR in some manner
to convey this, perhaps like so:

diff --git a/init/Kconfig b/init/Kconfig
index a19314933e54..1fc03e4b2af2 100644
--- a/init/Kconfig
+++ b/init/Kconfig
@@ -165,10 +165,12 @@ config WERROR
 	help
 	  A kernel build should not cause any compiler warnings, and this
 	  enables the '-Werror' (for C) and '-Dwarnings' (for Rust) flags
-	  to enforce that rule by default.
+	  to enforce that rule by default. Certain warnings from other tools
+	  such as the linker may be upgraded to errors with this option as
+	  well.
 
-	  However, if you have a new (or very old) compiler with odd and
-	  unusual warnings, or you have some architecture with problems,
+	  However, if you have a new (or very old) compiler or linker with odd
+	  and unusual warnings, or you have some architecture with problems,
 	  you may need to disable this config option in order to
 	  successfully build the kernel.
 
---

Cheers,
Nathan

Re: upgrade the orphan section warning to a hard link error

[Nathan Chancellor]

On Thu, Oct 20, 2022 at 05:17:35AM +0000, Li, Xin3 wrote:
> Hi Nathan,
> 
> > On Mon, Oct 17, 2022 at 12:32:39PM -0700, Kees Cook wrote:
> > > On Mon, Oct 17, 2022 at 11:26:47AM -0700, Nathan Chancellor wrote:
> > > > It might be interesting to turn orphan sections into an error if
> > > > CONFIG_WERROR is set. Perhaps something like the following (FYI, not
> > > > even compile tested)?
> > > >
> > > > diff --git a/Makefile b/Makefile
> > > > index 0837445110fc..485f47fc2c07 100644
> > > > --- a/Makefile
> > > > +++ b/Makefile
> > > > @@ -1119,7 +1119,7 @@ endif
> > > >  # We never want expected sections to be placed heuristically by the
> > > > # linker. All sections should be explicitly named in the linker script.
> > > >  ifdef CONFIG_LD_ORPHAN_WARN
> > > > -LDFLAGS_vmlinux += --orphan-handling=warn
> > > > +LDFLAGS_vmlinux += --orphan-handling=$(if
> > > > +$(CONFIG_WERROR),error,warn)
> > > >  endif
> > >
> > > Yes, this is much preferred.
> > >
> > > > Outright turning the warning into an error with no escape hatch
> > > > might be too aggressive, as we have had these warnings triggered by
> > > > new compiler generated sections, such as in commit 848378812e40
> > ("vmlinux.lds.h:
> > > > Handle clang's module.{c,d}tor sections"). Unconditionally breaking
> > > > the build in these situations is unfortunate but the warnings do
> > > > need to be dealt with so I think having it error by default with the
> > > > ability to opt-out is probably worth doing. I do not have a strong opinion
> > though.
> > >
> > > Correct; the mandate from Linus (disregarding his addition of
> > > CONFIG_WERROR for all*config builds), is that we should avoid breaking
> > > builds. It wrecks bisection, it causes problems across compiler
> > > versions, etc.
> > >
> > > So, yes, only on CONFIG_WERROR=y.
> > 
> > We would probably want to alter the text of CONFIG_WERROR in some manner
> > to convey this, perhaps like so:
> > 
> > diff --git a/init/Kconfig b/init/Kconfig index a19314933e54..1fc03e4b2af2
> > 100644
> > --- a/init/Kconfig
> > +++ b/init/Kconfig
> > @@ -165,10 +165,12 @@ config WERROR
> >  	help
> >  	  A kernel build should not cause any compiler warnings, and this
> >  	  enables the '-Werror' (for C) and '-Dwarnings' (for Rust) flags
> > -	  to enforce that rule by default.
> > +	  to enforce that rule by default. Certain warnings from other tools
> > +	  such as the linker may be upgraded to errors with this option as
> > +	  well.
> > 
> > -	  However, if you have a new (or very old) compiler with odd and
> > -	  unusual warnings, or you have some architecture with problems,
> > +	  However, if you have a new (or very old) compiler or linker with odd
> > +	  and unusual warnings, or you have some architecture with problems,
> >  	  you may need to disable this config option in order to
> >  	  successfully build the kernel.
> 
> Thanks a lot for making this crystal clear.
> 
> Do you want me to continue?  Or maybe it's easier for you to complete it?

Sure, I think it is reasonable for you to continue with this as you
brought up the idea initially! Feel free to just take those diffs
wholesale if they work and stick a

    Suggested-by: Nathan Chancellor <nathan@kernel.org>

or

    Co-developed-by: Nathan Chancellor <nathan@kernel.org>
    Signed-off-by: Nathan Chancellor <nathan@kernel.org>

on the patch if you are so inclined or rework them in a way you see fit,
I do not have a strong opinion.

> I will need to find resources to test the patch on other platforms besides x86.

In theory, we should have already cleaned up all these warnings when we
enabled CONFIG_LD_ORPHAN_WARN for all these architectures, so that
change should be a no-op. More testing is never a bad idea though :)

I can throw it into my LLVM testing matrix as well.

Cheers,
Nathan