Received-SPF: pass (google.com: domain of linux-kernel+bounces-155547-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Precedence: bulk
MIME-Version: 1.0
References: <20240420145303.238068-2-ardb+git@google.com> <CAK7LNARGZZC=5Pcy8qBpp1E94hRHHHdUu7KxVudH1iT-yugs=g@mail.gmail.com>
 <9fda72fa-e5e5-4d45-b268-dd98d28fb5a1@linaro.org>
In-Reply-To: <9fda72fa-e5e5-4d45-b268-dd98d28fb5a1@linaro.org>
From: Ard Biesheuvel <ardb@kernel.org>
Date: Tue, 23 Apr 2024 18:22:10 +0200
Message-ID: <CAMj1kXERSHjh0k9uCmYuNf31Fg79sd_6EHuS=Y_-xEdNGWeiAw@mail.gmail.com>
Subject: Re: [PATCH v5] kallsyms: Avoid weak references for kallsyms symbols
To: Konrad Dybcio <konrad.dybcio@linaro.org>
Cc: Masahiro Yamada <masahiroy@kernel.org>, Ard Biesheuvel <ardb+git@google.com>, 
	linux-kernel@vger.kernel.org, linux-kbuild@vger.kernel.org, 
	Nick Desaulniers <ndesaulniers@google.com>, Kees Cook <keescook@chromium.org>, 
	Arnd Bergmann <arnd@arndb.de>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Tue, 23 Apr 2024 at 18:01, Konrad Dybcio <konrad.dybcio@linaro.org> wrot=
e:
>
>
>
> On 4/22/24 18:02, Masahiro Yamada wrote:
> > On Sat, Apr 20, 2024 at 11:53=E2=80=AFPM Ard Biesheuvel <ardb+git@googl=
e.com> wrote:
> >>
> >> From: Ard Biesheuvel <ardb@kernel.org>
> >>
> >> kallsyms is a directory of all the symbols in the vmlinux binary, and =
so
> >> creating it poses somewhat of a chicken-and-egg problem, as its non-ze=
ro
> >> size affects the layout of the binary, and therefore the values of the
> >> symbols.
> >>
> >> For this reason, the kernel is linked more than once, and the first pa=
ss
> >> does not include any kallsyms data at all. For the linker to accept
> >> this, the symbol declarations describing the kallsyms metadata are
> >> emitted as having weak linkage, so they can remain unsatisfied. During
> >> the subsequent passes, the weak references are satisfied by the kallsy=
ms
> >> metadata that was constructed based on information gathered from the
> >> preceding passes.
> >>
> >> Weak references lead to somewhat worse codegen, because taking their
> >> address may need to produce NULL (if the reference was unsatisfied), a=
nd
> >> this is not usually supported by RIP or PC relative symbol references.
> >>
> >> Given that these references are ultimately always satisfied in the fin=
al
> >> link, let's drop the weak annotation on the declarations, and instead,
> >> provide fallback definitions with weak linkage. This informs the
> >> compiler that ultimately, the reference will always be satisfied.
> >>
> >> While at it, drop the FRV specific annotation that these symbols resid=
e
> >> in .rodata - FRV is long gone.
> >>
> >> Cc: Masahiro Yamada <masahiroy@kernel.org>
> >> Cc: linux-kbuild@vger.kernel.org
> >> Acked-by: Nick Desaulniers <ndesaulniers@google.com>
> >> Acked-by: Kees Cook <keescook@chromium.org>
> >> Acked-by: Arnd Bergmann <arnd@arndb.de>
> >> Link: https://lore.kernel.org/all/20240415075837.2349766-5-ardb+git@go=
ogle.com
> >> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> >> ---
> >> v5: - avoid PROVIDE() in the linker script, use weak definitions inste=
ad
> >>      - drop tested-by, replace reviewed-by with acked-by
> >>
> >
> > Applied to linux-kbuild. Thanks.
>
> Hi, this commit seems to break call traces, resulting in output like:
>
> [    2.777006] Call trace:
> [    2.777007]  _text+0x89e7e8/0x39e0000
> [    2.777008]  _text+0x89e82c/0x39e0000
> [    2.777009]  _text+0x2b940cc/0x2bd2a90
> [    2.777011]  _text+0x2b941a4/0x2bd2a90
> [    2.777012]  _text+0x145dc/0x39e0000
> [    2.777014]  _text+0x2b51184/0x2bd2a90
> [    2.777016]  _text+0x18fc6a4/0x39e0000
> [    2.777018]  _text+0x15644/0x39e0000
> [    2.777019] ---[ end trace 0000000000000000 ]---
>

This patch triggers an issue in the compiler, which appears to perform
constant propagation on variables defined as weak, and this is
definitely a compiler bug. (A weak variable can be superseded by
another instance from a different object at link time, so the compiler
cannot make assumptions based on the version of the variable it
observes at compile time)

It has already been dropped from the kbuild tree.