| Message ID | 20210412085545.2595431-6-hch@lst.de (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | [1/5] uapi: remove the unused HAVE_ARCH_STRUCT_FLOCK64 define | expand |
From: Christoph Hellwig > Sent: 12 April 2021 09:56 > > Provide a single common definition for the compat_flock and > compat_flock64 structures using the same tricks as for the native > variants. An 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. > */ > -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 That shouldn't need to be packed at all. (Since the 32bit variant isn't packed.) compat_loff_t should itself have __attribute__((aligned(4))) probably inherited from compat_s64. So l_start will be at offset 4 without the __packed. I'm guessing that compat_pid_t is 16 bits? So the native 32bit version has an unnamed 2 byte structure pad. The 'packed' removes this pad from the compat structure. AFAICT (apart from mips) the __ARCH_COMPAT_FLOCK_PAD is just adding an explicit pad for the implicit pad the compiler would generate because compat_pid_t is 16 bits. If the padding need not be named for the 64bit system calls. (Where there is probably rather more padding all over the place.) then it doesn't need to be named for the compat variants. Even the mips extra padding could be removed. F_GETLK might be expected to do a read-write of them, so return EFAULT if not mapped. But nothing should be testing the EFAULT is returned! David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
From: David Laight > Sent: 12 April 2021 10:37 ... > I'm guessing that compat_pid_t is 16 bits? > So the native 32bit version has an unnamed 2 byte structure pad. > The 'packed' removes this pad from the compat structure. > > AFAICT (apart from mips) the __ARCH_COMPAT_FLOCK_PAD is just > adding an explicit pad for the implicit pad the compiler > would generate because compat_pid_t is 16 bits. I've just looked at the header. compat_pid_t is 32 bits. So Linux must have gained 32bit pids at some earlier time. (Historically Unix pids were 16 bit - even on 32bit systems.) Which makes the explicit pad in 'sparc' rather 'interesting'. Actually the tail pad can just be removed from the compat structures. Just a comment that mips and sparc have extra fields in the uapi header is enough. The kernel never needs to read/write the pad. userspace must provide the pad in case the kernel writes it. oh - compat_loff_t is only used in a couple of other places. neither care in any way about the alignment. (Provided get_user() doesn't fault on a 8n+4 aligned address.) David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
On Mon, Apr 12, 2021 at 12:54 PM David Laight <David.Laight@aculab.com> wrote: > From: David Laight > Sent: 12 April 2021 10:37 > ... > > I'm guessing that compat_pid_t is 16 bits? > > So the native 32bit version has an unnamed 2 byte structure pad. > > The 'packed' removes this pad from the compat structure. > > > > AFAICT (apart from mips) the __ARCH_COMPAT_FLOCK_PAD is just > > adding an explicit pad for the implicit pad the compiler > > would generate because compat_pid_t is 16 bits. > > I've just looked at the header. > compat_pid_t is 32 bits. > So Linux must have gained 32bit pids at some earlier time. > (Historically Unix pids were 16 bit - even on 32bit systems.) > > Which makes the explicit pad in 'sparc' rather 'interesting'. I saw it was there since the sparc kernel support got merged in linux-1.3, possibly copied from an older sunos version. > oh - compat_loff_t is only used in a couple of other places. > neither care in any way about the alignment. > (Provided get_user() doesn't fault on a 8n+4 aligned address.) Ah right, I also see that after this series it's only used in to other places: compat_resume_swap_area, which could also lose the __packed annotation, and in the declaration of compat_sys_sendfile64, where it makes no difference. Arnd
From: Arnd Bergmann > Sent: 12 April 2021 12:26 > > On Mon, Apr 12, 2021 at 12:54 PM David Laight <David.Laight@aculab.com> wrote: > > From: David Laight > Sent: 12 April 2021 10:37 > > ... > > > I'm guessing that compat_pid_t is 16 bits? > > > So the native 32bit version has an unnamed 2 byte structure pad. > > > The 'packed' removes this pad from the compat structure. > > > > > > AFAICT (apart from mips) the __ARCH_COMPAT_FLOCK_PAD is just > > > adding an explicit pad for the implicit pad the compiler > > > would generate because compat_pid_t is 16 bits. > > > > I've just looked at the header. > > compat_pid_t is 32 bits. > > So Linux must have gained 32bit pids at some earlier time. > > (Historically Unix pids were 16 bit - even on 32bit systems.) > > > > Which makes the explicit pad in 'sparc' rather 'interesting'. > > I saw it was there since the sparc kernel support got merged in > linux-1.3, possibly copied from an older sunos version. Which had a 16bit pid when I used it. So this is a bug in the sparc merge! The explicit 'short' pad could be removed from the 64bit variant because there are always 4 bytes of pad after l_pid. But it does extend the application structure on 32bit sparc so must remain in the uapi header. It doesn't need to be in the 'compat' definition. > > oh - compat_loff_t is only used in a couple of other places. > > neither care in any way about the alignment. > > (Provided get_user() doesn't fault on a 8n+4 aligned address.) > > Ah right, I also see that after this series it's only used in to other > places: compat_resume_swap_area, which could also lose the > __packed annotation, That structure just defines 0 and 8, the structure size doesn't matter and the offsets are 'passed to' get_user() so byte accesses aren't performed. > and in the declaration of > compat_sys_sendfile64, where it makes no difference. Which should probably use get_user() rather than copy_from_user(). Although some architectures may need fallback code for misaligned get_user() ? Or is there a general 'cop out' that structures passed to the kernel are required to be correctly aligned. They should be aligned unless the kernel is 'playing games' like reading 'struct pollfd' as a 64bit item. David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
diff --git a/arch/arm64/include/asm/compat.h b/arch/arm64/include/asm/compat.h index a5fe4558a6ecc0..6f0908606b2b51 100644 --- a/arch/arm64/include/asm/compat.h +++ b/arch/arm64/include/asm/compat.h @@ -66,22 +66,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 a13436e91c3938..7df384e445de06 100644 --- a/arch/mips/include/asm/compat.h +++ b/arch/mips/include/asm/compat.h @@ -50,23 +50,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 1a609d38f6678e..972bf8911d1126 100644 --- a/arch/parisc/include/asm/compat.h +++ b/arch/parisc/include/asm/compat.h @@ -54,22 +54,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 b0f2c3f7fe45a5..e9d433bd561725 100644 --- a/arch/powerpc/include/asm/compat.h +++ b/arch/powerpc/include/asm/compat.h @@ -48,22 +48,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 e0896758779da4..5578607913b847 100644 --- a/arch/s390/include/asm/compat.h +++ b/arch/s390/include/asm/compat.h @@ -103,22 +103,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 4524997424043f..16fa333f28c274 100644 --- a/arch/sparc/include/asm/compat.h +++ b/arch/sparc/include/asm/compat.h @@ -76,23 +76,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 b1691cf148be10..d8e01bf51e7a26 100644 --- a/arch/x86/include/asm/compat.h +++ b/arch/x86/include/asm/compat.h @@ -51,25 +51,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 6e65be75360321..9d196f8a94d5ea 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;
Provide a single common definition for the compat_flock and compat_flock64 structures using the same tricks as for the native variants. An extra define is added for the packing required on x86. Signed-off-by: Christoph Hellwig <hch@lst.de> --- 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(-)