From 8597fd0bfc02105fe73d67f393c031a3aa358c12 Mon Sep 17 00:00:00 2001 From: Zach Brown Date: Tue, 25 Jun 2019 12:42:59 -0700 Subject: [PATCH] scoutfs-utils: naturally align ioctl structs Use natuturally aligned and explicitly padded ioctl structs. Signed-off-by: Zach Brown --- utils/src/ioctl.h | 70 ++++++++++++++++++++++++++++--------- utils/src/item-cache-keys.c | 14 ++++---- utils/src/key.h | 18 ++++++++++ utils/src/parse.c | 8 ++--- utils/src/parse.h | 4 +-- utils/src/setattr.c | 6 +++- 6 files changed, 91 insertions(+), 29 deletions(-) diff --git a/utils/src/ioctl.h b/utils/src/ioctl.h index b9966c68..0397b634 100644 --- a/utils/src/ioctl.h +++ b/utils/src/ioctl.h @@ -1,14 +1,41 @@ #ifndef _SCOUTFS_IOCTL_H_ #define _SCOUTFS_IOCTL_H_ +/* + * We naturally align explicit width fields in the ioctl structs so that + * userspace doesn't need to deal with padding or unaligned packing and + * we don't have to deal with 32/64 compat. It makes it a little + * awkward to communicate persistent packed structs through the ioctls + * but that happens very rarely. An interesting special case are + * 0length arrays that follow the structs. We make those start at the + * next aligned offset of the struct to be safe. + * + * This is enforced by pahole scripting in external build environments. + */ + /* XXX I have no idea how these are chosen. */ #define SCOUTFS_IOCTL_MAGIC 's' +/* + * Packed scoutfs keys rarely cross the ioctl boundary so we have a + * translation struct. + */ +struct scoutfs_ioctl_key { + __le64 _sk_first; + __le64 _sk_second; + __le64 _sk_third; + __u8 _sk_fourth; + __u8 sk_type; + __u8 sk_zone; + __u8 _pad[5]; +}; + struct scoutfs_ioctl_walk_inodes_entry { __u64 major; - __u32 minor; __u64 ino; -} __packed; + __u32 minor; + __u8 _pad[4]; +}; /* * Walk inodes in an index that is sorted by one of their fields. @@ -48,7 +75,8 @@ struct scoutfs_ioctl_walk_inodes { __u64 entries_ptr; __u32 nr_entries; __u8 index; -} __packed; + __u8 _pad[11]; /* padded to align walk_inodes_entry total size */ +}; enum { SCOUTFS_IOC_WALK_INODES_META_SEQ = 0, @@ -127,14 +155,16 @@ struct scoutfs_ioctl_ino_path { __u64 dir_pos; __u64 result_ptr; __u16 result_bytes; -} __packed; + __u8 _pad[6]; +}; struct scoutfs_ioctl_ino_path_result { __u64 dir_ino; __u64 dir_pos; __u16 path_bytes; + __u8 _pad[6]; __u8 path[0]; -} __packed; +}; /* Get a single path from the root to the given inode number */ #define SCOUTFS_IOC_INO_PATH _IOW(SCOUTFS_IOCTL_MAGIC, 2, \ @@ -168,7 +198,7 @@ struct scoutfs_ioctl_release { __u64 block; __u64 count; __u64 data_version; -} __packed; +}; #define SCOUTFS_IOC_RELEASE _IOW(SCOUTFS_IOCTL_MAGIC, 5, \ struct scoutfs_ioctl_release) @@ -178,7 +208,8 @@ struct scoutfs_ioctl_stage { __u64 buf_ptr; __u64 offset; __s32 count; -} __packed; + __u32 _pad; +}; #define SCOUTFS_IOC_STAGE _IOW(SCOUTFS_IOCTL_MAGIC, 6, \ struct scoutfs_ioctl_stage) @@ -203,11 +234,12 @@ struct scoutfs_ioctl_stat_more { __u64 data_version; __u64 online_blocks; __u64 offline_blocks; -} __packed; +}; #define SCOUTFS_IOC_STAT_MORE _IOW(SCOUTFS_IOCTL_MAGIC, 7, \ struct scoutfs_ioctl_stat_more) + /* * Fills the buffer with either the keys for the cached items or the * keys for the cached ranges found starting with the given key. The @@ -215,11 +247,12 @@ struct scoutfs_ioctl_stat_more { * keys the returned number will always be a multiple of two. */ struct scoutfs_ioctl_item_cache_keys { - struct scoutfs_key key; + struct scoutfs_ioctl_key ikey; __u64 buf_ptr; __u16 buf_nr; __u8 which; -} __packed; + __u8 _pad[21]; /* padded to align _ioctl_key total size */ +}; enum { SCOUTFS_IOC_ITEM_CACHE_KEYS_ITEMS = 0, @@ -233,7 +266,8 @@ struct scoutfs_ioctl_data_waiting_entry { __u64 ino; __u64 iblock; __u8 op; -} __packed; + __u8 _pad[7]; +}; #define SCOUTFS_IOC_DWO_READ (1 << 0) #define SCOUTFS_IOC_DWO_WRITE (1 << 1) @@ -246,7 +280,8 @@ struct scoutfs_ioctl_data_waiting { __u64 after_iblock; __u64 ents_ptr; __u16 ents_nr; -} __packed; + __u8 _pad[6]; +}; #define SCOUTFS_IOC_DATA_WAITING_FLAGS_UNKNOWN (U8_MAX << 0) @@ -262,8 +297,10 @@ struct scoutfs_ioctl_setattr_more { __u64 data_version; __u64 i_size; __u64 flags; - struct scoutfs_timespec ctime; -} __packed; + __u64 ctime_sec; + __u32 ctime_nsec; + __u8 _pad[4]; +}; #define SCOUTFS_IOC_SETATTR_MORE_OFFLINE (1 << 0) #define SCOUTFS_IOC_SETATTR_MORE_UNKNOWN (U8_MAX << 1) @@ -276,7 +313,7 @@ struct scoutfs_ioctl_listxattr_raw { __u64 buf_ptr; __u32 buf_bytes; __u32 hash_pos; -} __packed; +}; #define SCOUTFS_IOC_LISTXATTR_RAW _IOW(SCOUTFS_IOCTL_MAGIC, 11, \ struct scoutfs_ioctl_listxattr_raw) @@ -306,7 +343,8 @@ struct scoutfs_ioctl_find_xattrs { __u64 inodes_ptr; __u16 name_bytes; __u16 nr_inodes; -} __packed; + __u8 _pad[4]; +}; #define SCOUTFS_IOC_FIND_XATTRS _IOW(SCOUTFS_IOCTL_MAGIC, 12, \ struct scoutfs_ioctl_find_xattrs) diff --git a/utils/src/item-cache-keys.c b/utils/src/item-cache-keys.c index 68ca48c5..a7577cb3 100644 --- a/utils/src/item-cache-keys.c +++ b/utils/src/item-cache-keys.c @@ -19,7 +19,8 @@ static int item_cache_keys(int argc, char **argv, int which) { struct scoutfs_ioctl_item_cache_keys ick; - struct scoutfs_key keys[32]; + struct scoutfs_ioctl_key ikeys[32]; + struct scoutfs_key key; int ret; int fd; int i; @@ -38,8 +39,8 @@ static int item_cache_keys(int argc, char **argv, int which) } memset(&ick, 0, sizeof(ick)); - ick.buf_ptr = (unsigned long)keys; - ick.buf_nr = array_size(keys); + ick.buf_ptr = (unsigned long)ikeys; + ick.buf_nr = array_size(ikeys); ick.which = which; for (;;) { @@ -54,7 +55,8 @@ static int item_cache_keys(int argc, char **argv, int which) } for (i = 0; i < ret; i++) { - printf(SK_FMT, SK_ARG(&keys[i])); + scoutfs_key_copy_types(&key, &ikeys[i]); + printf(SK_FMT, SK_ARG(&key)); if (which == SCOUTFS_IOC_ITEM_CACHE_KEYS_ITEMS || (i & 1)) @@ -63,8 +65,8 @@ static int item_cache_keys(int argc, char **argv, int which) printf(" - "); } - ick.key = keys[i - 1]; - scoutfs_key_inc(&ick.key); + scoutfs_key_inc(&key); + scoutfs_key_copy_types(&ick.ikey, &key); } close(fd); diff --git a/utils/src/key.h b/utils/src/key.h index 9401286f..5a1b580e 100644 --- a/utils/src/key.h +++ b/utils/src/key.h @@ -38,6 +38,24 @@ static inline char *sk_type_str(u8 zone, u8 type) le64_to_cpu((key)->_sk_third), \ (key)->_sk_fourth +/* + * copy fields between keys with the same fields but different types. + * The destination type might have internal padding so we zero it. + */ +#define scoutfs_key_copy_types(a, b) \ +do { \ + __typeof__(a) _to = (a); \ + __typeof__(b) _from = (b); \ + \ + memset(_to, 0, sizeof(*_to)); \ + _to->sk_zone = _from->sk_zone; \ + _to->_sk_first = _from->_sk_first; \ + _to->sk_type = _from->sk_type; \ + _to->_sk_second = _from->_sk_second; \ + _to->_sk_third = _from->_sk_third; \ + _to->_sk_fourth = _from->_sk_fourth; \ +} while (0) + static inline void scoutfs_key_set_zeros(struct scoutfs_key *key) { key->sk_zone = 0; diff --git a/utils/src/parse.c b/utils/src/parse.c index 6e01b9d7..302dc762 100644 --- a/utils/src/parse.c +++ b/utils/src/parse.c @@ -45,13 +45,13 @@ int parse_u32(char *str, u32 *val_ret) return 0; } -int parse_timespec(char *str, struct scoutfs_timespec *ts) +int parse_timespec(char *str, struct timespec *ts) { unsigned long long sec; unsigned int nsec; int ret; - memset(ts, 0, sizeof(struct scoutfs_timespec)); + memset(ts, 0, sizeof(struct timespec)); ret = sscanf(str, "%llu.%u", &sec, &nsec); if (ret != 2) { @@ -64,8 +64,8 @@ int parse_timespec(char *str, struct scoutfs_timespec *ts) return -EINVAL; } - ts->sec = cpu_to_le64(sec); - ts->nsec = cpu_to_le32(nsec); + ts->tv_sec = sec; + ts->tv_nsec = nsec; return 0; } diff --git a/utils/src/parse.h b/utils/src/parse.h index e1361853..ad25f879 100644 --- a/utils/src/parse.h +++ b/utils/src/parse.h @@ -1,10 +1,10 @@ #ifndef _PARSE_H_ #define _PARSE_H_ -struct scoutfs_timespec; +#include int parse_u64(char *str, u64 *val_ret); int parse_u32(char *str, u32 *val_ret); -int parse_timespec(char *str, struct scoutfs_timespec *ts); +int parse_timespec(char *str, struct timespec *ts); #endif diff --git a/utils/src/setattr.c b/utils/src/setattr.c index 5c29f1f5..e9ab3b34 100644 --- a/utils/src/setattr.c +++ b/utils/src/setattr.c @@ -29,6 +29,7 @@ static struct option long_ops[] = { static int setattr_more_cmd(int argc, char **argv) { struct scoutfs_ioctl_setattr_more sm; + struct timespec ctime; char *path = NULL; int ret; int fd = -1; @@ -39,7 +40,7 @@ static int setattr_more_cmd(int argc, char **argv) while ((c = getopt_long(argc, argv, "c:d:f:os:", long_ops, NULL)) != -1) { switch (c) { case 'c': - ret = parse_timespec(optarg, &sm.ctime); + ret = parse_timespec(optarg, &ctime); if (ret) goto out; break; @@ -85,6 +86,9 @@ static int setattr_more_cmd(int argc, char **argv) goto out; } + sm.ctime_sec = ctime.tv_sec; + sm.ctime_nsec = ctime.tv_nsec; + ret = ioctl(fd, SCOUTFS_IOC_SETATTR_MORE, &sm); if (ret < 0) { ret = -errno;