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 <[email protected]>
---
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(-)
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 },
base-commit: 912dfe068c43fa13c587b8d30e73d335c5ba7d44
--
2.24.0
On 13/12/2019 23.23, Aleksa Sarai wrote:
> 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.
And when the first extension adds another u64 field, that padding has to
be added back in and checked for being 0, at which point the padding is
again yet-to-be-used fields. So what exactly is the problem with
returning EINVAL now?
> * 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).
Who knows, it does fit a u32. And if the struct is to be 8-byte aligned
(see below), it doesn't actually waste space.
> 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));
IIRC, gcc assumes such a struct has alignment 1, which means that it
will generate horrible code to access it. So if you do this (and I don't
think it's a good idea), I think you'd also want to include a
__attribute__((__aligned__(8))) - or perhaps that can be accomplished by
just keeping flags as an explicitly aligned member. But that will of
course bump its sizeof() back to 24, at which point it seems better to
just make the padding explicit.
Rasmus
On 2019-12-14, Rasmus Villemoes <[email protected]> wrote:
> On 13/12/2019 23.23, Aleksa Sarai wrote:
> > 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.
>
> And when the first extension adds another u64 field, that padding has to
> be added back in and checked for being 0, at which point the padding is
> again yet-to-be-used fields.
Maybe I'm missing something, but what is the issue with
struct open_how {
u64 flags;
u64 resolve;
u16 mode;
u64 next_extension;
} __attribute__((packed));
It was my understanding that __aligned_u64 was used to ensure consistent
layouts, not that it was needed for safety against unaligned accesses.
> So what exactly is the problem with returning EINVAL now?
I would argue that -EINVAL was the wrong choice of return code from the
outset (and if we do keep the padding, I will send a patch to switch it
to -E2BIG -- see below). The purpose of -E2BIG for the newer
"extensible" syscalls is to differentiate between using an unsupported
extension field and an unsupported (or invalid) flag.
This will be useful for a few other extension ideas for these types of
syscalls (related to allowing userspace to more efficiently figure out
what flags are supported by the kernel without having to try each one
separately).
> > * 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).
>
> Who knows, it does fit a u32. And if the struct is to be 8-byte aligned
> (see below), it doesn't actually waste space.
Yeah, though giving it some more thought I think this might be a better
layout to avoid this problem:
struct open_how {
__aligned_u64 flags;
__aligned_u64 resolve;
__u16 mode;
__u16 __padding[3]; /* must be zero */
};
That way, we won't end up with a u16 which we never use (and we won't
have multiple __padding fields in the future).
> > 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));
>
> IIRC, gcc assumes such a struct has alignment 1, which means that it
> will generate horrible code to access it. So if you do this (and I don't
> think it's a good idea), I think you'd also want to include a
> __attribute__((__aligned__(8))) - or perhaps that can be accomplished by
> just keeping flags as an explicitly aligned member. But that will of
> course bump its sizeof() back to 24, at which point it seems better to
> just make the padding explicit.
Yeah, you're quite right -- I was aware that GCC generated "less than
great" code for aligned(1) structures, but wasn't sure whether it would
be seen as being a serious enough issue to NACK the change.
There is an additional problem -- unfortunately, having the struct be
__attribute__((aligned(8))) doesn't solve the Rust representation
problem because Rust can't represent a struct as both being
#[repr(packed)] and #[repr(align(n))]. Obviously the kernel doesn't
really care about Rust language restrictions, but given one of the main
users of how->resolve will be libpathrs, I'd prefer to not make my own
life any harder if possible. ;)
So, given all of the above, I suggest that instead I send something like
this instead:
diff --git a/fs/open.c b/fs/open.c
index 50a46501bcc9..6c97f52453fe 100644
--- a/fs/open.c
+++ b/fs/open.c
@@ -994,7 +994,7 @@ static inline int build_open_flags(const struct open_how *how,
if (how->resolve & ~VALID_RESOLVE_FLAGS)
return -EINVAL;
if (memchr_inv(how->__padding, 0, sizeof(how->__padding)))
- return -EINVAL;
+ return -E2BIG;
/* Deal with the mode. */
if (WILL_CREATE(flags)) {
diff --git a/include/uapi/linux/fcntl.h b/include/uapi/linux/fcntl.h
index d886bdb585e4..c307640071c8 100644
--- a/include/uapi/linux/fcntl.h
+++ b/include/uapi/linux/fcntl.h
@@ -114,9 +114,9 @@
*/
struct open_how {
__aligned_u64 flags;
+ __aligned_u64 resolve;
__u16 mode;
__u16 __padding[3]; /* must be zeroed */
- __aligned_u64 resolve;
};
#define OPEN_HOW_SIZE_VER0 24 /* sizeof first published struct */
diff --git a/tools/testing/selftests/openat2/openat2_test.c b/tools/testing/selftests/openat2/openat2_test.c
index 0b64fedc008b..88e3614cbb3a 100644
--- a/tools/testing/selftests/openat2/openat2_test.c
+++ b/tools/testing/selftests/openat2/openat2_test.c
@@ -61,15 +61,15 @@ void test_openat2_struct(void)
{ .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 },
+ .size = sizeof(struct open_how_ext), .err = -E2BIG },
{ .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 },
+ .size = sizeof(struct open_how_ext), .err = -E2BIG },
{ .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 },
+ .size = sizeof(struct open_how_ext), .err = -E2BIG },
/* TODO: Once expanded, check zero-padding. */
--
Aleksa Sarai
Senior Software Engineer (Containers)
SUSE Linux GmbH
<https://www.cyphar.com/>
* 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.
On 2019-12-15, Florian Weimer <[email protected]> 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.
--
Aleksa Sarai
Senior Software Engineer (Containers)
SUSE Linux GmbH
<https://www.cyphar.com/>