[PATCH] MIPS: Differentiate between 32 and 64 bit ELF header

[PATCH] MIPS: Differentiate between 32 and 64 bit ELF header

[Daniel Wagner]

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.

Signed-off-by: Daniel Wagner <daniel.wagner@bmw-carit.de>
Reported-by: Fengguang Wu <fengguang.wu@intel.com>
---
Hi,

In the 'simple wait queue support' series is a patch
which turns on -Werror=incompatible-pointer-types which will
result in a compile error for MIPS.

https://lkml.org/lkml/2016/1/28/462

I am not completely sure if this is the right approach but I could
get rid of the errors by this.

I'll prepend this patch to the next version of the series in order
to see if I got rid of all incompatible pointer types errors caught
by the kbuild test robot.

cheers,
daniel


arch/mips/include/asm/elf.h | 68 ++++++++++++++++++++++++---------------------
 1 file changed, 37 insertions(+), 31 deletions(-)

diff --git a/arch/mips/include/asm/elf.h b/arch/mips/include/asm/elf.h
index cefb7a5..7ba0a47 100644
--- a/arch/mips/include/asm/elf.h
+++ b/arch/mips/include/asm/elf.h
@@ -205,27 +205,10 @@ struct mips_elf_abiflags_v0 {
 #define MIPS_ABI_FP_64		6	/* -mips32r2 -mfp64 */
 #define MIPS_ABI_FP_64A		7	/* -mips32r2 -mfp64 -mno-odd-spreg */
 
-#ifdef CONFIG_32BIT
-
-/*
- * In order to be sure that we don't attempt to execute an O32 binary which
- * requires 64 bit FP (FR=1) on a system which does not support it we refuse
- * to execute any binary which has bits specified by the following macro set
- * in its ELF header flags.
- */
-#ifdef CONFIG_MIPS_O32_FP64_SUPPORT
-# define __MIPS_O32_FP64_MUST_BE_ZERO	0
-#else
-# define __MIPS_O32_FP64_MUST_BE_ZERO	EF_MIPS_FP64
-#endif
-
-/*
- * This is used to ensure we don't load something for the wrong architecture.
- */
-#define elf_check_arch(hdr)						\
+#define elf_check_arch_32(hdr)						\
 ({									\
 	int __res = 1;							\
-	struct elfhdr *__h = (hdr);					\
+	Elf32_Ehdr *__h = (hdr);					\
 									\
 	if (__h->e_machine != EM_MIPS)					\
 		__res = 0;						\
@@ -242,6 +225,40 @@ struct mips_elf_abiflags_v0 {
 	__res;								\
 })
 
+#define elf_check_arch_64(hdr)						\
+({									\
+	int __res = 1;							\
+	Elf64_Ehdr *__h = (hdr);					\
+									\
+	if (__h->e_machine != EM_MIPS)					\
+		__res = 0;						\
+	if (__h->e_ident[EI_CLASS] != ELFCLASS64)			\
+		__res = 0;						\
+									\
+	__res;								\
+})
+
+#define vmcore_elf64_check_arch(x)	(elf_check_arch_64(x) || vmcore_elf_check_arch_cross(x))
+
+#ifdef CONFIG_32BIT
+
+/*
+ * In order to be sure that we don't attempt to execute an O32 binary which
+ * requires 64 bit FP (FR=1) on a system which does not support it we refuse
+ * to execute any binary which has bits specified by the following macro set
+ * in its ELF header flags.
+ */
+#ifdef CONFIG_MIPS_O32_FP64_SUPPORT
+# define __MIPS_O32_FP64_MUST_BE_ZERO	0
+#else
+# define __MIPS_O32_FP64_MUST_BE_ZERO	EF_MIPS_FP64
+#endif
+
+/*
+ * This is used to ensure we don't load something for the wrong architecture.
+ */
+#define elf_check_arch(x)	elf_check_arch_32(x)
+
 /*
  * These are used to set parameters in the core dumps.
  */
@@ -253,18 +270,7 @@ struct mips_elf_abiflags_v0 {
 /*
  * This is used to ensure we don't load something for the wrong architecture.
  */
-#define elf_check_arch(hdr)						\
-({									\
-	int __res = 1;							\
-	struct elfhdr *__h = (hdr);					\
-									\
-	if (__h->e_machine != EM_MIPS)					\
-		__res = 0;						\
-	if (__h->e_ident[EI_CLASS] != ELFCLASS64)			\
-		__res = 0;						\
-									\
-	__res;								\
-})
+#define elf_check_arch(x)	elf_check_arch_64(x)
 
 /*
  * These are used to set parameters in the core dumps.
-- 
2.5.0

Re: [PATCH] MIPS: Differentiate between 32 and 64 bit ELF header

[Maciej W. Rozycki]

On Fri, 29 Jan 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.

 For consistency I suggest making the same change to the `elf_check_arch' 
definitions in arch/mips/kernel/binfmt_elf*.c as well.

  Maciej

Re: [PATCH] MIPS: Differentiate between 32 and 64 bit ELF header

[Maciej W. Rozycki]

On Fri, 29 Jan 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.

 For consistency I suggest making the same change to the `elf_check_arch' 
definitions in arch/mips/kernel/binfmt_elf*.c as well.

  Maciej

Re: [PATCH] MIPS: Differentiate between 32 and 64 bit ELF header

[Daniel Wagner]

On 02/01/2016 01:52 AM, Maciej W. Rozycki wrote:
> On Fri, 29 Jan 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:


diff --git a/arch/mips/include/asm/elf.h b/arch/mips/include/asm/elf.h
index b01a6ff..8c88238 100644
--- a/arch/mips/include/asm/elf.h
+++ b/arch/mips/include/asm/elf.h
@@ -205,8 +205,6 @@ struct mips_elf_abiflags_v0 {
 #define MIPS_ABI_FP_64		6	/* -mips32r2 -mfp64 */
 #define MIPS_ABI_FP_64A		7	/* -mips32r2 -mfp64 -mno-odd-spreg */
 
-#ifdef CONFIG_32BIT
-
 /*
  * In order to be sure that we don't attempt to execute an O32 binary which
  * requires 64 bit FP (FR=1) on a system which does not support it we refuse
@@ -225,23 +223,30 @@ struct mips_elf_abiflags_v0 {
 #define elf_check_arch(hdr)						\
 ({									\
 	int __res = 1;							\
-	struct elfhdr *__h = (hdr);					\
+	typeof(*(hdr)) *__h = (hdr);					\
 									\
 	if (__h->e_machine != EM_MIPS)					\
 		__res = 0;						\
-	if (__h->e_ident[EI_CLASS] != ELFCLASS32)			\
-		__res = 0;						\
-	if ((__h->e_flags & EF_MIPS_ABI2) != 0)				\
-		__res = 0;						\
-	if (((__h->e_flags & EF_MIPS_ABI) != 0) &&			\
-	    ((__h->e_flags & EF_MIPS_ABI) != EF_MIPS_ABI_O32))		\
-		__res = 0;						\
-	if (__h->e_flags & __MIPS_O32_FP64_MUST_BE_ZERO)		\
-		__res = 0;						\
+	if (__same_type(hdr, Elf32_Ehdr *)) {				\
+		if (__h->e_ident[EI_CLASS] != ELFCLASS32)		\
+			__res = 0;					\
+		if ((__h->e_flags & EF_MIPS_ABI2) != 0)			\
+			__res = 0;					\
+		if (((__h->e_flags & EF_MIPS_ABI) != 0) &&		\
+			((__h->e_flags & EF_MIPS_ABI) != EF_MIPS_ABI_O32)) \
+			__res = 0;					\
+		if (__h->e_flags & __MIPS_O32_FP64_MUST_BE_ZERO)	\
+			__res = 0;					\
+	} else if (__same_type(hdr, Elf64_Ehdr *)) {			\
+		if (__h->e_ident[EI_CLASS] != ELFCLASS64)		\
+			__res = 0;					\
+	}								\
 									\
 	__res;								\
 })
 
+#ifdef CONFIG_32BIT
+
 /*
  * These are used to set parameters in the core dumps.
  */
@@ -250,21 +255,6 @@ struct mips_elf_abiflags_v0 {
 #endif /* CONFIG_32BIT */
 
 #ifdef CONFIG_64BIT
-/*
- * This is used to ensure we don't load something for the wrong architecture.
- */
-#define elf_check_arch(hdr)						\
-({									\
-	int __res = 1;							\
-	struct elfhdr *__h = (hdr);					\
-									\
-	if (__h->e_machine != EM_MIPS)					\
-		__res = 0;						\
-	if (__h->e_ident[EI_CLASS] != ELFCLASS64)			\
-		__res = 0;						\
-									\
-	__res;								\
-})
 
 /*
  * These are used to set parameters in the core dumps.


Not sure if that is what you had in mind.

cheers,
daniel

Re: [PATCH] MIPS: Differentiate between 32 and 64 bit ELF header

[Daniel Wagner]

On 02/01/2016 01:52 AM, Maciej W. Rozycki wrote:
> On Fri, 29 Jan 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:


diff --git a/arch/mips/include/asm/elf.h b/arch/mips/include/asm/elf.h
index b01a6ff..8c88238 100644
--- a/arch/mips/include/asm/elf.h
+++ b/arch/mips/include/asm/elf.h
@@ -205,8 +205,6 @@ struct mips_elf_abiflags_v0 {
 #define MIPS_ABI_FP_64		6	/* -mips32r2 -mfp64 */
 #define MIPS_ABI_FP_64A		7	/* -mips32r2 -mfp64 -mno-odd-spreg */
 
-#ifdef CONFIG_32BIT
-
 /*
  * In order to be sure that we don't attempt to execute an O32 binary which
  * requires 64 bit FP (FR=1) on a system which does not support it we refuse
@@ -225,23 +223,30 @@ struct mips_elf_abiflags_v0 {
 #define elf_check_arch(hdr)						\
 ({									\
 	int __res = 1;							\
-	struct elfhdr *__h = (hdr);					\
+	typeof(*(hdr)) *__h = (hdr);					\
 									\
 	if (__h->e_machine != EM_MIPS)					\
 		__res = 0;						\
-	if (__h->e_ident[EI_CLASS] != ELFCLASS32)			\
-		__res = 0;						\
-	if ((__h->e_flags & EF_MIPS_ABI2) != 0)				\
-		__res = 0;						\
-	if (((__h->e_flags & EF_MIPS_ABI) != 0) &&			\
-	    ((__h->e_flags & EF_MIPS_ABI) != EF_MIPS_ABI_O32))		\
-		__res = 0;						\
-	if (__h->e_flags & __MIPS_O32_FP64_MUST_BE_ZERO)		\
-		__res = 0;						\
+	if (__same_type(hdr, Elf32_Ehdr *)) {				\
+		if (__h->e_ident[EI_CLASS] != ELFCLASS32)		\
+			__res = 0;					\
+		if ((__h->e_flags & EF_MIPS_ABI2) != 0)			\
+			__res = 0;					\
+		if (((__h->e_flags & EF_MIPS_ABI) != 0) &&		\
+			((__h->e_flags & EF_MIPS_ABI) != EF_MIPS_ABI_O32)) \
+			__res = 0;					\
+		if (__h->e_flags & __MIPS_O32_FP64_MUST_BE_ZERO)	\
+			__res = 0;					\
+	} else if (__same_type(hdr, Elf64_Ehdr *)) {			\
+		if (__h->e_ident[EI_CLASS] != ELFCLASS64)		\
+			__res = 0;					\
+	}								\
 									\
 	__res;								\
 })
 
+#ifdef CONFIG_32BIT
+
 /*
  * These are used to set parameters in the core dumps.
  */
@@ -250,21 +255,6 @@ struct mips_elf_abiflags_v0 {
 #endif /* CONFIG_32BIT */
 
 #ifdef CONFIG_64BIT
-/*
- * This is used to ensure we don't load something for the wrong architecture.
- */
-#define elf_check_arch(hdr)						\
-({									\
-	int __res = 1;							\
-	struct elfhdr *__h = (hdr);					\
-									\
-	if (__h->e_machine != EM_MIPS)					\
-		__res = 0;						\
-	if (__h->e_ident[EI_CLASS] != ELFCLASS64)			\
-		__res = 0;						\
-									\
-	__res;								\
-})
 
 /*
  * These are used to set parameters in the core dumps.


Not sure if that is what you had in mind.

cheers,
daniel

Re: [PATCH] MIPS: Differentiate between 32 and 64 bit ELF header

[Maciej W. Rozycki]

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

Re: [PATCH] MIPS: Differentiate between 32 and 64 bit ELF header

[Maciej W. Rozycki]

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

[PATCH v3 0/3] Differentiate between 32 and 64 bit ELF header

[Daniel Wagner]

Hi Maciej,

Thanks a lot for your input. It looks like we getting somewhere.
This version is much smaller and not so invasive as the prevision one.

I had some trouble with my cross compile setup. The first patch addresses
this problem. If I got it right it is just a missing include wrapper file.

cheers,
daniel

Daniel Wagner (3):
  mips: Use arch specific auxvec.h instead of generic-asm version
  crash_dump: Add vmcore_elf32_check_arch
  mips: Differentiate between 32 and 64 bit ELF header

 arch/mips/include/asm/auxvec.h   | 1 +
 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 ++++++--
 6 files changed, 17 insertions(+), 7 deletions(-)
 create mode 100644 arch/mips/include/asm/auxvec.h

-- 
2.5.0

[PATCH v3 1/3] mips: Use arch specific auxvec.h instead of generic-asm version

[Daniel Wagner]

The generic auxvec.h is used instead the arch specific version.
This happens when cross compiling the kernel.

mips64-linux-gnu-gcc (GCC) 5.2.1 20151104 (Red Hat Cross 5.2.1-4)

arch/mips/kernel/../../../fs/binfmt_elf.c: In function ‘create_elf_tables’:
./arch/mips/include/asm/elf.h:425:14: error: ‘AT_SYSINFO_EHDR’ undeclared (first use in this function)
  NEW_AUX_ENT(AT_SYSINFO_EHDR,     \
              ^
arch/mips/kernel/../../../fs/binfmt_elf.c:222:26: note: in definition of macro ‘NEW_AUX_ENT’
   elf_info[ei_index++] = id; \
                          ^
arch/mips/kernel/../../../fs/binfmt_elf.c:233:2: note: in expansion of macro ‘ARCH_DLINFO’
  ARCH_DLINFO;
  ^
./arch/mips/include/asm/elf.h:425:14: note: each undeclared identifier is reported only once for each function it appears in
  NEW_AUX_ENT(AT_SYSINFO_EHDR,     \
              ^
arch/mips/kernel/../../../fs/binfmt_elf.c:222:26: note: in definition of macro ‘NEW_AUX_ENT’
   elf_info[ei_index++] = id; \
                          ^
arch/mips/kernel/../../../fs/binfmt_elf.c:233:2: note: in expansion of macro ‘ARCH_DLINFO’
  ARCH_DLINFO;
  ^

Signed-off-by: Daniel Wagner <daniel.wagner@bmw-carit.de>
---
 arch/mips/include/asm/auxvec.h | 1 +
 1 file changed, 1 insertion(+)
 create mode 100644 arch/mips/include/asm/auxvec.h

diff --git a/arch/mips/include/asm/auxvec.h b/arch/mips/include/asm/auxvec.h
new file mode 100644
index 0000000..fbd388c
--- /dev/null
+++ b/arch/mips/include/asm/auxvec.h
@@ -0,0 +1 @@
+#include <uapi/asm/auxvec.h>
-- 
2.5.0

[PATCH v3 2/3] crash_dump: Add vmcore_elf32_check_arch

[Daniel Wagner]

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

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

[Daniel Wagner]

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>
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..189e279 100644
--- a/arch/mips/include/asm/elf.h
+++ b/arch/mips/include/asm/elf.h
@@ -205,6 +205,11 @@ struct mips_elf_abiflags_v0 {
 #define MIPS_ABI_FP_64		6	/* -mips32r2 -mfp64 */
 #define MIPS_ABI_FP_64A		7	/* -mips32r2 -mfp64 -mno-odd-spreg */
 
+#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
+
 #ifdef CONFIG_32BIT
 
 /*
@@ -227,7 +232,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 +263,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;						\
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

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

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 2172 bytes --]

Hi Daniel,

[auto build test ERROR on v4.5-rc3]
[also build test ERROR on next-20160208]
[if your patch is applied to the wrong git tree, please drop us a note to help improving the system]

url:    https://github.com/0day-ci/linux/commits/Daniel-Wagner/Differentiate-between-32-and-64-bit-ELF-header/20160208-234759
config: mips-fuloong2e_defconfig (attached as .config)
reproduce:
        wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        make.cross ARCH=mips 

All error/warnings (new ones prefixed by >>):

   arch/mips/kernel/../../../fs/binfmt_elf.c: In function 'load_elf_interp':
>> arch/mips/kernel/binfmt_elfn32.c:38:7: error: implicit declaration of function 'mips_elf_check_machine' [-Werror=implicit-function-declaration]
     if (!mips_elf_check_machine(__h))    \
          ^
>> arch/mips/kernel/../../../fs/binfmt_elf.c:536:7: note: in expansion of macro 'elf_check_arch'
     if (!elf_check_arch(interp_elf_ex))
          ^
   cc1: some warnings being treated as errors
--
   arch/mips/kernel/../../../fs/binfmt_elf.c: In function 'load_elf_interp':
>> arch/mips/kernel/binfmt_elfo32.c:50:7: error: implicit declaration of function 'mips_elf_check_machine' [-Werror=implicit-function-declaration]
     if (!mips_elf_check_machine(__h))    \
          ^
>> arch/mips/kernel/../../../fs/binfmt_elf.c:536:7: note: in expansion of macro 'elf_check_arch'
     if (!elf_check_arch(interp_elf_ex))
          ^
   cc1: some warnings being treated as errors

vim +/mips_elf_check_machine +38 arch/mips/kernel/binfmt_elfn32.c

    32	 */
    33	#define elf_check_arch(hdr)						\
    34	({									\
    35		int __res = 1;							\
    36		struct elfhdr *__h = (hdr);					\
    37										\
  > 38		if (!mips_elf_check_machine(__h))				\
    39			__res = 0;						\
    40		if (__h->e_ident[EI_CLASS] != ELFCLASS32)			\
    41			__res = 0;						\

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/octet-stream, Size: 16752 bytes --]

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

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 2172 bytes --]

Hi Daniel,

[auto build test ERROR on v4.5-rc3]
[also build test ERROR on next-20160208]
[if your patch is applied to the wrong git tree, please drop us a note to help improving the system]

url:    https://github.com/0day-ci/linux/commits/Daniel-Wagner/Differentiate-between-32-and-64-bit-ELF-header/20160208-234759
config: mips-fuloong2e_defconfig (attached as .config)
reproduce:
        wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        make.cross ARCH=mips 

All error/warnings (new ones prefixed by >>):

   arch/mips/kernel/../../../fs/binfmt_elf.c: In function 'load_elf_interp':
>> arch/mips/kernel/binfmt_elfn32.c:38:7: error: implicit declaration of function 'mips_elf_check_machine' [-Werror=implicit-function-declaration]
     if (!mips_elf_check_machine(__h))    \
          ^
>> arch/mips/kernel/../../../fs/binfmt_elf.c:536:7: note: in expansion of macro 'elf_check_arch'
     if (!elf_check_arch(interp_elf_ex))
          ^
   cc1: some warnings being treated as errors
--
   arch/mips/kernel/../../../fs/binfmt_elf.c: In function 'load_elf_interp':
>> arch/mips/kernel/binfmt_elfo32.c:50:7: error: implicit declaration of function 'mips_elf_check_machine' [-Werror=implicit-function-declaration]
     if (!mips_elf_check_machine(__h))    \
          ^
>> arch/mips/kernel/../../../fs/binfmt_elf.c:536:7: note: in expansion of macro 'elf_check_arch'
     if (!elf_check_arch(interp_elf_ex))
          ^
   cc1: some warnings being treated as errors

vim +/mips_elf_check_machine +38 arch/mips/kernel/binfmt_elfn32.c

    32	 */
    33	#define elf_check_arch(hdr)						\
    34	({									\
    35		int __res = 1;							\
    36		struct elfhdr *__h = (hdr);					\
    37										\
  > 38		if (!mips_elf_check_machine(__h))				\
    39			__res = 0;						\
    40		if (__h->e_ident[EI_CLASS] != ELFCLASS32)			\
    41			__res = 0;						\

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/octet-stream, Size: 16752 bytes --]

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

[Maciej W. Rozycki]

On Mon, 8 Feb 2016, Daniel Wagner wrote:

> 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>
> Reported-by: Fengguang Wu <fengguang.wu@intel.com>
> ---

Reviewed-by: Maciej W. Rozycki <macro@imgtec.com>

  Maciej

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

[Maciej W. Rozycki]

On Mon, 8 Feb 2016, Daniel Wagner wrote:

> 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>
> Reported-by: Fengguang Wu <fengguang.wu@intel.com>
> ---

Reviewed-by: Maciej W. Rozycki <macro@imgtec.com>

  Maciej

Re: [PATCH v3 2/3] crash_dump: Add vmcore_elf32_check_arch

[Maciej W. Rozycki]

On Mon, 8 Feb 2016, Daniel Wagner wrote:

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

  Maciej

Re: [PATCH v3 2/3] crash_dump: Add vmcore_elf32_check_arch

[Maciej W. Rozycki]

On Mon, 8 Feb 2016, Daniel Wagner wrote:

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

  Maciej

Re: [PATCH v3 1/3] mips: Use arch specific auxvec.h instead of generic-asm version

[Maciej W. Rozycki]

On Mon, 8 Feb 2016, Daniel Wagner wrote:

> The generic auxvec.h is used instead the arch specific version.
> This happens when cross compiling the kernel.
> 
> mips64-linux-gnu-gcc (GCC) 5.2.1 20151104 (Red Hat Cross 5.2.1-4)
> 
> arch/mips/kernel/../../../fs/binfmt_elf.c: In function ‘create_elf_tables’:
> ./arch/mips/include/asm/elf.h:425:14: error: ‘AT_SYSINFO_EHDR’ undeclared (first use in this function)

 There must be something wrong with your setup, or maybe a bug somewhere 
in our build machinery you just happened to trigger.  Most of us routinely 
use a cross-compiler to build the kernel and you're the first one to 
report the problem.

 Can you report the compiler invocation that has lead to this error?  
Have you used a default config or a custom one?

> diff --git a/arch/mips/include/asm/auxvec.h b/arch/mips/include/asm/auxvec.h
> new file mode 100644
> index 0000000..fbd388c
> --- /dev/null
> +++ b/arch/mips/include/asm/auxvec.h
> @@ -0,0 +1 @@
> +#include <uapi/asm/auxvec.h>

 You're not supposed to require a header in asm/ merely to include a 
header of the same name from uapi/asm/ as there are normally 
-I./arch/mips/include and -I./arch/mips/include/uapi options present both 
at once, in this order, on the compiler's invocation line.  So:

#include <asm/auxvec.h>

will pull the header from uapi/asm/ if none is present in asm/.

  Maciej

Re: [PATCH v3 1/3] mips: Use arch specific auxvec.h instead of generic-asm version

[Maciej W. Rozycki]

On Mon, 8 Feb 2016, Daniel Wagner wrote:

> The generic auxvec.h is used instead the arch specific version.
> This happens when cross compiling the kernel.
> 
> mips64-linux-gnu-gcc (GCC) 5.2.1 20151104 (Red Hat Cross 5.2.1-4)
> 
> arch/mips/kernel/../../../fs/binfmt_elf.c: In function ‘create_elf_tables’:
> ./arch/mips/include/asm/elf.h:425:14: error: ‘AT_SYSINFO_EHDR’ undeclared (first use in this function)

 There must be something wrong with your setup, or maybe a bug somewhere 
in our build machinery you just happened to trigger.  Most of us routinely 
use a cross-compiler to build the kernel and you're the first one to 
report the problem.

 Can you report the compiler invocation that has lead to this error?  
Have you used a default config or a custom one?

> diff --git a/arch/mips/include/asm/auxvec.h b/arch/mips/include/asm/auxvec.h
> new file mode 100644
> index 0000000..fbd388c
> --- /dev/null
> +++ b/arch/mips/include/asm/auxvec.h
> @@ -0,0 +1 @@
> +#include <uapi/asm/auxvec.h>

 You're not supposed to require a header in asm/ merely to include a 
header of the same name from uapi/asm/ as there are normally 
-I./arch/mips/include and -I./arch/mips/include/uapi options present both 
at once, in this order, on the compiler's invocation line.  So:

#include <asm/auxvec.h>

will pull the header from uapi/asm/ if none is present in asm/.

  Maciej

Re: [PATCH v3 1/3] mips: Use arch specific auxvec.h instead of generic-asm version

[Daniel Wagner]

Good Morning,

On 02/08/2016 06:19 PM, Maciej W. Rozycki wrote:
> On Mon, 8 Feb 2016, Daniel Wagner wrote:
> 
>> The generic auxvec.h is used instead the arch specific version.
>> This happens when cross compiling the kernel.
>>
>> mips64-linux-gnu-gcc (GCC) 5.2.1 20151104 (Red Hat Cross 5.2.1-4)
>>
>> arch/mips/kernel/../../../fs/binfmt_elf.c: In function ‘create_elf_tables’:
>> ./arch/mips/include/asm/elf.h:425:14: error: ‘AT_SYSINFO_EHDR’ undeclared (first use in this function)
> 
>  There must be something wrong with your setup, or maybe a bug somewhere 
> in our build machinery you just happened to trigger.  Most of us routinely 
> use a cross-compiler to build the kernel and you're the first one to 
> report the problem.

Yeah, I thought so too and I would also bet on the toolchain. After
'fixing' this small problem I got a nice and shiny binary without any
other warnings or errors.

>  Can you report the compiler invocation that has lead to this error?  

 /usr/bin/mips64-linux-gnu-gcc -Wp,-MD,fs/.binfmt_elf.o.d  -nostdinc -isystem /usr/lib/gcc/mips64-linux-gnu/5.2.1/include -I./arch/mips/include -Iarch/mips/include/generated/uapi -Iarch/mips/include/generated  -Iinclude -I./arch/mips/include/uapi -Iarch/mips/include/generated/uapi -I./include/uapi -Iinclude/generated/uapi -include ./include/linux/kconfig.h -D__KERNEL__ -DVMLINUX_LOAD_ADDRESS=0xffffffff88002000 -DDATAOFFSET=0 -Wall -Wundef -Wstrict-prototypes -Wno-trigraphs -fno-strict-aliasing -fno-common -Werror-implicit-function-declaration -Wno-format-security -std=gnu89 -mno-check-zero-division -mabi=32 -G 0 -mno-abicalls -fno-pic -pipe -msoft-float -DGAS_HAS_SET_HARDFLOAT -Wa,-msoft-float -ffreestanding -march=r5000 -Wa,--trap -I./arch/mips/include/asm/mach-ip22 -I./arch/mips/include/asm/mach-generic -fno-delete-null-pointer-checks -O2 --param=allow-store-data-races=0 -Wframe-larger-than=1024 -fno-stack-protector -Wno-unused-but-set-variable -fomit-frame-pointer -fno-var-tracking
-assignments -Wdeclaration-after-statement -Wno-pointer-sign -fno-strict-overflow -fconserve-stack -Werror=implicit-int -Werror=strict-prototypes -Werror=date-time -DCC_HAVE_ASM_GOTO    -D"KBUILD_STR(s)=#s" -D"KBUILD_BASENAME=KBUILD_STR(binfmt_elf)"  -D"KBUILD_MODNAME=KBUILD_STR(binfmt_elf)" -c -o fs/.tmp_binfmt_elf.o fs/binfmt_elf.c

> Have you used a default config or a custom one?

I used the default one per 'make defconfig ARCH=mips
CROSS_COMPILE=/usr/bin/mips64-linux-gnu-' with Fedora 23 MIPS cross
toolchain.

>> diff --git a/arch/mips/include/asm/auxvec.h b/arch/mips/include/asm/auxvec.h
>> new file mode 100644
>> index 0000000..fbd388c
>> --- /dev/null
>> +++ b/arch/mips/include/asm/auxvec.h
>> @@ -0,0 +1 @@
>> +#include <uapi/asm/auxvec.h>
> 
>  You're not supposed to require a header in asm/ merely to include a 
> header of the same name from uapi/asm/ as there are normally 
> -I./arch/mips/include and -I./arch/mips/include/uapi options present both 
> at once, in this order, on the compiler's invocation line.  So:
> 
> #include <asm/auxvec.h>
> 
> will pull the header from uapi/asm/ if none is present in asm/.

Okay, thanks for the explanation. I was pretty confused by the build
machinery and saw this include for ARM arch which provides also a their
own uapi/asm/auxvec.h

Also I looked at the cpp output and saw that there was no uapi/asm/auxvec.h
included instead it pulls arch/mips/include/generated/uapi/asm/auxvec.h

Not working version:

# 1 "arch/mips/include/generated/uapi/asm/auxvec.h" 1
# 1 "./include/uapi/asm-generic/auxvec.h" 1
# 1 "arch/mips/include/generated/uapi/asm/auxvec.h" 2
# 5 "include/uapi/linux/auxvec.h" 2
# 5 "include/linux/auxvec.h" 2
# 12 "./arch/mips/include/asm/elf.h" 2
# 1 "include/linux/fs.h" 1

working version:

# 1 "./arch/mips/include/asm/auxvec.h" 1
# 1 "./arch/mips/include/uapi/asm/auxvec.h" 1
# 1 "./arch/mips/include/asm/auxvec.h" 2
# 5 "include/uapi/linux/auxvec.h" 2
# 5 "include/linux/auxvec.h" 2
# 12 "./arch/mips/include/asm/elf.h" 2
# 1 "include/linux/fs.h" 1

I've uploaded the cpp output and the config just in case:

https://www.monom.org/data/mips/auxvec/

I am still pretty confused about what should happen in which order. 
Maybe I should call the Confuse-A-Cat squat team.

cheers,
daniel

Re: [PATCH v3 1/3] mips: Use arch specific auxvec.h instead of generic-asm version

[Daniel Wagner]

Good Morning,

On 02/08/2016 06:19 PM, Maciej W. Rozycki wrote:
> On Mon, 8 Feb 2016, Daniel Wagner wrote:
> 
>> The generic auxvec.h is used instead the arch specific version.
>> This happens when cross compiling the kernel.
>>
>> mips64-linux-gnu-gcc (GCC) 5.2.1 20151104 (Red Hat Cross 5.2.1-4)
>>
>> arch/mips/kernel/../../../fs/binfmt_elf.c: In function ‘create_elf_tables’:
>> ./arch/mips/include/asm/elf.h:425:14: error: ‘AT_SYSINFO_EHDR’ undeclared (first use in this function)
> 
>  There must be something wrong with your setup, or maybe a bug somewhere 
> in our build machinery you just happened to trigger.  Most of us routinely 
> use a cross-compiler to build the kernel and you're the first one to 
> report the problem.

Yeah, I thought so too and I would also bet on the toolchain. After
'fixing' this small problem I got a nice and shiny binary without any
other warnings or errors.

>  Can you report the compiler invocation that has lead to this error?  

 /usr/bin/mips64-linux-gnu-gcc -Wp,-MD,fs/.binfmt_elf.o.d  -nostdinc -isystem /usr/lib/gcc/mips64-linux-gnu/5.2.1/include -I./arch/mips/include -Iarch/mips/include/generated/uapi -Iarch/mips/include/generated  -Iinclude -I./arch/mips/include/uapi -Iarch/mips/include/generated/uapi -I./include/uapi -Iinclude/generated/uapi -include ./include/linux/kconfig.h -D__KERNEL__ -DVMLINUX_LOAD_ADDRESS=0xffffffff88002000 -DDATAOFFSET=0 -Wall -Wundef -Wstrict-prototypes -Wno-trigraphs -fno-strict-aliasing -fno-common -Werror-implicit-function-declaration -Wno-format-security -std=gnu89 -mno-check-zero-division -mabi=32 -G 0 -mno-abicalls -fno-pic -pipe -msoft-float -DGAS_HAS_SET_HARDFLOAT -Wa,-msoft-float -ffreestanding -march=r5000 -Wa,--trap -I./arch/mips/include/asm/mach-ip22 -I./arch/mips/include/asm/mach-generic -fno-delete-null-pointer-checks -O2 --param=allow-store-data-races=0 -Wframe-larger-than=1024 -fno-stack-protector -Wno-unused-but-set-variable -fomit-frame-pointer -fno-var-tracking
-assignments -Wdeclaration-after-statement -Wno-pointer-sign -fno-strict-overflow -fconserve-stack -Werror=implicit-int -Werror=strict-prototypes -Werror=date-time -DCC_HAVE_ASM_GOTO    -D"KBUILD_STR(s)=#s" -D"KBUILD_BASENAME=KBUILD_STR(binfmt_elf)"  -D"KBUILD_MODNAME=KBUILD_STR(binfmt_elf)" -c -o fs/.tmp_binfmt_elf.o fs/binfmt_elf.c

> Have you used a default config or a custom one?

I used the default one per 'make defconfig ARCH=mips
CROSS_COMPILE=/usr/bin/mips64-linux-gnu-' with Fedora 23 MIPS cross
toolchain.

>> diff --git a/arch/mips/include/asm/auxvec.h b/arch/mips/include/asm/auxvec.h
>> new file mode 100644
>> index 0000000..fbd388c
>> --- /dev/null
>> +++ b/arch/mips/include/asm/auxvec.h
>> @@ -0,0 +1 @@
>> +#include <uapi/asm/auxvec.h>
> 
>  You're not supposed to require a header in asm/ merely to include a 
> header of the same name from uapi/asm/ as there are normally 
> -I./arch/mips/include and -I./arch/mips/include/uapi options present both 
> at once, in this order, on the compiler's invocation line.  So:
> 
> #include <asm/auxvec.h>
> 
> will pull the header from uapi/asm/ if none is present in asm/.

Okay, thanks for the explanation. I was pretty confused by the build
machinery and saw this include for ARM arch which provides also a their
own uapi/asm/auxvec.h

Also I looked at the cpp output and saw that there was no uapi/asm/auxvec.h
included instead it pulls arch/mips/include/generated/uapi/asm/auxvec.h

Not working version:

# 1 "arch/mips/include/generated/uapi/asm/auxvec.h" 1
# 1 "./include/uapi/asm-generic/auxvec.h" 1
# 1 "arch/mips/include/generated/uapi/asm/auxvec.h" 2
# 5 "include/uapi/linux/auxvec.h" 2
# 5 "include/linux/auxvec.h" 2
# 12 "./arch/mips/include/asm/elf.h" 2
# 1 "include/linux/fs.h" 1

working version:

# 1 "./arch/mips/include/asm/auxvec.h" 1
# 1 "./arch/mips/include/uapi/asm/auxvec.h" 1
# 1 "./arch/mips/include/asm/auxvec.h" 2
# 5 "include/uapi/linux/auxvec.h" 2
# 5 "include/linux/auxvec.h" 2
# 12 "./arch/mips/include/asm/elf.h" 2
# 1 "include/linux/fs.h" 1

I've uploaded the cpp output and the config just in case:

https://www.monom.org/data/mips/auxvec/

I am still pretty confused about what should happen in which order. 
Maybe I should call the Confuse-A-Cat squat team.

cheers,
daniel

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

[Daniel Wagner]

Hi Maciej,

On 02/08/2016 05:22 PM, kbuild test robot wrote:
> Hi Daniel,
> 
> [auto build test ERROR on v4.5-rc3]
> [also build test ERROR on next-20160208]
> [if your patch is applied to the wrong git tree, please drop us a note to help improving the system]
> 
> url:    https://github.com/0day-ci/linux/commits/Daniel-Wagner/Differentiate-between-32-and-64-bit-ELF-header/20160208-234759
> config: mips-fuloong2e_defconfig (attached as .config)
> reproduce:
>         wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross
>         chmod +x ~/bin/make.cross
>         # save the attached .config to linux build tree
>         make.cross ARCH=mips 
> 
> All error/warnings (new ones prefixed by >>):
> 
>    arch/mips/kernel/../../../fs/binfmt_elf.c: In function 'load_elf_interp':
>>> arch/mips/kernel/binfmt_elfn32.c:38:7: error: implicit declaration of function 'mips_elf_check_machine' [-Werror=implicit-function-declaration]
>      if (!mips_elf_check_machine(__h))    \
>           ^
>>> arch/mips/kernel/../../../fs/binfmt_elf.c:536:7: note: in expansion of macro 'elf_check_arch'
>      if (!elf_check_arch(interp_elf_ex))
>           ^
>    cc1: some warnings being treated as errors
> --
>    arch/mips/kernel/../../../fs/binfmt_elf.c: In function 'load_elf_interp':

Hmm how I was able to build binfmt_elfo32.o because it should suffer
from the same problem.

I think reusing mips_elf_check_machine() in binfmt_elf?32.c is only
going to work if we include arch/mips/include/asm/elf.h. Though this
looks kind of wrong.

Should I add a mips_elf_check_machine() to binfmt_elf?32.c as well or
just leave them as they are at this point?

cheers,
daniel

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

[Daniel Wagner]

Hi Maciej,

On 02/08/2016 05:22 PM, kbuild test robot wrote:
> Hi Daniel,
> 
> [auto build test ERROR on v4.5-rc3]
> [also build test ERROR on next-20160208]
> [if your patch is applied to the wrong git tree, please drop us a note to help improving the system]
> 
> url:    https://github.com/0day-ci/linux/commits/Daniel-Wagner/Differentiate-between-32-and-64-bit-ELF-header/20160208-234759
> config: mips-fuloong2e_defconfig (attached as .config)
> reproduce:
>         wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross
>         chmod +x ~/bin/make.cross
>         # save the attached .config to linux build tree
>         make.cross ARCH=mips 
> 
> All error/warnings (new ones prefixed by >>):
> 
>    arch/mips/kernel/../../../fs/binfmt_elf.c: In function 'load_elf_interp':
>>> arch/mips/kernel/binfmt_elfn32.c:38:7: error: implicit declaration of function 'mips_elf_check_machine' [-Werror=implicit-function-declaration]
>      if (!mips_elf_check_machine(__h))    \
>           ^
>>> arch/mips/kernel/../../../fs/binfmt_elf.c:536:7: note: in expansion of macro 'elf_check_arch'
>      if (!elf_check_arch(interp_elf_ex))
>           ^
>    cc1: some warnings being treated as errors
> --
>    arch/mips/kernel/../../../fs/binfmt_elf.c: In function 'load_elf_interp':

Hmm how I was able to build binfmt_elfo32.o because it should suffer
from the same problem.

I think reusing mips_elf_check_machine() in binfmt_elf?32.c is only
going to work if we include arch/mips/include/asm/elf.h. Though this
looks kind of wrong.

Should I add a mips_elf_check_machine() to binfmt_elf?32.c as well or
just leave them as they are at this point?

cheers,
daniel

Re: [PATCH v3 1/3] mips: Use arch specific auxvec.h instead of generic-asm version

[Maciej W. Rozycki]

On Tue, 9 Feb 2016, Daniel Wagner wrote:

> Also I looked at the cpp output and saw that there was no uapi/asm/auxvec.h
> included instead it pulls arch/mips/include/generated/uapi/asm/auxvec.h

 Hmm, did you update your source in an old build tree and reuse it for a 
new build?  The rule to make arch/mips/include/generated/uapi/asm/auxvec.h 
was removed with commit ebb5e78cc634 ("MIPS: Initial implementation of a 
VDSO") as arch/mips/include/uapi/asm/auxvec.h was added, in the 4.4-rc1 
timeframe.  So the generated version is not supposed to be there anymore.

 Can you try `make mrproper' (stash away your .config) and see if the 
problem goes away?

  Maciej

Re: [PATCH v3 1/3] mips: Use arch specific auxvec.h instead of generic-asm version

[Maciej W. Rozycki]

On Tue, 9 Feb 2016, Daniel Wagner wrote:

> Also I looked at the cpp output and saw that there was no uapi/asm/auxvec.h
> included instead it pulls arch/mips/include/generated/uapi/asm/auxvec.h

 Hmm, did you update your source in an old build tree and reuse it for a 
new build?  The rule to make arch/mips/include/generated/uapi/asm/auxvec.h 
was removed with commit ebb5e78cc634 ("MIPS: Initial implementation of a 
VDSO") as arch/mips/include/uapi/asm/auxvec.h was added, in the 4.4-rc1 
timeframe.  So the generated version is not supposed to be there anymore.

 Can you try `make mrproper' (stash away your .config) and see if the 
problem goes away?

  Maciej

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

[Maciej W. Rozycki]

On Tue, 9 Feb 2016, Daniel Wagner wrote:

> > All error/warnings (new ones prefixed by >>):
> > 
> >    arch/mips/kernel/../../../fs/binfmt_elf.c: In function 'load_elf_interp':
> >>> arch/mips/kernel/binfmt_elfn32.c:38:7: error: implicit declaration of function 'mips_elf_check_machine' [-Werror=implicit-function-declaration]
> >      if (!mips_elf_check_machine(__h))    \
> >           ^
> >>> arch/mips/kernel/../../../fs/binfmt_elf.c:536:7: note: in expansion of macro 'elf_check_arch'
> >      if (!elf_check_arch(interp_elf_ex))
> >           ^
> >    cc1: some warnings being treated as errors
> > --
> >    arch/mips/kernel/../../../fs/binfmt_elf.c: In function 'load_elf_interp':
> 
> Hmm how I was able to build binfmt_elfo32.o because it should suffer
> from the same problem.
> 
> I think reusing mips_elf_check_machine() in binfmt_elf?32.c is only
> going to work if we include arch/mips/include/asm/elf.h. Though this
> looks kind of wrong.

 But neither binfmt_elf?32.c actually expands `elf_check_arch' and both 
include fs/binfmt_elf.c at the end, which in turn includes <linux/elf.h>, 
which in turn does include <asm/elf.h> before expanding `elf_check_arch', 
and consequently at that point `mips_elf_check_machine' will have been 
already defined.  So things are all right, except you need to define the 
macro outside `#ifndef ELF_ARCH'.

 I suggest moving it down, right below the conditional, rather than up as 
the top of the file contains generic MIPS ELF stuff.  I think all the 
three macros can go together, it doesn't appear to me they need to depend 
on `ELF_ARCH', and we can fix it up if ever in the future they have to.

 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.

  Maciej

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

[Maciej W. Rozycki]

On Tue, 9 Feb 2016, Daniel Wagner wrote:

> > All error/warnings (new ones prefixed by >>):
> > 
> >    arch/mips/kernel/../../../fs/binfmt_elf.c: In function 'load_elf_interp':
> >>> arch/mips/kernel/binfmt_elfn32.c:38:7: error: implicit declaration of function 'mips_elf_check_machine' [-Werror=implicit-function-declaration]
> >      if (!mips_elf_check_machine(__h))    \
> >           ^
> >>> arch/mips/kernel/../../../fs/binfmt_elf.c:536:7: note: in expansion of macro 'elf_check_arch'
> >      if (!elf_check_arch(interp_elf_ex))
> >           ^
> >    cc1: some warnings being treated as errors
> > --
> >    arch/mips/kernel/../../../fs/binfmt_elf.c: In function 'load_elf_interp':
> 
> Hmm how I was able to build binfmt_elfo32.o because it should suffer
> from the same problem.
> 
> I think reusing mips_elf_check_machine() in binfmt_elf?32.c is only
> going to work if we include arch/mips/include/asm/elf.h. Though this
> looks kind of wrong.

 But neither binfmt_elf?32.c actually expands `elf_check_arch' and both 
include fs/binfmt_elf.c at the end, which in turn includes <linux/elf.h>, 
which in turn does include <asm/elf.h> before expanding `elf_check_arch', 
and consequently at that point `mips_elf_check_machine' will have been 
already defined.  So things are all right, except you need to define the 
macro outside `#ifndef ELF_ARCH'.

 I suggest moving it down, right below the conditional, rather than up as 
the top of the file contains generic MIPS ELF stuff.  I think all the 
three macros can go together, it doesn't appear to me they need to depend 
on `ELF_ARCH', and we can fix it up if ever in the future they have to.

 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.

  Maciej

Re: [PATCH v3 1/3] mips: Use arch specific auxvec.h instead of generic-asm version

[Daniel Wagner]

On 02/09/2016 12:46 PM, Maciej W. Rozycki wrote:
> On Tue, 9 Feb 2016, Daniel Wagner wrote:
> 
>> Also I looked at the cpp output and saw that there was no uapi/asm/auxvec.h
>> included instead it pulls arch/mips/include/generated/uapi/asm/auxvec.h
> 
>  Hmm, did you update your source in an old build tree and reuse it for a 
> new build?  The rule to make arch/mips/include/generated/uapi/asm/auxvec.h 
> was removed with commit ebb5e78cc634 ("MIPS: Initial implementation of a 
> VDSO") as arch/mips/include/uapi/asm/auxvec.h was added, in the 4.4-rc1 
> timeframe.  So the generated version is not supposed to be there anymore.
> 
>  Can you try `make mrproper' (stash away your .config) and see if the 
> problem goes away?

Indeed, 'make mrproper' did the trick. I am sorry for the noise. Until
now I never had to use mrproper before and therefore didn't think of it.

Thanks a lot!
Daniel

Re: [PATCH v3 1/3] mips: Use arch specific auxvec.h instead of generic-asm version

[Daniel Wagner]

On 02/09/2016 12:46 PM, Maciej W. Rozycki wrote:
> On Tue, 9 Feb 2016, Daniel Wagner wrote:
> 
>> Also I looked at the cpp output and saw that there was no uapi/asm/auxvec.h
>> included instead it pulls arch/mips/include/generated/uapi/asm/auxvec.h
> 
>  Hmm, did you update your source in an old build tree and reuse it for a 
> new build?  The rule to make arch/mips/include/generated/uapi/asm/auxvec.h 
> was removed with commit ebb5e78cc634 ("MIPS: Initial implementation of a 
> VDSO") as arch/mips/include/uapi/asm/auxvec.h was added, in the 4.4-rc1 
> timeframe.  So the generated version is not supposed to be there anymore.
> 
>  Can you try `make mrproper' (stash away your .config) and see if the 
> problem goes away?

Indeed, 'make mrproper' did the trick. I am sorry for the noise. Until
now I never had to use mrproper before and therefore didn't think of it.

Thanks a lot!
Daniel

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

[Daniel Wagner]

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

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

[Daniel Wagner]

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

Re: [PATCH v3 1/3] mips: Use arch specific auxvec.h instead of generic-asm version

[Maciej W. Rozycki]

On Tue, 9 Feb 2016, Daniel Wagner wrote:

> >  Can you try `make mrproper' (stash away your .config) and see if the 
> > problem goes away?
> 
> Indeed, 'make mrproper' did the trick. I am sorry for the noise. Until
> now I never had to use mrproper before and therefore didn't think of it.

 People have been being hit by stale generated files recently and I reckon 
effort has been taken to address the issue by removing them automagically 
somehow on rebuilds.  Until that has been complete you're advised to clean 
your build tree after an update, or maybe rebuild speculatively and then 
clean only if something actually breaks.

  Maciej

Re: [PATCH v3 1/3] mips: Use arch specific auxvec.h instead of generic-asm version

[Maciej W. Rozycki]

On Tue, 9 Feb 2016, Daniel Wagner wrote:

> >  Can you try `make mrproper' (stash away your .config) and see if the 
> > problem goes away?
> 
> Indeed, 'make mrproper' did the trick. I am sorry for the noise. Until
> now I never had to use mrproper before and therefore didn't think of it.

 People have been being hit by stale generated files recently and I reckon 
effort has been taken to address the issue by removing them automagically 
somehow on rebuilds.  Until that has been complete you're advised to clean 
your build tree after an update, or maybe rebuild speculatively and then 
clean only if something actually breaks.

  Maciej

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

[Maciej W. Rozycki]

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

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

[Maciej W. Rozycki]

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

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

[Daniel Wagner]

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

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

[Daniel Wagner]

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

Re: [PATCH v3 1/3] mips: Use arch specific auxvec.h instead of generic-asm version

[Daniel Wagner]

On 02/09/2016 03:51 PM, Maciej W. Rozycki wrote:
> On Tue, 9 Feb 2016, Daniel Wagner wrote:
> 
>>>  Can you try `make mrproper' (stash away your .config) and see if the 
>>> problem goes away?
>>
>> Indeed, 'make mrproper' did the trick. I am sorry for the noise. Until
>> now I never had to use mrproper before and therefore didn't think of it.
> 
>  People have been being hit by stale generated files recently and I reckon 
> effort has been taken to address the issue by removing them automagically 
> somehow on rebuilds.  Until that has been complete you're advised to clean 
> your build tree after an update, or maybe rebuild speculatively and then 
> clean only if something actually breaks.

FWIW, I just found out that running mrproper before building
fuloong2e_defconfig is fixing the problem reported kbuild robot too.

Re: [PATCH v3 1/3] mips: Use arch specific auxvec.h instead of generic-asm version

[Daniel Wagner]

On 02/09/2016 03:51 PM, Maciej W. Rozycki wrote:
> On Tue, 9 Feb 2016, Daniel Wagner wrote:
> 
>>>  Can you try `make mrproper' (stash away your .config) and see if the 
>>> problem goes away?
>>
>> Indeed, 'make mrproper' did the trick. I am sorry for the noise. Until
>> now I never had to use mrproper before and therefore didn't think of it.
> 
>  People have been being hit by stale generated files recently and I reckon 
> effort has been taken to address the issue by removing them automagically 
> somehow on rebuilds.  Until that has been complete you're advised to clean 
> your build tree after an update, or maybe rebuild speculatively and then 
> clean only if something actually breaks.

FWIW, I just found out that running mrproper before building
fuloong2e_defconfig is fixing the problem reported kbuild robot too.

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

[Daniel Wagner]

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

[PATCH v4 1/2] crash_dump: Add vmcore_elf32_check_arch

[Daniel Wagner]

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

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

[Daniel Wagner]

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

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

[Ralf Baechle]

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

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

[Maciej W. Rozycki]

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

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

[Maciej W. Rozycki]

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

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

[Daniel Wagner]

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

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

[Daniel Wagner]

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

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

[Maciej W. Rozycki]

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

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

[Maciej W. Rozycki]

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

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

[Ralf Baechle]

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