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