From: "Maciej W. Rozycki" <macro@imgtec.com>
To: Daniel Wagner <daniel.wagner@bmw-carit.de>
Cc: <linux-mips@linux-mips.org>, <linux-kernel@vger.kernel.org>,
Ralf Baechle <ralf@linux-mips.org>
Subject: Re: [PATCH] MIPS: Differentiate between 32 and 64 bit ELF header
Date: Sat, 6 Feb 2016 17:16:33 +0000 [thread overview]
Message-ID: <alpine.DEB.2.00.1602061624460.15885@tp.orcam.me.uk> (raw)
In-Reply-To: <56AF82AB.5010502@bmw-carit.de>
On Mon, 1 Feb 2016, Daniel Wagner wrote:
> >> Depending on the configuration either the 32 or 64 bit version of
> >> elf_check_arch() is defined. parse_crash_elf32_headers() does
> >> some basic verification of the ELF header via elf_check_arch().
> >> parse_crash_elf64_headers() does it via vmcore_elf64_check_arch()
> >> which expands to the same elf_check_check().
> >>
> >> In file included from include/linux/elf.h:4:0,
> >> from fs/proc/vmcore.c:13:
> >> fs/proc/vmcore.c: In function 'parse_crash_elf64_headers':
> >>>> arch/mips/include/asm/elf.h:228:23: error: initialization from incompatible pointer type [-Werror=incompatible-pointer-types]
> >> struct elfhdr *__h = (hdr); \
> >> ^
> >> include/linux/crash_dump.h:41:37: note: in expansion of macro 'elf_check_arch'
> >> #define vmcore_elf64_check_arch(x) (elf_check_arch(x) || vmcore_elf_check_arch_cross(x))
> >> ^
> >> fs/proc/vmcore.c:1015:4: note: in expansion of macro 'vmcore_elf64_check_arch'
> >> !vmcore_elf64_check_arch(&ehdr) ||
> >> ^
> >>
> >> Since the MIPS ELF header for 32 bit and 64 bit differ we need
> >> to check accordingly.
> >
> > I fail to see how it can work as it stands given that `elf_check_arch' is
> > called from the same source file both on a pointer to `Elf32_Ehdr' and one
> > to `Elf64_Ehdr'. However the MIPS implementations of `elf_check_arch'
> > only use an auxiliary variable to avoid multiple evaluation of a macro
> > argument and therefore instead I recommend the use of the usual approach
> > taken in such a situation within a statement expression, that is to
> > declare the variable with `typeof' rather than an explicit type. As an
> > upside this will minimise code disruption as well.
>
> Good point on the type for hdr. Thought elf_check_arch() implementation
> differ on 32 bit and 64 bit implementation. I played a bit around and the
> simplest version I found was this here:
Umm, somehow I didn't really realise this code wants ELF32 and ELF64
checks both at once -- does it actually make sense? Is a core file from a
kernel crash dump ever going to be the opposite kind to the newly booted
kernel?
Anyway sorry about my confusion and the point above aside I don't really
like the idea of merging both `elf_check_arch' variations into one, it
just looks messy to me. So your original patch was somewhat better after
all, but I think it wasn't enough; for one it didn't handle the 32-bit
case in a 64-bit kernel.
What I think we want to do here is to draw a clear line between ELF32 and
ELF64. So first in include/linux/crash_dump.h:
#ifndef vmcore_elf32_check_arch
#define vmcore_elf32_check_arch elf_check_arch
#endif
and use `vmcore_elf32_check_arch' rather than `elf_check_arch' in
`parse_crash_elf32_headers' in fs/proc/vmcore.c (I think the checks for
ELFCLASS32/ELFCLASS64 ought to go first too, although that'd have to be a
separate patch).
Then we can redefine `vmcore_elf32_check_arch' and
`vmcore_elf64_check_arch' along your first proposal (although we don't
need to refer to `vmcore_elf_check_arch_cross' as we don't define it
anyway). However given that IIUC we're dealing with kernel rather than
userland images here I think we want to skip all the ABI peculiarities and
just accept anything that is compatible with the architecture. It'll then
be the business of whatever tool is going to handle this image to sort out
the details.
So to make things plain we just need:
#define mips_elf_check_machine(x) ((x)->e_machine == EM_MIPS)
#define vmcore_elf32_check_arch mips_elf_check_machine
#define vmcore_elf64_check_arch mips_elf_check_machine
in arch/mips/include/asm/elf.h (and then our definitions of
`elf_check_arch' can be rewritten to use `mips_elf_check_machine' too,
also in arch/mips/kernel/binfmt_elf?32.c).
Questions or comments?
Maciej
next prev parent reply other threads:[~2016-02-06 17:16 UTC|newest]
Thread overview: 44+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-01-28 14:44 [PATCH tip v6 0/5] Simple wait queue support Daniel Wagner
2016-01-28 14:44 ` [PATCH tip v6 1/5] wait.[ch]: Introduce the simple waitqueue (swait) implementation Daniel Wagner
2016-01-28 14:44 ` [PATCH tip v6 2/5] kbuild: Add option to turn incompatible pointer check into error Daniel Wagner
2016-01-29 12:17 ` Daniel Wagner
2016-01-29 18:55 ` Paul Gortmaker
2016-02-01 6:49 ` Daniel Wagner
2016-02-05 8:16 ` Daniel Wagner
2016-02-07 4:39 ` Paul Gortmaker
2016-02-17 13:04 ` Daniel Wagner
2016-01-28 14:44 ` [PATCH tip v6 3/5] KVM: use simple waitqueue for vcpu->wq Daniel Wagner
2016-01-29 12:18 ` Daniel Wagner
2016-01-28 14:44 ` [PATCH tip v6 4/5] rcu: Do not call rcu_nocb_gp_cleanup() while holding rnp->lock Daniel Wagner
2016-01-28 14:44 ` [PATCH tip v6 5/5] rcu: use simple wait queues where possible in rcutree Daniel Wagner
2016-01-29 13:23 ` [PATCH] video: Use bool instead int pointer for get_opt_bool() argument Daniel Wagner
2016-01-29 13:28 ` [PATCH] MIPS: Differentiate between 32 and 64 bit ELF header Daniel Wagner
2016-02-01 0:52 ` Maciej W. Rozycki
2016-02-01 16:07 ` Daniel Wagner
2016-02-06 17:16 ` Maciej W. Rozycki [this message]
2016-02-08 15:44 ` [PATCH v3 0/3] " Daniel Wagner
2016-02-08 15:44 ` [PATCH v3 1/3] mips: Use arch specific auxvec.h instead of generic-asm version Daniel Wagner
2016-02-08 17:19 ` Maciej W. Rozycki
2016-02-09 7:01 ` Daniel Wagner
2016-02-09 11:46 ` Maciej W. Rozycki
2016-02-09 12:37 ` Daniel Wagner
2016-02-09 14:51 ` Maciej W. Rozycki
2016-02-10 8:51 ` Daniel Wagner
2016-02-08 15:44 ` [PATCH v3 2/3] crash_dump: Add vmcore_elf32_check_arch Daniel Wagner
2016-02-08 17:05 ` Maciej W. Rozycki
2016-02-08 15:44 ` [PATCH v3 3/3] mips: Differentiate between 32 and 64 bit ELF header Daniel Wagner
2016-02-08 16:22 ` kbuild test robot
2016-02-09 8:03 ` Daniel Wagner
2016-02-09 12:32 ` Maciej W. Rozycki
2016-02-09 12:38 ` Daniel Wagner
2016-02-09 19:44 ` Maciej W. Rozycki
2016-02-10 6:28 ` Daniel Wagner
2016-02-10 9:21 ` [PATCH v4 0/2] " Daniel Wagner
2016-02-10 9:21 ` [PATCH v4 1/2] crash_dump: Add vmcore_elf32_check_arch Daniel Wagner
2016-02-10 9:21 ` [PATCH v4 2/2] mips: Differentiate between 32 and 64 bit ELF header Daniel Wagner
2016-02-11 10:49 ` Ralf Baechle
2016-02-11 12:04 ` Maciej W. Rozycki
2016-02-11 12:14 ` Daniel Wagner
2016-02-11 14:58 ` Maciej W. Rozycki
2016-02-11 15:30 ` Ralf Baechle
2016-02-08 16:58 ` [PATCH v3 3/3] " Maciej W. Rozycki
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=alpine.DEB.2.00.1602061624460.15885@tp.orcam.me.uk \
--to=macro@imgtec.com \
--cc=daniel.wagner@bmw-carit.de \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mips@linux-mips.org \
--cc=ralf@linux-mips.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).