| Message ID | 1413266105-32491-2-git-send-email-victor.kamensky@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
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.
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".
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
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.
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
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).
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
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
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;
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(-)