| Message ID | 1405409946-1790-1-git-send-email-ijc@hellion.org.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Ian, On Tue, Jul 15, 2014 at 08:39:06AM +0100, Ian Campbell wrote: > This struct is part of the user API for compat tasks so I think it belongs in > uapi and should use the __uFOO types. I don't think we should be exposing compat structures through the arm64 UAPI. Compat tasks should be built against the native arm headers and nothing else. What happens if you turn compat off (CONFIG_COMPAT=n)? > This was exposed by the Debian kernel's application of the aufs patches which > led to: > > In file included from /«PKGBUILDDIR»/include/linux/mm.h:23:0, > from /«PKGBUILDDIR»/include/linux/pid_namespace.h:6, > from /«PKGBUILDDIR»/include/linux/ptrace.h:9, > from /«PKGBUILDDIR»/arch/arm64/include/asm/compat.h:26, > from /«PKGBUILDDIR»/arch/arm64/include/asm/stat.h:23, > from /«PKGBUILDDIR»/include/linux/stat.h:5, > from /«PKGBUILDDIR»/include/linux/module.h:10, > from /«PKGBUILDDIR»/init/main.c:15: > /«PKGBUILDDIR»/include/linux/fs.h:1575:64: warning: 'struct kstat' declared inside parameter list [enabled by default] > int (*getattr) (struct vfsmount *mnt, struct dentry *, struct kstat *); > ^ > /«PKGBUILDDIR»/include/linux/fs.h:1575:64: warning: its scope is only this definition or declaration, which is probably not what you want [enabled by default] > > This is due to an extra fs.h include added to mm.h. arm64 was the only arch built by > Debian which had an issue with this, so I think it is an issue with the arm64 > headers rather than the aufs patches. Hmm, but struct kstat is defined in linux/stat.h so I'm not sure why this is arm64-specific. linux/fs.h includes that on line 10. Can you help me unconfuse myself, please? Will
(dropping second incorrect CC to the wrong l-a-k) On Tue, 2014-07-15 at 09:20 +0100, Will Deacon wrote: > Hi Ian, > > On Tue, Jul 15, 2014 at 08:39:06AM +0100, Ian Campbell wrote: > > This struct is part of the user API for compat tasks so I think it belongs in > > uapi and should use the __uFOO types. > > I don't think we should be exposing compat structures through the arm64 > UAPI. Compat tasks should be built against the native arm headers and > nothing else. Other 64bit arches do appear to define a struct stat64 in uapi/asm/stat.h though, arm64 is the only arch which even has asm/stat.h at all. AIUI the idea is that the compat version provided by the 64bit headers is supposed to provide the appropriate padding/packing such that the in memory layout matches the 32-bit version even when built with a 64-bit compiler, which might have different sized longs, different alignment requirements etc. I suppose this is for the benefit of 64-bit tools which want to introspect 32-bit core dumps/processes or something. (I'm hoping David can clarify what the intention was here...) > What happens if you turn compat off (CONFIG_COMPAT=n)? Due to the #ifdef CONFIG_COMPAT in the current asm/stat.h I'm pretty sure it'll be fine. > > This was exposed by the Debian kernel's application of the aufs patches which > > led to: > > > > In file included from /«PKGBUILDDIR»/include/linux/mm.h:23:0, > > from /«PKGBUILDDIR»/include/linux/pid_namespace.h:6, > > from /«PKGBUILDDIR»/include/linux/ptrace.h:9, > > from /«PKGBUILDDIR»/arch/arm64/include/asm/compat.h:26, > > from /«PKGBUILDDIR»/arch/arm64/include/asm/stat.h:23, > > from /«PKGBUILDDIR»/include/linux/stat.h:5, > > from /«PKGBUILDDIR»/include/linux/module.h:10, > > from /«PKGBUILDDIR»/init/main.c:15: > > /«PKGBUILDDIR»/include/linux/fs.h:1575:64: warning: 'struct kstat' declared inside parameter list [enabled by default] > > int (*getattr) (struct vfsmount *mnt, struct dentry *, struct kstat *); > > ^ > > /«PKGBUILDDIR»/include/linux/fs.h:1575:64: warning: its scope is only this definition or declaration, which is probably not what you want [enabled by default] > > > > This is due to an extra fs.h include added to mm.h. arm64 was the only arch built by > > Debian which had an issue with this, so I think it is an issue with the arm64 > > headers rather than the aufs patches. > > Hmm, but struct kstat is defined in linux/stat.h so I'm not sure why this is > arm64-specific. linux/fs.h includes that on line 10. That include won't do anything because we already got here via linux/stat.h (6th line of "In file included from" above), so the guard has already been defined. IOW there is a loop in the includes, which starts before kstat is struct defined: linux/stat.h -> asm/stat.h -> arm/compat.h -> linux/ptrace.h -> linux_pidnamespace.h -> linux/mm.h -> linux/fs.h The last link here is added by aufs patches, but all the other arches are ok with this because none of them have asm/stat.h, they only have uapi/asm/stat.h. The issue ultimately is down to the asm/stat.h -> asm/compat.h link though I think so an alternative fix could be to switch to a different set of types for the existing definition and ditching asm/compat.h from asm/stat.h. I don't think that's the correct fix though. > Can you help me unconfuse myself, please? I hope I've managed to reduce rather than increase the confusion ;-) Ian.
On Tue, Jul 15, 2014 at 11:33:33AM +0100, Ian Campbell wrote: > On Tue, 2014-07-15 at 09:20 +0100, Will Deacon wrote: > > Hmm, but struct kstat is defined in linux/stat.h so I'm not sure why this is > > arm64-specific. linux/fs.h includes that on line 10. > > That include won't do anything because we already got here via > linux/stat.h (6th line of "In file included from" above), so the guard > has already been defined. IOW there is a loop in the includes, which > starts before kstat is struct defined: > > linux/stat.h -> asm/stat.h -> arm/compat.h -> linux/ptrace.h -> > linux_pidnamespace.h -> linux/mm.h -> linux/fs.h > > The last link here is added by aufs patches, but all the other arches > are ok with this because none of them have asm/stat.h, they only have > uapi/asm/stat.h. > > The issue ultimately is down to the asm/stat.h -> asm/compat.h link > though I think so an alternative fix could be to switch to a different > set of types for the existing definition and ditching asm/compat.h from > asm/stat.h. I don't think that's the correct fix though. Perhaps we could split asm/compat.h into asm/compat-types.h containing just the type definitions, then asm/compat.h can include that as well as defining any functions and macros. Then asm/stat.h just needs to include asm/compat-types.h to avoid the issue. > > Can you help me unconfuse myself, please? > > I hope I've managed to reduce rather than increase the confusion ;-) I think so, but it hurt :) Will
On Tuesday 15 July 2014 11:33:33 Ian Campbell wrote: > (dropping second incorrect CC to the wrong l-a-k) > > On Tue, 2014-07-15 at 09:20 +0100, Will Deacon wrote: > > Hi Ian, > > > > On Tue, Jul 15, 2014 at 08:39:06AM +0100, Ian Campbell wrote: > > > This struct is part of the user API for compat tasks so I think it belongs in > > > uapi and should use the __uFOO types. > > > > I don't think we should be exposing compat structures through the arm64 > > UAPI. Compat tasks should be built against the native arm headers and > > nothing else. > > Other 64bit arches do appear to define a struct stat64 in > uapi/asm/stat.h though, arm64 is the only arch which even has asm/stat.h > at all. Other 64-bit architectures share this header file with their 32-bit counterparts, arm64 does not, and should definitely not export a structure to user space that is useless to user space. If you look at the x86 variant for instance, you will notice that it only contains a stat64 definition for 32-bit native mode, which doesn't exist for the arm64 headers (we use the arm32 headers instead). What is special about arm64 is the use of 'struct stat64' to implement the compat version of stat64 syscalls for arm32, everything else defines their own syscalls, e.g. sys32_stat64 in arch/x86/ia32/sys_ia32.c. I think the arm64 method is much better than the traditional one though, so we shouldn't give that up easily. > > > This was exposed by the Debian kernel's application of the aufs patches which > > > led to: > > > > > > In file included from /«PKGBUILDDIR»/include/linux/mm.h:23:0, > > > from /«PKGBUILDDIR»/include/linux/pid_namespace.h:6, > > > from /«PKGBUILDDIR»/include/linux/ptrace.h:9, > > > from /«PKGBUILDDIR»/arch/arm64/include/asm/compat.h:26, > > > from /«PKGBUILDDIR»/arch/arm64/include/asm/stat.h:23, > > > from /«PKGBUILDDIR»/include/linux/stat.h:5, > > > from /«PKGBUILDDIR»/include/linux/module.h:10, > > > from /«PKGBUILDDIR»/init/main.c:15: > > > /«PKGBUILDDIR»/include/linux/fs.h:1575:64: warning: 'struct kstat' declared inside parameter list [enabled by default] > > > int (*getattr) (struct vfsmount *mnt, struct dentry *, struct kstat *); > > > ^ > > > /«PKGBUILDDIR»/include/linux/fs.h:1575:64: warning: its scope is only this definition or declaration, which is probably not what you want [enabled by default] > > > > > > This is due to an extra fs.h include added to mm.h. arm64 was the only arch built by > > > Debian which had an issue with this, so I think it is an issue with the arm64 > > > headers rather than the aufs patches. > > > > Hmm, but struct kstat is defined in linux/stat.h so I'm not sure why this is > > arm64-specific. linux/fs.h includes that on line 10. > > That include won't do anything because we already got here via > linux/stat.h (6th line of "In file included from" above), so the guard > has already been defined. IOW there is a loop in the includes, which > starts before kstat is struct defined: > > linux/stat.h -> asm/stat.h -> arm/compat.h -> linux/ptrace.h -> > linux_pidnamespace.h -> linux/mm.h -> linux/fs.h > > The last link here is added by aufs patches, but all the other arches > are ok with this because none of them have asm/stat.h, they only have > uapi/asm/stat.h. The added mm.h->fs.h looks like a mistake, it should not be there, and we have in the past worked hard to separate mm.h, sched.h and fs.h from one another. > The issue ultimately is down to the asm/stat.h -> asm/compat.h link > though I think so an alternative fix could be to switch to a different > set of types for the existing definition and ditching asm/compat.h from > asm/stat.h. I don't think that's the correct fix though. Much better than the move into the user-exported file though. Arnd
On Wed, 2014-07-16 at 14:27 +0100, Will Deacon wrote: > On Tue, Jul 15, 2014 at 11:33:33AM +0100, Ian Campbell wrote: > > On Tue, 2014-07-15 at 09:20 +0100, Will Deacon wrote: > > > Hmm, but struct kstat is defined in linux/stat.h so I'm not sure why this is > > > arm64-specific. linux/fs.h includes that on line 10. > > > > That include won't do anything because we already got here via > > linux/stat.h (6th line of "In file included from" above), so the guard > > has already been defined. IOW there is a loop in the includes, which > > starts before kstat is struct defined: > > > > linux/stat.h -> asm/stat.h -> arm/compat.h -> linux/ptrace.h -> > > linux_pidnamespace.h -> linux/mm.h -> linux/fs.h > > > > The last link here is added by aufs patches, but all the other arches > > are ok with this because none of them have asm/stat.h, they only have > > uapi/asm/stat.h. > > > > The issue ultimately is down to the asm/stat.h -> asm/compat.h link > > though I think so an alternative fix could be to switch to a different > > set of types for the existing definition and ditching asm/compat.h from > > asm/stat.h. I don't think that's the correct fix though. > > Perhaps we could split asm/compat.h into asm/compat-types.h containing just > the type definitions, then asm/compat.h can include that as well as defining > any functions and macros. > > Then asm/stat.h just needs to include asm/compat-types.h to avoid the > issue. I think that looks like it will work. I'll give it a go. > > > Can you help me unconfuse myself, please? > > > > I hope I've managed to reduce rather than increase the confusion ;-) > > I think so, but it hurt :) :) Ian.
diff --git a/arch/arm64/include/asm/stat.h b/arch/arm64/include/asm/stat.h deleted file mode 100644 index 15e3559..0000000 --- a/arch/arm64/include/asm/stat.h +++ /dev/null @@ -1,61 +0,0 @@ -/* - * Copyright (C) 2012 ARM Ltd. - * - * This program is free software; you can redistribute it and/or modify - * it under the terms of the GNU General Public License version 2 as - * published by the Free Software Foundation. - * - * This program is distributed in the hope that it will be useful, - * but WITHOUT ANY WARRANTY; without even the implied warranty of - * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the - * GNU General Public License for more details. - * - * You should have received a copy of the GNU General Public License - * along with this program. If not, see <http://www.gnu.org/licenses/>. - */ -#ifndef __ASM_STAT_H -#define __ASM_STAT_H - -#include <uapi/asm/stat.h> - -#ifdef CONFIG_COMPAT - -#include <asm/compat.h> - -/* - * struct stat64 is needed for compat tasks only. Its definition is different - * from the generic struct stat64. - */ -struct stat64 { - compat_u64 st_dev; - unsigned char __pad0[4]; - -#define STAT64_HAS_BROKEN_ST_INO 1 - compat_ulong_t __st_ino; - compat_uint_t st_mode; - compat_uint_t st_nlink; - - compat_ulong_t st_uid; - compat_ulong_t st_gid; - - compat_u64 st_rdev; - unsigned char __pad3[4]; - - compat_s64 st_size; - compat_ulong_t st_blksize; - compat_u64 st_blocks; /* Number of 512-byte blocks allocated. */ - - compat_ulong_t st_atime; - compat_ulong_t st_atime_nsec; - - compat_ulong_t st_mtime; - compat_ulong_t st_mtime_nsec; - - compat_ulong_t st_ctime; - compat_ulong_t st_ctime_nsec; - - compat_u64 st_ino; -}; - -#endif -#endif diff --git a/arch/arm64/include/uapi/asm/stat.h b/arch/arm64/include/uapi/asm/stat.h index eeb702e..4bc1f59 100644 --- a/arch/arm64/include/uapi/asm/stat.h +++ b/arch/arm64/include/uapi/asm/stat.h @@ -13,4 +13,46 @@ * You should have received a copy of the GNU General Public License * along with this program. If not, see <http://www.gnu.org/licenses/>. */ +#ifndef __ASM_STAT_H +#define __ASM_STAT_H + +#include <asm/types.h> + #include <asm-generic/stat.h> + +/* + * struct stat64 is needed for compat tasks only. Its definition is different + * from the generic struct stat64. + */ +struct stat64 { + __u64 st_dev; + __u8 __pad0[4]; + +#define STAT64_HAS_BROKEN_ST_INO 1 + __u32 __st_ino; + __u32 st_mode; + __u32 st_nlink; + + __u32 st_uid; + __u32 st_gid; + + __u64 st_rdev; + __u8 __pad3[4]; + + __s64 st_size; + __u32 st_blksize; + __u64 st_blocks; /* Number of 512-byte blocks allocated. */ + + __u32 st_atime; + __u32 st_atime_nsec; + + __u32 st_mtime; + __u32 st_mtime_nsec; + + __u32 st_ctime; + __u32 st_ctime_nsec; + + __u64 st_ino; +}; + +#endif
This struct is part of the user API for compat tasks so I think it belongs in uapi and should use the __uFOO types. This was exposed by the Debian kernel's application of the aufs patches which led to: In file included from /«PKGBUILDDIR»/include/linux/mm.h:23:0, from /«PKGBUILDDIR»/include/linux/pid_namespace.h:6, from /«PKGBUILDDIR»/include/linux/ptrace.h:9, from /«PKGBUILDDIR»/arch/arm64/include/asm/compat.h:26, from /«PKGBUILDDIR»/arch/arm64/include/asm/stat.h:23, from /«PKGBUILDDIR»/include/linux/stat.h:5, from /«PKGBUILDDIR»/include/linux/module.h:10, from /«PKGBUILDDIR»/init/main.c:15: /«PKGBUILDDIR»/include/linux/fs.h:1575:64: warning: 'struct kstat' declared inside parameter list [enabled by default] int (*getattr) (struct vfsmount *mnt, struct dentry *, struct kstat *); ^ /«PKGBUILDDIR»/include/linux/fs.h:1575:64: warning: its scope is only this definition or declaration, which is probably not what you want [enabled by default] This is due to an extra fs.h include added to mm.h. arm64 was the only arch built by Debian which had an issue with this, so I think it is an issue with the arm64 headers rather than the aufs patches. Full logs in http://buildd.debian-ports.org/status/fetch.php?pkg=linux&arch=arm64&ver=3.14.12-1&stamp=1405234443 Signed-off-by: Ian Campbell <ijc@hellion.org.uk> Cc: Catalin Marinas <catalin.marinas@arm.com> Cc: Will Deacon <will.deacon@arm.com> Cc: David Howells <dhowells@redhat.com> Cc: linux-arm-kernel@lists.infradead.org --- arch/arm64/include/asm/stat.h | 61 -------------------------------------- arch/arm64/include/uapi/asm/stat.h | 42 ++++++++++++++++++++++++++ 2 files changed, 42 insertions(+), 61 deletions(-) delete mode 100644 arch/arm64/include/asm/stat.h