| Message ID | 20220227162831.674483-4-guoren@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | riscv: compat: Add COMPAT mode support for rv64 | expand |
From: guoren@kernel.org > Sent: 27 February 2022 16:28 > > From: Christoph Hellwig <hch@lst.de> > > 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. ... > diff --git a/arch/x86/include/asm/compat.h b/arch/x86/include/asm/compat.h ... > /* > - * 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 > > struct compat_statfs { > int f_type; > diff --git a/include/linux/compat.h b/include/linux/compat.h > index 1c758b0e0359..a0481fe6c5d5 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_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; > + Provided compat_loff_t are correctly defined with __aligned__(4) marking the structure packed isn't needed. I believe compat_u64 and compat_s64 both have aligned(4). It is also wrong, consider: struct foo { char x; struct compat_flock64 fl64; }; There should be 3 bytes of padding after 'x'. But you've removed it. David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
On Mon, Feb 28, 2022 at 2:40 PM David Laight <David.Laight@aculab.com> wrote: > > From: guoren@kernel.org > > Sent: 27 February 2022 16:28 > > > > From: Christoph Hellwig <hch@lst.de> > > > > 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. > ... > > diff --git a/arch/x86/include/asm/compat.h b/arch/x86/include/asm/compat.h > ... > > /* > > - * 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 > > > > struct compat_statfs { > > int f_type; > > diff --git a/include/linux/compat.h b/include/linux/compat.h > > index 1c758b0e0359..a0481fe6c5d5 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_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; > > + > > Provided compat_loff_t are correctly defined with __aligned__(4) See include/asm-generic/compat.h typedef s64 compat_loff_t; Only: #ifdef CONFIG_COMPAT_FOR_U64_ALIGNMENT typedef s64 __attribute__((aligned(4))) compat_s64; So how do you think compat_loff_t could be defined with __aligned__(4)? > marking the structure packed isn't needed. > I believe compat_u64 and compat_s64 both have aligned(4). > It is also wrong, consider: > > struct foo { > char x; > struct compat_flock64 fl64; > }; > > There should be 3 bytes of padding after 'x'. > But you've removed it. > > David > > - > Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK > Registration No: 1397386 (Wales) >
From: Guo Ren > Sent: 28 February 2022 11:52 > > On Mon, Feb 28, 2022 at 2:40 PM David Laight <David.Laight@aculab.com> wrote: > > > > From: guoren@kernel.org > > > Sent: 27 February 2022 16:28 > > > > > > From: Christoph Hellwig <hch@lst.de> > > > > > > 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. > > ... > > > diff --git a/arch/x86/include/asm/compat.h b/arch/x86/include/asm/compat.h > > ... > > > /* > > > - * 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 > > > > > > struct compat_statfs { > > > int f_type; > > > diff --git a/include/linux/compat.h b/include/linux/compat.h > > > index 1c758b0e0359..a0481fe6c5d5 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_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; > > > + > > > > Provided compat_loff_t are correctly defined with __aligned__(4) > See include/asm-generic/compat.h > > typedef s64 compat_loff_t; > > Only: > #ifdef CONFIG_COMPAT_FOR_U64_ALIGNMENT > typedef s64 __attribute__((aligned(4))) compat_s64; > > So how do you think compat_loff_t could be defined with __aligned__(4)? compat_loff_t should be compat_s64 not s64. The same should be done for all 64bit 'compat' types. David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
On Mon, Feb 28, 2022 at 8:02 PM David Laight <David.Laight@aculab.com> wrote: > > From: Guo Ren > > Sent: 28 February 2022 11:52 > > > > On Mon, Feb 28, 2022 at 2:40 PM David Laight <David.Laight@aculab.com> wrote: > > > > > > From: guoren@kernel.org > > > > Sent: 27 February 2022 16:28 > > > > > > > > From: Christoph Hellwig <hch@lst.de> > > > > > > > > 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. > > > ... > > > > diff --git a/arch/x86/include/asm/compat.h b/arch/x86/include/asm/compat.h > > > ... > > > > /* > > > > - * 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 > > > > > > > > struct compat_statfs { > > > > int f_type; > > > > diff --git a/include/linux/compat.h b/include/linux/compat.h > > > > index 1c758b0e0359..a0481fe6c5d5 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_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; > > > > + > > > > > > Provided compat_loff_t are correctly defined with __aligned__(4) > > See include/asm-generic/compat.h > > > > typedef s64 compat_loff_t; > > > > Only: > > #ifdef CONFIG_COMPAT_FOR_U64_ALIGNMENT > > typedef s64 __attribute__((aligned(4))) compat_s64; > > > > So how do you think compat_loff_t could be defined with __aligned__(4)? > > compat_loff_t should be compat_s64 not s64. > > The same should be done for all 64bit 'compat' types. Changing typedef s64 compat_loff_t; to typedef compat_s64 compat_loff_t; should be another patch and it affects all architectures, I don't think we should involve it in this series. look at kernel/power/user.c: struct compat_resume_swap_area { compat_loff_t offset; u32 dev; } __packed; I thnk keep "typedef s64 compat_loff_t;" is a sensible choice for COMPAT support patchset series. > > David > > - > Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK > Registration No: 1397386 (Wales) -- Best Regards Guo Ren ML: https://lore.kernel.org/linux-csky/
From: Guo Ren > Sent: 28 February 2022 12:13 > > On Mon, Feb 28, 2022 at 8:02 PM David Laight <David.Laight@aculab.com> wrote: > > > > From: Guo Ren > > > Sent: 28 February 2022 11:52 > > > > > > On Mon, Feb 28, 2022 at 2:40 PM David Laight <David.Laight@aculab.com> wrote: > > > > > > > > From: guoren@kernel.org > > > > > Sent: 27 February 2022 16:28 > > > > > > > > > > From: Christoph Hellwig <hch@lst.de> > > > > > > > > > > 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. > > > > ... > > > > > diff --git a/arch/x86/include/asm/compat.h b/arch/x86/include/asm/compat.h > > > > ... > > > > > /* > > > > > - * 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 > > > > > > > > > > struct compat_statfs { > > > > > int f_type; > > > > > diff --git a/include/linux/compat.h b/include/linux/compat.h > > > > > index 1c758b0e0359..a0481fe6c5d5 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_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; > > > > > + > > > > > > > > Provided compat_loff_t are correctly defined with __aligned__(4) > > > See include/asm-generic/compat.h > > > > > > typedef s64 compat_loff_t; > > > > > > Only: > > > #ifdef CONFIG_COMPAT_FOR_U64_ALIGNMENT > > > typedef s64 __attribute__((aligned(4))) compat_s64; > > > > > > So how do you think compat_loff_t could be defined with __aligned__(4)? > > > > compat_loff_t should be compat_s64 not s64. > > > > The same should be done for all 64bit 'compat' types. > Changing > typedef s64 compat_loff_t; > to > typedef compat_s64 compat_loff_t; > > should be another patch and it affects all architectures, I don't > think we should involve it in this series. Except that I think only x86 sets CONFIG_COMPAT_FOR_U64_ALIGNMENT. > look at kernel/power/user.c: > struct compat_resume_swap_area { > compat_loff_t offset; > u32 dev; > } __packed; That is a bug! The size should be 16 bytes on most 32bit architectures. So the compat code won't fault if the last 4 bytes aren't mapped whereas the native 32bit version will fault. Hopefully the compiler realises the on-stack item is actually aligned and doesn't use byte loads and shifts on (eg) sparc64. > I thnk keep "typedef s64 compat_loff_t;" is a sensible choice for > COMPAT support patchset series. But it is wrong :-) compat_[su]64 exist so that compat syscalls that contain 64bit values get the correct alignment. Which is exactly what you have here. AFAICT most of the uses of __packed in the kernel are wrong. It should only be used (on a structure) if the structure might be on a misaligned address. This can happen in data for some network protocols. It should not be used because the structure should have no holes. (Especially in ones that don't have any holes.) David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
On Mon, Feb 28, 2022 at 1:13 PM Guo Ren <guoren@kernel.org> wrote: > On Mon, Feb 28, 2022 at 8:02 PM David Laight <David.Laight@aculab.com> wrote: > > From: Guo Ren Sent: 28 February 2022 11:52 > > > On Mon, Feb 28, 2022 at 2:40 PM David Laight <David.Laight@aculab.com> wrote: > > > > ... > > > > > +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; > > > > > + > > > > > > > > Provided compat_loff_t are correctly defined with __aligned__(4) > > > See include/asm-generic/compat.h > > > > > > typedef s64 compat_loff_t; > > > > > > Only: > > > #ifdef CONFIG_COMPAT_FOR_U64_ALIGNMENT > > > typedef s64 __attribute__((aligned(4))) compat_s64; > > > > > > So how do you think compat_loff_t could be defined with __aligned__(4)? > > > > compat_loff_t should be compat_s64 not s64. > > > > The same should be done for all 64bit 'compat' types. > Changing > typedef s64 compat_loff_t; > to > typedef compat_s64 compat_loff_t; > > should be another patch and it affects all architectures, I don't > think we should involve it in this series. Agreed, your patch (originally from Christoph) looks fine, it correctly transforms the seven copies of the structure into a shared version. There is always more that can be improved, but for this series, I think you have already done enough. > look at kernel/power/user.c: > struct compat_resume_swap_area { > compat_loff_t offset; > u32 dev; > } __packed; > > I thnk keep "typedef s64 compat_loff_t;" is a sensible choice for > COMPAT support patchset series. The only references to compat_loff_t that we have in the kernel could all be simplified by defining compat_loff_t as compat_s64 instead of s64, but it has no impact on correctness here. Let's make sure you get your series into 5.18, and then David can follow-up with any further cleanups after that. Arnd
diff --git a/arch/arm64/include/asm/compat.h b/arch/arm64/include/asm/compat.h index 276328765408..e0faec1984a1 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 6a350c1f70d7..6d6e5a451f4d 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 c04f5a637c39..a1e4534d8050 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 83d8f70779cb..5ef3c7c83c34 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 0f14b3188b1b..07f04d37068b 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 108078751bb5..d78fb44942e0 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 8d19a212f4f2..de794d895866 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 1c758b0e0359..a0481fe6c5d5 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;