Re: [PATCH V3 03/17] asm-generic: compat: Cleanup duplicate definitions

From: Guo Ren
Date: Fri Jan 28 2022 - 02:20:27 EST

On Thu, Jan 20, 2022 at 9:02 PM Arnd Bergmann <arnd@xxxxxxxx> wrote:
>
> On Thu, Jan 20, 2022 at 8:38 AM <guoren@xxxxxxxxxx> wrote:
> >
> > From: Guo Ren <guoren@xxxxxxxxxxxxxxxxx>
> >
> > 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 <guoren@xxxxxxxxxxxxxxxxx>
> > Signed-off-by: Guo Ren <guoren@xxxxxxxxxx>
> > Cc: Arnd Bergmann <arnd@xxxxxxxx>
>
> > ---
> > 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 0xffffffff
> ...
> > +#ifndef COMPAT_RLIM_INFINITY
> > +#define COMPAT_RLIM_INFINITY 0x7fffffff
> > +#endif
>
> While this is a correct conversion, I think the default should
> be 0xffffffff, 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_t l_start;
> > - compat_off_t l_len;
> > - compat_pid_t l_pid;
> > -};
> ...
> > +#ifndef compat_flock
> > +struct compat_flock {
> > + compat_short_t l_type;
> > + compat_short_t l_whence;
> > + compat_off_t l_start;
> > + compat_off_t l_len;
> > + compat_pid_t l_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_t l_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_t l_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 attribute at the moment,
> so please don't add one here.
>
> Changing compat_fsid_t to __kernel_fsid_t is harmless, but seems
> unnecessary.
Okay. I would add another
typedef __kernel_fsid_t compat_fsid_t;
in the file.

>
> 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
> values").
> The description of that patch sounds like this was changed to fix
> a bug, but I don't see what the actual problem would be in the
> put_compat_statfs().
>
> For the moment I'd suggest leaving this with the signed version,
> with s390 being another exception next to mips. We can follow-up
> with merging s390 into the common definition using either the
> signed or unsigned types, but I think that needs to be a separate
> patch with a detailed explanation.
Okay, I would leave s390 origin first.

>
> +#ifndef compat_ipc64_perm
> > +struct compat_ipc64_perm {
> > + compat_key_t key;
> > + __compat_uid32_t uid;
> > + __compat_gid32_t gid;
> > + __compat_uid32_t cuid;
> > + __compat_gid32_t cgid;
> > + compat_mode_t mode;
> > + unsigned char __pad1[4 - sizeof(compat_mode_t)];
> > + compat_ushort_t seq;
> > + compat_ushort_t __pad2;
> > + compat_ulong_t unused1;
> > + compat_ulong_t unused2;
> > +} __attribute__((packed));
> > +
> > +struct compat_semid64_ds {
> > + struct compat_ipc64_perm sem_perm;
> > + compat_ulong_t sem_otime;
> > + compat_ulong_t sem_otime_high;
> > + compat_ulong_t sem_ctime;
> > + compat_ulong_t sem_ctime_high;
> > + compat_ulong_t sem_nsems;
> > + compat_ulong_t __unused3;
> > + compat_ulong_t __unused4;
> > +} __attribute__((packed));
> > +
> > +struct compat_msqid64_ds {
> > + struct compat_ipc64_perm msg_perm;
> > + compat_ulong_t msg_stime;
> > + compat_ulong_t msg_stime_high;
> > + compat_ulong_t msg_rtime;
> > + compat_ulong_t msg_rtime_high;
> > + compat_ulong_t msg_ctime;
> > + compat_ulong_t msg_ctime_high;
> > + compat_ulong_t msg_cbytes;
> > + compat_ulong_t msg_qnum;
> > + compat_ulong_t msg_qbytes;
> > + compat_pid_t msg_lspid;
> > + compat_pid_t msg_lrpid;
> > + compat_ulong_t __unused4;
> > + compat_ulong_t __unused5;
> > +} __attribute__((packed));
> > +
> > +struct compat_shmid64_ds {
> > + struct compat_ipc64_perm shm_perm;
> > + compat_size_t shm_segsz;
> > + compat_ulong_t shm_atime;
> > + compat_ulong_t shm_atime_high;
> > + compat_ulong_t shm_dtime;
> > + compat_ulong_t shm_dtime_high;
> > + compat_ulong_t shm_ctime;
> > + compat_ulong_t shm_ctime_high;
> > + compat_pid_t shm_cpid;
> > + compat_pid_t shm_lpid;
> > + compat_ulong_t shm_nattch;
> > + compat_ulong_t __unused4;
> > + compat_ulong_t __unused5;
> > +} __attribute__((packed));
> > +#endif
>
> I checked these in detail, looking at the seven architectures, and your
> conversion looks exactly right (I had initially missed the part about
> compat_mode_t that you got right).
>
> As with compat_flock, the packed attribute has no impact on the layout
> here, but please drop it anyway for consistency.
>
> Arnd



--
Best Regards
Guo Ren

ML: https://lore.kernel.org/linux-csky/