| Message ID | 20200714093236.6107-1-johannes.thumshirn@wdc.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] btrfs: assert sizes of ioctl structures | expand |
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
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.
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.
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
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.
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 {
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(-)