RE: [PATCH 5/5] compat: consolidate the compat_flock{,64} definition
From: Christoph Hellwig
> Sent: 11 January 2022 08:35
>
> 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.
>
...
> /*
> - * 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
Maybe:
#define __ARCH_COMPAT_FLOCK64_ATTR (packed),(aligned(4)
...
Delete this bit:
> +#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_tl_pid;
> +#ifdef __ARCH_COMPAT_FLOCK64_PAD
> + __ARCH_COMPAT_FLOCK64_PAD
> +#endif
> +} __ARCH_COMPAT_FLOCK64_PACK;
then:
#ifdef __ARCH_COMPAT_FLOCK64_ATTR
} __attribute__(__ARCH_COMPAT_FLOCK64_ATTR);
#else
};
#endif
Makes it a bit more like the xxx_PAD bits.
Although the trailing ; does cause a bit of grief.
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT,
UK
Registration No: 1397386 (Wales)
RE: [PATCH 5/5] compat: consolidate the compat_flock{,64} definition
From: Arnd Bergmann > Sent: 12 April 2021 12:26 > > On Mon, Apr 12, 2021 at 12:54 PM David Laight wrote: > > From: David Laight > Sent: 12 April 2021 10:37 > > ... > > > I'm guessing that compat_pid_t is 16 bits? > > > So the native 32bit version has an unnamed 2 byte structure pad. > > > The 'packed' removes this pad from the compat structure. > > > > > > AFAICT (apart from mips) the __ARCH_COMPAT_FLOCK_PAD is just > > > adding an explicit pad for the implicit pad the compiler > > > would generate because compat_pid_t is 16 bits. > > > > I've just looked at the header. > > compat_pid_t is 32 bits. > > So Linux must have gained 32bit pids at some earlier time. > > (Historically Unix pids were 16 bit - even on 32bit systems.) > > > > Which makes the explicit pad in 'sparc' rather 'interesting'. > > I saw it was there since the sparc kernel support got merged in > linux-1.3, possibly copied from an older sunos version. Which had a 16bit pid when I used it. So this is a bug in the sparc merge! The explicit 'short' pad could be removed from the 64bit variant because there are always 4 bytes of pad after l_pid. But it does extend the application structure on 32bit sparc so must remain in the uapi header. It doesn't need to be in the 'compat' definition. > > oh - compat_loff_t is only used in a couple of other places. > > neither care in any way about the alignment. > > (Provided get_user() doesn't fault on a 8n+4 aligned address.) > > Ah right, I also see that after this series it's only used in to other > places: compat_resume_swap_area, which could also lose the > __packed annotation, That structure just defines 0 and 8, the structure size doesn't matter and the offsets are 'passed to' get_user() so byte accesses aren't performed. > and in the declaration of > compat_sys_sendfile64, where it makes no difference. Which should probably use get_user() rather than copy_from_user(). Although some architectures may need fallback code for misaligned get_user() ? Or is there a general 'cop out' that structures passed to the kernel are required to be correctly aligned. They should be aligned unless the kernel is 'playing games' like reading 'struct pollfd' as a 64bit item. David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
Re: [PATCH 5/5] compat: consolidate the compat_flock{,64} definition
On Mon, Apr 12, 2021 at 12:54 PM David Laight wrote: > From: David Laight > Sent: 12 April 2021 10:37 > ... > > I'm guessing that compat_pid_t is 16 bits? > > So the native 32bit version has an unnamed 2 byte structure pad. > > The 'packed' removes this pad from the compat structure. > > > > AFAICT (apart from mips) the __ARCH_COMPAT_FLOCK_PAD is just > > adding an explicit pad for the implicit pad the compiler > > would generate because compat_pid_t is 16 bits. > > I've just looked at the header. > compat_pid_t is 32 bits. > So Linux must have gained 32bit pids at some earlier time. > (Historically Unix pids were 16 bit - even on 32bit systems.) > > Which makes the explicit pad in 'sparc' rather 'interesting'. I saw it was there since the sparc kernel support got merged in linux-1.3, possibly copied from an older sunos version. > oh - compat_loff_t is only used in a couple of other places. > neither care in any way about the alignment. > (Provided get_user() doesn't fault on a 8n+4 aligned address.) Ah right, I also see that after this series it's only used in to other places: compat_resume_swap_area, which could also lose the __packed annotation, and in the declaration of compat_sys_sendfile64, where it makes no difference. Arnd
RE: [PATCH 5/5] compat: consolidate the compat_flock{,64} definition
From: David Laight > Sent: 12 April 2021 10:37 ... > I'm guessing that compat_pid_t is 16 bits? > So the native 32bit version has an unnamed 2 byte structure pad. > The 'packed' removes this pad from the compat structure. > > AFAICT (apart from mips) the __ARCH_COMPAT_FLOCK_PAD is just > adding an explicit pad for the implicit pad the compiler > would generate because compat_pid_t is 16 bits. I've just looked at the header. compat_pid_t is 32 bits. So Linux must have gained 32bit pids at some earlier time. (Historically Unix pids were 16 bit - even on 32bit systems.) Which makes the explicit pad in 'sparc' rather 'interesting'. Actually the tail pad can just be removed from the compat structures. Just a comment that mips and sparc have extra fields in the uapi header is enough. The kernel never needs to read/write the pad. userspace must provide the pad in case the kernel writes it. oh - compat_loff_t is only used in a couple of other places. neither care in any way about the alignment. (Provided get_user() doesn't fault on a 8n+4 aligned address.) David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
RE: [PATCH 5/5] compat: consolidate the compat_flock{,64} definition
From: Christoph Hellwig
> Sent: 12 April 2021 09:56
>
> Provide a single common definition for the compat_flock and
> compat_flock64 structures using the same tricks as for the native
> variants. An extra define is added for the packing required on x86.
>
...
> /*
> - * 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_tl_pid;
> -} __attribute__((packed));
> +#define __ARCH_NEED_COMPAT_FLOCK64_PACKED
That shouldn't need to be packed at all.
(Since the 32bit variant isn't packed.)
compat_loff_t should itself have __attribute__((aligned(4)))
probably inherited from compat_s64.
So l_start will be at offset 4 without the __packed.
I'm guessing that compat_pid_t is 16 bits?
So the native 32bit version has an unnamed 2 byte structure pad.
The 'packed' removes this pad from the compat structure.
AFAICT (apart from mips) the __ARCH_COMPAT_FLOCK_PAD is just
adding an explicit pad for the implicit pad the compiler
would generate because compat_pid_t is 16 bits.
If the padding need not be named for the 64bit system calls.
(Where there is probably rather more padding all over the place.)
then it doesn't need to be named for the compat variants.
Even the mips extra padding could be removed.
F_GETLK might be expected to do a read-write of them, so
return EFAULT if not mapped.
But nothing should be testing the EFAULT is returned!
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT,
UK
Registration No: 1397386 (Wales)
