[PATCH] arm64: compat: fix compat types affecting struct compat_elf_prpsinfo

[PATCH] arm64: compat: fix compat types affecting struct compat_elf_prpsinfo

[Victor Kamensky]

The compat_elf_prpsinfo structure does not match the arch/arm struct
elf_pspsinfo definition. As result NT_PRPSINFO note in core file
created by arm64 kernel for aarch32 (compat) process has wrong size.
So gdb cannot display command that caused process crash.

Fix is to change size of __compat_uid_t, __compat_gid_t so it would
match size of similar fields in arch/arm case.

Signed-off-by: Victor Kamensky <victor.kamensky@linaro.org>
---
 arch/arm64/include/asm/compat.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/arm64/include/asm/compat.h b/arch/arm64/include/asm/compat.h
index 253e33b..56de5aa 100644
--- a/arch/arm64/include/asm/compat.h
+++ b/arch/arm64/include/asm/compat.h
@@ -37,8 +37,8 @@ typedef s32		compat_ssize_t;
 typedef s32		compat_time_t;
 typedef s32		compat_clock_t;
 typedef s32		compat_pid_t;
-typedef u32		__compat_uid_t;
-typedef u32		__compat_gid_t;
+typedef u16		__compat_uid_t;
+typedef u16		__compat_gid_t;
 typedef u16		__compat_uid16_t;
 typedef u16		__compat_gid16_t;
 typedef u32		__compat_uid32_t;
-- 
1.8.1.4

[PATCH] arm64: compat: fix compat types affecting struct compat_elf_prpsinfo

[Victor Kamensky]

[sorry, it is resend: fixed cover-latter email subject line]

Hi,

I've run into this issue while running gdb testsuite in aarch32 
rootfs on top of V8 kernel. It turns out that V8 kernel
when write core file creates NT_PRPSINFO of wrong size.
Core file psinfo note created by V7 kernel size is 124, but 
V8 when creates aarch32 process core for psinfo note is puts 
size as 128.

Root cause is that __compat_ui d_t and __compat_gid_t types defined
as u32 but corresponding fields types in V7 case are u16. As result
V8 sizeof(struct compat_elf_prpsinfo) is 128, whereas V7 
sizeof(struct elf_prpsinfo) is 124.

Below are test case that illustrate the issue in the way how 
it manifests itself in user visible form: gdb fails to display what
executable crashed (PRPSINFO.command) and 'file' over core cannot
display crashed executable name either. Test case is followed by 
similar commands executed in V7 rootfs/kernel. And it follows 
with rerunning test case on top of kernel that contains fix that.

Proposed fix changes __compat_uid_t and __compat_gid_t typedefs
to u16, so layout of V8 struct compat_elf_prpsinfo matches 
layout of V7 struct elf_prpsinfo. Note as result of the fix
some other compat structures like compat_ipc_perm, compat_msqid_ds,
compat_semid_ds, compat_shmid_ds change their size because
they use fields of __compat_uid_t and __compat_gid_t types.
And after the fix they match their V7 counterparts, as they were
not before. I strongly believe such change is the right thing,
although I don't know how to test those directly.

Thanks,
Victor

aarch32 process core file (before fix)
--------------------------------------

In below output note gdb failed to indicate 'Core was generated 
by `/home/root/cf/cf' and 'file core' command does not test what process
created core file.

root at genericarmv7a:~/cf# uname -a
Linux genericarmv7a 3.17.0 #1 SMP PREEMPT Sat Oct 11 22:44:23 PDT 2014 aarch64 GNU/Linux
root at genericarmv7a:~/cf# pwd
/home/root/cf
root at genericarmv7a:~/cf# ls
cf.c
root at genericarmv7a:~/cf# cat cf.c
#include <stdlib.h>

int main (void)
{
	abort();
	return 0;
}
root at genericarmv7a:~/cf# gcc -g -o cf cf.c
root at genericarmv7a:~/cf# ulimit -c unlimited
root at genericarmv7a:~/cf# ls
cf  cf.c
root at genericarmv7a:~/cf# /home/root/cf/cf
Aborted (core dumped)
root at genericarmv7a:~/cf# ls
cf  cf.c  core
root at genericarmv7a:~/cf# gdb -core=./core
GNU gdb (Linaro GDB) 7.6.1-2013.10
Copyright (C) 2013 Free Software Foundation, Inc.
License GPLv3+: GNU GPL version 3 or later <http://gnu.org/licenses/gpl.html>
This is free software: you are free to change and redistribute it.
There is NO WARRANTY, to the extent permitted by law.  Type "show copying"
and "show warranty" for details.
This GDB was configured as "arm-oe-linux-gnueabi".
For bug reporting instructions, please see:
<http://bugs.launchpad.net/gdb-linaro/>.
[New LWP 970]
Program terminated with signal 6, Aborted.
#0  0xf72a2ed4 in ?? ()
(gdb) info shared
No shared libraries loaded at this time.
(gdb) quit
root at genericarmv7a:~/cf# file core
core: ELF 32-bit LSB core file ARM, version 1 (SYSV), SVR4-style
root at genericarmv7a:~/cf# readelf --notes core | head -10

Displaying notes found at file offset 0x000001f4 with length 0x000003d8:
  Owner                 Data size	Description
  CORE                 0x00000094	NT_PRSTATUS (prstatus structure)
  CORE                 0x00000080	NT_PRPSINFO (prpsinfo structure) <---- size of note is 0x80
  CORE                 0x00000080	NT_SIGINFO (siginfo_t data)
  CORE                 0x00000098	NT_AUXV (auxiliary vector)
  CORE                 0x00000146	NT_FILE (mapped files)
    Page size: 4096
         Start         End Page Offset


v7 process core file
--------------------

Here is how it is supposed to look. Note 'Core was generated 
by `/home/root/cf/cf' output and note difference in output of 'file
core' command.

root at genericarmv7a:~/cf# uname -a
Linux genericarmv7a 3.17.0-rc7 #1 SMP Tue Sep 30 22:11:51 PDT 2014 armv7l GNU/Linux
root at genericarmv7a:~/cf# pwd
/home/root/cf
root at genericarmv7a:~/cf# ls
cf.c
root@genericarmv7a:~/cf# cat cf.c
#include <stdlib.h>

int main (void)
{
	abort();
	return 0;
}
root at genericarmv7a:~/cf# gcc -g -o cf cf.c
root at genericarmv7a:~/cf# ulimit -c unlimited
root at genericarmv7a:~/cf# ls
cf  cf.c
root at genericarmv7a:~/cf# /home/root/cf/cf
Aborted (core dumped)
root at genericarmv7a:~/cf# ls              
cf  cf.c  core
root at genericarmv7a:~/cf# gdb -core=./core
GNU gdb (Linaro GDB) 7.8-2014.09
Copyright (C) 2014 Free Software Foundation, Inc.
License GPLv3+: GNU GPL version 3 or later <http://gnu.org/licenses/gpl.html>
This is free software: you are free to change and redistribute it.
There is NO WARRANTY, to the extent permitted by law.  Type "show copying"
and "show warranty" for details.
This GDB was configured as "arm-oe-linux-gnueabi".
Type "show configuration" for configuration details.
For bug reporting instructions, please see:
<http://bugs.linaro.org>.
Find the GDB manual and other documentation resources online at:
<http://www.gnu.org/software/gdb/documentation/>.
For help, type "help".
Type "apropos word" to search for commands related to "word".
[New LWP 1527]
Core was generated by `/home/root/cf/cf'.
Program terminated with signal SIGABRT, Aborted.
#0  0xb6e21344 in ?? ()
(gdb) quit
root at genericarmv7a:~/cf# file core
core: ELF 32-bit LSB core file ARM, version 1 (SYSV), SVR4-style, from '/home/root/cf/cf'
root at genericarmv7a:~/cf# readelf --notes core | head -10

Displaying notes found at file offset 0x00000234 with length 0x00000574:
  Owner                 Data size	Description
  CORE                 0x00000094	NT_PRSTATUS (prstatus structure)
  CORE                 0x0000007c	NT_PRPSINFO (prpsinfo structure) <---- size of note is 0x7c
  CORE                 0x00000080	NT_SIGINFO (siginfo_t data)
  CORE                 0x00000098	NT_AUXV (auxiliary vector)
  CORE                 0x00000146	NT_FILE (mapped files)
    Page size: 4096
         Start         End Page Offset

After the fix
--------------

Here is test case rerun in V8 kernel with proposed fix.

root at genericarmv7a:~/cf# uname -a
Linux genericarmv7a 3.17.0+ #1 SMP PREEMPT Sat Oct 11 23:24:27 PDT 2014 aarch64 GNU/Linux
root at genericarmv7a:~/cf# pwd
/home/root/cf
root at genericarmv7a:~/cf# ls
cf  cf.c  core
root at genericarmv7a:~/cf# mv core core.broken
root at genericarmv7a:~/cf# ulimit -c unlimited
root at genericarmv7a:~/cf# /home/root/cf/cf
Aborted (core dumped)
root at genericarmv7a:~/cf# ls              
cf  cf.c  core	core.broken
root@genericarmv7a:~/cf# gdb -core=./core
GNU gdb (Linaro GDB) 7.6.1-2013.10
Copyright (C) 2013 Free Software Foundation, Inc.
License GPLv3+: GNU GPL version 3 or later <http://gnu.org/licenses/gpl.html>
This is free software: you are free to change and redistribute it.
There is NO WARRANTY, to the extent permitted by law.  Type "show copying"
and "show warranty" for details.
This GDB was configured as "arm-oe-linux-gnueabi".
For bug reporting instructions, please see:
<http://bugs.launchpad.net/gdb-linaro/>.
[New LWP 937]
Core was generated by `/home/root/cf/cf'.
Program terminated with signal 6, Aborted.
#0  0xf7358ed4 in ?? ()
(gdb) quit
root at genericarmv7a:~/cf# file core
core: ELF 32-bit LSB core file ARM, version 1 (SYSV), SVR4-style, from '/home/root/cf/cf'
root at genericarmv7a:~/cf# readelf --notes core | head -10

Displaying notes found at file offset 0x000001f4 with length 0x000003d4:
  Owner                 Data size	Description
  CORE                 0x00000094	NT_PRSTATUS (prstatus structure)
  CORE                 0x0000007c	NT_PRPSINFO (prpsinfo structure)
  CORE                 0x00000080	NT_SIGINFO (siginfo_t data)
  CORE                 0x00000098	NT_AUXV (auxiliary vector)
  CORE                 0x00000146	NT_FILE (mapped files)
    Page size: 4096
         Start         End Page Offset

Victor Kamensky (1):
  arm64: compat: fix compat types affecting struct compat_elf_prpsinfo

 arch/arm64/include/asm/compat.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

-- 
1.8.1.4

[PATCH] arm64: compat: fix compat types affecting struct compat_elf_prpsinfo

[Catalin Marinas]

On Tue, Oct 14, 2014 at 06:55:05AM +0100, Victor Kamensky wrote:
> The compat_elf_prpsinfo structure does not match the arch/arm struct
> elf_pspsinfo definition. As result NT_PRPSINFO note in core file
> created by arm64 kernel for aarch32 (compat) process has wrong size.
> So gdb cannot display command that caused process crash.
> 
> Fix is to change size of __compat_uid_t, __compat_gid_t so it would
> match size of similar fields in arch/arm case.
> 
> Signed-off-by: Victor Kamensky <victor.kamensky@linaro.org>
> ---
>  arch/arm64/include/asm/compat.h | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/compat.h b/arch/arm64/include/asm/compat.h
> index 253e33b..56de5aa 100644
> --- a/arch/arm64/include/asm/compat.h
> +++ b/arch/arm64/include/asm/compat.h
> @@ -37,8 +37,8 @@ typedef s32		compat_ssize_t;
>  typedef s32		compat_time_t;
>  typedef s32		compat_clock_t;
>  typedef s32		compat_pid_t;
> -typedef u32		__compat_uid_t;
> -typedef u32		__compat_gid_t;
> +typedef u16		__compat_uid_t;
> +typedef u16		__compat_gid_t;
>  typedef u16		__compat_uid16_t;
>  typedef u16		__compat_gid16_t;
>  typedef u32		__compat_uid32_t;

__compat_uid_t is defined to match the arm32 uid_t and that would be
__kernel_uid32_t (or __compat_uid32_t). So this is the correct fix.

The problem is that elf_prpsinfo uses __kernel_uid_t which arm32 defines
as (unsigned short) while compat_elf_prspinfo uses __compat_uid_t which
is 32-bit. In reality compat_uid_t is different from the arm32
kernel_uid_t (other 32-bit architectures may use a 32-bit kernel_uid_t).

So we either define a compat_kernel_uid_t or allow per-arch
compat_elf_prspinfo. I would go for the former and also grep the kernel
for other uses of compat_uid_t assuming the same size as the 32-bit
__kernel_uid_t.

-- 
Catalin

[PATCH] arm64: compat: fix compat types affecting struct compat_elf_prpsinfo

[Catalin Marinas]

On Tue, Oct 14, 2014 at 09:51:25AM +0100, Catalin Marinas wrote:
> On Tue, Oct 14, 2014 at 06:55:05AM +0100, Victor Kamensky wrote:
> > The compat_elf_prpsinfo structure does not match the arch/arm struct
> > elf_pspsinfo definition. As result NT_PRPSINFO note in core file
> > created by arm64 kernel for aarch32 (compat) process has wrong size.
> > So gdb cannot display command that caused process crash.
> > 
> > Fix is to change size of __compat_uid_t, __compat_gid_t so it would
> > match size of similar fields in arch/arm case.
> > 
> > Signed-off-by: Victor Kamensky <victor.kamensky@linaro.org>
> > ---
> >  arch/arm64/include/asm/compat.h | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/arch/arm64/include/asm/compat.h b/arch/arm64/include/asm/compat.h
> > index 253e33b..56de5aa 100644
> > --- a/arch/arm64/include/asm/compat.h
> > +++ b/arch/arm64/include/asm/compat.h
> > @@ -37,8 +37,8 @@ typedef s32		compat_ssize_t;
> >  typedef s32		compat_time_t;
> >  typedef s32		compat_clock_t;
> >  typedef s32		compat_pid_t;
> > -typedef u32		__compat_uid_t;
> > -typedef u32		__compat_gid_t;
> > +typedef u16		__compat_uid_t;
> > +typedef u16		__compat_gid_t;
> >  typedef u16		__compat_uid16_t;
> >  typedef u16		__compat_gid16_t;
> >  typedef u32		__compat_uid32_t;
> 
> __compat_uid_t is defined to match the arm32 uid_t and that would be
> __kernel_uid32_t (or __compat_uid32_t). So this is the correct fix.

I forgot a "not" in here. The above reads as "this is _not_ the correct
fix".

-- 
Catalin

[PATCH] arm64: compat: fix compat types affecting struct compat_elf_prpsinfo

[Arnd Bergmann]

On Tuesday 14 October 2014 09:51:25 Catalin Marinas wrote:
> On Tue, Oct 14, 2014 at 06:55:05AM +0100, Victor Kamensky wrote:
> > The compat_elf_prpsinfo structure does not match the arch/arm struct
> > elf_pspsinfo definition. As result NT_PRPSINFO note in core file
> > created by arm64 kernel for aarch32 (compat) process has wrong size.
> > So gdb cannot display command that caused process crash.
> > 
> > Fix is to change size of __compat_uid_t, __compat_gid_t so it would
> > match size of similar fields in arch/arm case.
> > 
> > Signed-off-by: Victor Kamensky <victor.kamensky@linaro.org>
> > ---
> >  arch/arm64/include/asm/compat.h | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/arch/arm64/include/asm/compat.h b/arch/arm64/include/asm/compat.h
> > index 253e33b..56de5aa 100644
> > --- a/arch/arm64/include/asm/compat.h
> > +++ b/arch/arm64/include/asm/compat.h
> > @@ -37,8 +37,8 @@ typedef s32         compat_ssize_t;
> >  typedef s32          compat_time_t;
> >  typedef s32          compat_clock_t;
> >  typedef s32          compat_pid_t;
> > -typedef u32          __compat_uid_t;
> > -typedef u32          __compat_gid_t;
> > +typedef u16          __compat_uid_t;
> > +typedef u16          __compat_gid_t;
> >  typedef u16          __compat_uid16_t;
> >  typedef u16          __compat_gid16_t;
> >  typedef u32          __compat_uid32_t;
> 
> __compat_uid_t is defined to match the arm32 uid_t and that would be
> __kernel_uid32_t (or __compat_uid32_t). So this is not the correct fix.

No, I think Victor is right: __compat_uid_t should match the arm32
__kernel_uid_t, not the arm32 uid_t, which is just a kernel-internal
definition, while the __kernel_uid_t is the one used in all user
visible interfaces.

The definition in your asm/compat.h file seems to be a mistake.

> The problem is that elf_prpsinfo uses __kernel_uid_t which arm32 defines
> as (unsigned short) while compat_elf_prspinfo uses __compat_uid_t which
> is 32-bit. In reality compat_uid_t is different from the arm32
> kernel_uid_t (other 32-bit architectures may use a 32-bit kernel_uid_t).

compat_uid_t should match the __kernel_uid_t for all 32-bit architectures
that are emulated on a 64-bit architecture, that is the definition.

	Arnd

[PATCH] arm64: compat: fix compat types affecting struct compat_elf_prpsinfo

[Catalin Marinas]

On Tue, Oct 14, 2014 at 10:29:14AM +0100, Arnd Bergmann wrote:
> On Tuesday 14 October 2014 09:51:25 Catalin Marinas wrote:
> > On Tue, Oct 14, 2014 at 06:55:05AM +0100, Victor Kamensky wrote:
> > > The compat_elf_prpsinfo structure does not match the arch/arm struct
> > > elf_pspsinfo definition. As result NT_PRPSINFO note in core file
> > > created by arm64 kernel for aarch32 (compat) process has wrong size.
> > > So gdb cannot display command that caused process crash.
> > > 
> > > Fix is to change size of __compat_uid_t, __compat_gid_t so it would
> > > match size of similar fields in arch/arm case.
> > > 
> > > Signed-off-by: Victor Kamensky <victor.kamensky@linaro.org>
> > > ---
> > >  arch/arm64/include/asm/compat.h | 4 ++--
> > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/arch/arm64/include/asm/compat.h b/arch/arm64/include/asm/compat.h
> > > index 253e33b..56de5aa 100644
> > > --- a/arch/arm64/include/asm/compat.h
> > > +++ b/arch/arm64/include/asm/compat.h
> > > @@ -37,8 +37,8 @@ typedef s32         compat_ssize_t;
> > >  typedef s32          compat_time_t;
> > >  typedef s32          compat_clock_t;
> > >  typedef s32          compat_pid_t;
> > > -typedef u32          __compat_uid_t;
> > > -typedef u32          __compat_gid_t;
> > > +typedef u16          __compat_uid_t;
> > > +typedef u16          __compat_gid_t;
> > >  typedef u16          __compat_uid16_t;
> > >  typedef u16          __compat_gid16_t;
> > >  typedef u32          __compat_uid32_t;
> > 
> > __compat_uid_t is defined to match the arm32 uid_t and that would be
> > __kernel_uid32_t (or __compat_uid32_t). So this is not the correct fix.
> 
> No, I think Victor is right: __compat_uid_t should match the arm32
> __kernel_uid_t, not the arm32 uid_t, which is just a kernel-internal
> definition, while the __kernel_uid_t is the one used in all user
> visible interfaces.

Ah, I think you are right. The compat_uid_t (without underscores) should
match the arm32 uid_t while __compat_uid_t would match arm32
__kernel_uid_t.

> The definition in your asm/compat.h file seems to be a mistake.

What's weird is that 32-bit LTP on top of the arm64 kernel hasn't caught
this for the past years.

> > The problem is that elf_prpsinfo uses __kernel_uid_t which arm32 defines
> > as (unsigned short) while compat_elf_prspinfo uses __compat_uid_t which
> > is 32-bit. In reality compat_uid_t is different from the arm32
> > kernel_uid_t (other 32-bit architectures may use a 32-bit kernel_uid_t).
> 
> compat_uid_t should match the __kernel_uid_t for all 32-bit architectures
> that are emulated on a 64-bit architecture, that is the definition.

I guess you meant __compat_uid_t here. The compat_uid_t type is u32
already.

So that patch is fine, I'll take it for 3.17 (and cc stable all the way
back to 3.7).

Thanks.

-- 
Catalin

[PATCH] arm64: compat: fix compat types affecting struct compat_elf_prpsinfo

[Arnd Bergmann]

On Tuesday 14 October 2014 10:53:53 Catalin Marinas wrote:
> > > The problem is that elf_prpsinfo uses __kernel_uid_t which arm32 defines
> > > as (unsigned short) while compat_elf_prspinfo uses __compat_uid_t which
> > > is 32-bit. In reality compat_uid_t is different from the arm32
> > > kernel_uid_t (other 32-bit architectures may use a 32-bit kernel_uid_t).
> > 
> > compat_uid_t should match the __kernel_uid_t for all 32-bit architectures
> > that are emulated on a 64-bit architecture, that is the definition.
> 
> I guess you meant __compat_uid_t here. The compat_uid_t type is u32
> already.

Ah, that's weird: compat_uid_t is not used anywhere in the kernel,
and the definition is odd. Apparently it was intentional back in 2005
when Stephen Rothwell introduced it as part of 202e5979af4d9
("compat: be more consistent about [ug]id_t"), but I have trouble
understanding the intention.

> So that patch is fine, I'll take it for 3.17 (and cc stable all the way
> back to 3.7).

Ok. It might be worth checking if there are any uses of __compat_uid_t
in arm64 that should have been __compat_uid32_t. Currently they are
the same, but after Victor's patch, they would be different, which could
cause regressions.

	Arnd

[PATCH] arm64: compat: fix compat types affecting struct compat_elf_prpsinfo

[Catalin Marinas]

On Tue, Oct 14, 2014 at 11:08:19AM +0100, Arnd Bergmann wrote:
> On Tuesday 14 October 2014 10:53:53 Catalin Marinas wrote:
> > > > The problem is that elf_prpsinfo uses __kernel_uid_t which arm32 defines
> > > > as (unsigned short) while compat_elf_prspinfo uses __compat_uid_t which
> > > > is 32-bit. In reality compat_uid_t is different from the arm32
> > > > kernel_uid_t (other 32-bit architectures may use a 32-bit kernel_uid_t).
> > > 
> > > compat_uid_t should match the __kernel_uid_t for all 32-bit architectures
> > > that are emulated on a 64-bit architecture, that is the definition.
> > 
> > I guess you meant __compat_uid_t here. The compat_uid_t type is u32
> > already.
> 
> Ah, that's weird: compat_uid_t is not used anywhere in the kernel,
> and the definition is odd. Apparently it was intentional back in 2005
> when Stephen Rothwell introduced it as part of 202e5979af4d9
> ("compat: be more consistent about [ug]id_t"), but I have trouble
> understanding the intention.

It may be worth removing to avoid confusion.

> > So that patch is fine, I'll take it for 3.17 (and cc stable all the way
> > back to 3.7).
> 
> Ok. It might be worth checking if there are any uses of __compat_uid_t
> in arm64 that should have been __compat_uid32_t. Currently they are
> the same, but after Victor's patch, they would be different, which could
> cause regressions.

A quick grep shows __compat_uid_t being used for:

struct compat_ncp_mount_data
struct compat_elf_prpsinfo
struct compat_ipc_perm

In all these cases, the native structures on arm32 would use
__kernel_uid_t. The arch/arm64 code doesn't make any use of
__compat_uid_t, apart from defining it.

But I'll run some LTP again to make sure (though I don't have many hopes
of it being useful since this bug wasn't previously detected).

-- 
Catalin

[PATCH] arm64: compat: fix compat types affecting struct compat_elf_prpsinfo

[Victor Kamensky]

On 14 October 2014 03:28, Catalin Marinas <catalin.marinas@arm.com> wrote:
> On Tue, Oct 14, 2014 at 11:08:19AM +0100, Arnd Bergmann wrote:
>> On Tuesday 14 October 2014 10:53:53 Catalin Marinas wrote:
>> > > > The problem is that elf_prpsinfo uses __kernel_uid_t which arm32 defines
>> > > > as (unsigned short) while compat_elf_prspinfo uses __compat_uid_t which
>> > > > is 32-bit. In reality compat_uid_t is different from the arm32
>> > > > kernel_uid_t (other 32-bit architectures may use a 32-bit kernel_uid_t).
>> > >
>> > > compat_uid_t should match the __kernel_uid_t for all 32-bit architectures
>> > > that are emulated on a 64-bit architecture, that is the definition.
>> >
>> > I guess you meant __compat_uid_t here. The compat_uid_t type is u32
>> > already.
>>
>> Ah, that's weird: compat_uid_t is not used anywhere in the kernel,
>> and the definition is odd. Apparently it was intentional back in 2005
>> when Stephen Rothwell introduced it as part of 202e5979af4d9
>> ("compat: be more consistent about [ug]id_t"), but I have trouble
>> understanding the intention.
>
> It may be worth removing to avoid confusion.

Do I need to do that? Or it can be done latter? I think, if anyone will do
that, it should be separate commit anyway.

>> > So that patch is fine, I'll take it for 3.17 (and cc stable all the way
>> > back to 3.7).

Catalin, Arnd, do I have permission to use your Acked-by with next
post of the patch (where I would "cc stable")?

>>
>> Ok. It might be worth checking if there are any uses of __compat_uid_t
>> in arm64 that should have been __compat_uid32_t. Currently they are
>> the same, but after Victor's patch, they would be different, which could
>> cause regressions.
>
> A quick grep shows __compat_uid_t being used for:
>
> struct compat_ncp_mount_data
> struct compat_elf_prpsinfo
> struct compat_ipc_perm
>
> In all these cases, the native structures on arm32 would use
> __kernel_uid_t. The arch/arm64 code doesn't make any use of
> __compat_uid_t, apart from defining it.

When I run into the issue, I've tried to look for similar mismatch issues
in other places. I wrote quick awk script that would parse
'readelf --debug-dump vmlinux'
output and dump names and sizes of all arm64 structs that starts
with compat_ and then compared them with corresponding structures
sizes in TC2 image. I saw that compat_ncp_mount_data,
compat_elf_prpsinfo, compat_ipc_perm and three other that use
compat_ipc_perm sizes changed. But after the fix applied they
match arch/arm sizes, and they were not matching before.

Besides those in all other cases arm64 compat and corresponding
arch/arm struct sizes match each other (modulo missing features in
TC2 image that were not checked; like cdrom, floppy, video related,
and few others).

Thanks,
Victor

> But I'll run some LTP again to make sure (though I don't have many hopes
> of it being useful since this bug wasn't previously detected).
>
> --
> Catalin

[PATCH] arm64: compat: fix compat types affecting struct compat_elf_prpsinfo

[Arnd Bergmann]

On Tuesday 14 October 2014 09:38:15 Victor Kamensky wrote:
> On 14 October 2014 03:28, Catalin Marinas <catalin.marinas@arm.com> wrote:
> > On Tue, Oct 14, 2014 at 11:08:19AM +0100, Arnd Bergmann wrote:
> >> On Tuesday 14 October 2014 10:53:53 Catalin Marinas wrote:
> >> > > > The problem is that elf_prpsinfo uses __kernel_uid_t which arm32 defines
> >> > > > as (unsigned short) while compat_elf_prspinfo uses __compat_uid_t which
> >> > > > is 32-bit. In reality compat_uid_t is different from the arm32
> >> > > > kernel_uid_t (other 32-bit architectures may use a 32-bit kernel_uid_t).
> >> > >
> >> > > compat_uid_t should match the __kernel_uid_t for all 32-bit architectures
> >> > > that are emulated on a 64-bit architecture, that is the definition.
> >> >
> >> > I guess you meant __compat_uid_t here. The compat_uid_t type is u32
> >> > already.
> >>
> >> Ah, that's weird: compat_uid_t is not used anywhere in the kernel,
> >> and the definition is odd. Apparently it was intentional back in 2005
> >> when Stephen Rothwell introduced it as part of 202e5979af4d9
> >> ("compat: be more consistent about [ug]id_t"), but I have trouble
> >> understanding the intention.
> >
> > It may be worth removing to avoid confusion.
> 
> Do I need to do that? Or it can be done latter? I think, if anyone will do
> that, it should be separate commit anyway.

Yes, a separate commit is best, most importantly because it makes no sense
to backport that to stable.

> >> > So that patch is fine, I'll take it for 3.17 (and cc stable all the way
> >> > back to 3.7).
> 
> Catalin, Arnd, do I have permission to use your Acked-by with next
> post of the patch (where I would "cc stable")?

Please add mine.

> >> Ok. It might be worth checking if there are any uses of __compat_uid_t
> >> in arm64 that should have been __compat_uid32_t. Currently they are
> >> the same, but after Victor's patch, they would be different, which could
> >> cause regressions.
> >
> > A quick grep shows __compat_uid_t being used for:
> >
> > struct compat_ncp_mount_data
> > struct compat_elf_prpsinfo
> > struct compat_ipc_perm
> >
> > In all these cases, the native structures on arm32 would use
> > __kernel_uid_t. The arch/arm64 code doesn't make any use of
> > __compat_uid_t, apart from defining it.
> 
> When I run into the issue, I've tried to look for similar mismatch issues
> in other places. I wrote quick awk script that would parse
> 'readelf --debug-dump vmlinux'
> output and dump names and sizes of all arm64 structs that starts
> with compat_ and then compared them with corresponding structures
> sizes in TC2 image. I saw that compat_ncp_mount_data,
> compat_elf_prpsinfo, compat_ipc_perm and three other that use
> compat_ipc_perm sizes changed. But after the fix applied they
> match arch/arm sizes, and they were not matching before.

Oh, cool. I didn't even know about readelf --debug-dump. I'll
definitely need that soon, thanks for mentioning it!

> Besides those in all other cases arm64 compat and corresponding
> arch/arm struct sizes match each other (modulo missing features in
> TC2 image that were not checked; like cdrom, floppy, video related,
> and few others).

Ok.

	Arnd

[PATCH] arm64: compat: fix compat types affecting struct compat_elf_prpsinfo

[Victor Kamensky]

The compat_elf_prpsinfo structure does not match the arch/arm struct
elf_pspsinfo definition. As result NT_PRPSINFO note in core file
created by arm64 kernel for aarch32 (compat) process has wrong size.
So gdb cannot display command that caused process crash.

Fix is to change size of __compat_uid_t, __compat_gid_t so it would
match size of similar fields in arch/arm case.

Signed-off-by: Victor Kamensky <victor.kamensky@linaro.org>
Acked-by: Arnd Bergmann <arnd@arndb.de>
Cc: stable at vger.kernel.org
---
 arch/arm64/include/asm/compat.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/arm64/include/asm/compat.h b/arch/arm64/include/asm/compat.h
index 253e33b..56de5aa 100644
--- a/arch/arm64/include/asm/compat.h
+++ b/arch/arm64/include/asm/compat.h
@@ -37,8 +37,8 @@ typedef s32		compat_ssize_t;
 typedef s32		compat_time_t;
 typedef s32		compat_clock_t;
 typedef s32		compat_pid_t;
-typedef u32		__compat_uid_t;
-typedef u32		__compat_gid_t;
+typedef u16		__compat_uid_t;
+typedef u16		__compat_gid_t;
 typedef u16		__compat_uid16_t;
 typedef u16		__compat_gid16_t;
 typedef u32		__compat_uid32_t;
-- 
1.8.1.4

[PATCH] arm64: compat: fix compat types affecting struct compat_elf_prpsinfo

[Catalin Marinas]

On Wed, Oct 15, 2014 at 07:11:36AM +0100, Victor Kamensky wrote:
> The compat_elf_prpsinfo structure does not match the arch/arm struct
> elf_pspsinfo definition. As result NT_PRPSINFO note in core file
> created by arm64 kernel for aarch32 (compat) process has wrong size.
> So gdb cannot display command that caused process crash.
> 
> Fix is to change size of __compat_uid_t, __compat_gid_t so it would
> match size of similar fields in arch/arm case.
> 
> Signed-off-by: Victor Kamensky <victor.kamensky@linaro.org>
> Acked-by: Arnd Bergmann <arnd@arndb.de>
> Cc: stable at vger.kernel.org

Applied, thanks (it will go in after -rc1).

-- 
Catalin