[PATCH] elf: Correct note name comment

[PATCH] elf: Correct note name comment

[Akihiko Odaki]

NT_PRSTATUS note is also named "CORE". Correct the comment accordingly.

Fixes: 00e19ceec80b ("ELF: Add ELF program property parsing support")
Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
---
 include/uapi/linux/elf.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/include/uapi/linux/elf.h b/include/uapi/linux/elf.h
index b54b313bcf07..4f00cdca38b2 100644
--- a/include/uapi/linux/elf.h
+++ b/include/uapi/linux/elf.h
@@ -372,8 +372,8 @@ typedef struct elf64_shdr {
  * Notes used in ET_CORE. Architectures export some of the arch register sets
  * using the corresponding note types via the PTRACE_GETREGSET and
  * PTRACE_SETREGSET requests.
- * The note name for these types is "LINUX", except NT_PRFPREG that is named
- * "CORE".
+ * The note name for these types is "LINUX", except NT_PRSTATUS and NT_PRFPREG
+ * that are named "CORE".
  */
 #define NT_PRSTATUS	1
 #define NT_PRFPREG	2

---
base-commit: 752ebcbe87aceeb6334e846a466116197711a982
change-id: 20241210-elf-b80ea3949c39

Best regards,
-- 
Akihiko Odaki <akihiko.odaki@daynix.com>

Re: [PATCH] elf: Correct note name comment

[Dave Martin]

Hi,

On Wed, Dec 25, 2024 at 03:46:44PM +0900, Akihiko Odaki wrote:
> NT_PRSTATUS note is also named "CORE". Correct the comment accordingly.
> 
> Fixes: 00e19ceec80b ("ELF: Add ELF program property parsing support")
> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
> ---
>  include/uapi/linux/elf.h | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/include/uapi/linux/elf.h b/include/uapi/linux/elf.h
> index b54b313bcf07..4f00cdca38b2 100644
> --- a/include/uapi/linux/elf.h
> +++ b/include/uapi/linux/elf.h
> @@ -372,8 +372,8 @@ typedef struct elf64_shdr {
>   * Notes used in ET_CORE. Architectures export some of the arch register sets
>   * using the corresponding note types via the PTRACE_GETREGSET and
>   * PTRACE_SETREGSET requests.
> - * The note name for these types is "LINUX", except NT_PRFPREG that is named
> - * "CORE".
> + * The note name for these types is "LINUX", except NT_PRSTATUS and NT_PRFPREG
> + * that are named "CORE".
>   */
>  #define NT_PRSTATUS	1
>  #define NT_PRFPREG	2

[...]

This still seems rather confusing.  It's not clear which note types are
being referred to in "for these types".  I think this statement was
supposed to refer only to the architectural regset notes.

I guess "CORE" was for generic coredump notes goverened by common
specs, and LINUX was for Linux-specific stuff, but I suspect that this
distinction may have bitrotted.  It looks like the ELF specs never
defined the core dump format, so the concept of non-OS-specific
coredump notes may not make much sense.

The ELF specs _do_ explicitly say [1] that the note name must be taken
into account when identifying the type of a note, so the note name for
each kind if note should really be documented explicitly.

Is it worth adding explicit #defines for the note name of each kind
of note, to make the ABI contract explicit?

Cheers
---Dave


[1] e.g.: System V ABI - DRAFT 24 April 2001
https://refspecs.linuxfoundation.org/elf/gabi4+/contents.html

(Linked from ELF and ABI Standards,
https://refspecs.linuxfoundation.org/elf/index.html )

Re: [PATCH] elf: Correct note name comment

[Akihiko Odaki]

On 2025/01/02 23:40, Dave Martin wrote:
> Hi,
> 
> On Wed, Dec 25, 2024 at 03:46:44PM +0900, Akihiko Odaki wrote:
>> NT_PRSTATUS note is also named "CORE". Correct the comment accordingly.
>>
>> Fixes: 00e19ceec80b ("ELF: Add ELF program property parsing support")
>> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
>> ---
>>   include/uapi/linux/elf.h | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/include/uapi/linux/elf.h b/include/uapi/linux/elf.h
>> index b54b313bcf07..4f00cdca38b2 100644
>> --- a/include/uapi/linux/elf.h
>> +++ b/include/uapi/linux/elf.h
>> @@ -372,8 +372,8 @@ typedef struct elf64_shdr {
>>    * Notes used in ET_CORE. Architectures export some of the arch register sets
>>    * using the corresponding note types via the PTRACE_GETREGSET and
>>    * PTRACE_SETREGSET requests.
>> - * The note name for these types is "LINUX", except NT_PRFPREG that is named
>> - * "CORE".
>> + * The note name for these types is "LINUX", except NT_PRSTATUS and NT_PRFPREG
>> + * that are named "CORE".
>>    */
>>   #define NT_PRSTATUS	1
>>   #define NT_PRFPREG	2
> 
> [...]
> 
> This still seems rather confusing.  It's not clear which note types are
> being referred to in "for these types".  I think this statement was
> supposed to refer only to the architectural regset notes.

I'm not sure about the original intention, but this comment starts with 
"notes used in ET_CORE" so I think it should ideally describe all types 
used in ET_CORE.

> 
> I guess "CORE" was for generic coredump notes goverened by common
> specs, and LINUX was for Linux-specific stuff, but I suspect that this
> distinction may have bitrotted.  It looks like the ELF specs never
> defined the core dump format, so the concept of non-OS-specific
> coredump notes may not make much sense.
> 
> The ELF specs _do_ explicitly say [1] that the note name must be taken
> into account when identifying the type of a note, so the note name for
> each kind if note should really be documented explicitly.
> 
> Is it worth adding explicit #defines for the note name of each kind
> of note, to make the ABI contract explicit?

Maybe so. I don't have a particular idea how such #defines should be 
written though as I don't have actual code that may utilize them.

Regards,
Akihiko Odaki

Re: [PATCH] elf: Correct note name comment

[Dave Martin]

On Fri, Jan 03, 2025 at 02:25:00PM +0900, Akihiko Odaki wrote:
> On 2025/01/02 23:40, Dave Martin wrote:
> > Hi,
> > 
> > On Wed, Dec 25, 2024 at 03:46:44PM +0900, Akihiko Odaki wrote:
> > > NT_PRSTATUS note is also named "CORE". Correct the comment accordingly.
> > > 
> > > Fixes: 00e19ceec80b ("ELF: Add ELF program property parsing support")
> > > Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
> > > ---
> > >   include/uapi/linux/elf.h | 4 ++--
> > >   1 file changed, 2 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/include/uapi/linux/elf.h b/include/uapi/linux/elf.h
> > > index b54b313bcf07..4f00cdca38b2 100644
> > > --- a/include/uapi/linux/elf.h
> > > +++ b/include/uapi/linux/elf.h
> > > @@ -372,8 +372,8 @@ typedef struct elf64_shdr {
> > >    * Notes used in ET_CORE. Architectures export some of the arch register sets
> > >    * using the corresponding note types via the PTRACE_GETREGSET and
> > >    * PTRACE_SETREGSET requests.
> > > - * The note name for these types is "LINUX", except NT_PRFPREG that is named
> > > - * "CORE".
> > > + * The note name for these types is "LINUX", except NT_PRSTATUS and NT_PRFPREG
> > > + * that are named "CORE".
> > >    */
> > >   #define NT_PRSTATUS	1
> > >   #define NT_PRFPREG	2
> > 
> > [...]
> > 
> > This still seems rather confusing.  It's not clear which note types are
> > being referred to in "for these types".  I think this statement was
> > supposed to refer only to the architectural regset notes.
> 
> I'm not sure about the original intention, but this comment starts with
> "notes used in ET_CORE" so I think it should ideally describe all types used
> in ET_CORE.

Perhaps that was the intention, but if so then isn't the statement
wrong?

The following notes (and possibly others) seem to be generated with
name "CORE"; see fs/binfmt_elf.c.

	NT_AUXV
	NT_SIGINFO
	NT_FILE
	NT_PRPSINFO

> > I guess "CORE" was for generic coredump notes goverened by common
> > specs, and LINUX was for Linux-specific stuff, but I suspect that this
> > distinction may have bitrotted.  It looks like the ELF specs never
> > defined the core dump format, so the concept of non-OS-specific
> > coredump notes may not make much sense.
> > 
> > The ELF specs _do_ explicitly say [1] that the note name must be taken
> > into account when identifying the type of a note, so the note name for
> > each kind if note should really be documented explicitly.
> > 
> > Is it worth adding explicit #defines for the note name of each kind
> > of note, to make the ABI contract explicit?
> 
> Maybe so. I don't have a particular idea how such #defines should be written
> though as I don't have actual code that may utilize them.
> 
> Regards,
> Akihiko Odaki

My thinking was that the clearest way to document the name for each
note type would be to state it explicity for each note type, rather
than relying on general statements that are open to misinterpretation
and bitrot.

But from there it seems easy to go one better, and make the statements
machine-readable (i.e., a #define rather than a comment).

A comment for each note type would still be better than nothing,
though.


For an example user, maybe see libbfd's elfcore_grok_note(): (from the
GDB 15.2 release).  gdb probably uses this to parse a core file, though
I've not tried to follow the code path to get there:

https://sourceware.org/git?p=binutils-gdb.git;a=blob;f=bfd/elf.c;h=74236a658fd9a10a61f466d9a2191998c2f4ce06;hb=23c84db5b3cb4e8a0d555c76e1a0ab56dc8355f3#l11187

In the the snippet below [1], I imagine that "NN_foo" #defines have
been added for the note names.  There seems already to be a precedent
for this in one case [2], but it would make more sense to have this all
in one place.

Interestingly, this code ignores the note name for "CORE" notes, which
looks like a potential bug (if the ELF spec is followed strictly).

Cheers
---Dave


[1]:

@@ -11222,14 +11222,14 @@ elfcore_grok_note (bfd *abfd, Elf_Internal_Note *note)
 
     case NT_PRXFPREG:		/* Linux SSE extension */
       if (note->namesz == 6
-	  && strcmp (note->namedata, "LINUX") == 0)
+	  && strcmp (note->namedata, NN_PRXFPREG) == 0)
 	return elfcore_grok_prxfpreg (abfd, note);
       else
 	return true;
 
     case NT_X86_XSTATE:		/* Linux XSAVE extension */
       if (note->namesz == 6
-	  && strcmp (note->namedata, "LINUX") == 0)
+	  && strcmp (note->namedata, NN_X86_XSTATE) == 0)
 	return elfcore_grok_xstatereg (abfd, note);
       else
 	return true;

[2]

linux/include/uapi/linux/vmcore.h:

<snip>

#define VMCOREDD_NOTE_NAME "LINUX"

</snip>