Hi all,
currenty the compat fcnt definitions are duplicate for all compat
architectures, and the native fcntl64 definitions aren't even usable
from userspace due to a bogus CONFIG_64BIT ifdef. This series tries
to sort out all that.
Diffstat:
arch/arm64/include/asm/compat.h | 20 --------------------
arch/mips/include/asm/compat.h | 23 ++---------------------
arch/mips/include/uapi/asm/fcntl.h | 28 +++-------------------------
arch/parisc/include/asm/compat.h | 16 ----------------
arch/powerpc/include/asm/compat.h | 20 --------------------
arch/s390/include/asm/compat.h | 20 --------------------
arch/sparc/include/asm/compat.h | 22 +---------------------
arch/x86/include/asm/compat.h | 24 +++---------------------
include/linux/compat.h | 31 +++++++++++++++++++++++++++++++
include/uapi/asm-generic/fcntl.h | 21 +++++++--------------
tools/include/uapi/asm-generic/fcntl.h | 21 +++++++--------------
11 files changed, 54 insertions(+), 192 deletions(-)
Signed-off-by: Christoph Hellwig <[email protected]>
---
include/uapi/asm-generic/fcntl.h | 2 --
tools/include/uapi/asm-generic/fcntl.h | 2 --
2 files changed, 4 deletions(-)
diff --git a/include/uapi/asm-generic/fcntl.h b/include/uapi/asm-generic/fcntl.h
index ecd0f5bdfc1d6..caa482e3b01af 100644
--- a/include/uapi/asm-generic/fcntl.h
+++ b/include/uapi/asm-generic/fcntl.h
@@ -207,7 +207,6 @@ struct flock {
};
#endif
-#ifndef HAVE_ARCH_STRUCT_FLOCK64
#ifndef __ARCH_FLOCK64_PAD
#define __ARCH_FLOCK64_PAD
#endif
@@ -220,6 +219,5 @@ struct flock64 {
__kernel_pid_t l_pid;
__ARCH_FLOCK64_PAD
};
-#endif
#endif /* _ASM_GENERIC_FCNTL_H */
diff --git a/tools/include/uapi/asm-generic/fcntl.h b/tools/include/uapi/asm-generic/fcntl.h
index ac190958c9814..4a49d33ca4d55 100644
--- a/tools/include/uapi/asm-generic/fcntl.h
+++ b/tools/include/uapi/asm-generic/fcntl.h
@@ -202,7 +202,6 @@ struct flock {
};
#endif
-#ifndef HAVE_ARCH_STRUCT_FLOCK64
#ifndef __ARCH_FLOCK64_PAD
#define __ARCH_FLOCK64_PAD
#endif
@@ -215,6 +214,5 @@ struct flock64 {
__kernel_pid_t l_pid;
__ARCH_FLOCK64_PAD
};
-#endif
#endif /* _ASM_GENERIC_FCNTL_H */
--
2.30.2
Don't bother to define the symbols empty, just don't use them. That
makes the intent a little more clear.
Signed-off-by: Christoph Hellwig <[email protected]>
---
include/uapi/asm-generic/fcntl.h | 12 ++++--------
tools/include/uapi/asm-generic/fcntl.h | 12 ++++--------
2 files changed, 8 insertions(+), 16 deletions(-)
diff --git a/include/uapi/asm-generic/fcntl.h b/include/uapi/asm-generic/fcntl.h
index caa482e3b01af..c53897ca5d402 100644
--- a/include/uapi/asm-generic/fcntl.h
+++ b/include/uapi/asm-generic/fcntl.h
@@ -193,22 +193,16 @@ struct f_owner_ex {
#define F_LINUX_SPECIFIC_BASE 1024
#ifndef HAVE_ARCH_STRUCT_FLOCK
-#ifndef __ARCH_FLOCK_PAD
-#define __ARCH_FLOCK_PAD
-#endif
-
struct flock {
short l_type;
short l_whence;
__kernel_off_t l_start;
__kernel_off_t l_len;
__kernel_pid_t l_pid;
+#ifdef __ARCH_FLOCK_PAD
__ARCH_FLOCK_PAD
-};
#endif
-
-#ifndef __ARCH_FLOCK64_PAD
-#define __ARCH_FLOCK64_PAD
+};
#endif
struct flock64 {
@@ -217,7 +211,9 @@ struct flock64 {
__kernel_loff_t l_start;
__kernel_loff_t l_len;
__kernel_pid_t l_pid;
+#ifdef __ARCH_FLOCK64_PAD
__ARCH_FLOCK64_PAD
+#endif
};
#endif /* _ASM_GENERIC_FCNTL_H */
diff --git a/tools/include/uapi/asm-generic/fcntl.h b/tools/include/uapi/asm-generic/fcntl.h
index 4a49d33ca4d55..82054502b9748 100644
--- a/tools/include/uapi/asm-generic/fcntl.h
+++ b/tools/include/uapi/asm-generic/fcntl.h
@@ -188,22 +188,16 @@ struct f_owner_ex {
#define F_LINUX_SPECIFIC_BASE 1024
#ifndef HAVE_ARCH_STRUCT_FLOCK
-#ifndef __ARCH_FLOCK_PAD
-#define __ARCH_FLOCK_PAD
-#endif
-
struct flock {
short l_type;
short l_whence;
__kernel_off_t l_start;
__kernel_off_t l_len;
__kernel_pid_t l_pid;
+#ifdef __ARCH_FLOCK_PAD
__ARCH_FLOCK_PAD
-};
#endif
-
-#ifndef __ARCH_FLOCK64_PAD
-#define __ARCH_FLOCK64_PAD
+};
#endif
struct flock64 {
@@ -212,7 +206,9 @@ struct flock64 {
__kernel_loff_t l_start;
__kernel_loff_t l_len;
__kernel_pid_t l_pid;
+#ifdef __ARCH_FLOCK64_PAD
__ARCH_FLOCK64_PAD
+#endif
};
#endif /* _ASM_GENERIC_FCNTL_H */
--
2.30.2
Add a new __ARCH_FLOCK_EXTRA_SYSID macro following the style of
__ARCH_FLOCK_PAD to avoid having a separate definition just for one
architecture.
Signed-off-by: Christoph Hellwig <[email protected]>
---
arch/mips/include/uapi/asm/fcntl.h | 26 +++-----------------------
include/uapi/asm-generic/fcntl.h | 5 +++--
tools/include/uapi/asm-generic/fcntl.h | 5 +++--
3 files changed, 9 insertions(+), 27 deletions(-)
diff --git a/arch/mips/include/uapi/asm/fcntl.h b/arch/mips/include/uapi/asm/fcntl.h
index 42e13dead5431..9e44ac810db94 100644
--- a/arch/mips/include/uapi/asm/fcntl.h
+++ b/arch/mips/include/uapi/asm/fcntl.h
@@ -50,30 +50,10 @@
#define F_SETLKW64 35
#endif
-/*
- * The flavours of struct flock. "struct flock" is the ABI compliant
- * variant. Finally struct flock64 is the LFS variant of struct flock. As
- * a historic accident and inconsistence with the ABI definition it doesn't
- * contain all the same fields as struct flock.
- */
-
#if _MIPS_SIM != _MIPS_SIM_ABI64
-
-#include <linux/types.h>
-
-struct flock {
- short l_type;
- short l_whence;
- __kernel_off_t l_start;
- __kernel_off_t l_len;
- long l_sysid;
- __kernel_pid_t l_pid;
- long pad[4];
-};
-
-#define HAVE_ARCH_STRUCT_FLOCK
-
-#endif /* _MIPS_SIM == _MIPS_SIM_ABI32 */
+#define __ARCH_FLOCK_EXTRA_SYSID long l_sysid;
+#define __ARCH_FLOCK_PAD long pad[4];
+#endif
#include <asm-generic/fcntl.h>
diff --git a/include/uapi/asm-generic/fcntl.h b/include/uapi/asm-generic/fcntl.h
index c53897ca5d402..98f4ff165b776 100644
--- a/include/uapi/asm-generic/fcntl.h
+++ b/include/uapi/asm-generic/fcntl.h
@@ -192,18 +192,19 @@ struct f_owner_ex {
#define F_LINUX_SPECIFIC_BASE 1024
-#ifndef HAVE_ARCH_STRUCT_FLOCK
struct flock {
short l_type;
short l_whence;
__kernel_off_t l_start;
__kernel_off_t l_len;
+#ifdef __ARCH_FLOCK_EXTRA_SYSID
+ __ARCH_FLOCK_EXTRA_SYSID
+#endif
__kernel_pid_t l_pid;
#ifdef __ARCH_FLOCK_PAD
__ARCH_FLOCK_PAD
#endif
};
-#endif
struct flock64 {
short l_type;
diff --git a/tools/include/uapi/asm-generic/fcntl.h b/tools/include/uapi/asm-generic/fcntl.h
index 82054502b9748..bf961a71802e0 100644
--- a/tools/include/uapi/asm-generic/fcntl.h
+++ b/tools/include/uapi/asm-generic/fcntl.h
@@ -187,18 +187,19 @@ struct f_owner_ex {
#define F_LINUX_SPECIFIC_BASE 1024
-#ifndef HAVE_ARCH_STRUCT_FLOCK
struct flock {
short l_type;
short l_whence;
__kernel_off_t l_start;
__kernel_off_t l_len;
+#ifdef __ARCH_FLOCK_EXTRA_SYSID
+ __ARCH_FLOCK_EXTRA_SYSID
+#endif
__kernel_pid_t l_pid;
#ifdef __ARCH_FLOCK_PAD
__ARCH_FLOCK_PAD
#endif
};
-#endif
struct flock64 {
short l_type;
--
2.30.2
The fcntl F_GETLK64/F_SETLK64/F_SETLKW64 are only implemented for the
32-bit syscall APIs, but we also need them for compat handling on 64-bit
builds. Redefining them is error prone (as shown by the example that
parisc gets it wrong currently), so we should use the same defines for
both case. In theory we could try to hide them from userspace, but
given that only MIPS actually gets that right, while the asm-generic
version used by most architectures relies on a Kconfig symbol that can't
be relied on to be set properly by userspace is a clear indicator to not
bother.
Signed-off-by: Christoph Hellwig <[email protected]>
---
arch/arm64/include/asm/compat.h | 4 ----
arch/mips/include/asm/compat.h | 4 ----
arch/mips/include/uapi/asm/fcntl.h | 2 --
arch/powerpc/include/asm/compat.h | 4 ----
arch/s390/include/asm/compat.h | 4 ----
arch/sparc/include/asm/compat.h | 4 ----
arch/x86/include/asm/compat.h | 4 ----
include/uapi/asm-generic/fcntl.h | 2 --
tools/include/uapi/asm-generic/fcntl.h | 2 --
9 files changed, 30 deletions(-)
diff --git a/arch/arm64/include/asm/compat.h b/arch/arm64/include/asm/compat.h
index eaa6ca062d89b..2763287654081 100644
--- a/arch/arm64/include/asm/compat.h
+++ b/arch/arm64/include/asm/compat.h
@@ -73,10 +73,6 @@ struct compat_flock {
compat_pid_t l_pid;
};
-#define F_GETLK64 12 /* using 'struct flock64' */
-#define F_SETLK64 13
-#define F_SETLKW64 14
-
struct compat_flock64 {
short l_type;
short l_whence;
diff --git a/arch/mips/include/asm/compat.h b/arch/mips/include/asm/compat.h
index bbb3bc5a42fd8..6a350c1f70d7e 100644
--- a/arch/mips/include/asm/compat.h
+++ b/arch/mips/include/asm/compat.h
@@ -65,10 +65,6 @@ struct compat_flock {
s32 pad[4];
};
-#define F_GETLK64 33
-#define F_SETLK64 34
-#define F_SETLKW64 35
-
struct compat_flock64 {
short l_type;
short l_whence;
diff --git a/arch/mips/include/uapi/asm/fcntl.h b/arch/mips/include/uapi/asm/fcntl.h
index 9e44ac810db94..1769fc50d35f0 100644
--- a/arch/mips/include/uapi/asm/fcntl.h
+++ b/arch/mips/include/uapi/asm/fcntl.h
@@ -44,11 +44,9 @@
#define F_SETOWN 24 /* for sockets. */
#define F_GETOWN 23 /* for sockets. */
-#ifndef __mips64
#define F_GETLK64 33 /* using 'struct flock64' */
#define F_SETLK64 34
#define F_SETLKW64 35
-#endif
#if _MIPS_SIM != _MIPS_SIM_ABI64
#define __ARCH_FLOCK_EXTRA_SYSID long l_sysid;
diff --git a/arch/powerpc/include/asm/compat.h b/arch/powerpc/include/asm/compat.h
index 7afc96fb6524b..83d8f70779cbc 100644
--- a/arch/powerpc/include/asm/compat.h
+++ b/arch/powerpc/include/asm/compat.h
@@ -52,10 +52,6 @@ struct compat_flock {
compat_pid_t l_pid;
};
-#define F_GETLK64 12 /* using 'struct flock64' */
-#define F_SETLK64 13
-#define F_SETLKW64 14
-
struct compat_flock64 {
short l_type;
short l_whence;
diff --git a/arch/s390/include/asm/compat.h b/arch/s390/include/asm/compat.h
index cdc7ae72529d8..0f14b3188b1bb 100644
--- a/arch/s390/include/asm/compat.h
+++ b/arch/s390/include/asm/compat.h
@@ -110,10 +110,6 @@ struct compat_flock {
compat_pid_t l_pid;
};
-#define F_GETLK64 12
-#define F_SETLK64 13
-#define F_SETLKW64 14
-
struct compat_flock64 {
short l_type;
short l_whence;
diff --git a/arch/sparc/include/asm/compat.h b/arch/sparc/include/asm/compat.h
index bd949fcf9d63b..108078751bb5a 100644
--- a/arch/sparc/include/asm/compat.h
+++ b/arch/sparc/include/asm/compat.h
@@ -84,10 +84,6 @@ struct compat_flock {
short __unused;
};
-#define F_GETLK64 12
-#define F_SETLK64 13
-#define F_SETLKW64 14
-
struct compat_flock64 {
short l_type;
short l_whence;
diff --git a/arch/x86/include/asm/compat.h b/arch/x86/include/asm/compat.h
index 7516e4199b3c6..8d19a212f4f26 100644
--- a/arch/x86/include/asm/compat.h
+++ b/arch/x86/include/asm/compat.h
@@ -58,10 +58,6 @@ struct compat_flock {
compat_pid_t l_pid;
};
-#define F_GETLK64 12 /* using 'struct flock64' */
-#define F_SETLK64 13
-#define F_SETLKW64 14
-
/*
* IA32 uses 4 byte alignment for 64 bit quantities,
* so we need to pack this structure.
diff --git a/include/uapi/asm-generic/fcntl.h b/include/uapi/asm-generic/fcntl.h
index 98f4ff165b776..43d7c44031be0 100644
--- a/include/uapi/asm-generic/fcntl.h
+++ b/include/uapi/asm-generic/fcntl.h
@@ -116,13 +116,11 @@
#define F_GETSIG 11 /* for sockets. */
#endif
-#ifndef CONFIG_64BIT
#ifndef F_GETLK64
#define F_GETLK64 12 /* using 'struct flock64' */
#define F_SETLK64 13
#define F_SETLKW64 14
#endif
-#endif
#ifndef F_SETOWN_EX
#define F_SETOWN_EX 15
diff --git a/tools/include/uapi/asm-generic/fcntl.h b/tools/include/uapi/asm-generic/fcntl.h
index bf961a71802e0..6e16722026f39 100644
--- a/tools/include/uapi/asm-generic/fcntl.h
+++ b/tools/include/uapi/asm-generic/fcntl.h
@@ -115,13 +115,11 @@
#define F_GETSIG 11 /* for sockets. */
#endif
-#ifndef CONFIG_64BIT
#ifndef F_GETLK64
#define F_GETLK64 12 /* using 'struct flock64' */
#define F_SETLK64 13
#define F_SETLKW64 14
#endif
-#endif
#ifndef F_SETOWN_EX
#define F_SETOWN_EX 15
--
2.30.2
Provide a single common definition for the compat_flock and
compat_flock64 structures using the same tricks as for the native
variants. Another extra define is added for the packing required on
x86.
Signed-off-by: Christoph Hellwig <[email protected]>
---
arch/arm64/include/asm/compat.h | 16 ----------------
arch/mips/include/asm/compat.h | 19 ++-----------------
arch/parisc/include/asm/compat.h | 16 ----------------
arch/powerpc/include/asm/compat.h | 16 ----------------
arch/s390/include/asm/compat.h | 16 ----------------
arch/sparc/include/asm/compat.h | 18 +-----------------
arch/x86/include/asm/compat.h | 20 +++-----------------
include/linux/compat.h | 31 +++++++++++++++++++++++++++++++
8 files changed, 37 insertions(+), 115 deletions(-)
diff --git a/arch/arm64/include/asm/compat.h b/arch/arm64/include/asm/compat.h
index 2763287654081..e0faec1984a1c 100644
--- a/arch/arm64/include/asm/compat.h
+++ b/arch/arm64/include/asm/compat.h
@@ -65,22 +65,6 @@ struct compat_stat {
compat_ulong_t __unused4[2];
};
-struct compat_flock {
- short l_type;
- short l_whence;
- compat_off_t l_start;
- compat_off_t l_len;
- compat_pid_t l_pid;
-};
-
-struct compat_flock64 {
- short l_type;
- short l_whence;
- compat_loff_t l_start;
- compat_loff_t l_len;
- compat_pid_t l_pid;
-};
-
struct compat_statfs {
int f_type;
int f_bsize;
diff --git a/arch/mips/include/asm/compat.h b/arch/mips/include/asm/compat.h
index 6a350c1f70d7e..6d6e5a451f4d9 100644
--- a/arch/mips/include/asm/compat.h
+++ b/arch/mips/include/asm/compat.h
@@ -55,23 +55,8 @@ struct compat_stat {
s32 st_pad4[14];
};
-struct compat_flock {
- short l_type;
- short l_whence;
- compat_off_t l_start;
- compat_off_t l_len;
- s32 l_sysid;
- compat_pid_t l_pid;
- s32 pad[4];
-};
-
-struct compat_flock64 {
- short l_type;
- short l_whence;
- compat_loff_t l_start;
- compat_loff_t l_len;
- compat_pid_t l_pid;
-};
+#define __ARCH_COMPAT_FLOCK_EXTRA_SYSID s32 l_sysid;
+#define __ARCH_COMPAT_FLOCK_PAD s32 pad[4];
struct compat_statfs {
int f_type;
diff --git a/arch/parisc/include/asm/compat.h b/arch/parisc/include/asm/compat.h
index c04f5a637c390..a1e4534d80509 100644
--- a/arch/parisc/include/asm/compat.h
+++ b/arch/parisc/include/asm/compat.h
@@ -53,22 +53,6 @@ struct compat_stat {
u32 st_spare4[3];
};
-struct compat_flock {
- short l_type;
- short l_whence;
- compat_off_t l_start;
- compat_off_t l_len;
- compat_pid_t l_pid;
-};
-
-struct compat_flock64 {
- short l_type;
- short l_whence;
- compat_loff_t l_start;
- compat_loff_t l_len;
- compat_pid_t l_pid;
-};
-
struct compat_statfs {
s32 f_type;
s32 f_bsize;
diff --git a/arch/powerpc/include/asm/compat.h b/arch/powerpc/include/asm/compat.h
index 83d8f70779cbc..5ef3c7c83c343 100644
--- a/arch/powerpc/include/asm/compat.h
+++ b/arch/powerpc/include/asm/compat.h
@@ -44,22 +44,6 @@ struct compat_stat {
u32 __unused4[2];
};
-struct compat_flock {
- short l_type;
- short l_whence;
- compat_off_t l_start;
- compat_off_t l_len;
- compat_pid_t l_pid;
-};
-
-struct compat_flock64 {
- short l_type;
- short l_whence;
- compat_loff_t l_start;
- compat_loff_t l_len;
- compat_pid_t l_pid;
-};
-
struct compat_statfs {
int f_type;
int f_bsize;
diff --git a/arch/s390/include/asm/compat.h b/arch/s390/include/asm/compat.h
index 0f14b3188b1bb..07f04d37068b6 100644
--- a/arch/s390/include/asm/compat.h
+++ b/arch/s390/include/asm/compat.h
@@ -102,22 +102,6 @@ struct compat_stat {
u32 __unused5;
};
-struct compat_flock {
- short l_type;
- short l_whence;
- compat_off_t l_start;
- compat_off_t l_len;
- compat_pid_t l_pid;
-};
-
-struct compat_flock64 {
- short l_type;
- short l_whence;
- compat_loff_t l_start;
- compat_loff_t l_len;
- compat_pid_t l_pid;
-};
-
struct compat_statfs {
u32 f_type;
u32 f_bsize;
diff --git a/arch/sparc/include/asm/compat.h b/arch/sparc/include/asm/compat.h
index 108078751bb5a..d78fb44942e0f 100644
--- a/arch/sparc/include/asm/compat.h
+++ b/arch/sparc/include/asm/compat.h
@@ -75,23 +75,7 @@ struct compat_stat64 {
unsigned int __unused5;
};
-struct compat_flock {
- short l_type;
- short l_whence;
- compat_off_t l_start;
- compat_off_t l_len;
- compat_pid_t l_pid;
- short __unused;
-};
-
-struct compat_flock64 {
- short l_type;
- short l_whence;
- compat_loff_t l_start;
- compat_loff_t l_len;
- compat_pid_t l_pid;
- short __unused;
-};
+#define __ARCH_COMPAT_FLOCK_PAD short __unused;
struct compat_statfs {
int f_type;
diff --git a/arch/x86/include/asm/compat.h b/arch/x86/include/asm/compat.h
index 8d19a212f4f26..de794d8958663 100644
--- a/arch/x86/include/asm/compat.h
+++ b/arch/x86/include/asm/compat.h
@@ -50,25 +50,11 @@ struct compat_stat {
u32 __unused5;
};
-struct compat_flock {
- short l_type;
- short l_whence;
- compat_off_t l_start;
- compat_off_t l_len;
- compat_pid_t l_pid;
-};
-
/*
- * IA32 uses 4 byte alignment for 64 bit quantities,
- * so we need to pack this structure.
+ * IA32 uses 4 byte alignment for 64 bit quantities, so we need to pack the
+ * compat flock64 structure.
*/
-struct compat_flock64 {
- short l_type;
- short l_whence;
- compat_loff_t l_start;
- compat_loff_t l_len;
- compat_pid_t l_pid;
-} __attribute__((packed));
+#define __ARCH_NEED_COMPAT_FLOCK64_PACKED
struct compat_statfs {
int f_type;
diff --git a/include/linux/compat.h b/include/linux/compat.h
index 1c758b0e03598..a0481fe6c5d51 100644
--- a/include/linux/compat.h
+++ b/include/linux/compat.h
@@ -258,6 +258,37 @@ struct compat_rlimit {
compat_ulong_t rlim_max;
};
+#ifdef __ARCH_NEED_COMPAT_FLOCK64_PACKED
+#define __ARCH_COMPAT_FLOCK64_PACK __attribute__((packed))
+#else
+#define __ARCH_COMPAT_FLOCK64_PACK
+#endif
+
+struct compat_flock {
+ short l_type;
+ short l_whence;
+ compat_off_t l_start;
+ compat_off_t l_len;
+#ifdef __ARCH_COMPAT_FLOCK_EXTRA_SYSID
+ __ARCH_COMPAT_FLOCK_EXTRA_SYSID
+#endif
+ compat_pid_t l_pid;
+#ifdef __ARCH_COMPAT_FLOCK_PAD
+ __ARCH_COMPAT_FLOCK_PAD
+#endif
+};
+
+struct compat_flock64 {
+ short l_type;
+ short l_whence;
+ compat_loff_t l_start;
+ compat_loff_t l_len;
+ compat_pid_t l_pid;
+#ifdef __ARCH_COMPAT_FLOCK64_PAD
+ __ARCH_COMPAT_FLOCK64_PAD
+#endif
+} __ARCH_COMPAT_FLOCK64_PACK;
+
struct compat_rusage {
struct old_timeval32 ru_utime;
struct old_timeval32 ru_stime;
--
2.30.2
From: Christoph Hellwig
> Sent: 11 January 2022 08:35
>
> Provide a single common definition for the compat_flock and
> compat_flock64 structures using the same tricks as for the native
> variants. Another extra define is added for the packing required on
> x86.
>
...
> /*
> - * IA32 uses 4 byte alignment for 64 bit quantities,
> - * so we need to pack this structure.
> + * IA32 uses 4 byte alignment for 64 bit quantities, so we need to pack the
> + * compat flock64 structure.
> */
...
> +#define __ARCH_NEED_COMPAT_FLOCK64_PACKED
Maybe:
#define __ARCH_COMPAT_FLOCK64_ATTR (packed),(aligned(4)
...
Delete this bit:
> +#ifdef __ARCH_NEED_COMPAT_FLOCK64_PACKED
> +#define __ARCH_COMPAT_FLOCK64_PACK __attribute__((packed))
> +#else
> +#define __ARCH_COMPAT_FLOCK64_PACK
> +#endif
...
> +struct compat_flock64 {
> + short l_type;
> + short l_whence;
> + compat_loff_t l_start;
> + compat_loff_t l_len;
> + compat_pid_t l_pid;
> +#ifdef __ARCH_COMPAT_FLOCK64_PAD
> + __ARCH_COMPAT_FLOCK64_PAD
> +#endif
> +} __ARCH_COMPAT_FLOCK64_PACK;
then:
#ifdef __ARCH_COMPAT_FLOCK64_ATTR
} __attribute__(__ARCH_COMPAT_FLOCK64_ATTR);
#else
};
#endif
Makes it a bit more like the xxx_PAD bits.
Although the trailing ; does cause a bit of grief.
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
On Tue, Jan 11, 2022 at 9:35 AM Christoph Hellwig <[email protected]> wrote:
>
> The fcntl F_GETLK64/F_SETLK64/F_SETLKW64 are only implemented for the
> 32-bit syscall APIs, but we also need them for compat handling on 64-bit
> builds. Redefining them is error prone (as shown by the example that
> parisc gets it wrong currently), so we should use the same defines for
> both case. In theory we could try to hide them from userspace, but
> given that only MIPS actually gets that right, while the asm-generic
> version used by most architectures relies on a Kconfig symbol that can't
> be relied on to be set properly by userspace is a clear indicator to not
> bother.
>
> Signed-off-by: Christoph Hellwig <[email protected]>
> ---
> diff --git a/include/uapi/asm-generic/fcntl.h b/include/uapi/asm-generic/fcntl.h
> index 98f4ff165b776..43d7c44031be0 100644
> --- a/include/uapi/asm-generic/fcntl.h
> +++ b/include/uapi/asm-generic/fcntl.h
> @@ -116,13 +116,11 @@
> #define F_GETSIG 11 /* for sockets. */
> #endif
>
> -#ifndef CONFIG_64BIT
> #ifndef F_GETLK64
> #define F_GETLK64 12 /* using 'struct flock64' */
> #define F_SETLK64 13
> #define F_SETLKW64 14
> #endif
> -#endif
>
> #ifndef F_SETOWN_EX
> #define F_SETOWN_EX 15
This is a very subtle change to the exported UAPI header contents:
On 64-bit architectures, the three unusable numbers are now always
shown, rather than depending on a user-controlled symbol.
This is probably what we want here for compatibility reasons, but I think
it should be explained in the changelog text, and I'd like Jeff or Bruce
to comment on it as well: the alternative here would be to make the
uapi definition depend on __BITS_PER_LONG==32, which is
technically the right thing to do but more a of a change.
Arnd
On Tue, Jan 11, 2022 at 9:35 AM Christoph Hellwig <[email protected]> wrote:
>
> Hi all,
>
> currenty the compat fcnt definitions are duplicate for all compat
> architectures, and the native fcntl64 definitions aren't even usable
> from userspace due to a bogus CONFIG_64BIT ifdef. This series tries
> to sort out all that.
The changes look good, but I have the same comment on your last patch that
I had for Guo Ren's version. Once we have resolved that, I can apply the
series in the asm-generic tree, or provide an Ack to have it all merged
along with the compat mode changes in the risc-v tree.
Arnd
On Tue, Jan 11, 2022 at 11:33 PM Arnd Bergmann <[email protected]> wrote:
>
> On Tue, Jan 11, 2022 at 9:35 AM Christoph Hellwig <[email protected]> wrote:
> >
> > The fcntl F_GETLK64/F_SETLK64/F_SETLKW64 are only implemented for the
> > 32-bit syscall APIs, but we also need them for compat handling on 64-bit
> > builds. Redefining them is error prone (as shown by the example that
> > parisc gets it wrong currently), so we should use the same defines for
> > both case. In theory we could try to hide them from userspace, but
> > given that only MIPS actually gets that right, while the asm-generic
> > version used by most architectures relies on a Kconfig symbol that can't
> > be relied on to be set properly by userspace is a clear indicator to not
> > bother.
> >
> > Signed-off-by: Christoph Hellwig <[email protected]>
> > ---
>
> > diff --git a/include/uapi/asm-generic/fcntl.h b/include/uapi/asm-generic/fcntl.h
> > index 98f4ff165b776..43d7c44031be0 100644
> > --- a/include/uapi/asm-generic/fcntl.h
> > +++ b/include/uapi/asm-generic/fcntl.h
> > @@ -116,13 +116,11 @@
> > #define F_GETSIG 11 /* for sockets. */
> > #endif
> >
> > -#ifndef CONFIG_64BIT
> > #ifndef F_GETLK64
> > #define F_GETLK64 12 /* using 'struct flock64' */
> > #define F_SETLK64 13
> > #define F_SETLKW64 14
> > #endif
> > -#endif
> >
> > #ifndef F_SETOWN_EX
> > #define F_SETOWN_EX 15
>
> This is a very subtle change to the exported UAPI header contents:
> On 64-bit architectures, the three unusable numbers are now always
> shown, rather than depending on a user-controlled symbol.
>
> This is probably what we want here for compatibility reasons, but I think
> it should be explained in the changelog text, and I'd like Jeff or Bruce
> to comment on it as well: the alternative here would be to make the
> uapi definition depend on __BITS_PER_LONG==32, which is
__BITS_PER_LONG==32 || __KERNEL__ just for kernel use in compat.
> technically the right thing to do but more a of a change.
>
> Arnd
--
Best Regards
Guo Ren
ML: https://lore.kernel.org/linux-csky/
On Tue, Jan 11, 2022 at 04:33:30PM +0100, Arnd Bergmann wrote:
> This is a very subtle change to the exported UAPI header contents:
> On 64-bit architectures, the three unusable numbers are now always
> shown, rather than depending on a user-controlled symbol.
Well, the change is bigger and less subtle. Before this change the
constants were never visible to userspace at all (except on mips),
because the #ifdef CONFIG_64BIT it never set for userspace builds.
> This is probably what we want here for compatibility reasons, but I think
> it should be explained in the changelog text, and I'd like Jeff or Bruce
> to comment on it as well: the alternative here would be to make the
> uapi definition depend on __BITS_PER_LONG==32, which is
> technically the right thing to do but more a of a change.
I can change this to #if __BITS_PER_LONG==32 || defined(__KERNEL__),
but it will still be change in what userspace sees.
On Wed, Jan 12, 2022 at 8:56 AM Christoph Hellwig <[email protected]> wrote:
>
> On Tue, Jan 11, 2022 at 04:33:30PM +0100, Arnd Bergmann wrote:
> > This is a very subtle change to the exported UAPI header contents:
> > On 64-bit architectures, the three unusable numbers are now always
> > shown, rather than depending on a user-controlled symbol.
>
> Well, the change is bigger and less subtle. Before this change the
> constants were never visible to userspace at all (except on mips),
> because the #ifdef CONFIG_64BIT it never set for userspace builds.
I suppose you mean /always/ visible here, with that ifndef.
> > This is probably what we want here for compatibility reasons, but I think
> > it should be explained in the changelog text, and I'd like Jeff or Bruce
> > to comment on it as well: the alternative here would be to make the
> > uapi definition depend on __BITS_PER_LONG==32, which is
> > technically the right thing to do but more a of a change.
>
> I can change this to #if __BITS_PER_LONG==32 || defined(__KERNEL__),
> but it will still be change in what userspace sees.
Exactly, that is the tradeoff, which is why I'd like the flock maintainers
to say which way they prefer. We can either do it more correctly (hiding
the constants from user space when they are not usable), or with less
change (removing the incorrect #ifdef). Either way sounds reasonable
to me, I mainly care that this is explained in the changelog and that the
maintainers are aware of the two options.
Arnd
Hello!
On 1/11/22 11:35 AM, Christoph Hellwig wrote:
> currenty the compat fcnt definitions are duplicate for all compat
fcntl?
> architectures, and the native fcntl64 definitions aren't even usable
> from userspace due to a bogus CONFIG_64BIT ifdef. This series tries
> to sort out all that.
[...]
MBR, Sergey
On Wed, 2022-01-12 at 09:28 +0100, Arnd Bergmann wrote:
> On Wed, Jan 12, 2022 at 8:56 AM Christoph Hellwig <[email protected]> wrote:
> >
> > On Tue, Jan 11, 2022 at 04:33:30PM +0100, Arnd Bergmann wrote:
> > > This is a very subtle change to the exported UAPI header contents:
> > > On 64-bit architectures, the three unusable numbers are now always
> > > shown, rather than depending on a user-controlled symbol.
> >
> > Well, the change is bigger and less subtle. Before this change the
> > constants were never visible to userspace at all (except on mips),
> > because the #ifdef CONFIG_64BIT it never set for userspace builds.
>
> I suppose you mean /always/ visible here, with that ifndef.
>
> > > This is probably what we want here for compatibility reasons, but I think
> > > it should be explained in the changelog text, and I'd like Jeff or Bruce
> > > to comment on it as well: the alternative here would be to make the
> > > uapi definition depend on __BITS_PER_LONG==32, which is
> > > technically the right thing to do but more a of a change.
> >
> > I can change this to #if __BITS_PER_LONG==32 || defined(__KERNEL__),
> > but it will still be change in what userspace sees.
>
> Exactly, that is the tradeoff, which is why I'd like the flock maintainers
> to say which way they prefer. We can either do it more correctly (hiding
> the constants from user space when they are not usable), or with less
> change (removing the incorrect #ifdef). Either way sounds reasonable
> to me, I mainly care that this is explained in the changelog and that the
> maintainers are aware of the two options.
>
I don't have a strong opinion here. If we were taking symbols away that
were previously visible to userland it would be one thing, but since
we're just adding symbols that may not have been there before, this
seems less likely to break anything.
I probably lean toward Christoph's original solution instead of keeping
the conditional definitions. It's hard to imagine there are many
programs that care whether these other symbols are defined or not.
You can add this to the original patch:
Acked-by: Jeff Layton <[email protected]>
On Wed, Jan 12, 2022 at 12:15 PM Jeff Layton <[email protected]> wrote:
> On Wed, 2022-01-12 at 09:28 +0100, Arnd Bergmann wrote:
> > On Wed, Jan 12, 2022 at 8:56 AM Christoph Hellwig <[email protected]> wrote:
> >
> > Exactly, that is the tradeoff, which is why I'd like the flock maintainers
> > to say which way they prefer. We can either do it more correctly (hiding
> > the constants from user space when they are not usable), or with less
> > change (removing the incorrect #ifdef). Either way sounds reasonable
> > to me, I mainly care that this is explained in the changelog and that the
> > maintainers are aware of the two options.
> >
>
> I don't have a strong opinion here. If we were taking symbols away that
> were previously visible to userland it would be one thing, but since
> we're just adding symbols that may not have been there before, this
> seems less likely to break anything.
Changing
#ifndef CONFIG_64BIT
to
#if __BITS_PER_LONG==32 || defined(__KERNEL__),
would take symbols away, since the CONFIG_64BIT macro is never
set in user space.
> I probably lean toward Christoph's original solution instead of keeping
> the conditional definitions. It's hard to imagine there are many
> programs that care whether these other symbols are defined or not.
>
> You can add this to the original patch:
>
> Acked-by: Jeff Layton <[email protected]>
Sounds good, thanks
Arnd
On Wed, Jan 12, 2022 at 01:08:24PM +0100, Arnd Bergmann wrote:
> > I don't have a strong opinion here. If we were taking symbols away that
> > were previously visible to userland it would be one thing, but since
> > we're just adding symbols that may not have been there before, this
> > seems less likely to break anything.
>
> Changing
>
> #ifndef CONFIG_64BIT
>
> to
>
> #if __BITS_PER_LONG==32 || defined(__KERNEL__),
>
> would take symbols away, since the CONFIG_64BIT macro is never
> set in user space.
Yes.
> > I probably lean toward Christoph's original solution instead of keeping
> > the conditional definitions. It's hard to imagine there are many
> > programs that care whether these other symbols are defined or not.
> >
> > You can add this to the original patch:
> >
> > Acked-by: Jeff Layton <[email protected]>
>
> Sounds good, thanks
So should we go ahead with the series as-is? Or respin it? Or add
the above change ontop?