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

[Guo Ren <guoren@kernel.org>]

On Thu, Jan 20, 2022 at 9:02 PM Arnd Bergmann <arnd@arndb.de> wrote:
>
>   On Thu, Jan 20, 2022 at 8:38 AM <guoren@kernel.org> wrote:
> >
> > From: Guo Ren <guoren@linux.alibaba.com>
> >
> > 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@linux.alibaba.com>
> > Signed-off-by: Guo Ren <guoren@kernel.org>
> > Cc: Arnd Bergmann <arnd@arndb.de>
>
> > ---
> >  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/