Re: [PATCH V3 03/17] asm-generic: compat: Cleanup duplicate definitions
On Thu, Jan 20, 2022 at 9:02 PM Arnd Bergmann wrote:
>
> On Thu, Jan 20, 2022 at 8:38 AM wrote:
> >
> > From: Guo Ren
> >
> > There are 7 64bit architectures that support Linux COMPAT mode to
> > run 32bit applications. A lot of definitions are duplicate:
> > - COMPAT_USER_HZ
> > - COMPAT_RLIM_INFINITY
> > - COMPAT_OFF_T_MAX
> > - __compat_uid_t, __compat_uid_t
> > - compat_dev_t
> > - compat_ipc_pid_t
> > - struct compat_flock
> > - struct compat_flock64
> > - struct compat_statfs
> > - struct compat_ipc64_perm, compat_semid64_ds,
> > compat_msqid64_ds, compat_shmid64_ds
> >
> > Cleanup duplicate definitions and merge them into asm-generic.
> >
> > Signed-off-by: Guo Ren
> > Signed-off-by: Guo Ren
> > Cc: Arnd Bergmann
>
> > ---
> > arch/arm64/include/asm/compat.h | 108 +++---
> > arch/mips/include/asm/compat.h| 24 ++
> > arch/parisc/include/asm/compat.h | 47 ++--
> > arch/powerpc/include/asm/compat.h | 47 ++--
> > arch/s390/include/asm/compat.h| 109 +++---
> > arch/sparc/include/asm/compat.h | 39 --
> > arch/x86/include/asm/compat.h | 114 +++-
> > include/asm-generic/compat.h | 122 ++
> > 8 files changed, 191 insertions(+), 419 deletions(-)
> >
> > diff --git a/arch/arm64/include/asm/compat.h
> > b/arch/arm64/include/asm/compat.h
> > index eaa6ca062d89..f54f295efae3 100644
> > --- a/arch/arm64/include/asm/compat.h
> > +++ b/arch/arm64/include/asm/compat.h
> > @@ -5,9 +5,18 @@
> > #ifndef __ASM_COMPAT_H
> > #define __ASM_COMPAT_H
> >
> > +#define COMPAT_RLIM_INFINITY 0x
> ...
> > +#ifndef COMPAT_RLIM_INFINITY
> > +#define COMPAT_RLIM_INFINITY 0x7fff
> > +#endif
>
> While this is a correct conversion, I think the default should
> be 0x, to match the asm-generic RLIM_INFINITY
> definition, with only mips and sparc getting the exception
Okay
>
> > -struct compat_flock {
> > - short l_type;
> > - short l_whence;
> > - compat_off_tl_start;
> > - compat_off_tl_len;
> > - compat_pid_tl_pid;
> > -};
> ...
> > +#ifndef compat_flock
> > +struct compat_flock {
> > + compat_short_t l_type;
> > + compat_short_t l_whence;
> > + compat_off_tl_start;
> > + compat_off_tl_len;
> > + compat_pid_tl_pid;
> > +} __attribute__((packed));
> > +#endif
>
> You are adding __attribute__((packed)) here, which I think has
> no effect on the layout on the structure on any of the architectures
> but it does change the alignment requirements needlessly.
>
> Better leave it without the attribute.
Okay
>
> > -struct compat_flock64 {
> > - short l_type;
> > - short l_whence;
> > - compat_loff_t l_start;
> > - compat_loff_t l_len;
> > - compat_pid_tl_pid;
> > -};
> ...
> > +#ifndef compat_flock64
> > +struct compat_flock64 {
> > + compat_short_t l_type;
> > + compat_short_t l_whence;
> > + compat_loff_t l_start;
> > + compat_loff_t l_len;
> > + compat_pid_tl_pid;
> > +} __attribute__((packed));
> > +#endif
>
> This one is different: on all architectures other than x86,
> the added packed attribute changes the size of the
> structure by removing the four padding bytes at the
> end. x86 originally added the attribute here to work around
> the weirdness of the x86-32 ABI that aligns 64-bit values
> on a 4-byte boundary.
>
> The easiest workaround would be to have x86 keep its
> custom definition. A slightly nicer version would drop the
> attribute on x86 as well but instead change the compat_loff_t
> definition to use compat_s64 instead of s64, giving it the
> correct alignment.
Okay, I would leave x86 origin first.
>
> > -struct compat_statfs {
> > - int f_type;
> > - int f_bsize;
> > - int f_blocks;
> > - int f_bfree;
> > - int f_bavail;
> > - int f_files;
> > - int f_ffree;
> > - compat_fsid_t f_fsid;
> > - int f_namelen; /* SunOS ignores this field. */
> > - int f_frsize;
> > - int f_flags;
> > - int f_spare[4];
> > -};
> ...
> > +#ifndef compat_statfs
> > +struct compat_statfs {
> > + compat_uint_t f_type;
> > + compat_uint_t f_bsize;
> > + compat_uint_t f_blocks;
> > + compat_uint_t f_bfree;
> > + compat_uint_t f_bavail;
> > + compat_uint_t f_files;
> > + compat_uint_t f_ffree;
> > + __kernel_fsid_t f_fsid;
> > + compat_uint_t f_namelen;
> > + compat_uint_t f_frsize;
> > + compat_uint_t f_flags;
> > + compat_uint_t f_spare[4];
> > +} __attribute__((packed));
> > +#endif
>
> None of the architectures use the packed attribut
RE: [PATCH V3 03/17] asm-generic: compat: Cleanup duplicate definitions
From: Arnd Bergmann > Sent: 20 January 2022 11:52 .. > As with compat_flock, the packed attribute has no impact on the layout > here, but please drop it anyway for consistency. Never mind the structure layout, because 'packed' allows the structure to be aligned on any boundary it forces the compiler to use byte memory accesses and shifts on some architectures. This is a horrid performance penalty. 'packed' should only be specified for structures that can occur on any address boundary. ie basically never. If you need to remove the implicit pad before a field (eg 64bit fields on x64-32) then you can mark the field itself as 'packed'. Although, in that case, putting the attribute on the type (compat_s64) is generally cleaner. David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
Re: [PATCH V3 03/17] asm-generic: compat: Cleanup duplicate definitions
On Thu, Jan 20, 2022 at 8:38 AM wrote:
>
> From: Guo Ren
>
> There are 7 64bit architectures that support Linux COMPAT mode to
> run 32bit applications. A lot of definitions are duplicate:
> - COMPAT_USER_HZ
> - COMPAT_RLIM_INFINITY
> - COMPAT_OFF_T_MAX
> - __compat_uid_t, __compat_uid_t
> - compat_dev_t
> - compat_ipc_pid_t
> - struct compat_flock
> - struct compat_flock64
> - struct compat_statfs
> - struct compat_ipc64_perm, compat_semid64_ds,
> compat_msqid64_ds, compat_shmid64_ds
>
> Cleanup duplicate definitions and merge them into asm-generic.
>
> Signed-off-by: Guo Ren
> Signed-off-by: Guo Ren
> Cc: Arnd Bergmann
> ---
> arch/arm64/include/asm/compat.h | 108 +++---
> arch/mips/include/asm/compat.h| 24 ++
> arch/parisc/include/asm/compat.h | 47 ++--
> arch/powerpc/include/asm/compat.h | 47 ++--
> arch/s390/include/asm/compat.h| 109 +++---
> arch/sparc/include/asm/compat.h | 39 --
> arch/x86/include/asm/compat.h | 114 +++-
> include/asm-generic/compat.h | 122 ++
> 8 files changed, 191 insertions(+), 419 deletions(-)
>
> diff --git a/arch/arm64/include/asm/compat.h b/arch/arm64/include/asm/compat.h
> index eaa6ca062d89..f54f295efae3 100644
> --- a/arch/arm64/include/asm/compat.h
> +++ b/arch/arm64/include/asm/compat.h
> @@ -5,9 +5,18 @@
> #ifndef __ASM_COMPAT_H
> #define __ASM_COMPAT_H
>
> +#define COMPAT_RLIM_INFINITY 0x
...
> +#ifndef COMPAT_RLIM_INFINITY
> +#define COMPAT_RLIM_INFINITY 0x7fff
> +#endif
While this is a correct conversion, I think the default should
be 0x, to match the asm-generic RLIM_INFINITY
definition, with only mips and sparc getting the exception
> -struct compat_flock {
> - short l_type;
> - short l_whence;
> - compat_off_tl_start;
> - compat_off_tl_len;
> - compat_pid_tl_pid;
> -};
...
> +#ifndef compat_flock
> +struct compat_flock {
> + compat_short_t l_type;
> + compat_short_t l_whence;
> + compat_off_tl_start;
> + compat_off_tl_len;
> + compat_pid_tl_pid;
> +} __attribute__((packed));
> +#endif
You are adding __attribute__((packed)) here, which I think has
no effect on the layout on the structure on any of the architectures
but it does change the alignment requirements needlessly.
Better leave it without the attribute.
> -struct compat_flock64 {
> - short l_type;
> - short l_whence;
> - compat_loff_t l_start;
> - compat_loff_t l_len;
> - compat_pid_tl_pid;
> -};
...
> +#ifndef compat_flock64
> +struct compat_flock64 {
> + compat_short_t l_type;
> + compat_short_t l_whence;
> + compat_loff_t l_start;
> + compat_loff_t l_len;
> + compat_pid_tl_pid;
> +} __attribute__((packed));
> +#endif
This one is different: on all architectures other than x86,
the added packed attribute changes the size of the
structure by removing the four padding bytes at the
end. x86 originally added the attribute here to work around
the weirdness of the x86-32 ABI that aligns 64-bit values
on a 4-byte boundary.
The easiest workaround would be to have x86 keep its
custom definition. A slightly nicer version would drop the
attribute on x86 as well but instead change the compat_loff_t
definition to use compat_s64 instead of s64, giving it the
correct alignment.
> -struct compat_statfs {
> - int f_type;
> - int f_bsize;
> - int f_blocks;
> - int f_bfree;
> - int f_bavail;
> - int f_files;
> - int f_ffree;
> - compat_fsid_t f_fsid;
> - int f_namelen; /* SunOS ignores this field. */
> - int f_frsize;
> - int f_flags;
> - int f_spare[4];
> -};
...
> +#ifndef compat_statfs
> +struct compat_statfs {
> + compat_uint_t f_type;
> + compat_uint_t f_bsize;
> + compat_uint_t f_blocks;
> + compat_uint_t f_bfree;
> + compat_uint_t f_bavail;
> + compat_uint_t f_files;
> + compat_uint_t f_ffree;
> + __kernel_fsid_t f_fsid;
> + compat_uint_t f_namelen;
> + compat_uint_t f_frsize;
> + compat_uint_t f_flags;
> + compat_uint_t f_spare[4];
> +} __attribute__((packed));
> +#endif
None of the architectures use the packed attribute at the moment,
so please don't add one here.
Changing compat_fsid_t to __kernel_fsid_t is harmless, but seems
unnecessary.
Changing the signed int to an unsigned int (regardless of notation)
may be a change in behavior. s390 is the only architecture
using unsigned members here at the moment, as of b8668fd0a7e1
("s390/uapi: change struct statfs[64] member types to unsigned
value
