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

[Akihiko Odaki <akihiko.odaki@daynix.com>]

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

> 
> It would be possible to add a .core_note_name entry in struct
> user_regset, and define a helper macro to populate the note type and
> name, something like the following:
> 
> struct user_regset {
> 	...
> 	unsigned int core_note_type;
> +	unsigned int core_note_name;
> };
> 
> #define USER_REGSET_NOTE_TYPE(type) \
> 	.core_note_type = NT_ ## type, \
> 	.core_note_name = NN_ ## name,
> 
> ...and then replace every .core_note_type assignment with an invocation
> of this macro.  A quick git grep should easily find all the affected
> cases.
> 
> 
> Alternatively, as discussed in the last review round, a helper could
> be defined to get the name for a note type:
> 
> const char *elf_note_name(int Elf32_Word n_type)
> {
> 	switch (n_type) {
> 	case NT_PRSTATUS:	return NN_PRSTATUS;
> 	case NT_PRFPREG:	return NN_PRFPREG;
> 	/* ...and all the rest..., then: */
> 
> 	default:
> 		WARN();
> 		return "LINUX";
> 	}
> }
> 
> This avoids the caller having to specify the name explicitly, but only
> works if all the n_type values are unique for the note types that Linux
> knows about (currently true).
> 
> Experimenting with this shows that GCC 11.4.0 (for example) doesn't do
> a very good job with this switch, though, and it requires building
> knowledge about irrelevant arch-specific note types into every kernel.
> I think that extending struct user_regset is probably the better
> approach -- though other people may disagree.
> 
> Cheers
> ---Dave