[v2] btrfs: assert sizes of ioctl structures
diff mbox series

Message ID 20200714093236.6107-1-johannes.thumshirn@wdc.com
State New
Headers show
Series
  • [v2] btrfs: assert sizes of ioctl structures
Related show

Commit Message

Johannes Thumshirn July 14, 2020, 9:32 a.m. UTC
When expanding ioctl interfaces we want to make sure we're not changing
the size of the structures, otherwise it can lead to incorrect transfers
between kernel and user-space.

Build time assert the size of each structure so we're not running into any
incompatibilities.

Signed-off-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>

---
Changes to v1:
- Stub out static_assert() for user-space (0day Bot)
- Sync asserts with the ones from btrfs-progs (David)
---
 include/uapi/linux/btrfs.h | 36 ++++++++++++++++++++++++++++++++++--
 1 file changed, 34 insertions(+), 2 deletions(-)

Comments

David Sterba July 14, 2020, 12:32 p.m. UTC | #1
On Tue, Jul 14, 2020 at 06:32:36PM +0900, Johannes Thumshirn wrote:
> When expanding ioctl interfaces we want to make sure we're not changing
> the size of the structures, otherwise it can lead to incorrect transfers
> between kernel and user-space.
> 
> Build time assert the size of each structure so we're not running into any
> incompatibilities.
> 
> Signed-off-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>

I've tried 32bit build and the assertion fails for many structures, but
I was expecting only the send one because it contains the pointer.

  CALL    scripts/checksyscalls.sh
  CALL    scripts/atomic/check-atomics.sh
  CC [M]  fs/btrfs/super.o
In file included from ./include/linux/bits.h:22,
                 from ./include/linux/bitops.h:5,
                 from ./include/linux/kernel.h:12,
                 from ./arch/x86/include/asm/percpu.h:45,
                 from ./arch/x86/include/asm/current.h:6,
                 from ./include/linux/sched.h:12,
                 from ./include/linux/blkdev.h:5,
                 from fs/btrfs/super.c:6:
./include/linux/build_bug.h:78:41: error: static assertion failed: "sizeof(struct btrfs_ioctl_dev_replace_start_params) == 2072"
   78 | #define __static_assert(expr, msg, ...) _Static_assert(expr, msg)
      |                                         ^~~~~~~~~~~~~~
./include/linux/build_bug.h:77:34: note: in expansion of macro ‘__static_assert’
   77 | #define static_assert(expr, ...) __static_assert(expr, ##__VA_ARGS__, #expr)
      |                                  ^~~~~~~~~~~~~~~
./include/uapi/linux/btrfs.h:213:1: note: in expansion of macro ‘static_assert’
  213 | static_assert(sizeof(struct btrfs_ioctl_dev_replace_start_params) == 2072);
      | ^~~~~~~~~~~~~
./include/linux/build_bug.h:78:41: error: static assertion failed: "sizeof(struct btrfs_ioctl_dev_replace_args) == 2600"
   78 | #define __static_assert(expr, msg, ...) _Static_assert(expr, msg)
      |                                         ^~~~~~~~~~~~~~
./include/linux/build_bug.h:77:34: note: in expansion of macro ‘__static_assert’
   77 | #define static_assert(expr, ...) __static_assert(expr, ##__VA_ARGS__, #expr)
      |                                  ^~~~~~~~~~~~~~~
./include/uapi/linux/btrfs.h:248:1: note: in expansion of macro ‘static_assert’
  248 | static_assert(sizeof(struct btrfs_ioctl_dev_replace_args) == 2600);
      | ^~~~~~~~~~~~~
./include/linux/build_bug.h:78:41: error: static assertion failed: "sizeof(struct btrfs_ioctl_received_subvol_args) == 200"
   78 | #define __static_assert(expr, msg, ...) _Static_assert(expr, msg)
      |                                         ^~~~~~~~~~~~~~
./include/linux/build_bug.h:77:34: note: in expansion of macro ‘__static_assert’
   77 | #define static_assert(expr, ...) __static_assert(expr, ##__VA_ARGS__, #expr)
      |                                  ^~~~~~~~~~~~~~~
./include/uapi/linux/btrfs.h:781:1: note: in expansion of macro ‘static_assert’
  781 | static_assert(sizeof(struct btrfs_ioctl_received_subvol_args) == 200);
      | ^~~~~~~~~~~~~
./include/linux/build_bug.h:78:41: error: static assertion failed: "sizeof(struct btrfs_ioctl_send_args) == 72"
   78 | #define __static_assert(expr, msg, ...) _Static_assert(expr, msg)
      |                                         ^~~~~~~~~~~~~~
./include/linux/build_bug.h:77:34: note: in expansion of macro ‘__static_assert’
   77 | #define static_assert(expr, ...) __static_assert(expr, ##__VA_ARGS__, #expr)
      |                                  ^~~~~~~~~~~~~~~
./include/uapi/linux/btrfs.h:816:1: note: in expansion of macro ‘static_assert’
  816 | static_assert(sizeof(struct btrfs_ioctl_send_args) == 72);
      | ^~~~~~~~~~~~~
make[2]: *** [scripts/Makefile.build:281: fs/btrfs/super.o] Error 1
make[1]: *** [scripts/Makefile.build:497: fs/btrfs] Error 2
make: *** [Makefile:1756: fs] Error 2
Johannes Thumshirn July 14, 2020, 2:49 p.m. UTC | #2
On 14/07/2020 14:33, David Sterba wrote:
> On Tue, Jul 14, 2020 at 06:32:36PM +0900, Johannes Thumshirn wrote:
>> When expanding ioctl interfaces we want to make sure we're not changing
>> the size of the structures, otherwise it can lead to incorrect transfers
>> between kernel and user-space.
>>
>> Build time assert the size of each structure so we're not running into any
>> incompatibilities.
>>
>> Signed-off-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
> 
> I've tried 32bit build and the assertion fails for many structures, but
> I was expecting only the send one because it contains the pointer.

I wonder if we should have two different asserts for 32 and 64bit for 
these structures or remove the asserts from them.

Having a 32 and 64bit assert will add some ifdeffery, let me see how 
ugly this will get.
David Sterba July 14, 2020, 2:55 p.m. UTC | #3
On Tue, Jul 14, 2020 at 02:49:31PM +0000, Johannes Thumshirn wrote:
> On 14/07/2020 14:33, David Sterba wrote:
> > On Tue, Jul 14, 2020 at 06:32:36PM +0900, Johannes Thumshirn wrote:
> >> When expanding ioctl interfaces we want to make sure we're not changing
> >> the size of the structures, otherwise it can lead to incorrect transfers
> >> between kernel and user-space.
> >>
> >> Build time assert the size of each structure so we're not running into any
> >> incompatibilities.
> >>
> >> Signed-off-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
> > 
> > I've tried 32bit build and the assertion fails for many structures, but
> > I was expecting only the send one because it contains the pointer.
> 
> I wonder if we should have two different asserts for 32 and 64bit for 
> these structures or remove the asserts from them.
> 
> Having a 32 and 64bit assert will add some ifdeffery, let me see how 
> ugly this will get.

Progs do the switch using sizeof(long) and ?: operator but I don't know
if this works with _Static_assert as progs use the struct + bitfield
way.
Johannes Thumshirn July 15, 2020, 7:12 a.m. UTC | #4
On 14/07/2020 16:56, David Sterba wrote:
> On Tue, Jul 14, 2020 at 02:49:31PM +0000, Johannes Thumshirn wrote:
>> On 14/07/2020 14:33, David Sterba wrote:
>>> On Tue, Jul 14, 2020 at 06:32:36PM +0900, Johannes Thumshirn wrote:
>>>> When expanding ioctl interfaces we want to make sure we're not changing
>>>> the size of the structures, otherwise it can lead to incorrect transfers
>>>> between kernel and user-space.
>>>>
>>>> Build time assert the size of each structure so we're not running into any
>>>> incompatibilities.
>>>>
>>>> Signed-off-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
>>>
>>> I've tried 32bit build and the assertion fails for many structures, but
>>> I was expecting only the send one because it contains the pointer.
>>
>> I wonder if we should have two different asserts for 32 and 64bit for 
>> these structures or remove the asserts from them.
>>
>> Having a 32 and 64bit assert will add some ifdeffery, let me see how 
>> ugly this will get.
> 
> Progs do the switch using sizeof(long) and ?: operator but I don't know
> if this works with _Static_assert as progs use the struct + bitfield
> way.
> 

I can try but it's ugly as hell IMHO
David Sterba July 15, 2020, 7:56 a.m. UTC | #5
On Wed, Jul 15, 2020 at 07:12:09AM +0000, Johannes Thumshirn wrote:
> On 14/07/2020 16:56, David Sterba wrote:
> > On Tue, Jul 14, 2020 at 02:49:31PM +0000, Johannes Thumshirn wrote:
> >> On 14/07/2020 14:33, David Sterba wrote:
> >>> On Tue, Jul 14, 2020 at 06:32:36PM +0900, Johannes Thumshirn wrote:
> >>>> When expanding ioctl interfaces we want to make sure we're not changing
> >>>> the size of the structures, otherwise it can lead to incorrect transfers
> >>>> between kernel and user-space.
> >>>>
> >>>> Build time assert the size of each structure so we're not running into any
> >>>> incompatibilities.
> >>>>
> >>>> Signed-off-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
> >>>
> >>> I've tried 32bit build and the assertion fails for many structures, but
> >>> I was expecting only the send one because it contains the pointer.
> >>
> >> I wonder if we should have two different asserts for 32 and 64bit for 
> >> these structures or remove the asserts from them.
> >>
> >> Having a 32 and 64bit assert will add some ifdeffery, let me see how 
> >> ugly this will get.
> > 
> > Progs do the switch using sizeof(long) and ?: operator but I don't know
> > if this works with _Static_assert as progs use the struct + bitfield
> > way.
> > 
> 
> I can try but it's ugly as hell IMHO

Or we can add macros like

ASSERT_STRUCT_SIZE(struct name, 64);

ASSERT_STRUCT_SIZE_32_64(struct name, 32, 64);

and then do the ifdefs at the definition time.

Patch
diff mbox series

diff --git a/include/uapi/linux/btrfs.h b/include/uapi/linux/btrfs.h
index 69b88f6cb57f..dc7205972f1c 100644
--- a/include/uapi/linux/btrfs.h
+++ b/include/uapi/linux/btrfs.h
@@ -22,6 +22,10 @@ 
 #include <linux/types.h>
 #include <linux/ioctl.h>
 
+#ifndef static_assert
+#define static_assert(x)
+#endif
+
 #define BTRFS_IOCTL_MAGIC 0x94
 #define BTRFS_VOL_NAME_MAX 255
 #define BTRFS_LABEL_SIZE 256
@@ -32,6 +36,7 @@  struct btrfs_ioctl_vol_args {
 	__s64 fd;
 	char name[BTRFS_PATH_NAME_MAX + 1];
 };
+static_assert(sizeof(struct btrfs_ioctl_vol_args) == 4096);
 
 #define BTRFS_DEVICE_PATH_NAME_MAX	1024
 #define BTRFS_SUBVOL_NAME_MAX 		4039
@@ -78,6 +83,7 @@  struct btrfs_qgroup_limit {
 	__u64	rsv_rfer;
 	__u64	rsv_excl;
 };
+static_assert(sizeof(struct btrfs_qgroup_limit) == 40);
 
 /*
  * flags definition for qgroup inheritance
@@ -95,11 +101,13 @@  struct btrfs_qgroup_inherit {
 	struct btrfs_qgroup_limit lim;
 	__u64	qgroups[0];
 };
+static_assert(sizeof(struct btrfs_qgroup_inherit) == 72);
 
 struct btrfs_ioctl_qgroup_limit_args {
 	__u64	qgroupid;
 	struct btrfs_qgroup_limit lim;
 };
+static_assert(sizeof(struct btrfs_ioctl_qgroup_limit_args) == 48);
 
 /*
  * Arguments for specification of subvolumes or devices, supporting by-name or
@@ -142,6 +150,7 @@  struct btrfs_ioctl_vol_args_v2 {
 		__u64 subvolid;
 	};
 };
+static_assert(sizeof(struct btrfs_ioctl_vol_args_v2) == 4096);
 
 /*
  * structure to report errors and progress to userspace, either as a
@@ -190,6 +199,7 @@  struct btrfs_ioctl_scrub_args {
 	/* pad to 1k */
 	__u64 unused[(1024-32-sizeof(struct btrfs_scrub_progress))/8];
 };
+static_assert(sizeof(struct 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
@@ -200,6 +210,7 @@  struct btrfs_ioctl_dev_replace_start_params {
 	__u8 srcdev_name[BTRFS_DEVICE_PATH_NAME_MAX + 1];	/* in */
 	__u8 tgtdev_name[BTRFS_DEVICE_PATH_NAME_MAX + 1];	/* in */
 };
+static_assert(sizeof(struct btrfs_ioctl_dev_replace_start_params) == 2072);
 
 #define BTRFS_IOCTL_DEV_REPLACE_STATE_NEVER_STARTED	0
 #define BTRFS_IOCTL_DEV_REPLACE_STATE_STARTED		1
@@ -214,6 +225,7 @@  struct btrfs_ioctl_dev_replace_status_params {
 	__u64 num_write_errors;	/* out */
 	__u64 num_uncorrectable_read_errors;	/* out */
 };
+static_assert(sizeof(struct btrfs_ioctl_dev_replace_status_params) == 48);
 
 #define BTRFS_IOCTL_DEV_REPLACE_CMD_START			0
 #define BTRFS_IOCTL_DEV_REPLACE_CMD_STATUS			1
@@ -233,6 +245,7 @@  struct btrfs_ioctl_dev_replace_args {
 
 	__u64 spare[64];
 };
+static_assert(sizeof(struct btrfs_ioctl_dev_replace_args) == 2600);
 
 struct btrfs_ioctl_dev_info_args {
 	__u64 devid;				/* in/out */
@@ -242,6 +255,7 @@  struct btrfs_ioctl_dev_info_args {
 	__u64 unused[379];			/* pad to 4k */
 	__u8 path[BTRFS_DEVICE_PATH_NAME_MAX];	/* out */
 };
+static_assert(sizeof(struct btrfs_ioctl_dev_info_args) == 4096);
 
 /*
  * Retrieve information about the filesystem
@@ -270,7 +284,7 @@  struct btrfs_ioctl_fs_info_args {
 	__u8 metadata_uuid[BTRFS_FSID_SIZE];	/* out */
 	__u8 reserved[944];			/* pad to 1k */
 };
-
+static_assert(sizeof(struct btrfs_ioctl_fs_info_args) == 1024);
 
 /*
  * feature flags
@@ -314,6 +328,7 @@  struct btrfs_ioctl_feature_flags {
 	__u64 compat_ro_flags;
 	__u64 incompat_flags;
 };
+static_assert(sizeof(struct btrfs_ioctl_feature_flags) == 24);
 
 /* balance control ioctl modes */
 #define BTRFS_BALANCE_CTL_PAUSE		1
@@ -453,6 +468,7 @@  struct btrfs_ioctl_balance_args {
 
 	__u64 unused[72];			/* pad to 1k */
 };
+static_assert(sizeof(struct btrfs_ioctl_balance_args) == 1024);
 
 #define BTRFS_INO_LOOKUP_PATH_MAX 4080
 struct btrfs_ioctl_ino_lookup_args {
@@ -460,6 +476,7 @@  struct btrfs_ioctl_ino_lookup_args {
 	__u64 objectid;
 	char name[BTRFS_INO_LOOKUP_PATH_MAX];
 };
+static_assert(sizeof(struct 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 {
@@ -475,6 +492,7 @@  struct btrfs_ioctl_ino_lookup_user_args {
 	 */
 	char path[BTRFS_INO_LOOKUP_USER_PATH_MAX];
 };
+static_assert(sizeof(struct btrfs_ioctl_ino_lookup_user_args) == 4096);
 
 /* Search criteria for the btrfs SEARCH ioctl family. */
 struct btrfs_ioctl_search_key {
@@ -553,6 +571,7 @@  struct btrfs_ioctl_search_args {
 	struct btrfs_ioctl_search_key key;
 	char buf[BTRFS_SEARCH_ARGS_BUFSIZE];
 };
+static_assert(sizeof(struct btrfs_ioctl_search_args) == 4096);
 
 struct btrfs_ioctl_search_args_v2 {
 	struct btrfs_ioctl_search_key key; /* in/out - search parameters */
@@ -561,12 +580,14 @@  struct btrfs_ioctl_search_args_v2 {
 					    *       to store item */
 	__u64 buf[0];                       /* out - found items */
 };
+static_assert(sizeof(struct btrfs_ioctl_search_args_v2) == 112);
 
 struct btrfs_ioctl_clone_range_args {
   __s64 src_fd;
   __u64 src_offset, src_length;
   __u64 dest_offset;
 };
+static_assert(sizeof(struct btrfs_ioctl_clone_range_args) == 32);
 
 /*
  * flags definition for the defrag range ioctl
@@ -606,7 +627,7 @@  struct btrfs_ioctl_defrag_range_args {
 	/* spare for later */
 	__u32 unused[4];
 };
-
+static_assert(sizeof(struct btrfs_ioctl_defrag_range_args) == 48);
 
 #define BTRFS_SAME_DATA_DIFFERS	1
 /* For extent-same ioctl */
@@ -632,6 +653,7 @@  struct btrfs_ioctl_same_args {
 	__u32 reserved2;
 	struct btrfs_ioctl_same_extent_info info[0];
 };
+static_assert(sizeof(struct btrfs_ioctl_same_args) == 24);
 
 struct btrfs_ioctl_space_info {
 	__u64 flags;
@@ -644,6 +666,7 @@  struct btrfs_ioctl_space_args {
 	__u64 total_spaces;
 	struct btrfs_ioctl_space_info spaces[0];
 };
+static_assert(sizeof(struct btrfs_ioctl_space_args) == 16);
 
 struct btrfs_data_container {
 	__u32	bytes_left;	/* out -- bytes not needed to deliver output */
@@ -660,6 +683,7 @@  struct btrfs_ioctl_ino_path_args {
 	/* struct btrfs_data_container	*fspath;	   out */
 	__u64				fspath;		/* out */
 };
+static_assert(sizeof(struct btrfs_ioctl_ino_path_args) == 56);
 
 struct btrfs_ioctl_logical_ino_args {
 	__u64				logical;	/* in */
@@ -710,6 +734,7 @@  struct btrfs_ioctl_get_dev_stats {
 	 */
 	__u64 unused[128 - 2 - BTRFS_DEV_STAT_VALUES_MAX];
 };
+static_assert(sizeof(struct btrfs_ioctl_get_dev_stats) == 1032);
 
 #define BTRFS_QUOTA_CTL_ENABLE	1
 #define BTRFS_QUOTA_CTL_DISABLE	2
@@ -718,12 +743,14 @@  struct btrfs_ioctl_quota_ctl_args {
 	__u64 cmd;
 	__u64 status;
 };
+static_assert(sizeof(struct btrfs_ioctl_quota_ctl_args) == 16);
 
 struct btrfs_ioctl_quota_rescan_args {
 	__u64	flags;
 	__u64   progress;
 	__u64   reserved[6];
 };
+static_assert(sizeof(struct btrfs_ioctl_quota_rescan_args) == 64);
 
 struct btrfs_ioctl_qgroup_assign_args {
 	__u64 assign;
@@ -735,6 +762,8 @@  struct btrfs_ioctl_qgroup_create_args {
 	__u64 create;
 	__u64 qgroupid;
 };
+static_assert(sizeof(struct btrfs_ioctl_qgroup_create_args) == 16);
+
 struct btrfs_ioctl_timespec {
 	__u64 sec;
 	__u32 nsec;
@@ -749,6 +778,7 @@  struct btrfs_ioctl_received_subvol_args {
 	__u64	flags;			/* in */
 	__u64	reserved[16];		/* in */
 };
+static_assert(sizeof(struct btrfs_ioctl_received_subvol_args) == 200);
 
 /*
  * Caller doesn't want file data in the send stream, even if the
@@ -783,6 +813,7 @@  struct btrfs_ioctl_send_args {
 	__u64 flags;			/* in */
 	__u64 reserved[4];		/* in */
 };
+static_assert(sizeof(struct btrfs_ioctl_send_args) == 72);
 
 /*
  * Information about a fs tree root.
@@ -859,6 +890,7 @@  struct btrfs_ioctl_get_subvol_rootref_args {
 		__u8 num_items;
 		__u8 align[7];
 };
+static_assert(sizeof(struct btrfs_ioctl_get_subvol_rootref_args) == 4096);
 
 /* Error codes as returned by the kernel */
 enum btrfs_err_code {