Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1330200 > unrolled thread

Re: [PATCH v3 3/3] mips: Differentiate between 32 and 64 bit ELF header

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v3 3/3] mips: Differentiate between 32 and 64 bit ELF  header Daniel Wagner <daniel.wagner@bmw-carit.de> - 2016-02-09 13:40 +0100
    Re: [PATCH v3 3/3] mips: Differentiate between 32 and 64 bit ELF  header "Maciej W. Rozycki" <macro@imgtec.com> - 2016-02-09 21:10 +0100
      Re: [PATCH v3 3/3] mips: Differentiate between 32 and 64 bit ELF  header Daniel Wagner <daniel.wagner@bmw-carit.de> - 2016-02-10 07:30 +0100
        [PATCH v4 2/2] mips: Differentiate between 32 and 64 bit ELF header Daniel Wagner <daniel.wagner@bmw-carit.de> - 2016-02-10 10:30 +0100
          Re: [PATCH v4 2/2] mips: Differentiate between 32 and 64 bit ELF  header Ralf Baechle <ralf@linux-mips.org> - 2016-02-11 12:20 +0100
            Re: [PATCH v4 2/2] mips: Differentiate between 32 and 64 bit ELF  header "Maciej W. Rozycki" <macro@imgtec.com> - 2016-02-11 13:10 +0100
              Re: [PATCH v4 2/2] mips: Differentiate between 32 and 64 bit ELF  header Daniel Wagner <daniel.wagner@bmw-carit.de> - 2016-02-11 13:20 +0100
              Re: [PATCH v4 2/2] mips: Differentiate between 32 and 64 bit ELF  header "Maciej W. Rozycki" <macro@imgtec.com> - 2016-02-11 16:00 +0100
                Re: [PATCH v4 2/2] mips: Differentiate between 32 and 64 bit ELF  header Ralf Baechle <ralf@linux-mips.org> - 2016-02-11 16:40 +0100
        [PATCH v4 1/2] crash_dump: Add vmcore_elf32_check_arch Daniel Wagner <daniel.wagner@bmw-carit.de> - 2016-02-10 10:30 +0100
        [PATCH v4 0/2]  Differentiate between 32 and 64 bit ELF header Daniel Wagner <daniel.wagner@bmw-carit.de> - 2016-02-10 10:30 +0100

#1330200 — Re: [PATCH v3 3/3] mips: Differentiate between 32 and 64 bit ELF header

On 02/09/2016 01:32 PM, Maciej W. Rozycki wrote:
>  FWIW I think all the MIPS ABI flags stuff also needs to go outside the 
> conditional, because it's ABI agnostic.  I'll make the right change myself 
> on top of your fixes.  It'll remove a little bit of code duplication, 
> which is always welcome.

Great, thanks for taking care of it.

cheers,
daniel

[toc] | [next] | [standalone]

#1330620

On Tue, 9 Feb 2016, Daniel Wagner wrote:

> >  FWIW I think all the MIPS ABI flags stuff also needs to go outside the 
> > conditional, because it's ABI agnostic.  I'll make the right change myself 
> > on top of your fixes.  It'll remove a little bit of code duplication, 
> > which is always welcome.
> 
> Great, thanks for taking care of it.

 My ABI flags change has passed testing and I'm ready to post it, will you 
be respinning your patch soon?

  Maciej

[toc] | [prev] | [next] | [standalone]

#1330920

On 02/09/2016 08:44 PM, Maciej W. Rozycki wrote:
> On Tue, 9 Feb 2016, Daniel Wagner wrote:
> 
>>>  FWIW I think all the MIPS ABI flags stuff also needs to go outside the 
>>> conditional, because it's ABI agnostic.  I'll make the right change myself 
>>> on top of your fixes.  It'll remove a little bit of code duplication, 
>>> which is always welcome.
>>
>> Great, thanks for taking care of it.
> 
>  My ABI flags change has passed testing and I'm ready to post it, will you 
> be respinning your patch soon?

I was waiting for your cleanups and base my patches on top of it. Looks
like a small misunderstanding on my side :) I'll start on v4 right now.

cheers,
daniel

[toc] | [prev] | [next] | [standalone]

#1330966 — [PATCH v4 2/2] mips: Differentiate between 32 and 64 bit ELF header

Depending on the configuration either the 32 or 64 bit version of
elf_check_arch() is defined. parse_crash_elf{32|64}_headers() does
some basic verification of the ELF header via
vmcore_elf{32|64}_check_arch() which happen to map to elf_check_arch().
Since the implementation 32 and 64 bit version of elf_check_arch()
differ, we use the wrong type:

   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) ||
       ^

Therefore, we rather define vmcore_elf{32|64}_check_arch() as a
basic machine check and use it also in binfm_elf?32.c as well.

Signed-off-by: Daniel Wagner <daniel.wagner@bmw-carit.de>
Suggested-by: Maciej W. Rozycki <macro@imgtec.com>
Reviewed-by: Maciej W. Rozycki <macro@imgtec.com>
Reported-by: Fengguang Wu <fengguang.wu@intel.com>
---
 arch/mips/include/asm/elf.h      | 9 +++++++--
 arch/mips/kernel/binfmt_elfn32.c | 2 +-
 arch/mips/kernel/binfmt_elfo32.c | 2 +-
 3 files changed, 9 insertions(+), 4 deletions(-)

diff --git a/arch/mips/include/asm/elf.h b/arch/mips/include/asm/elf.h
index cefb7a5..e090fc3 100644
--- a/arch/mips/include/asm/elf.h
+++ b/arch/mips/include/asm/elf.h
@@ -227,7 +227,7 @@ struct mips_elf_abiflags_v0 {
 	int __res = 1;							\
 	struct elfhdr *__h = (hdr);					\
 									\
-	if (__h->e_machine != EM_MIPS)					\
+	if (!mips_elf_check_machine(__h))				\
 		__res = 0;						\
 	if (__h->e_ident[EI_CLASS] != ELFCLASS32)			\
 		__res = 0;						\
@@ -258,7 +258,7 @@ struct mips_elf_abiflags_v0 {
 	int __res = 1;							\
 	struct elfhdr *__h = (hdr);					\
 									\
-	if (__h->e_machine != EM_MIPS)					\
+	if (!mips_elf_check_machine(__h))				\
 		__res = 0;						\
 	if (__h->e_ident[EI_CLASS] != ELFCLASS64)			\
 		__res = 0;						\
@@ -285,6 +285,11 @@ struct mips_elf_abiflags_v0 {
 
 #endif /* !defined(ELF_ARCH) */
 
+#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
+
 struct mips_abi;
 
 extern struct mips_abi mips_abi;
diff --git a/arch/mips/kernel/binfmt_elfn32.c b/arch/mips/kernel/binfmt_elfn32.c
index 1188e00..1b992c6 100644
--- a/arch/mips/kernel/binfmt_elfn32.c
+++ b/arch/mips/kernel/binfmt_elfn32.c
@@ -35,7 +35,7 @@ typedef elf_fpreg_t elf_fpregset_t[ELF_NFPREG];
 	int __res = 1;							\
 	struct elfhdr *__h = (hdr);					\
 									\
-	if (__h->e_machine != EM_MIPS)					\
+	if (!mips_elf_check_machine(__h))				\
 		__res = 0;						\
 	if (__h->e_ident[EI_CLASS] != ELFCLASS32)			\
 		__res = 0;						\
diff --git a/arch/mips/kernel/binfmt_elfo32.c b/arch/mips/kernel/binfmt_elfo32.c
index 9287678..abd3aff 100644
--- a/arch/mips/kernel/binfmt_elfo32.c
+++ b/arch/mips/kernel/binfmt_elfo32.c
@@ -47,7 +47,7 @@ typedef elf_fpreg_t elf_fpregset_t[ELF_NFPREG];
 	int __res = 1;							\
 	struct elfhdr *__h = (hdr);					\
 									\
-	if (__h->e_machine != EM_MIPS)					\
+	if (!mips_elf_check_machine(__h))				\
 		__res = 0;						\
 	if (__h->e_ident[EI_CLASS] != ELFCLASS32)			\
 		__res = 0;						\
-- 
2.5.0

[toc] | [prev] | [next] | [standalone]

#1331827 — Re: [PATCH v4 2/2] mips: Differentiate between 32 and 64 bit ELF header

On Wed, Feb 10, 2016 at 10:21:21AM +0100, Daniel Wagner wrote:

> Depending on the configuration either the 32 or 64 bit version of
> elf_check_arch() is defined. parse_crash_elf{32|64}_headers() does
> some basic verification of the ELF header via
> vmcore_elf{32|64}_check_arch() which happen to map to elf_check_arch().
> Since the implementation 32 and 64 bit version of elf_check_arch()
> differ, we use the wrong type:
> 
>    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) ||
>        ^
> 
> Therefore, we rather define vmcore_elf{32|64}_check_arch() as a
> basic machine check and use it also in binfm_elf?32.c as well.
> 
> Signed-off-by: Daniel Wagner <daniel.wagner@bmw-carit.de>
> Suggested-by: Maciej W. Rozycki <macro@imgtec.com>
> Reviewed-by: Maciej W. Rozycki <macro@imgtec.com>
> Reported-by: Fengguang Wu <fengguang.wu@intel.com>

Thanks, applied.

I'm getting a less spectacular warning from gcc 5.2:

  CC      fs/proc/vmcore.o
fs/proc/vmcore.c: In function ‘parse_crash_elf64_headers’:
fs/proc/vmcore.c:939:47: warning: initialization from incompatible pointer type [-Wincompatible-pointer-types]

  Ralf

[toc] | [prev] | [next] | [standalone]

#1331875 — Re: [PATCH v4 2/2] mips: Differentiate between 32 and 64 bit ELF header

On Thu, 11 Feb 2016, Ralf Baechle wrote:

> > Signed-off-by: Daniel Wagner <daniel.wagner@bmw-carit.de>
> > Suggested-by: Maciej W. Rozycki <macro@imgtec.com>
> > Reviewed-by: Maciej W. Rozycki <macro@imgtec.com>
> > Reported-by: Fengguang Wu <fengguang.wu@intel.com>
> 
> Thanks, applied.
> 
> I'm getting a less spectacular warning from gcc 5.2:
> 
>   CC      fs/proc/vmcore.o
> fs/proc/vmcore.c: In function ‘parse_crash_elf64_headers’:
> fs/proc/vmcore.c:939:47: warning: initialization from incompatible pointer type [-Wincompatible-pointer-types]

 Yes, the temporaries still need to have their pointed types changed, to 
`Elf32_Ehdr' and `Elf64_Ehdr' respectively, as in the original change.

 I had it mentioned in a WIP version of my review (stating that it would 
verify that the correct type is used by the caller), but then deleted that 
part inadvertently, sigh.

 Daniel, sorry for the extra iteration.

  Maciej

[toc] | [prev] | [next] | [standalone]

#1331892 — Re: [PATCH v4 2/2] mips: Differentiate between 32 and 64 bit ELF header

On 02/11/2016 01:04 PM, Maciej W. Rozycki wrote:
> On Thu, 11 Feb 2016, Ralf Baechle wrote:
>> Thanks, applied.
>>
>> I'm getting a less spectacular warning from gcc 5.2:
>>
>>   CC      fs/proc/vmcore.o
>> fs/proc/vmcore.c: In function ‘parse_crash_elf64_headers’:
>> fs/proc/vmcore.c:939:47: warning: initialization from incompatible pointer type [-Wincompatible-pointer-types]
> 
>  Yes, the temporaries still need to have their pointed types changed, to 
> `Elf32_Ehdr' and `Elf64_Ehdr' respectively, as in the original change.
> 
>  I had it mentioned in a WIP version of my review (stating that it would 
> verify that the correct type is used by the caller), but then deleted that 
> part inadvertently, sigh.

Yeah, that part fall through the cracks.

>  Daniel, sorry for the extra iteration.

No problem. Just a sec.

cheers,
daniel

[toc] | [prev] | [next] | [standalone]

#1332124 — Re: [PATCH v4 2/2] mips: Differentiate between 32 and 64 bit ELF header

On Thu, 11 Feb 2016, Maciej W. Rozycki wrote:

> > > Signed-off-by: Daniel Wagner <daniel.wagner@bmw-carit.de>
> > > Suggested-by: Maciej W. Rozycki <macro@imgtec.com>
> > > Reviewed-by: Maciej W. Rozycki <macro@imgtec.com>
> > > Reported-by: Fengguang Wu <fengguang.wu@intel.com>
> > 
> > Thanks, applied.
> > 
> > I'm getting a less spectacular warning from gcc 5.2:
> > 
> >   CC      fs/proc/vmcore.o
> > fs/proc/vmcore.c: In function ‘parse_crash_elf64_headers’:
> > fs/proc/vmcore.c:939:47: warning: initialization from incompatible pointer type [-Wincompatible-pointer-types]
> 
>  Yes, the temporaries still need to have their pointed types changed, to 
> `Elf32_Ehdr' and `Elf64_Ehdr' respectively, as in the original change.
> 
>  I had it mentioned in a WIP version of my review (stating that it would 
> verify that the correct type is used by the caller), but then deleted that 
> part inadvertently, sigh.

 Hold on, I was right in dropping it actually.

 With your v4 change in place all `parse_crash_elf64_headers' is supposed 
to call is `mips_elf_check_machine' and that doesn't make any 
intialisations, it just dereferences the pointer passed once.  This error 
does not make any sense to me and line 939 isn't even in 
`parse_crash_elf64_headers', which starts at line 999, it's in 
`process_ptload_program_headers_elf32'.

 So Ralf, what tree are you using that is off from LMO/Linus by 60 lines?

 BTW line 939 at LMO and in Linus's tree looks like:

	Elf32_Phdr *phdr_ptr;

and the pointer is assigned to at line 944 like this:

	phdr_ptr = (Elf32_Phdr*)(elfptr + sizeof(Elf32_Ehdr)); /* PT_NOTE hdr */

so this does not explain the error.  I get a clean build with this version 
of Daniel's patches, both 32-bit and 64-bit, and FAOD with 
CONFIG_PROC_VMCORE=y.

 Daniel, please hold on with further updates, before this is cleared.  
Your v4 looks fine to me AFAICT, no need to change types.  Sorry about 
this all confusion, something's clearly broken somewhere -- maybe due to 
someone else's unpublished patch.

  Maciej

[toc] | [prev] | [next] | [standalone]

#1332163 — Re: [PATCH v4 2/2] mips: Differentiate between 32 and 64 bit ELF header

On Thu, Feb 11, 2016 at 02:58:55PM +0000, Maciej W. Rozycki wrote:
> Date:   Thu, 11 Feb 2016 14:58:55 +0000
> From: "Maciej W. Rozycki" <macro@imgtec.com>
> To: Ralf Baechle <ralf@linux-mips.org>
> CC: Daniel Wagner <daniel.wagner@bmw-carit.de>,
>  linux-kernel@vger.kernel.org, linux-mips@linux-mips.org
> Subject: Re: [PATCH v4 2/2] mips: Differentiate between 32 and 64 bit ELF
>  header
> Content-Type: text/plain; charset="ISO-8859-7"
> 
> On Thu, 11 Feb 2016, Maciej W. Rozycki wrote:
> 
> > > > Signed-off-by: Daniel Wagner <daniel.wagner@bmw-carit.de>
> > > > Suggested-by: Maciej W. Rozycki <macro@imgtec.com>
> > > > Reviewed-by: Maciej W. Rozycki <macro@imgtec.com>
> > > > Reported-by: Fengguang Wu <fengguang.wu@intel.com>
> > > 
> > > Thanks, applied.
> > > 
> > > I'm getting a less spectacular warning from gcc 5.2:
> > > 
> > >   CC      fs/proc/vmcore.o
> > > fs/proc/vmcore.c: In function ‘parse_crash_elf64_headers’:
> > > fs/proc/vmcore.c:939:47: warning: initialization from incompatible pointer type [-Wincompatible-pointer-types]
> > 
> >  Yes, the temporaries still need to have their pointed types changed, to 
> > `Elf32_Ehdr' and `Elf64_Ehdr' respectively, as in the original change.
> > 
> >  I had it mentioned in a WIP version of my review (stating that it would 
> > verify that the correct type is used by the caller), but then deleted that 
> > part inadvertently, sigh.
> 
>  Hold on, I was right in dropping it actually.
> 
>  With your v4 change in place all `parse_crash_elf64_headers' is supposed 
> to call is `mips_elf_check_machine' and that doesn't make any 
> intialisations, it just dereferences the pointer passed once.  This error 
> does not make any sense to me and line 939 isn't even in 
> `parse_crash_elf64_headers', which starts at line 999, it's in 
> `process_ptload_program_headers_elf32'.
> 
>  So Ralf, what tree are you using that is off from LMO/Linus by 60 lines?

That was 3.16, the oldest version affected.  But I'm getting the same
messages with different line numbers on more recent kernels including
the master branch.

  Ralf

[toc] | [prev] | [next] | [standalone]

#1330968 — [PATCH v4 1/2] crash_dump: Add vmcore_elf32_check_arch

parse_crash_elf{32|64}_headers will check the headers via the
elf_check_arch respectively vmcore_elf64_check_arch macro.

The MIPS architecture implements those two macros differently.
In order to make the differentiation more explicit, let's introduce
an vmcore_elf32_check_arch to allow the archs to overwrite it.

Signed-off-by: Daniel Wagner <daniel.wagner@bmw-carit.de>
Suggested-by: Maciej W. Rozycki <macro@imgtec.com>
Reviewed-by: Maciej W. Rozycki <macro@imgtec.com>
---
 fs/proc/vmcore.c           | 2 +-
 include/linux/crash_dump.h | 8 ++++++--
 2 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/fs/proc/vmcore.c b/fs/proc/vmcore.c
index 4e61388..c8ed209 100644
--- a/fs/proc/vmcore.c
+++ b/fs/proc/vmcore.c
@@ -1068,7 +1068,7 @@ static int __init parse_crash_elf32_headers(void)
 	/* Do some basic Verification. */
 	if (memcmp(ehdr.e_ident, ELFMAG, SELFMAG) != 0 ||
 		(ehdr.e_type != ET_CORE) ||
-		!elf_check_arch(&ehdr) ||
+		!vmcore_elf32_check_arch(&ehdr) ||
 		ehdr.e_ident[EI_CLASS] != ELFCLASS32||
 		ehdr.e_ident[EI_VERSION] != EV_CURRENT ||
 		ehdr.e_version != EV_CURRENT ||
diff --git a/include/linux/crash_dump.h b/include/linux/crash_dump.h
index 3849fce..3873697 100644
--- a/include/linux/crash_dump.h
+++ b/include/linux/crash_dump.h
@@ -34,9 +34,13 @@ void vmcore_cleanup(void);
 
 /*
  * Architecture code can redefine this if there are any special checks
- * needed for 64-bit ELF vmcores. In case of 32-bit only architecture,
- * this can be set to zero.
+ * needed for 32-bit ELF or 64-bit ELF vmcores.  In case of 32-bit
+ * only architecture, vmcore_elf64_check_arch can be set to zero.
  */
+#ifndef vmcore_elf32_check_arch
+#define vmcore_elf32_check_arch(x) elf_check_arch(x)
+#endif
+
 #ifndef vmcore_elf64_check_arch
 #define vmcore_elf64_check_arch(x) (elf_check_arch(x) || vmcore_elf_check_arch_cross(x))
 #endif
-- 
2.5.0

[toc] | [prev] | [next] | [standalone]

#1330969 — [PATCH v4 0/2] Differentiate between 32 and 64 bit ELF header

Hi Maciej,

I did test compile a few different configurations and with and without
mrproper upfront. All looks fine now. Let's see what still goes wrong :)

cheers,
daniel

Daniel Wagner (2):
  crash_dump: Add vmcore_elf32_check_arch
  mips: Differentiate between 32 and 64 bit ELF header

 arch/mips/include/asm/elf.h      | 9 +++++++--
 arch/mips/kernel/binfmt_elfn32.c | 2 +-
 arch/mips/kernel/binfmt_elfo32.c | 2 +-
 fs/proc/vmcore.c                 | 2 +-
 include/linux/crash_dump.h       | 8 ++++++--
 5 files changed, 16 insertions(+), 7 deletions(-)

-- 
2.5.0

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web