diff mbox

arm64: Move struct stat64 to uapi.

Message ID 1405409946-1790-1-git-send-email-ijc@hellion.org.uk (mailing list archive)
State New, archived
Headers show

Commit Message

Ian Campbell July 15, 2014, 7:39 a.m. UTC
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

Comments

Will Deacon July 15, 2014, 8:20 a.m. UTC | #1
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
Ian Campbell July 15, 2014, 10:33 a.m. UTC | #2
(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.
Will Deacon July 16, 2014, 1:27 p.m. UTC | #3
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
Arnd Bergmann July 16, 2014, 2 p.m. UTC | #4
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
Ian Campbell July 16, 2014, 3:50 p.m. UTC | #5
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 mbox

Patch

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