[PATCH 0/2] openat2: minor uapi cleanups

[PATCH 0/2] openat2: minor uapi cleanups

[Aleksa Sarai]

While openat2(2) is still not yet in Linus's tree, we can take this
opportunity to iron out some small warts that weren't noticed earlier:

  * A fix was suggested by Florian Weimer, to separate the openat2
    definitions so glibc can use the header directly. I've put the
    maintainership under VFS but let me know if you'd prefer it belong
    ot the fcntl folks.

  * Having heterogenous field sizes in an extensible struct results in
    "padding hole" problems when adding new fields (in addition the
    correct error to use for non-zero padding isn't entirely clear ).
    The simplest solution is to just copy clone(3)'s model -- always use
    u64s. It will waste a little more space in the struct, but it
    removes a possible future headache.

Aleksa Sarai (2):
  uapi: split openat2(2) definitions from fcntl.h
  openat2: drop open_how->__padding field

 MAINTAINERS                                   |  1 +
 fs/open.c                                     |  2 -
 include/uapi/linux/fcntl.h                    | 37 +----------------
 include/uapi/linux/openat2.h                  | 40 +++++++++++++++++++
 tools/testing/selftests/openat2/helpers.h     |  3 +-
 .../testing/selftests/openat2/openat2_test.c  | 24 ++++-------
 6 files changed, 51 insertions(+), 56 deletions(-)
 create mode 100644 include/uapi/linux/openat2.h


base-commit: 912dfe068c43fa13c587b8d30e73d335c5ba7d44
-- 
2.24.0

[PATCH 1/2] uapi: split openat2(2) definitions from fcntl.h

[Aleksa Sarai]

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

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 */
-- 
2.24.0

Re: [PATCH 1/2] uapi: split openat2(2) definitions from fcntl.h

[Florian Weimer]

* 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

Re: [PATCH 1/2] uapi: split openat2(2) definitions from fcntl.h

[Aleksa Sarai]

[-- Attachment #1: Type: text/plain, Size: 2020 bytes --]

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).

-- 
Aleksa Sarai
Senior Software Engineer (Containers)
SUSE Linux GmbH
<https://www.cyphar.com/>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

RE: [PATCH 1/2] uapi: split openat2(2) definitions from fcntl.h

[David Laight]

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)

Re: [PATCH 1/2] uapi: split openat2(2) definitions from fcntl.h

[Aleksa Sarai]

[-- Attachment #1: Type: text/plain, Size: 1424 bytes --]

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.

-- 
Aleksa Sarai
Senior Software Engineer (Containers)
SUSE Linux GmbH
<https://www.cyphar.com/>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

RE: [PATCH 1/2] uapi: split openat2(2) definitions from fcntl.h

[David Laight]

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)

[PATCH 2/2] openat2: drop open_how->__padding field

[Aleksa Sarai]

The purpose of explicit padding was to allow us to use the space in the
future (C provides no guarantee about the value of padding bytes and
thus userspace could've provided garbage).

However, the downside of explicit padding is that any extension we wish
to add should fit the space exactly (otherwise we may end up with a u16
which will never be used). In addition, the correct error to return for
non-zero padding is not clear (-EINVAL doesn't imply "you're using an
extension field unsupported by this kernel", but -E2BIG seems a bit odd
if the structure size isn't different).

The simplest solution is to just match the design of clone3(2) -- use
u64s for all fields. The extra few-bytes cost of extra fields is not
significant (it's unlikely configuration structs will ever be extremely
large) and it allows for more flag space if necessary.

As openat2(2) is not yet in Linus's tree, we can iron out these minor
warts before we commit to this as a stable ABI.

Suggested-by: David Laight <david.laight@aculab.com>
Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
---
 fs/open.c                                     |  2 --
 include/uapi/linux/openat2.h                  |  3 +--
 tools/testing/selftests/openat2/helpers.h     |  3 +--
 .../testing/selftests/openat2/openat2_test.c  | 24 +++++++------------
 4 files changed, 10 insertions(+), 22 deletions(-)

diff --git a/fs/open.c b/fs/open.c
index 50a46501bcc9..8cdb2b675867 100644
--- a/fs/open.c
+++ b/fs/open.c
@@ -993,8 +993,6 @@ static inline int build_open_flags(const struct open_how *how,
 		return -EINVAL;
 	if (how->resolve & ~VALID_RESOLVE_FLAGS)
 		return -EINVAL;
-	if (memchr_inv(how->__padding, 0, sizeof(how->__padding)))
-		return -EINVAL;
 
 	/* Deal with the mode. */
 	if (WILL_CREATE(flags)) {
diff --git a/include/uapi/linux/openat2.h b/include/uapi/linux/openat2.h
index 19ef775e8e5e..76fad4ada2d4 100644
--- a/include/uapi/linux/openat2.h
+++ b/include/uapi/linux/openat2.h
@@ -16,8 +16,7 @@
  */
 struct open_how {
 	__aligned_u64 flags;
-	__u16 mode;
-	__u16 __padding[3]; /* must be zeroed */
+	__aligned_u64 mode;
 	__aligned_u64 resolve;
 };
 
diff --git a/tools/testing/selftests/openat2/helpers.h b/tools/testing/selftests/openat2/helpers.h
index 43ca5ceab6e3..d756775d0725 100644
--- a/tools/testing/selftests/openat2/helpers.h
+++ b/tools/testing/selftests/openat2/helpers.h
@@ -37,8 +37,7 @@
  */
 struct open_how {
 	__aligned_u64 flags;
-	__u16 mode;
-	__u16 __padding[3]; /* must be zeroed */
+	__aligned_u64 mode;
 	__aligned_u64 resolve;
 };
 
diff --git a/tools/testing/selftests/openat2/openat2_test.c b/tools/testing/selftests/openat2/openat2_test.c
index 0b64fedc008b..b386367c606b 100644
--- a/tools/testing/selftests/openat2/openat2_test.c
+++ b/tools/testing/selftests/openat2/openat2_test.c
@@ -40,7 +40,7 @@ struct struct_test {
 	int err;
 };
 
-#define NUM_OPENAT2_STRUCT_TESTS 10
+#define NUM_OPENAT2_STRUCT_TESTS 7
 #define NUM_OPENAT2_STRUCT_VARIATIONS 13
 
 void test_openat2_struct(void)
@@ -57,20 +57,6 @@ void test_openat2_struct(void)
 		  .arg.inner.flags = O_RDONLY,
 		  .size = sizeof(struct open_how_ext) },
 
-		/* Normal struct with broken padding. */
-		{ .name = "normal struct (non-zero padding[0])",
-		  .arg.inner.flags = O_RDONLY,
-		  .arg.inner.__padding = {0xa0, 0x00, 0x00},
-		  .size = sizeof(struct open_how_ext), .err = -EINVAL },
-		{ .name = "normal struct (non-zero padding[1])",
-		  .arg.inner.flags = O_RDONLY,
-		  .arg.inner.__padding = {0x00, 0x1a, 0x00},
-		  .size = sizeof(struct open_how_ext), .err = -EINVAL },
-		{ .name = "normal struct (non-zero padding[2])",
-		  .arg.inner.flags = O_RDONLY,
-		  .arg.inner.__padding = {0x00, 0x00, 0xef},
-		  .size = sizeof(struct open_how_ext), .err = -EINVAL },
-
 		/* TODO: Once expanded, check zero-padding. */
 
 		/* Smaller than version-0 struct. */
@@ -169,7 +155,7 @@ struct flag_test {
 	int err;
 };
 
-#define NUM_OPENAT2_FLAG_TESTS 21
+#define NUM_OPENAT2_FLAG_TESTS 23
 
 void test_openat2_flags(void)
 {
@@ -214,9 +200,15 @@ void test_openat2_flags(void)
 		{ .name = "invalid how.mode and O_CREAT",
 		  .how.flags = O_CREAT,
 		  .how.mode = 0xFFFF, .err = -EINVAL },
+		{ .name = "invalid (very large) how.mode and O_CREAT",
+		  .how.flags = O_CREAT,
+		  .how.mode = 0xC000000000000000ULL, .err = -EINVAL },
 		{ .name = "invalid how.mode and O_TMPFILE",
 		  .how.flags = O_TMPFILE | O_RDWR,
 		  .how.mode = 0x1337, .err = -EINVAL },
+		{ .name = "invalid (very large) how.mode and O_TMPFILE",
+		  .how.flags = O_TMPFILE | O_RDWR,
+		  .how.mode = 0x0000A00000000000ULL, .err = -EINVAL },
 
 		/* ->resolve must only contain RESOLVE_* flags. */
 		{ .name = "invalid how.resolve and O_RDONLY",
-- 
2.24.0

Re: [PATCH 2/2] openat2: drop open_how->__padding field

[Christian Brauner]

On Thu, Dec 19, 2019 at 09:55:30PM +1100, Aleksa Sarai wrote:
> The purpose of explicit padding was to allow us to use the space in the
> future (C provides no guarantee about the value of padding bytes and
> thus userspace could've provided garbage).
> 
> However, the downside of explicit padding is that any extension we wish
> to add should fit the space exactly (otherwise we may end up with a u16
> which will never be used). In addition, the correct error to return for
> non-zero padding is not clear (-EINVAL doesn't imply "you're using an
> extension field unsupported by this kernel", but -E2BIG seems a bit odd
> if the structure size isn't different).
> 
> The simplest solution is to just match the design of clone3(2) -- use
> u64s for all fields. The extra few-bytes cost of extra fields is not
> significant (it's unlikely configuration structs will ever be extremely
> large) and it allows for more flag space if necessary.
> 
> As openat2(2) is not yet in Linus's tree, we can iron out these minor
> warts before we commit to this as a stable ABI.
> 
> Suggested-by: David Laight <david.laight@aculab.com>
> Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>

Fine with me.
Acked-by: Christian Brauner <christian.brauner@ubuntu.com>

> ---
>  fs/open.c                                     |  2 --
>  include/uapi/linux/openat2.h                  |  3 +--
>  tools/testing/selftests/openat2/helpers.h     |  3 +--
>  .../testing/selftests/openat2/openat2_test.c  | 24 +++++++------------
>  4 files changed, 10 insertions(+), 22 deletions(-)
> 
> diff --git a/fs/open.c b/fs/open.c
> index 50a46501bcc9..8cdb2b675867 100644
> --- a/fs/open.c
> +++ b/fs/open.c
> @@ -993,8 +993,6 @@ static inline int build_open_flags(const struct open_how *how,
>  		return -EINVAL;
>  	if (how->resolve & ~VALID_RESOLVE_FLAGS)
>  		return -EINVAL;
> -	if (memchr_inv(how->__padding, 0, sizeof(how->__padding)))
> -		return -EINVAL;
>  
>  	/* Deal with the mode. */
>  	if (WILL_CREATE(flags)) {
> diff --git a/include/uapi/linux/openat2.h b/include/uapi/linux/openat2.h
> index 19ef775e8e5e..76fad4ada2d4 100644
> --- a/include/uapi/linux/openat2.h
> +++ b/include/uapi/linux/openat2.h
> @@ -16,8 +16,7 @@
>   */
>  struct open_how {
>  	__aligned_u64 flags;
> -	__u16 mode;
> -	__u16 __padding[3]; /* must be zeroed */
> +	__aligned_u64 mode;
>  	__aligned_u64 resolve;
>  };
>  
> diff --git a/tools/testing/selftests/openat2/helpers.h b/tools/testing/selftests/openat2/helpers.h
> index 43ca5ceab6e3..d756775d0725 100644
> --- a/tools/testing/selftests/openat2/helpers.h
> +++ b/tools/testing/selftests/openat2/helpers.h
> @@ -37,8 +37,7 @@
>   */
>  struct open_how {
>  	__aligned_u64 flags;
> -	__u16 mode;
> -	__u16 __padding[3]; /* must be zeroed */
> +	__aligned_u64 mode;
>  	__aligned_u64 resolve;
>  };
>  
> diff --git a/tools/testing/selftests/openat2/openat2_test.c b/tools/testing/selftests/openat2/openat2_test.c
> index 0b64fedc008b..b386367c606b 100644
> --- a/tools/testing/selftests/openat2/openat2_test.c
> +++ b/tools/testing/selftests/openat2/openat2_test.c
> @@ -40,7 +40,7 @@ struct struct_test {
>  	int err;
>  };
>  
> -#define NUM_OPENAT2_STRUCT_TESTS 10
> +#define NUM_OPENAT2_STRUCT_TESTS 7
>  #define NUM_OPENAT2_STRUCT_VARIATIONS 13
>  
>  void test_openat2_struct(void)
> @@ -57,20 +57,6 @@ void test_openat2_struct(void)
>  		  .arg.inner.flags = O_RDONLY,
>  		  .size = sizeof(struct open_how_ext) },
>  
> -		/* Normal struct with broken padding. */
> -		{ .name = "normal struct (non-zero padding[0])",
> -		  .arg.inner.flags = O_RDONLY,
> -		  .arg.inner.__padding = {0xa0, 0x00, 0x00},
> -		  .size = sizeof(struct open_how_ext), .err = -EINVAL },
> -		{ .name = "normal struct (non-zero padding[1])",
> -		  .arg.inner.flags = O_RDONLY,
> -		  .arg.inner.__padding = {0x00, 0x1a, 0x00},
> -		  .size = sizeof(struct open_how_ext), .err = -EINVAL },
> -		{ .name = "normal struct (non-zero padding[2])",
> -		  .arg.inner.flags = O_RDONLY,
> -		  .arg.inner.__padding = {0x00, 0x00, 0xef},
> -		  .size = sizeof(struct open_how_ext), .err = -EINVAL },
> -
>  		/* TODO: Once expanded, check zero-padding. */
>  
>  		/* Smaller than version-0 struct. */
> @@ -169,7 +155,7 @@ struct flag_test {
>  	int err;
>  };
>  
> -#define NUM_OPENAT2_FLAG_TESTS 21
> +#define NUM_OPENAT2_FLAG_TESTS 23
>  
>  void test_openat2_flags(void)
>  {
> @@ -214,9 +200,15 @@ void test_openat2_flags(void)
>  		{ .name = "invalid how.mode and O_CREAT",
>  		  .how.flags = O_CREAT,
>  		  .how.mode = 0xFFFF, .err = -EINVAL },
> +		{ .name = "invalid (very large) how.mode and O_CREAT",
> +		  .how.flags = O_CREAT,
> +		  .how.mode = 0xC000000000000000ULL, .err = -EINVAL },
>  		{ .name = "invalid how.mode and O_TMPFILE",
>  		  .how.flags = O_TMPFILE | O_RDWR,
>  		  .how.mode = 0x1337, .err = -EINVAL },
> +		{ .name = "invalid (very large) how.mode and O_TMPFILE",
> +		  .how.flags = O_TMPFILE | O_RDWR,
> +		  .how.mode = 0x0000A00000000000ULL, .err = -EINVAL },
>  
>  		/* ->resolve must only contain RESOLVE_* flags. */
>  		{ .name = "invalid how.resolve and O_RDONLY",
> -- 
> 2.24.0
> 

[PATCH 0/2] openat2: minor uapi cleanups

[Aleksa Sarai]

While openat2(2) is still not yet in Linus's tree, we can take this
opportunity to iron out some small warts that weren't noticed earlier:

  * A fix was suggested by Florian Weimer, to separate the openat2
    definitions so glibc can use the header directly. I've put the
    maintainership under VFS but let me know if you'd prefer it belong
    ot the fcntl folks.

  * Having heterogenous field sizes in an extensible struct results in
    "padding hole" problems when adding new fields (in addition the
    correct error to use for non-zero padding isn't entirely clear ).
    The simplest solution is to just copy clone(3)'s model -- always use
    u64s. It will waste a little more space in the struct, but it
    removes a possible future headache.

Aleksa Sarai (2):
  uapi: split openat2(2) definitions from fcntl.h
  openat2: drop open_how->__padding field

 MAINTAINERS                                   |  1 +
 fs/open.c                                     |  2 -
 include/uapi/linux/fcntl.h                    | 37 +----------------
 include/uapi/linux/openat2.h                  | 40 +++++++++++++++++++
 tools/testing/selftests/openat2/helpers.h     |  3 +-
 .../testing/selftests/openat2/openat2_test.c  | 24 ++++-------
 6 files changed, 51 insertions(+), 56 deletions(-)
 create mode 100644 include/uapi/linux/openat2.h


base-commit: 912dfe068c43fa13c587b8d30e73d335c5ba7d44
-- 
2.24.0

[PATCH 1/2] uapi: split openat2(2) definitions from fcntl.h

[Aleksa Sarai]

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

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 */
-- 
2.24.0

[PATCH 2/2] openat2: drop open_how->__padding field

[Aleksa Sarai]

The purpose of explicit padding was to allow us to use the space in the
future (C provides no guarantee about the value of padding bytes and
thus userspace could've provided garbage).

However, the downside of explicit padding is that any extension we wish
to add should fit the space exactly (otherwise we may end up with a u16
which will never be used). In addition, the correct error to return for
non-zero padding is not clear (-EINVAL doesn't imply "you're using an
extension field unsupported by this kernel", but -E2BIG seems a bit odd
if the structure size isn't different).

The simplest solution is to just match the design of clone3(2) -- use
u64s for all fields. The extra few-bytes cost of extra fields is not
significant (it's unlikely configuration structs will ever be extremely
large) and it allows for more flag space if necessary.

As openat2(2) is not yet in Linus's tree, we can iron out these minor
warts before we commit to this as a stable ABI.

Suggested-by: David Laight <david.laight@aculab.com>
Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
---
 fs/open.c                                     |  2 --
 include/uapi/linux/openat2.h                  |  3 +--
 tools/testing/selftests/openat2/helpers.h     |  3 +--
 .../testing/selftests/openat2/openat2_test.c  | 24 +++++++------------
 4 files changed, 10 insertions(+), 22 deletions(-)

diff --git a/fs/open.c b/fs/open.c
index 50a46501bcc9..8cdb2b675867 100644
--- a/fs/open.c
+++ b/fs/open.c
@@ -993,8 +993,6 @@ static inline int build_open_flags(const struct open_how *how,
 		return -EINVAL;
 	if (how->resolve & ~VALID_RESOLVE_FLAGS)
 		return -EINVAL;
-	if (memchr_inv(how->__padding, 0, sizeof(how->__padding)))
-		return -EINVAL;
 
 	/* Deal with the mode. */
 	if (WILL_CREATE(flags)) {
diff --git a/include/uapi/linux/openat2.h b/include/uapi/linux/openat2.h
index 19ef775e8e5e..76fad4ada2d4 100644
--- a/include/uapi/linux/openat2.h
+++ b/include/uapi/linux/openat2.h
@@ -16,8 +16,7 @@
  */
 struct open_how {
 	__aligned_u64 flags;
-	__u16 mode;
-	__u16 __padding[3]; /* must be zeroed */
+	__aligned_u64 mode;
 	__aligned_u64 resolve;
 };
 
diff --git a/tools/testing/selftests/openat2/helpers.h b/tools/testing/selftests/openat2/helpers.h
index 43ca5ceab6e3..d756775d0725 100644
--- a/tools/testing/selftests/openat2/helpers.h
+++ b/tools/testing/selftests/openat2/helpers.h
@@ -37,8 +37,7 @@
  */
 struct open_how {
 	__aligned_u64 flags;
-	__u16 mode;
-	__u16 __padding[3]; /* must be zeroed */
+	__aligned_u64 mode;
 	__aligned_u64 resolve;
 };
 
diff --git a/tools/testing/selftests/openat2/openat2_test.c b/tools/testing/selftests/openat2/openat2_test.c
index 0b64fedc008b..b386367c606b 100644
--- a/tools/testing/selftests/openat2/openat2_test.c
+++ b/tools/testing/selftests/openat2/openat2_test.c
@@ -40,7 +40,7 @@ struct struct_test {
 	int err;
 };
 
-#define NUM_OPENAT2_STRUCT_TESTS 10
+#define NUM_OPENAT2_STRUCT_TESTS 7
 #define NUM_OPENAT2_STRUCT_VARIATIONS 13
 
 void test_openat2_struct(void)
@@ -57,20 +57,6 @@ void test_openat2_struct(void)
 		  .arg.inner.flags = O_RDONLY,
 		  .size = sizeof(struct open_how_ext) },
 
-		/* Normal struct with broken padding. */
-		{ .name = "normal struct (non-zero padding[0])",
-		  .arg.inner.flags = O_RDONLY,
-		  .arg.inner.__padding = {0xa0, 0x00, 0x00},
-		  .size = sizeof(struct open_how_ext), .err = -EINVAL },
-		{ .name = "normal struct (non-zero padding[1])",
-		  .arg.inner.flags = O_RDONLY,
-		  .arg.inner.__padding = {0x00, 0x1a, 0x00},
-		  .size = sizeof(struct open_how_ext), .err = -EINVAL },
-		{ .name = "normal struct (non-zero padding[2])",
-		  .arg.inner.flags = O_RDONLY,
-		  .arg.inner.__padding = {0x00, 0x00, 0xef},
-		  .size = sizeof(struct open_how_ext), .err = -EINVAL },
-
 		/* TODO: Once expanded, check zero-padding. */
 
 		/* Smaller than version-0 struct. */
@@ -169,7 +155,7 @@ struct flag_test {
 	int err;
 };
 
-#define NUM_OPENAT2_FLAG_TESTS 21
+#define NUM_OPENAT2_FLAG_TESTS 23
 
 void test_openat2_flags(void)
 {
@@ -214,9 +200,15 @@ void test_openat2_flags(void)
 		{ .name = "invalid how.mode and O_CREAT",
 		  .how.flags = O_CREAT,
 		  .how.mode = 0xFFFF, .err = -EINVAL },
+		{ .name = "invalid (very large) how.mode and O_CREAT",
+		  .how.flags = O_CREAT,
+		  .how.mode = 0xC000000000000000ULL, .err = -EINVAL },
 		{ .name = "invalid how.mode and O_TMPFILE",
 		  .how.flags = O_TMPFILE | O_RDWR,
 		  .how.mode = 0x1337, .err = -EINVAL },
+		{ .name = "invalid (very large) how.mode and O_TMPFILE",
+		  .how.flags = O_TMPFILE | O_RDWR,
+		  .how.mode = 0x0000A00000000000ULL, .err = -EINVAL },
 
 		/* ->resolve must only contain RESOLVE_* flags. */
 		{ .name = "invalid how.resolve and O_RDONLY",
-- 
2.24.0

Re: [PATCH 0/2] openat2: minor uapi cleanups

[Christian Brauner]

On Thu, Dec 19, 2019 at 09:55:28PM +1100, Aleksa Sarai wrote:
> While openat2(2) is still not yet in Linus's tree, we can take this
> opportunity to iron out some small warts that weren't noticed earlier:
> 
>   * A fix was suggested by Florian Weimer, to separate the openat2
>     definitions so glibc can use the header directly. I've put the
>     maintainership under VFS but let me know if you'd prefer it belong
>     ot the fcntl folks.
> 
>   * Having heterogenous field sizes in an extensible struct results in
>     "padding hole" problems when adding new fields (in addition the
>     correct error to use for non-zero padding isn't entirely clear ).
>     The simplest solution is to just copy clone(3)'s model -- always use
>     u64s. It will waste a little more space in the struct, but it
>     removes a possible future headache.

Am I imagining things or did I get the same patch series twice?

Christian

Re: [PATCH 0/2] openat2: minor uapi cleanups

[Aleksa Sarai]

[-- Attachment #1: Type: text/plain, Size: 1246 bytes --]

On 2019-12-19, Christian Brauner <christian.brauner@ubuntu.com> wrote:
> On Thu, Dec 19, 2019 at 09:55:28PM +1100, Aleksa Sarai wrote:
> > While openat2(2) is still not yet in Linus's tree, we can take this
> > opportunity to iron out some small warts that weren't noticed earlier:
> > 
> >   * A fix was suggested by Florian Weimer, to separate the openat2
> >     definitions so glibc can use the header directly. I've put the
> >     maintainership under VFS but let me know if you'd prefer it belong
> >     ot the fcntl folks.
> > 
> >   * Having heterogenous field sizes in an extensible struct results in
> >     "padding hole" problems when adding new fields (in addition the
> >     correct error to use for non-zero padding isn't entirely clear ).
> >     The simplest solution is to just copy clone(3)'s model -- always use
> >     u64s. It will waste a little more space in the struct, but it
> >     removes a possible future headache.
> 
> Am I imagining things or did I get the same patch series twice?

Not unless it's a coincidence -- I accidentally ran

  % git send-email *.patch [some flags] *.patch

-- 
Aleksa Sarai
Senior Software Engineer (Containers)
SUSE Linux GmbH
<https://www.cyphar.com/>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]