RE: [PATCH V7 03/20] compat: consolidate the compat_flock{,64} definition

[David Laight <David.Laight@ACULAB.COM>]

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)