openat2: switch to __attribute__((packed)) for open_how

Submitted by Aleksa Sarai on Dec. 13, 2019, 10:23 p.m.

Details

Message ID 20191213222351.14071-1-cyphar@cyphar.com
State New
Series "openat2: switch to __attribute__((packed)) for open_how"
Headers show

Commit Message

Aleksa Sarai Dec. 13, 2019, 10:23 p.m.
The design of the original open_how struct layout was such that it
ensured that there would be no un-labelled (and thus potentially
non-zero) padding to avoid issues with struct expansion, as well as
providing a uniform representation on all architectures (to avoid
complications with OPEN_HOW_SIZE versioning).

However, there were a few other desirable features which were not
fulfilled by the previous struct layout:

 * Adding new features (other than new flags) should always result in
   the struct getting larger. However, by including a padding field, it
   was possible for new fields to be added without expanding the
   structure. This would somewhat complicate version-number based
   checking of feature support.

 * A non-zero bit in __padding yielded -EINVAL when it should arguably
   have been -E2BIG (because the padding bits are effectively
   yet-to-be-used fields). However, the semantics are not entirely clear
   because userspace may expect -E2BIG to only signify that the
   structure is too big. It's much simpler to just provide the guarantee
   that new fields will always result in a struct size increase, and
   -E2BIG indicates you're using a field that's too recent for an older
   kernel.

 * While the alignment for u64s was manually backed by extra padding
   fields, some languages (such as Rust) do not currently support
   enforcing alignment of struct field members.

 * The padding wasted space needlessly, and would very likely not be
   used up entirely by future extensions for a long time (because it
   couldn't fit a u64).

While none of these outstanding issues are deal-breakers, we can iron
out these warts before openat2(2) lands in Linus's tree. Instead of
using alignment and padding, we simply pack the structure with
__attribute__((packed)). Rust supports #[repr(packed)] and it removes
all of the issues with having explicit padding.

Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
---
 fs/open.c                                      |  2 --
 include/uapi/linux/fcntl.h                     | 11 +++++------
 tools/testing/selftests/openat2/helpers.h      | 11 +++++------
 tools/testing/selftests/openat2/openat2_test.c | 18 +-----------------
 4 files changed, 11 insertions(+), 31 deletions(-)


base-commit: 912dfe068c43fa13c587b8d30e73d335c5ba7d44

Patch hide | download patch | download mbox

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/fcntl.h b/include/uapi/linux/fcntl.h
index d886bdb585e4..0e070c7f568a 100644
--- a/include/uapi/linux/fcntl.h
+++ b/include/uapi/linux/fcntl.h
@@ -109,17 +109,16 @@ 
  * O_TMPFILE} are set.
  *
  * @flags: O_* flags.
- * @mode: O_CREAT/O_TMPFILE file mode.
  * @resolve: RESOLVE_* flags.
+ * @mode: O_CREAT/O_TMPFILE file mode.
  */
 struct open_how {
-	__aligned_u64 flags;
+	__u64 flags;
+	__u64 resolve;
 	__u16 mode;
-	__u16 __padding[3]; /* must be zeroed */
-	__aligned_u64 resolve;
-};
+} __attribute__((packed));
 
-#define OPEN_HOW_SIZE_VER0	24 /* sizeof first published struct */
+#define OPEN_HOW_SIZE_VER0	18 /* sizeof first published struct */
 #define OPEN_HOW_SIZE_LATEST	OPEN_HOW_SIZE_VER0
 
 /* how->resolve flags for openat2(2). */
diff --git a/tools/testing/selftests/openat2/helpers.h b/tools/testing/selftests/openat2/helpers.h
index 43ca5ceab6e3..eb1535c8fa2e 100644
--- a/tools/testing/selftests/openat2/helpers.h
+++ b/tools/testing/selftests/openat2/helpers.h
@@ -32,17 +32,16 @@ 
  * O_TMPFILE} are set.
  *
  * @flags: O_* flags.
- * @mode: O_CREAT/O_TMPFILE file mode.
  * @resolve: RESOLVE_* flags.
+ * @mode: O_CREAT/O_TMPFILE file mode.
  */
 struct open_how {
-	__aligned_u64 flags;
+	__u64 flags;
+	__u64 resolve;
 	__u16 mode;
-	__u16 __padding[3]; /* must be zeroed */
-	__aligned_u64 resolve;
-};
+} __attribute__((packed));
 
-#define OPEN_HOW_SIZE_VER0	24 /* sizeof first published struct */
+#define OPEN_HOW_SIZE_VER0	18 /* sizeof first published struct */
 #define OPEN_HOW_SIZE_LATEST	OPEN_HOW_SIZE_VER0
 
 bool needs_openat2(const struct open_how *how);
diff --git a/tools/testing/selftests/openat2/openat2_test.c b/tools/testing/selftests/openat2/openat2_test.c
index 0b64fedc008b..cbf95d160b1b 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,22 +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. */
 		{ .name = "zero-sized 'struct'",
 		  .arg.inner.flags = O_RDONLY, .size = 0, .err = -EINVAL },

Comments

Florian Weimer Dec. 15, 2019, 7:48 p.m.
* Aleksa Sarai:

> diff --git a/tools/testing/selftests/openat2/helpers.h b/tools/testing/selftests/openat2/helpers.h
> index 43ca5ceab6e3..eb1535c8fa2e 100644
> --- a/tools/testing/selftests/openat2/helpers.h
> +++ b/tools/testing/selftests/openat2/helpers.h
> @@ -32,17 +32,16 @@
>   * O_TMPFILE} are set.
>   *
>   * @flags: O_* flags.
> - * @mode: O_CREAT/O_TMPFILE file mode.
>   * @resolve: RESOLVE_* flags.
> + * @mode: O_CREAT/O_TMPFILE file mode.
>   */
>  struct open_how {
> -	__aligned_u64 flags;
> +	__u64 flags;
> +	__u64 resolve;
>  	__u16 mode;
> -	__u16 __padding[3]; /* must be zeroed */
> -	__aligned_u64 resolve;
> -};
> +} __attribute__((packed));
>  
> -#define OPEN_HOW_SIZE_VER0	24 /* sizeof first published struct */
> +#define OPEN_HOW_SIZE_VER0	18 /* sizeof first published struct */
>  #define OPEN_HOW_SIZE_LATEST	OPEN_HOW_SIZE_VER0

A userspace ABI that depends on GCC extensions probably isn't a good
idea.  Even with GCC, it will not work well with some future
extensions because it pretty much rules out having arrays or other
members that are access through pointers.  Current GCC does not carry
over the packed-ness of the struct to addresses of its members.
Aleksa Sarai Dec. 15, 2019, 8:55 p.m.
On 2019-12-15, Florian Weimer <fw@deneb.enyo.de> wrote:
> * Aleksa Sarai:
> 
> > diff --git a/tools/testing/selftests/openat2/helpers.h b/tools/testing/selftests/openat2/helpers.h
> > index 43ca5ceab6e3..eb1535c8fa2e 100644
> > --- a/tools/testing/selftests/openat2/helpers.h
> > +++ b/tools/testing/selftests/openat2/helpers.h
> > @@ -32,17 +32,16 @@
> >   * O_TMPFILE} are set.
> >   *
> >   * @flags: O_* flags.
> > - * @mode: O_CREAT/O_TMPFILE file mode.
> >   * @resolve: RESOLVE_* flags.
> > + * @mode: O_CREAT/O_TMPFILE file mode.
> >   */
> >  struct open_how {
> > -	__aligned_u64 flags;
> > +	__u64 flags;
> > +	__u64 resolve;
> >  	__u16 mode;
> > -	__u16 __padding[3]; /* must be zeroed */
> > -	__aligned_u64 resolve;
> > -};
> > +} __attribute__((packed));
> >  
> > -#define OPEN_HOW_SIZE_VER0	24 /* sizeof first published struct */
> > +#define OPEN_HOW_SIZE_VER0	18 /* sizeof first published struct */
> >  #define OPEN_HOW_SIZE_LATEST	OPEN_HOW_SIZE_VER0
> 
> A userspace ABI that depends on GCC extensions probably isn't a good
> idea.  Even with GCC, it will not work well with some future
> extensions because it pretty much rules out having arrays or other
> members that are access through pointers.  Current GCC does not carry
> over the packed-ness of the struct to addresses of its members.

Right, those are also good points.

Okay, I'm going to send a separate patch which changes the return value
for invalid __padding to -E2BIG, and moves the padding to the end of the
struct (along with open_how.mode). That should fix all of the warts I
raised, without running into the numerous problems with
__attribute__((packed)) of which I am now aware.