btrfs: Introduce compile time structure size check

PATCH In review Qu Wenruo 5 participants 9 messages Jul 12, 2018 – Jul 13, 2018 Btrfs file system

lore

Share & tools

lore

Qu Wenruo

author

2018-07-12 06:27 UTC #0 lore raw

PATCH 1/1 btrfs: Introduce compile time structure size check

Introduce a new macro based compile time check for ioctl structures.

The new macro is BTRFS_ASSERT_SIZE(), which is mostly copied from VMMDEV_ASSERT_SIZE().

Such check is only added to structure pended to power of 2. And exposed one structure, btrfs_ioctl_get_dev_stats() is not aligned well.
The misalign is introduced by commit b27f7c0c150f ("btrfs: join DEV_STATS ioctls to one").

Signed-off-by Qu Wenruo <[email protected]>

---
 include/uapi/linux/btrfs.h | 18 +++++++++++++++++-
 1 file changed, 17 insertions(+), 1 deletion(-)

include/uapi/linux/btrfs.h +17 -1

index 5ca1d21fc4a7..be1213c10080 100644
@@ -22,6 +22,14 @@
 #include <linux/types.h>
 #include <linux/ioctl.h>
 
+/*
+ * Compile time check on structure size, same method as
+ * VMMDEV_ASSERT_SIZE() from linux/vbox_vmmdev_types.h, to trigger a negative
+ * array size at compile time if size doesn't match.
+ */
+#define BTRFS_ASSERT_SIZE(type, size) \
+	typedef char type ## _asrt_size[1 - 2 * !!(sizeof(struct type) != (size))]
+
 #define BTRFS_IOCTL_MAGIC 0x94
 #define BTRFS_VOL_NAME_MAX 255
 #define BTRFS_LABEL_SIZE 256

@@ -32,6 +40,7 @@ struct btrfs_ioctl_vol_args {
 	__s64 fd;
 	char name[BTRFS_PATH_NAME_MAX + 1];
 };
+BTRFS_ASSERT_SIZE(btrfs_ioctl_vol_args, 4096);
 
 #define BTRFS_DEVICE_PATH_NAME_MAX	1024
 #define BTRFS_SUBVOL_NAME_MAX 		4039

@@ -171,6 +180,7 @@ struct btrfs_ioctl_scrub_args {
 	/* pad to 1k */
 	__u64 unused[(1024-32-sizeof(struct btrfs_scrub_progress))/8];
 };
+BTRFS_ASSERT_SIZE(btrfs_ioctl_scrub_args, 1024);
 
 #define BTRFS_IOCTL_DEV_REPLACE_CONT_READING_FROM_SRCDEV_MODE_ALWAYS	0
 #define BTRFS_IOCTL_DEV_REPLACE_CONT_READING_FROM_SRCDEV_MODE_AVOID	1

@@ -223,6 +233,7 @@ struct btrfs_ioctl_dev_info_args {
 	__u64 unused[379];			/* pad to 4k */
 	__u8 path[BTRFS_DEVICE_PATH_NAME_MAX];	/* out */
 };
+BTRFS_ASSERT_SIZE(btrfs_ioctl_dev_info_args, 4096);
 
 struct btrfs_ioctl_fs_info_args {
 	__u64 max_id;				/* out */

@@ -234,6 +245,7 @@ struct btrfs_ioctl_fs_info_args {
 	__u32 reserved32;
 	__u64 reserved[122];			/* pad to 1k */
 };
+BTRFS_ASSERT_SIZE(btrfs_ioctl_fs_info_args, 1024);
 
 /*
  * feature flags

@@ -414,6 +426,7 @@ struct btrfs_ioctl_balance_args {
 
 	__u64 unused[72];			/* pad to 1k */
 };
+BTRFS_ASSERT_SIZE(btrfs_ioctl_balance_args, 1024);
 
 #define BTRFS_INO_LOOKUP_PATH_MAX 4080
 struct btrfs_ioctl_ino_lookup_args {

@@ -421,6 +434,7 @@ struct btrfs_ioctl_ino_lookup_args {
 	__u64 objectid;
 	char name[BTRFS_INO_LOOKUP_PATH_MAX];
 };
+BTRFS_ASSERT_SIZE(btrfs_ioctl_ino_lookup_args, 4096);
 
 #define BTRFS_INO_LOOKUP_USER_PATH_MAX (4080 - BTRFS_VOL_NAME_MAX - 1)
 struct btrfs_ioctl_ino_lookup_user_args {

@@ -436,6 +450,7 @@ struct btrfs_ioctl_ino_lookup_user_args {
 	 */
 	char path[BTRFS_INO_LOOKUP_USER_PATH_MAX];
 };
+BTRFS_ASSERT_SIZE(btrfs_ioctl_ino_lookup_args, 4096);
 
 /* Search criteria for the btrfs SEARCH ioctl family. */
 struct btrfs_ioctl_search_key {

@@ -664,8 +679,9 @@ struct btrfs_ioctl_get_dev_stats {
 	/* out values: */
 	__u64 values[BTRFS_DEV_STAT_VALUES_MAX];
 
-	__u64 unused[128 - 2 - BTRFS_DEV_STAT_VALUES_MAX]; /* pad to 1k */
+	__u64 unused[128 - 3 - BTRFS_DEV_STAT_VALUES_MAX]; /* pad to 1k */
 };
+BTRFS_ASSERT_SIZE(btrfs_ioctl_get_dev_stats, 1024);
 
 #define BTRFS_QUOTA_CTL_ENABLE	1
 #define BTRFS_QUOTA_CTL_DISABLE	2

Signature · 1 line

2.18.0

Nikolay Borisov

2018-07-12 06:36 UTC #1 lore raw

Quoted 10 lines · Qu Wenruo

Introduce a new macro based compile time check for ioctl structures.
The new macro is BTRFS_ASSERT_SIZE(), which is mostly copied from VMMDEV_ASSERT_SIZE().
Such check is only added to structure pended to power of 2. And exposed one structure, btrfs_ioctl_get_dev_stats() is not aligned well.
The misalign is introduced by commit b27f7c0c150f ("btrfs: join DEV_STATS ioctls to one").

So what's the negative effect if a structure is not aligned to a power of 2?

Quoted 93 lines

Signed-off-by Qu Wenruo <[email protected]>

---
 include/uapi/linux/btrfs.h | 18 +++++++++++++++++-
 1 file changed, 17 insertions(+), 1 deletion(-)

include/uapi/linux/btrfs.h +17 -1

index 5ca1d21fc4a7..be1213c10080 100644
@@ -22,6 +22,14 @@
 #include <linux/types.h>
 #include <linux/ioctl.h>
 
+/*
+ * Compile time check on structure size, same method as
+ * VMMDEV_ASSERT_SIZE() from linux/vbox_vmmdev_types.h, to trigger a negative
+ * array size at compile time if size doesn't match.
+ */
+#define BTRFS_ASSERT_SIZE(type, size) \
+	typedef char type ## _asrt_size[1 - 2 * !!(sizeof(struct type) != (size))]
+
 #define BTRFS_IOCTL_MAGIC 0x94
 #define BTRFS_VOL_NAME_MAX 255
 #define BTRFS_LABEL_SIZE 256

@@ -32,6 +40,7 @@ struct btrfs_ioctl_vol_args {
 	__s64 fd;
 	char name[BTRFS_PATH_NAME_MAX + 1];
 };
+BTRFS_ASSERT_SIZE(btrfs_ioctl_vol_args, 4096);
 
 #define BTRFS_DEVICE_PATH_NAME_MAX	1024
 #define BTRFS_SUBVOL_NAME_MAX 		4039

@@ -171,6 +180,7 @@ struct btrfs_ioctl_scrub_args {
 	/* pad to 1k */
 	__u64 unused[(1024-32-sizeof(struct btrfs_scrub_progress))/8];
 };
+BTRFS_ASSERT_SIZE(btrfs_ioctl_scrub_args, 1024);
 
 #define BTRFS_IOCTL_DEV_REPLACE_CONT_READING_FROM_SRCDEV_MODE_ALWAYS	0
 #define BTRFS_IOCTL_DEV_REPLACE_CONT_READING_FROM_SRCDEV_MODE_AVOID	1

@@ -223,6 +233,7 @@ struct btrfs_ioctl_dev_info_args {
 	__u64 unused[379];			/* pad to 4k */
 	__u8 path[BTRFS_DEVICE_PATH_NAME_MAX];	/* out */
 };
+BTRFS_ASSERT_SIZE(btrfs_ioctl_dev_info_args, 4096);
 
 struct btrfs_ioctl_fs_info_args {
 	__u64 max_id;				/* out */

@@ -234,6 +245,7 @@ struct btrfs_ioctl_fs_info_args {
 	__u32 reserved32;
 	__u64 reserved[122];			/* pad to 1k */
 };
+BTRFS_ASSERT_SIZE(btrfs_ioctl_fs_info_args, 1024);
 
 /*
  * feature flags

@@ -414,6 +426,7 @@ struct btrfs_ioctl_balance_args {
 
 	__u64 unused[72];			/* pad to 1k */
 };
+BTRFS_ASSERT_SIZE(btrfs_ioctl_balance_args, 1024);
 
 #define BTRFS_INO_LOOKUP_PATH_MAX 4080
 struct btrfs_ioctl_ino_lookup_args {

@@ -421,6 +434,7 @@ struct btrfs_ioctl_ino_lookup_args {
 	__u64 objectid;
 	char name[BTRFS_INO_LOOKUP_PATH_MAX];
 };
+BTRFS_ASSERT_SIZE(btrfs_ioctl_ino_lookup_args, 4096);
 
 #define BTRFS_INO_LOOKUP_USER_PATH_MAX (4080 - BTRFS_VOL_NAME_MAX - 1)
 struct btrfs_ioctl_ino_lookup_user_args {

@@ -436,6 +450,7 @@ struct btrfs_ioctl_ino_lookup_user_args {
 	 */
 	char path[BTRFS_INO_LOOKUP_USER_PATH_MAX];
 };
+BTRFS_ASSERT_SIZE(btrfs_ioctl_ino_lookup_args, 4096);
 
 /* Search criteria for the btrfs SEARCH ioctl family. */
 struct btrfs_ioctl_search_key {

@@ -664,8 +679,9 @@ struct btrfs_ioctl_get_dev_stats {
 	/* out values: */
 	__u64 values[BTRFS_DEV_STAT_VALUES_MAX];
 
-	__u64 unused[128 - 2 - BTRFS_DEV_STAT_VALUES_MAX]; /* pad to 1k */
+	__u64 unused[128 - 3 - BTRFS_DEV_STAT_VALUES_MAX]; /* pad to 1k */
 };
+BTRFS_ASSERT_SIZE(btrfs_ioctl_get_dev_stats, 1024);
 
 #define BTRFS_QUOTA_CTL_ENABLE	1
 #define BTRFS_QUOTA_CTL_DISABLE	2

Qu Wenruo

2018-07-12 07:05 UTC #2 lore raw

Quoted 8 lines · Nikolay Borisov

Quoted 5 lines · Qu Wenruo
Introduce a new macro based compile time check for ioctl structures.
The new macro is BTRFS_ASSERT_SIZE(), which is mostly copied from VMMDEV_ASSERT_SIZE().

Quoted 7 lines

Quoted 5 lines
Such check is only added to structure pended to power of 2. And exposed one structure, btrfs_ioctl_get_dev_stats() is not aligned well.
The misalign is introduced by commit b27f7c0c150f ("btrfs: join DEV_STATS ioctls to one").
So what's the negative effect if a structure is not aligned to a power of 2?

No real negative effect, until one day one may find the comment is not correct.

Thanks,
Qu

Quoted 99 lines

Quoted 84 lines

Signed-off-by Qu Wenruo <[email protected]>

---
 include/uapi/linux/btrfs.h | 18 +++++++++++++++++-
 1 file changed, 17 insertions(+), 1 deletion(-)

include/uapi/linux/btrfs.h +15 -0

index 5ca1d21fc4a7..be1213c10080 100644
@@ -22,6 +22,14 @@
 #include <linux/types.h>
 #include <linux/ioctl.h>
 
+/*
+ * Compile time check on structure size, same method as
+ * VMMDEV_ASSERT_SIZE() from linux/vbox_vmmdev_types.h, to trigger a negative
+ * array size at compile time if size doesn't match.
+ */
+#define BTRFS_ASSERT_SIZE(type, size) \
+	typedef char type ## _asrt_size[1 - 2 * !!(sizeof(struct type) != (size))]
+
 #define BTRFS_IOCTL_MAGIC 0x94
 #define BTRFS_VOL_NAME_MAX 255
 #define BTRFS_LABEL_SIZE 256

@@ -32,6 +40,7 @@ struct btrfs_ioctl_vol_args {
 	__s64 fd;
 	char name[BTRFS_PATH_NAME_MAX + 1];
 };
+BTRFS_ASSERT_SIZE(btrfs_ioctl_vol_args, 4096);
 
 #define BTRFS_DEVICE_PATH_NAME_MAX	1024
 #define BTRFS_SUBVOL_NAME_MAX 		4039

@@ -171,6 +180,7 @@ struct btrfs_ioctl_scrub_args {
 	/* pad to 1k */
 	__u64 unused[(1024-32-sizeof(struct btrfs_scrub_progress))/8];
 };
+BTRFS_ASSERT_SIZE(btrfs_ioctl_scrub_args, 1024);
 
 #define BTRFS_IOCTL_DEV_REPLACE_CONT_READING_FROM_SRCDEV_MODE_ALWAYS	0
 #define BTRFS_IOCTL_DEV_REPLACE_CONT_READING_FROM_SRCDEV_MODE_AVOID	1

@@ -223,6 +233,7 @@ struct btrfs_ioctl_dev_info_args {
 	__u64 unused[379];			/* pad to 4k */
 	__u8 path[BTRFS_DEVICE_PATH_NAME_MAX];	/* out */
 };
+BTRFS_ASSERT_SIZE(btrfs_ioctl_dev_info_args, 4096);
 
 struct btrfs_ioctl_fs_info_args {
 	__u64 max_id;				/* out */

@@ -234,6 +245,7 @@ struct btrfs_ioctl_fs_info_args {
 	__u32 reserved32;
 	__u64 reserved[122];			/* pad to 1k */
 };
+BTRFS_ASSERT_SIZE(btrfs_ioctl_fs_info_args, 1024);
 
 /*
  * feature flags

@@ -414,6 +426,7 @@ struct btrfs_ioctl_balance_args {
 
 	__u64 unused[72];			/* pad to 1k */
 };
+BTRFS_ASSERT_SIZE(btrfs_ioctl_balance_args, 1024);
 
 #define BTRFS_INO_LOOKUP_PATH_MAX 4080
 struct btrfs_ioctl_ino_lookup_args {

@@ -421,6 +434,7 @@ struct btrfs_ioctl_ino_lookup_args {
 	__u64 objectid;
 	char name[BTRFS_INO_LOOKUP_PATH_MAX];
 };
+BTRFS_ASSERT_SIZE(btrfs_ioctl_ino_lookup_args, 4096);
 
 #define BTRFS_INO_LOOKUP_USER_PATH_MAX (4080 - BTRFS_VOL_NAME_MAX - 1)
 struct btrfs_ioctl_ino_lookup_user_args {

@@ -436,6 +450,7 @@ struct btrfs_ioctl_ino_lookup_user_args {
 	 */
 	char path[BTRFS_INO_LOOKUP_USER_PATH_MAX];
 };
+BTRFS_ASSERT_SIZE(btrfs_ioctl_ino_lookup_args, 4096);
 
 /* Search criteria for the btrfs SEARCH ioctl family. */
 struct btrfs_ioctl_search_key {

include/uapi/linux/btrfs.h struct btrfs_ioctl_get_dev_stats { · line 687

Quoted 10 lines

include/uapi/linux/btrfs.h +2 -1

@@ -664,8 +679,9 @@ struct btrfs_ioctl_get_dev_stats {
 	/* out values: */
 	__u64 values[BTRFS_DEV_STAT_VALUES_MAX];
 
-	__u64 unused[128 - 2 - BTRFS_DEV_STAT_VALUES_MAX]; /* pad to 1k */
+	__u64 unused[128 - 3 - BTRFS_DEV_STAT_VALUES_MAX]; /* pad to 1k */
 };
+BTRFS_ASSERT_SIZE(btrfs_ioctl_get_dev_stats, 1024);
 
 #define BTRFS_QUOTA_CTL_ENABLE	1
 #define BTRFS_QUOTA_CTL_DISABLE	2

--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

David Sterba

2018-07-13 14:49 UTC #3 lore raw

Quoted 17 lines · Qu Wenruo

Quoted 14 lines · Nikolay Borisov
Quoted 10 lines · Qu Wenruo
Introduce a new macro based compile time check for ioctl structures.
The new macro is BTRFS_ASSERT_SIZE(), which is mostly copied from VMMDEV_ASSERT_SIZE().
Such check is only added to structure pended to power of 2. And exposed one structure, btrfs_ioctl_get_dev_stats() is not aligned well.
The misalign is introduced by commit b27f7c0c150f ("btrfs: join DEV_STATS ioctls to one").

Quoted 4 lines

Quoted 1 line
So what's the negative effect if a structure is not aligned to a power of 2?
No real negative effect, until one day one may find the comment is not correct.

So the comment needs to be fixed.

David Sterba

2018-07-13 14:49 UTC #4 lore raw

In reply to Qu Wenruo

Quoted 4 lines · Qu Wenruo

Introduce a new macro based compile time check for ioctl structures.
The new macro is BTRFS_ASSERT_SIZE(), which is mostly copied from VMMDEV_ASSERT_SIZE().

The macro should be generic, there's nothing specific to btrfs. There's a similar one in the progs.

Quoted 1 line

Such check is only added to structure pended to power of 2.

There's no constraint about power of 2 sizes, it works for any size.

Quoted 4 lines

And exposed one structure, btrfs_ioctl_get_dev_stats() is not aligned well.
The misalign is introduced by commit b27f7c0c150f ("btrfs: join DEV_STATS ioctls to one").

Yeah, that was an oversight, so the correct value to check against is 1024 + 8 = 1032, see ioctl.h in progs.

Qu Wenruo

2018-07-13 14:51 UTC #5 lore raw

Quoted 6 lines · David Sterba

Quoted 4 lines · Qu Wenruo
Introduce a new macro based compile time check for ioctl structures.
The new macro is BTRFS_ASSERT_SIZE(), which is mostly copied from VMMDEV_ASSERT_SIZE().

Quoted 6 lines

The macro should be generic, there's nothing specific to btrfs. There's a similar one in the progs.
Quoted 1 line
Such check is only added to structure pended to power of 2.
There's no constraint about power of 2 sizes, it works for any size.

While some structure doesn't do padding at all, and it looks like they may get expanded later.
Do we really need to limit the size right now?

Quoted 8 lines

Quoted 5 lines
And exposed one structure, btrfs_ioctl_get_dev_stats() is not aligned well.
The misalign is introduced by commit b27f7c0c150f ("btrfs: join DEV_STATS ioctls to one").
Yeah, that was an oversight, so the correct value to check against is 1024 + 8 = 1032, see ioctl.h in progs.

Can't we just revert to 1024?
That plus 8 doesn't really look well, and shrink the size shouldn't bring any problem AFAIK.

Thanks,
Qu

Quoted 1 line

David Sterba

2018-07-13 15:01 UTC #6 lore raw

Quoted 12 lines · Qu Wenruo

Quoted 9 lines · David Sterba
Quoted 4 lines · Qu Wenruo
Introduce a new macro based compile time check for ioctl structures.
The new macro is BTRFS_ASSERT_SIZE(), which is mostly copied from VMMDEV_ASSERT_SIZE().
The macro should be generic, there's nothing specific to btrfs. There's a similar one in the progs.

Quoted 7 lines

Quoted 3 lines
Quoted 1 line
Such check is only added to structure pended to power of 2.
There's no constraint about power of 2 sizes, it works for any size.
While some structure doesn't do padding at all, and it looks like they may get expanded later.
Do we really need to limit the size right now?

The size of an ioctl structure is part of kernel<->userspace API and must not change once public. The ioctl number is calculated using the structure size.

Quoted 9 lines

Quoted 7 lines
Quoted 4 lines
And exposed one structure, btrfs_ioctl_get_dev_stats() is not aligned well.
The misalign is introduced by commit b27f7c0c150f ("btrfs: join DEV_STATS ioctls to one").
Yeah, that was an oversight, so the correct value to check against is 1024 + 8 = 1032, see ioctl.h in progs.
Can't we just revert to 1024?

Answered by the above, we absolutely cannot change the numbers now.

Quoted 2 lines

That plus 8 doesn't really look well, and shrink the size shouldn't bring any problem AFAIK.

See

https://elixir.bootlin.com/linux/latest/source/include/uapi/asm-generic/ioctl.h#L88

for _IOWR definition,

btrfs-progs:ioctl.h

817 #define BTRFS_IOC_GET_DEV_STATS _IOWR(BTRFS_IOCTL_MAGIC, 52, \
818 struct btrfs_ioctl_get_dev_stats)

kernel:fs/btrfs/ioctl.c

5914 case BTRFS_IOC_GET_DEV_STATS:
5915 return btrfs_ioctl_get_dev_stats(fs_info, argp);

The values must match, otherwise dev stats will randomly break on old/new kernel and progs combinations.

Qu Wenruo

2018-07-13 15:12 UTC #7 lore raw

Quoted 51 lines · David Sterba

Quoted 12 lines · Qu Wenruo
Quoted 9 lines · David Sterba
Quoted 4 lines · Qu Wenruo
Introduce a new macro based compile time check for ioctl structures.
The new macro is BTRFS_ASSERT_SIZE(), which is mostly copied from VMMDEV_ASSERT_SIZE().
The macro should be generic, there's nothing specific to btrfs. There's a similar one in the progs.
Quoted 7 lines
Quoted 3 lines
Quoted 1 line
Such check is only added to structure pended to power of 2.
There's no constraint about power of 2 sizes, it works for any size.
While some structure doesn't do padding at all, and it looks like they may get expanded later.
Do we really need to limit the size right now?
The size of an ioctl structure is part of kernel<->userspace API and must not change once public. The ioctl number is calculated using the structure size.
Quoted 9 lines
Quoted 7 lines
Quoted 4 lines
And exposed one structure, btrfs_ioctl_get_dev_stats() is not aligned well.
The misalign is introduced by commit b27f7c0c150f ("btrfs: join DEV_STATS ioctls to one").
Yeah, that was an oversight, so the correct value to check against is 1024 + 8 =032, see ioctl.h in progs.
Can't we just revert to 1024?
Answered by the above, we absolutely cannot change the numbers now.
Quoted 2 lines
That plus 8 doesn't really look well, and shrink the size shouldn't bring any problem AFAIK.
See
https://elixir.bootlin.com/linux/latest/source/include/uapi/asm-generic/ioctl.h#L88
for _IOWR definition,
btrfs-progs:ioctl.h
817 #define BTRFS_IOC_GET_DEV_STATS _IOWR(BTRFS_IOCTL_MAGIC, 52, \
818 struct btrfs_ioctl_get_dev_stats)

Quoted 7 lines

kernel:fs/btrfs/ioctl.c
5914 case BTRFS_IOC_GET_DEV_STATS:
5915 return btrfs_ioctl_get_dev_stats(fs_info, argp);
The values must match, otherwise dev stats will randomly break on old/new kernel and progs combinations.

Ok, for old kernel and new progs, if that 1024 bytes from progs is allocated from stack we indeed could screw up user space stack memory.

While for old progs and new kernel it won't cause any problem though.

I'll keep the ugly unaligned number in next version.

But really, we should have such alignment check way before we find the mismatch.

Thanks,
Qu

Quoted 5 lines

--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

David Sterba

2018-07-13 15:40 UTC #8 lore raw

Quoted 56 lines · Qu Wenruo

Quoted 53 lines · David Sterba
Quoted 12 lines · Qu Wenruo
Quoted 9 lines · David Sterba
Quoted 4 lines · Qu Wenruo
Introduce a new macro based compile time check for ioctl structures.
The new macro is BTRFS_ASSERT_SIZE(), which is mostly copied from VMMDEV_ASSERT_SIZE().
The macro should be generic, there's nothing specific to btrfs. There's a similar one in the progs.
Quoted 7 lines
Quoted 3 lines
Quoted 1 line
Such check is only added to structure pended to power of 2.
There's no constraint about power of 2 sizes, it works for any size.
While some structure doesn't do padding at all, and it looks like they may get expanded later.
Do we really need to limit the size right now?
The size of an ioctl structure is part of kernel<->userspace API and must not change once public. The ioctl number is calculated using the structure size.
Quoted 9 lines
Quoted 7 lines
Quoted 4 lines
And exposed one structure, btrfs_ioctl_get_dev_stats() is not aligned well.
The misalign is introduced by commit b27f7c0c150f ("btrfs: join DEV_STATS ioctls to one").
Yeah, that was an oversight, so the correct value to check against is 1024 + 8 =032, see ioctl.h in progs.
Can't we just revert to 1024?
Answered by the above, we absolutely cannot change the numbers now.
Quoted 2 lines
That plus 8 doesn't really look well, and shrink the size shouldn't bring any problem AFAIK.
See
https://elixir.bootlin.com/linux/latest/source/include/uapi/asm-generic/ioctl.h#L88
for _IOWR definition,
btrfs-progs:ioctl.h
817 #define BTRFS_IOC_GET_DEV_STATS _IOWR(BTRFS_IOCTL_MAGIC, 52, \
818 struct btrfs_ioctl_get_dev_stats)
kernel:fs/btrfs/ioctl.c

Quoted 8 lines

Quoted 5 lines
5914 case BTRFS_IOC_GET_DEV_STATS:
5915 return btrfs_ioctl_get_dev_stats(fs_info, argp);
The values must match, otherwise dev stats will randomly break on old/new kernel and progs combinations.
Ok, for old kernel and new progs, if that 1024 bytes from progs is allocated from stack we indeed could screw up user space stack memory.

The main concern is the ioctl number, but the structure size must match too of course as it's copied back from kernel.

But the ioctl number is the interface, so the userspace calls ioctl(NUMBER, ...) that krenel answers by looing up the right handler for the NUMBER.

If there are two numbers, then patched userspace will get only ENOTTY which translates to 'no such ioctl'.

Quoted 6 lines

While for old progs and new kernel it won't cause any problem though.
I'll keep the ugly unaligned number in next version.
But really, we should have such alignment check way before we find the mismatch.

This unfortunatelly escaped during the review, now we have to live with that but besides that it's not a power of two (which would be arbitrary as well), I don't se any problem.

We have a worse problem with the send ioctl structure when there's a pointer that's of different size on 32bit and 64bit so there needs to be stub ioctls and structures so all the combinations work. Interfaces are hard.