scoutfs-utils: naturally align ioctl structs

Use natuturally aligned and explicitly padded ioctl structs.

Signed-off-by: Zach Brown <zab@versity.com>

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)

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

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;

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;
 }

utils/src/parse.h

@@ -1,10 +1,10 @@
 #ifndef _PARSE_H_
 #define _PARSE_H_
 
-struct scoutfs_timespec;
+#include <sys/time.h>
 
 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

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;