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

From: Nathan Chancellor <nathan@kernel.org>
To: "Li, Xin3" <xin3.li@intel.com>
Cc: "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"linux-arch@vger.kernel.org" <linux-arch@vger.kernel.org>,
	"H.Peter Anvin" <hpa@zytor.com>,
	Peter Zijlstra <peterz@infradead.org>,
	Kees Cook <keescook@chromium.org>,
	llvm@lists.linux.dev, linux-kbuild@vger.kernel.org
Subject: Re: upgrade the orphan section warning to a hard link error
Date: Mon, 17 Oct 2022 11:26:47 -0700	[thread overview]
Message-ID: <Y02eZ6A/vlj8+B/c@dev-arch.thelio-3990X> (raw)
In-Reply-To: <BN6PR1101MB216105D169D482FC8C539059A8269@BN6PR1101MB2161.namprd11.prod.outlook.com>

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
