Re: [PATCH] recordmcount: avoid using ABS symbol as reference

[Peter Zijlstra]

On Mon, Jun 07, 2021 at 11:50:40AM +0200, Ard Biesheuvel wrote:
> On Mon, 7 Jun 2021 at 10:06, Mark-PK Tsai <mark-pk.tsai@xxxxxxxxxxxx> wrote:
> >
> > > > On Mon, 7 Jun 2021 at 08:59, Mark-PK Tsai <mark-pk.tsai@xxxxxxxxxxxx> wrote:
> > > > >
> > > > > > > On Mon, 7 Jun 2021 at 04:42, Mark-PK Tsai <mark-pk.tsai@xxxxxxxxxxxx> wrote:
> > > > > > >
> > > > > > > Avoid using ABS symbol, which won't be relocate, as reference.
> > > > > > >
> > > > > > > On arm64 platform, if there's shndx equals SHN_ABS(0xfff1).
> > > > > > >
> > > > > > > Section Headers:
> > > > > > > [Nr]    Name                         Type      Address          Off      Size   ES  Flg Lk     Inf    Al
> > > > > > > [65521] .text.n_tty_receive_buf      PROGBITS  0000000000000000 3cdab520 000054 00  AX  0      0      4
> > > > > > > [65522] .rela.text.n_tty_receive_buf RELA      0000000000000000 3cdab578 000030 18  I   152076 65521  8
> > > > > > >
> > > > > >
> > > > > > A RELA section's r_info field points to the section to which it
> > > > > > applies. This is why in the example above section #65522 points to
> > > > > > section #65521. This has nothing to do with the numerical value of
> > > > > > SHN_ABS.
> > > > >
> > > > > If the r_info of RELA section is 65521(0xfff1),
> > >
> > > Oh sorry, I mean sh_info here.
> > >
> > > > > find_secsym_ndx() will use it to find the base symbol.
> > > > >
> > > >
> > > > But what does that have to do with the sh_info field of the RELA
> > > > section's Elf_Shdr struct? IOW, what is the relevance of section
> > > > #65521 here?
> > > >
> > >
> > > So what I mean is the problem occur if the sh_info of a RELA section
> > > is #65521.
> >
> > Actually the problem occur if the sh_info of a RELA section is in
> > the special section index range(SHN_LORESERVE ~ SHN_HIRESERVE).
> > Maybe I should add a is_shndx_special() to check this like
> > scripts/mod/modpost.h did?
> >
> 
> So if I understand all of this correctly, we are running into a
> fundamental issue here, where the linker emits more sections than the
> sh_info field can describe, overflowing into the reserved range.
> 
> I don't think papering over it like this is going to be maintainable
> going forward.

There's an extended section header index section for just that. And
recordmcount actually seems to use that as well.

I can't seem to find enough of the thread to figure out what the actual
problem is though. The lore archive doesn't have anything prior to this
message.

One should only use st_shndx when >SHN_UDEF and <SHN_LORESERVE. When
SHN_XINDEX, then use .symtab_shndx.

Apparently you've found a case where neither is true? In that case
objtool seems to use shndx 0. A matching recordmcount patch would be
something like this.


diff --git a/scripts/recordmcount.h b/scripts/recordmcount.h
index f9b19524da11..d99cc0aed6fe 100644
--- a/scripts/recordmcount.h
+++ b/scripts/recordmcount.h
@@ -194,13 +194,18 @@ static unsigned int get_symindex(Elf_Sym const *sym, Elf32_Word const *symtab,
 	unsigned long offset;
 	int index;
 
-	if (sym->st_shndx != SHN_XINDEX)
+	if (sym->st_shndx > SHN_UDEF &&
+	    sym->st_shndx < SHN_LORESERVE)
 		return w2(sym->st_shndx);
 
-	offset = (unsigned long)sym - (unsigned long)symtab;
-	index = offset / sizeof(*sym);
+	if (sym->st_shndx == SHN_XINDEX) {
+		offset = (unsigned long)sym - (unsigned long)symtab;
+		index = offset / sizeof(*sym);
 
-	return w(symtab_shndx[index]);
+		return w(symtab_shndx[index]);
+	}
+
+	return 0;
 }
 
 static unsigned int get_shnum(Elf_Ehdr const *ehdr, Elf_Shdr const *shdr0)