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

Akihiko Odaki
2025-01-08T04:34:31Z

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