arm64: Move struct stat64 to uapi.

PATCH New Ian Campbell 3 participants 6 messages Jul 15, 2014 – Jul 16, 2014 arm64

lore

Share & tools

lore

Ian Campbell

author

2014-07-15 07:39 UTC #0 lore raw

PATCH 1/1 arm64: Move struct stat64 to uapi.

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 <[email protected]>

Cc Catalin Marinas <[email protected]>

Cc Will Deacon <[email protected]>

Cc David Howells <[email protected]>

Cc linux-arm-kernel at 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

arch/arm64/include/asm/stat.h deleted +0 ~~-61~~

deleted file mode 100644
index 15e3559..0000000
@@ -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

arch/arm64/include/uapi/asm/stat.h +42 -0

46 lines not on this page · raw message

Signature · 1 line

2.0.1

Will Deacon

2014-07-15 08:20 UTC #1 lore raw

PATCH 1/1 arm64: Move struct stat64 to uapi.

Hi Ian,

Quoted 2 lines · Ian Campbell

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

Quoted 14 lines

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 *);
                                                                ^

Quoted 5 lines

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

author

2014-07-15 10:33 UTC #2 lore raw

PATCH 1/1 arm64: Move struct stat64 to uapi.

(dropping second incorrect CC to the wrong l-a-k)

Quoted 9 lines · Will Deacon

Hi Ian,
Quoted 2 lines · Ian Campbell
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...)

Quoted 1 line

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.

Quoted 16 lines

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]

Quoted 6 lines

Quoted 3 lines
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.

Quoted 1 line

Can you help me unconfuse myself, please?

I hope I've managed to reduce rather than increase the confusion ;-)

Ian.

Will Deacon

2014-07-16 13:27 UTC #3 lore raw

PATCH 1/1 arm64: Move struct stat64 to uapi.

Quoted 12 lines · Ian Campbell

Quoted 2 lines · Will Deacon
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

Quoted 8 lines

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.

Quoted 3 lines

Quoted 1 line
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

Ian Campbell

author

2014-07-16 15:50 UTC #4 lore raw

PATCH 1/1 arm64: Move struct stat64 to uapi.

Quoted 22 lines · Will Deacon

Quoted 12 lines · Ian Campbell
Quoted 2 lines · Will Deacon
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
Quoted 8 lines
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.

Quoted 6 lines

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.

Quoted 5 lines

Quoted 3 lines
Quoted 1 line
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.

Arnd Bergmann

2014-07-16 14:00 UTC #5 lore raw

PATCH 1/1 arm64: Move struct stat64 to uapi.

In reply to Ian Campbell

Quoted 9 lines · On Tuesday 15 July 2014 11:33:33 Ian Campbell wrote:

(dropping second incorrect CC to the wrong l-a-k)
Quoted 6 lines · Will Deacon
Hi Ian,
Quoted 2 lines · Ian Campbell
This struct is part of the user API for compat tasks so I think it belongs in uapi and should use the __uFOO types.

Quoted 7 lines

Quoted 3 lines
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.

Quoted 28 lines

Quoted 16 lines
Quoted 16 lines
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]
Quoted 6 lines
Quoted 3 lines
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:

Quoted 6 lines

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.

Quoted 4 lines

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