On Wed, 14 Jan 2015, James Cowgill wrote:
> Commit 90cee759f08a ("MIPS: ELF: Set FP mode according to .MIPS.abiflags")
> introduced checking of the .MIPS.abiflags ELF section but did so through
> the native sized "elfhdr" and "elf_phdr" structures regardless whether the
> ELF was actually 32-bit or 64-bit. This produces wrong results when trying
> to use a 64-bit kernel to load o32 ELF files.
>
> Change the uses of the generic elf structures to their 32-bit versions.
> Since the code bails out on any 64-bit cases, this is OK until they are
> implemented.
I'd say this will never happen as FR=0 with n64/n32 makes no sense. So
the fix itself looks good to me as it stands and hence here's my:
Reviewed-by: Maciej W. Rozycki <macro@xxxxxxxxxxxxxx>
I have some concerns about the original change that I think need
addressing though:
1. This piece in `arch_check_elf':
/* Ignore non-O32 binaries */
if (config_enabled(CONFIG_64BIT) &&
(ehdr->e_ident[EI_CLASS] != ELFCLASS32))
return 0;
is incomplete, contrary to the comment the check only trips on n64
binaries, but lets n32 ones through (as a side note `o32' would IMO
better be spelled with a lowercase `o' as this is the original spelling
of the names of the MIPS ABIs and they only way it was spelled for at
least a decade from its inception BTW; though I realise there's been
confusion about the spelling that happened sometime in the recent
past). Similarly in `arch_elf_pt_proc'.
2. Code added with 90cee759f08a lacks commentary that would make the use
of `elf32_hdr', etc. obvious and might have prevented the bugs this fix
corrects from happening in the first place. While not a requirement
for Linux (unlike for some other free software) it's always good to
write a sentence or two above a function definition to say what it is
supposed to do.
Paul -- I think the two concerns are action items for you!
Maciej
