| Message ID | 20191219105533.12508-2-cyphar@cyphar.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | openat2: minor uapi cleanups | expand |
* Aleksa Sarai: > diff --git a/include/uapi/linux/openat2.h b/include/uapi/linux/openat2.h > new file mode 100644 > index 000000000000..19ef775e8e5e > --- /dev/null > +++ b/include/uapi/linux/openat2.h > @@ -0,0 +1,41 @@ > +/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */ > +#ifndef _UAPI_LINUX_OPENAT2_H > +#define _UAPI_LINUX_OPENAT2_H I think you should include the relevant header for __align_u64 etc. here. […] > + * Arguments for how openat2(2) should open the target path. If @resolve is > + * zero, then openat2(2) operates very similarly to openat(2). > + * > + * However, unlike openat(2), unknown bits in @flags result in -EINVAL rather > + * than being silently ignored. @mode must be zero unless one of {O_CREAT, > + * O_TMPFILE} are set. > + * > + * @flags: O_* flags. > + * @mode: O_CREAT/O_TMPFILE file mode. > + * @resolve: RESOLVE_* flags. > + */ > +struct open_how { > + __aligned_u64 flags; > + __u16 mode; > + __u16 __padding[3]; /* must be zeroed */ > + __aligned_u64 resolve; > +}; > + > +#define OPEN_HOW_SIZE_VER0 24 /* sizeof first published struct */ > +#define OPEN_HOW_SIZE_LATEST OPEN_HOW_SIZE_VER0 Are these really useful for the UAPI header? Is there a situation where OPEN_HOW_SIZE_LATEST would be different from sizeof (struct open_how)? The header is not compatible with the assembler anyway, so the numeric constant does not seem useful. Thanks, Florian
On 2019-12-19, Florian Weimer <fweimer@redhat.com> wrote: > * Aleksa Sarai: > > > diff --git a/include/uapi/linux/openat2.h b/include/uapi/linux/openat2.h > > new file mode 100644 > > index 000000000000..19ef775e8e5e > > --- /dev/null > > +++ b/include/uapi/linux/openat2.h > > @@ -0,0 +1,41 @@ > > +/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */ > > +#ifndef _UAPI_LINUX_OPENAT2_H > > +#define _UAPI_LINUX_OPENAT2_H > > I think you should include the relevant header for __align_u64 > etc. here. Right -- no idea why I forgot to include them. > […] > > + * Arguments for how openat2(2) should open the target path. If @resolve is > > + * zero, then openat2(2) operates very similarly to openat(2). > > + * > > + * However, unlike openat(2), unknown bits in @flags result in -EINVAL rather > > + * than being silently ignored. @mode must be zero unless one of {O_CREAT, > > + * O_TMPFILE} are set. > > + * > > + * @flags: O_* flags. > > + * @mode: O_CREAT/O_TMPFILE file mode. > > + * @resolve: RESOLVE_* flags. > > + */ > > +struct open_how { > > + __aligned_u64 flags; > > + __u16 mode; > > + __u16 __padding[3]; /* must be zeroed */ > > + __aligned_u64 resolve; > > +}; > > + > > +#define OPEN_HOW_SIZE_VER0 24 /* sizeof first published struct */ > > +#define OPEN_HOW_SIZE_LATEST OPEN_HOW_SIZE_VER0 > > Are these really useful for the UAPI header? Is there a situation where > OPEN_HOW_SIZE_LATEST would be different from sizeof (struct open_how)? > > The header is not compatible with the assembler anyway, so the numeric > constant does not seem useful. OPEN_HOW_SIZE_VER0 could conceivably be useful (in the future we may do size-based checks) but maybe we can just expose it if someone actually ends up needing it. I will move them to the in-kernel header (we use them for BUILD_BUG_ONs to make sure that the sizes are correct).
From: Aleksa Sarai > Sent: 19 December 2019 13:45 > On 2019-12-19, Florian Weimer <fweimer@redhat.com> wrote: > > * Aleksa Sarai: > > > > > diff --git a/include/uapi/linux/openat2.h b/include/uapi/linux/openat2.h > > > new file mode 100644 > > > index 000000000000..19ef775e8e5e > > > --- /dev/null > > > +++ b/include/uapi/linux/openat2.h > > > @@ -0,0 +1,41 @@ > > > +/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */ > > > +#ifndef _UAPI_LINUX_OPENAT2_H > > > +#define _UAPI_LINUX_OPENAT2_H > > > > I think you should include the relevant header for __align_u64 > > etc. here. > > Right -- no idea why I forgot to include them. I'm guessing that is just 64bit aligned on 32bit archs like x86? No need to enforce it provided the structure will have no padding on archs where the 64bit fields are 64bit aligned. A plain __u64 should be fine. David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
On 2019-12-19, David Laight <David.Laight@ACULAB.COM> wrote: > From: Aleksa Sarai > > Sent: 19 December 2019 13:45 > > On 2019-12-19, Florian Weimer <fweimer@redhat.com> wrote: > > > * Aleksa Sarai: > > > > > > > diff --git a/include/uapi/linux/openat2.h b/include/uapi/linux/openat2.h > > > > new file mode 100644 > > > > index 000000000000..19ef775e8e5e > > > > --- /dev/null > > > > +++ b/include/uapi/linux/openat2.h > > > > @@ -0,0 +1,41 @@ > > > > +/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */ > > > > +#ifndef _UAPI_LINUX_OPENAT2_H > > > > +#define _UAPI_LINUX_OPENAT2_H > > > > > > I think you should include the relevant header for __align_u64 > > > etc. here. > > > > Right -- no idea why I forgot to include them. > > I'm guessing that is just 64bit aligned on 32bit archs like x86? Yeah, #define __aligned_u64 __u64 __attribute__((aligned(8))) > No need to enforce it provided the structure will have no padding on > archs where the 64bit fields are 64bit aligned. A plain __u64 should > be fine. Will this cause problems for x86-on-x86_64 emulation? Requiring an 8-byte alignment for 'struct open_how' really isn't that undue of a burden IMHO. Then again, clone3 is a bit of an outlier since both perf_event_open and sched_setattr just use __u64s.
From: Aleksa Sarai > Sent: 20 December 2019 09:32 ... > > I'm guessing that is just 64bit aligned on 32bit archs like x86? > > Yeah, > > #define __aligned_u64 __u64 __attribute__((aligned(8))) > > > No need to enforce it provided the structure will have no padding on > > archs where the 64bit fields are 64bit aligned. A plain __u64 should > > be fine. > > Will this cause problems for x86-on-x86_64 emulation? Requiring an > 8-byte alignment for 'struct open_how' really isn't that undue of a > burden IMHO. Then again, clone3 is a bit of an outlier since both > perf_event_open and sched_setattr just use __u64s. Makes diddly-squit difference. The 64bit kernel will 64bit align the structure. The kernel must allow for the userspace structure having arbitrary alignment. So there is no reason to (try to) align the user structure. David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
diff --git a/MAINTAINERS b/MAINTAINERS index bd5847e802de..737ada377ac3 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -6397,6 +6397,7 @@ F: fs/* F: include/linux/fs.h F: include/linux/fs_types.h F: include/uapi/linux/fs.h +F: include/uapi/linux/openat2.h FINTEK F75375S HARDWARE MONITOR AND FAN CONTROLLER DRIVER M: Riku Voipio <riku.voipio@iki.fi> diff --git a/include/uapi/linux/fcntl.h b/include/uapi/linux/fcntl.h index d886bdb585e4..ca88b7bce553 100644 --- a/include/uapi/linux/fcntl.h +++ b/include/uapi/linux/fcntl.h @@ -3,6 +3,7 @@ #define _UAPI_LINUX_FCNTL_H #include <asm/fcntl.h> +#include <linux/openat2.h> #define F_SETLEASE (F_LINUX_SPECIFIC_BASE + 0) #define F_GETLEASE (F_LINUX_SPECIFIC_BASE + 1) @@ -100,40 +101,4 @@ #define AT_RECURSIVE 0x8000 /* Apply to the entire subtree */ -/* - * Arguments for how openat2(2) should open the target path. If @resolve is - * zero, then openat2(2) operates very similarly to openat(2). - * - * However, unlike openat(2), unknown bits in @flags result in -EINVAL rather - * than being silently ignored. @mode must be zero unless one of {O_CREAT, - * O_TMPFILE} are set. - * - * @flags: O_* flags. - * @mode: O_CREAT/O_TMPFILE file mode. - * @resolve: RESOLVE_* flags. - */ -struct open_how { - __aligned_u64 flags; - __u16 mode; - __u16 __padding[3]; /* must be zeroed */ - __aligned_u64 resolve; -}; - -#define OPEN_HOW_SIZE_VER0 24 /* sizeof first published struct */ -#define OPEN_HOW_SIZE_LATEST OPEN_HOW_SIZE_VER0 - -/* how->resolve flags for openat2(2). */ -#define RESOLVE_NO_XDEV 0x01 /* Block mount-point crossings - (includes bind-mounts). */ -#define RESOLVE_NO_MAGICLINKS 0x02 /* Block traversal through procfs-style - "magic-links". */ -#define RESOLVE_NO_SYMLINKS 0x04 /* Block traversal through all symlinks - (implies OEXT_NO_MAGICLINKS) */ -#define RESOLVE_BENEATH 0x08 /* Block "lexical" trickery like - "..", symlinks, and absolute - paths which escape the dirfd. */ -#define RESOLVE_IN_ROOT 0x10 /* Make all jumps to "/" and ".." - be scoped inside the dirfd - (similar to chroot(2)). */ - #endif /* _UAPI_LINUX_FCNTL_H */ diff --git a/include/uapi/linux/openat2.h b/include/uapi/linux/openat2.h new file mode 100644 index 000000000000..19ef775e8e5e --- /dev/null +++ b/include/uapi/linux/openat2.h @@ -0,0 +1,41 @@ +/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */ +#ifndef _UAPI_LINUX_OPENAT2_H +#define _UAPI_LINUX_OPENAT2_H + +/* + * Arguments for how openat2(2) should open the target path. If @resolve is + * zero, then openat2(2) operates very similarly to openat(2). + * + * However, unlike openat(2), unknown bits in @flags result in -EINVAL rather + * than being silently ignored. @mode must be zero unless one of {O_CREAT, + * O_TMPFILE} are set. + * + * @flags: O_* flags. + * @mode: O_CREAT/O_TMPFILE file mode. + * @resolve: RESOLVE_* flags. + */ +struct open_how { + __aligned_u64 flags; + __u16 mode; + __u16 __padding[3]; /* must be zeroed */ + __aligned_u64 resolve; +}; + +#define OPEN_HOW_SIZE_VER0 24 /* sizeof first published struct */ +#define OPEN_HOW_SIZE_LATEST OPEN_HOW_SIZE_VER0 + +/* how->resolve flags for openat2(2). */ +#define RESOLVE_NO_XDEV 0x01 /* Block mount-point crossings + (includes bind-mounts). */ +#define RESOLVE_NO_MAGICLINKS 0x02 /* Block traversal through procfs-style + "magic-links". */ +#define RESOLVE_NO_SYMLINKS 0x04 /* Block traversal through all symlinks + (implies OEXT_NO_MAGICLINKS) */ +#define RESOLVE_BENEATH 0x08 /* Block "lexical" trickery like + "..", symlinks, and absolute + paths which escape the dirfd. */ +#define RESOLVE_IN_ROOT 0x10 /* Make all jumps to "/" and ".." + be scoped inside the dirfd + (similar to chroot(2)). */ + +#endif /* _UAPI_LINUX_OPENAT2_H */
Florian mentioned that glibc doesn't use fcntl.h because it has some issues with namespace cleanliness, and that we should have a separate header for openat2(2) if possible. Suggested-by: Florian Weimer <fweimer@redhat.com> Signed-off-by: Aleksa Sarai <cyphar@cyphar.com> --- MAINTAINERS | 1 + include/uapi/linux/fcntl.h | 37 +------------------------------- include/uapi/linux/openat2.h | 41 ++++++++++++++++++++++++++++++++++++ 3 files changed, 43 insertions(+), 36 deletions(-) create mode 100644 include/uapi/linux/openat2.h