In the presence of both weak and strong function definitions, the
linker drops the weak symbol in favor of a strong symbol, but
leaves the code in place. Code in ignore_unreachable_insn() has
some heuristics to suppress the warning, but it does not work when
-ffunction-sections is enabled.

Suppose function foo has both strong and weak definitions.
Case 1: The strong definition has an annotated section name,
like .init.text. Only the weak definition will be placed into
.text.foo. But since the section has no symbols, there will be no
"hole" in the section.

Case 2: Both sections are without an annotated section name.
Both will be placed into .text.foo section, but there will be only one
symbol (the strong one). If the weak code is before the strong code,
there is no "hole" as it fails to find the right-most symbol before
the offset.

The fix is to use the first node to compute the hole if hole.sym
is empty. If there is no symbol in the section, the first node
will be NULL, in which case, -1 is returned to skip the whole
section.

Co-developed-by: Han Shen <shenhan@google.com>
Signed-off-by: Han Shen <shenhan@google.com>
Signed-off-by: Rong Xu <xur@google.com>
Suggested-by: Sriraman Tallam <tmsriram@google.com>
Suggested-by: Krzysztof Pszeniczny <kpszeniczny@google.com>
Tested-by: Yonghong Song <yonghong.song@linux.dev>
Tested-by: Yabin Cui <yabinc@google.com>
Change-Id: Ib3a484b6f056db8ba4f8e91e567e3165bbeb51ea
---
 tools/objtool/elf.c | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/tools/objtool/elf.c b/tools/objtool/elf.c
index 3d27983dc908d..6f64d611faea9 100644
--- a/tools/objtool/elf.c
+++ b/tools/objtool/elf.c
@@ -224,12 +224,17 @@ int find_symbol_hole_containing(const struct section *sec, unsigned long offset)
 	if (n)
 		return 0; /* not a hole */
 
-	/* didn't find a symbol for which @offset is after it */
-	if (!hole.sym)
-		return 0; /* not a hole */
+	/*
+	 * @offset >= sym->offset + sym->len, find symbol after it.
+	 * When hole.sym is empty, use the first node to compute the hole.
+	 * If there is no symbol in the section, the first node will be NULL,
+	 * in which case, -1 is returned to skip the whole section.
+	 */
+	if (hole.sym)
+		n = rb_next(&hole.sym->node);
+	else
+		n = rb_first_cached(&sec->symbol_tree);
 
-	/* @offset >= sym->offset + sym->len, find symbol after it */
-	n = rb_next(&hole.sym->node);
 	if (!n)
 		return -1; /* until end of address space */
 
-- 
2.47.0.163.g1226f6d8fa-goog

On Fri, Oct 25, 2024 at 10:14:04PM -0700, Rong Xu wrote:
> In the presence of both weak and strong function definitions, the
> linker drops the weak symbol in favor of a strong symbol, but
> leaves the code in place. Code in ignore_unreachable_insn() has
> some heuristics to suppress the warning, but it does not work when
> -ffunction-sections is enabled.
> 
> Suppose function foo has both strong and weak definitions.
> Case 1: The strong definition has an annotated section name,
> like .init.text. Only the weak definition will be placed into
> .text.foo. But since the section has no symbols, there will be no
> "hole" in the section.
> 
> Case 2: Both sections are without an annotated section name.
> Both will be placed into .text.foo section, but there will be only one
> symbol (the strong one). If the weak code is before the strong code,
> there is no "hole" as it fails to find the right-most symbol before
> the offset.
> 
> The fix is to use the first node to compute the hole if hole.sym
> is empty. If there is no symbol in the section, the first node
> will be NULL, in which case, -1 is returned to skip the whole
> section.
> 
> Co-developed-by: Han Shen <shenhan@google.com>
> Signed-off-by: Han Shen <shenhan@google.com>

This seems logically correct to me, but I'd love to see review from Josh
and/or Peter Z on this change too.

Reviewed-by: Kees Cook <kees@kernel.org>

-- 
Kees Cook

On Mon, Oct 28, 2024 at 05:16:44PM -0700, Kees Cook wrote:
> On Fri, Oct 25, 2024 at 10:14:04PM -0700, Rong Xu wrote:
> > In the presence of both weak and strong function definitions, the
> > linker drops the weak symbol in favor of a strong symbol, but
> > leaves the code in place. Code in ignore_unreachable_insn() has
> > some heuristics to suppress the warning, but it does not work when
> > -ffunction-sections is enabled.
> > 
> > Suppose function foo has both strong and weak definitions.
> > Case 1: The strong definition has an annotated section name,
> > like .init.text. Only the weak definition will be placed into
> > .text.foo. But since the section has no symbols, there will be no
> > "hole" in the section.
> > 
> > Case 2: Both sections are without an annotated section name.
> > Both will be placed into .text.foo section, but there will be only one
> > symbol (the strong one). If the weak code is before the strong code,
> > there is no "hole" as it fails to find the right-most symbol before
> > the offset.
> > 
> > The fix is to use the first node to compute the hole if hole.sym
> > is empty. If there is no symbol in the section, the first node
> > will be NULL, in which case, -1 is returned to skip the whole
> > section.
> > 
> > Co-developed-by: Han Shen <shenhan@google.com>
> > Signed-off-by: Han Shen <shenhan@google.com>
> 
> This seems logically correct to me, but I'd love to see review from Josh
> and/or Peter Z on this change too.
> 
> Reviewed-by: Kees Cook <kees@kernel.org>

LGTM, thanks!

Acked-by: Josh Poimboeuf <jpoimboe@kernel.org>

-- 
Josh

On 10/28/24 17:16, Kees Cook wrote:
> On Fri, Oct 25, 2024 at 10:14:04PM -0700, Rong Xu wrote:
>> In the presence of both weak and strong function definitions, the
>> linker drops the weak symbol in favor of a strong symbol, but
>> leaves the code in place. Code in ignore_unreachable_insn() has
>> some heuristics to suppress the warning, but it does not work when
>> -ffunction-sections is enabled.
>>
>> Suppose function foo has both strong and weak definitions.
>> Case 1: The strong definition has an annotated section name,
>> like .init.text. Only the weak definition will be placed into
>> .text.foo. But since the section has no symbols, there will be no
>> "hole" in the section.
>>
>> Case 2: Both sections are without an annotated section name.
>> Both will be placed into .text.foo section, but there will be only one
>> symbol (the strong one). If the weak code is before the strong code,
>> there is no "hole" as it fails to find the right-most symbol before
>> the offset.
>>
>> The fix is to use the first node to compute the hole if hole.sym
>> is empty. If there is no symbol in the section, the first node
>> will be NULL, in which case, -1 is returned to skip the whole
>> section.
>>
>> Co-developed-by: Han Shen <shenhan@google.com>
>> Signed-off-by: Han Shen <shenhan@google.com>
> 
> This seems logically correct to me, but I'd love to see review from Josh
> and/or Peter Z on this change too.
> 
> Reviewed-by: Kees Cook <kees@kernel.org>
> 

Does this happen even with -Wl,--gc-sections?

	-hpa

I think the objtool works on individual object files (or vmlinux.o).
The -Wl,--gc-sections flag, on the other hand, is a linker flag that
acts on the final link -- it's applied after objtool invocations.
Therefore, even with -Wl,--gc-sections, we'll still encounter those
spurious warnings from objtool.

-Rong

On Mon, Oct 28, 2024 at 5:19 PM H. Peter Anvin <hpa@zytor.com> wrote:
>
> On 10/28/24 17:16, Kees Cook wrote:
> > On Fri, Oct 25, 2024 at 10:14:04PM -0700, Rong Xu wrote:
> >> In the presence of both weak and strong function definitions, the
> >> linker drops the weak symbol in favor of a strong symbol, but
> >> leaves the code in place. Code in ignore_unreachable_insn() has
> >> some heuristics to suppress the warning, but it does not work when
> >> -ffunction-sections is enabled.
> >>
> >> Suppose function foo has both strong and weak definitions.
> >> Case 1: The strong definition has an annotated section name,
> >> like .init.text. Only the weak definition will be placed into
> >> .text.foo. But since the section has no symbols, there will be no
> >> "hole" in the section.
> >>
> >> Case 2: Both sections are without an annotated section name.
> >> Both will be placed into .text.foo section, but there will be only one
> >> symbol (the strong one). If the weak code is before the strong code,
> >> there is no "hole" as it fails to find the right-most symbol before
> >> the offset.
> >>
> >> The fix is to use the first node to compute the hole if hole.sym
> >> is empty. If there is no symbol in the section, the first node
> >> will be NULL, in which case, -1 is returned to skip the whole
> >> section.
> >>
> >> Co-developed-by: Han Shen <shenhan@google.com>
> >> Signed-off-by: Han Shen <shenhan@google.com>
> >
> > This seems logically correct to me, but I'd love to see review from Josh
> > and/or Peter Z on this change too.
> >
> > Reviewed-by: Kees Cook <kees@kernel.org>
> >
>
> Does this happen even with -Wl,--gc-sections?
>
>         -hpa
>