Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Thu, 20 Oct 2022 10:53:50 -0700
From:   Nathan Chancellor <nathan@kernel.org>
To:     "Li, Xin3" <xin3.li@intel.com>
Cc:     Kees Cook <keescook@chromium.org>,
        "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>,
        "llvm@lists.linux.dev" <llvm@lists.linux.dev>,
        "linux-kbuild@vger.kernel.org" <linux-kbuild@vger.kernel.org>
Subject: Re: upgrade the orphan section warning to a hard link error
Message-ID: <Y1GLLnYsEC8lYTdp@dev-arch.thelio-3990X>
References: <BN6PR1101MB216105D169D482FC8C539059A8269@BN6PR1101MB2161.namprd11.prod.outlook.com>
 <Y02eZ6A/vlj8+B/c@dev-arch.thelio-3990X>
 <202210171230.CC40461C@keescook>
 <Y02zWFxC92VDSpdZ@dev-arch.thelio-3990X>
 <BN6PR1101MB216126260E3A9AFEE3F9CFB8A82A9@BN6PR1101MB2161.namprd11.prod.outlook.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <BN6PR1101MB216126260E3A9AFEE3F9CFB8A82A9@BN6PR1101MB2161.namprd11.prod.outlook.com>
Precedence: bulk

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