Re: [PATCH v3 2/6] binfmt_elf: Use note name macros

[Dave Martin <Dave.Martin@arm.com>]

Hi,

On Wed, Jan 08, 2025 at 01:34:24PM +0900, Akihiko Odaki wrote:
> On 2025/01/08 1:18, Dave Martin wrote:
> > On Tue, Jan 07, 2025 at 09:45:53PM +0900, Akihiko Odaki wrote:
> > > Use note name macros to match with the userspace's expectation.
> > 
> > Also (and more importantly) get rid of duplicated knowledge about the
> > mapping of note types to note names, so that elf.h is the authoritative
> > source of this information?
> > 
> > > 
> > > Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
> > > Acked-by: Baoquan He <bhe@redhat.com>
> > > ---
> > >   fs/binfmt_elf.c       | 21 ++++++++++-----------
> > >   fs/binfmt_elf_fdpic.c |  8 ++++----
> > >   2 files changed, 14 insertions(+), 15 deletions(-)
> > > 
> > > diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c
> > > index 106f0e8af177..5b4a92e5e508 100644
> > > --- a/fs/binfmt_elf.c
> > > +++ b/fs/binfmt_elf.c
> > 
> > [...]
> > 
> > > @@ -1538,7 +1538,7 @@ static int elf_fdpic_core_dump(struct coredump_params *cprm)
> > >   	do
> > >   		i += 2;
> > >   	while (auxv[i - 2] != AT_NULL);
> > > -	fill_note(&auxv_note, "CORE", NT_AUXV, i * sizeof(elf_addr_t), auxv);
> > > +	fill_note(&auxv_note, NN_AUXV, NT_AUXV, i * sizeof(elf_addr_t), auxv);
> > >   	thread_status_size += notesize(&auxv_note);
> > >   	offset = sizeof(*elf);				/* ELF header */
> > 
> > Looking at this code, it appears that the right name is explicitly
> > taken from elf.h for a few specific notes, but for those that are
> > specified by the arch code (e.g., in struct user_regset entries) the
> > name is still guessed locally:
> > 
> > static int fill_thread_core_info(...) {
> > 
> > ...
> > 
> > 	fill_note(&t->notes[note_iter], is_fpreg ? "CORE" : "LINUX",
> > 		note_type, ret, data);
> > 
> > 
> > It would be preferable to clean this up if we want elf.h to be the
> > authoritative source for the names.
> 
> If we want elf.h to be the authoritative source, yes, but I like the current
> form as it ensures nobody adds a note with a name different from "LINUX" and
> it is also simpler. There is a trade-off so I'd like to keep the current
> form unless anyone has a strong preference for one option.
> 
> Regards,
> Akihiko Odaki

I can see where you're coming from here.

It would be nice to at least be able to check that elf.h is consistent
with the behaviour here, but you're right -- there is a tradeoff.

Maybe add a comment in elf.h at the end of the block of #defines saying
that new Linux-specific entries should use the name "LINUX"?

Either way, I don't think it's a huge deal.  If people are happy with
this code as-is, then I don't have an issue with it.


I might follow up with a separate patch if this series is merged, and
people can consider it on its own merits (or lack thereof).

Cheers
---Dave