Message ID | 20200713122901.1773-5-johannes.thumshirn@wdc.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | Two furhter additions for fsinfo ioctl | expand |
On Mon, Jul 13, 2020 at 09:29:01PM +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> The structure size is ABI once it's part of the ioctl defintion, ie. in any of the _IOWR/_IOR/_IOW macros. I don't see the assert added for btrfs_ioctl_vol_args_v2 or btrfs_ioctl_quota_rescan_args and I haven't checked all. Can you please add the static asserts for all of them? The file ioctl.h in progs should have that already so you can copy or cross-verify from there. I'll merge the patches 1-3 now.
On 13/07/2020 16:58, David Sterba wrote: > On Mon, Jul 13, 2020 at 09:29:01PM +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> > > The structure size is ABI once it's part of the ioctl defintion, ie. in > any of the _IOWR/_IOR/_IOW macros. I don't see the assert added for > btrfs_ioctl_vol_args_v2 or btrfs_ioctl_quota_rescan_args and I haven't > checked all. > > Can you please add the static asserts for all of them? The file ioctl.h > in progs should have that already so you can copy or cross-verify from > there. OK will do. > I'll merge the patches 1-3 now. >
Hi Johannes, I love your patch! Yet something to improve: [auto build test ERROR on v5.8-rc5] [cannot apply to kdave/for-next btrfs/next next-20200713] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use as documented in https://git-scm.com/docs/git-format-patch] url: https://github.com/0day-ci/linux/commits/Johannes-Thumshirn/Two-furhter-additions-for-fsinfo-ioctl/20200713-203321 base: 11ba468877bb23f28956a35e896356252d63c983 config: x86_64-randconfig-a016-20200713 (attached as .config) compiler: clang version 11.0.0 (https://github.com/llvm/llvm-project 02946de3802d3bc65bc9f2eb9b8d4969b5a7add8) reproduce (this is a W=1 build): wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross # install x86_64 cross compiling tool for clang build # apt-get install binutils-x86-64-linux-gnu # save the attached .config to linux build tree COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=x86_64 If you fix the issue, kindly add following tag as appropriate Reported-by: kernel test robot <lkp@intel.com> All error/warnings (new ones prefixed by >>): In file included from <built-in>:1: In file included from ./usr/include/linux/btrfs_tree.h:5: >> ./usr/include/linux/btrfs.h:35:15: error: expected parameter declarator static_assert(sizeof(struct btrfs_ioctl_vol_args) == 4096); ^ >> ./usr/include/linux/btrfs.h:35:15: error: expected ')' ./usr/include/linux/btrfs.h:35:14: note: to match this '(' static_assert(sizeof(struct btrfs_ioctl_vol_args) == 4096); ^ >> ./usr/include/linux/btrfs.h:35:1: warning: declaration specifier missing, defaulting to 'int' static_assert(sizeof(struct btrfs_ioctl_vol_args) == 4096); ^ int ./usr/include/linux/btrfs.h:192:15: error: expected parameter declarator static_assert(sizeof(struct btrfs_ioctl_scrub_args) == 1024); ^ ./usr/include/linux/btrfs.h:192:15: error: expected ')' ./usr/include/linux/btrfs.h:192:14: note: to match this '(' static_assert(sizeof(struct btrfs_ioctl_scrub_args) == 1024); ^ ./usr/include/linux/btrfs.h:192:1: warning: declaration specifier missing, defaulting to 'int' static_assert(sizeof(struct btrfs_ioctl_scrub_args) == 1024); ^ int ./usr/include/linux/btrfs.h:245:15: error: expected parameter declarator static_assert(sizeof(struct btrfs_ioctl_dev_info_args) == 4096); ^ ./usr/include/linux/btrfs.h:245:15: error: expected ')' ./usr/include/linux/btrfs.h:245:14: note: to match this '(' static_assert(sizeof(struct btrfs_ioctl_dev_info_args) == 4096); ^ ./usr/include/linux/btrfs.h:245:1: warning: declaration specifier missing, defaulting to 'int' static_assert(sizeof(struct btrfs_ioctl_dev_info_args) == 4096); ^ int ./usr/include/linux/btrfs.h:274:15: error: expected parameter declarator static_assert(sizeof(struct btrfs_ioctl_fs_info_args) == 1024); ^ ./usr/include/linux/btrfs.h:274:15: error: expected ')' ./usr/include/linux/btrfs.h:274:14: note: to match this '(' static_assert(sizeof(struct btrfs_ioctl_fs_info_args) == 1024); ^ ./usr/include/linux/btrfs.h:274:1: warning: declaration specifier missing, defaulting to 'int' static_assert(sizeof(struct btrfs_ioctl_fs_info_args) == 1024); ^ int ./usr/include/linux/btrfs.h:457:15: error: expected parameter declarator static_assert(sizeof(struct btrfs_ioctl_balance_args) == 1024); ^ ./usr/include/linux/btrfs.h:457:15: error: expected ')' ./usr/include/linux/btrfs.h:457:14: note: to match this '(' static_assert(sizeof(struct btrfs_ioctl_balance_args) == 1024); ^ ./usr/include/linux/btrfs.h:457:1: warning: declaration specifier missing, defaulting to 'int' static_assert(sizeof(struct btrfs_ioctl_balance_args) == 1024); ^ int ./usr/include/linux/btrfs.h:465:15: error: expected parameter declarator static_assert(sizeof(struct btrfs_ioctl_ino_lookup_args) == 4096); ^ ./usr/include/linux/btrfs.h:465:15: error: expected ')' ./usr/include/linux/btrfs.h:465:14: note: to match this '(' static_assert(sizeof(struct btrfs_ioctl_ino_lookup_args) == 4096); ^ ./usr/include/linux/btrfs.h:465:1: warning: declaration specifier missing, defaulting to 'int' static_assert(sizeof(struct btrfs_ioctl_ino_lookup_args) == 4096); ^ int ./usr/include/linux/btrfs.h:481:15: error: expected parameter declarator static_assert(sizeof(struct btrfs_ioctl_ino_lookup_user_args) == 4096); ^ ./usr/include/linux/btrfs.h:481:15: error: expected ')' ./usr/include/linux/btrfs.h:481:14: note: to match this '(' static_assert(sizeof(struct btrfs_ioctl_ino_lookup_user_args) == 4096); ^ ./usr/include/linux/btrfs.h:481:1: warning: declaration specifier missing, defaulting to 'int' static_assert(sizeof(struct btrfs_ioctl_ino_lookup_user_args) == 4096); ^ int ./usr/include/linux/btrfs.h:560:15: error: expected parameter declarator static_assert(sizeof(struct btrfs_ioctl_search_args) == 4096); ^ ./usr/include/linux/btrfs.h:560:15: error: expected ')' ./usr/include/linux/btrfs.h:560:14: note: to match this '(' static_assert(sizeof(struct btrfs_ioctl_search_args) == 4096); ^ ./usr/include/linux/btrfs.h:560:1: warning: declaration specifier missing, defaulting to 'int' static_assert(sizeof(struct btrfs_ioctl_search_args) == 4096); ^ int ./usr/include/linux/btrfs.h:718:15: error: expected parameter declarator static_assert(sizeof(struct btrfs_ioctl_get_dev_stats) == 1032); ^ ./usr/include/linux/btrfs.h:718:15: error: expected ')' ./usr/include/linux/btrfs.h:718:14: note: to match this '(' static_assert(sizeof(struct btrfs_ioctl_get_dev_stats) == 1032); ^ ./usr/include/linux/btrfs.h:718:1: warning: declaration specifier missing, defaulting to 'int' static_assert(sizeof(struct btrfs_ioctl_get_dev_stats) == 1032); ^ int 9 warnings and 18 errors generated. --- 0-DAY CI Kernel Test Service, Intel Corporation https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
On Tue, Jul 14, 2020 at 05:01:21AM +0800, kernel test robot wrote: > Hi Johannes, > > I love your patch! Yet something to improve: > > [auto build test ERROR on v5.8-rc5] > [cannot apply to kdave/for-next btrfs/next next-20200713] > [If your patch is applied to the wrong git tree, kindly drop us a note. > And when submitting patch, we suggest to use as documented in > https://git-scm.com/docs/git-format-patch] > > url: https://github.com/0day-ci/linux/commits/Johannes-Thumshirn/Two-furhter-additions-for-fsinfo-ioctl/20200713-203321 > base: 11ba468877bb23f28956a35e896356252d63c983 > config: x86_64-randconfig-a016-20200713 (attached as .config) > compiler: clang version 11.0.0 (https://github.com/llvm/llvm-project 02946de3802d3bc65bc9f2eb9b8d4969b5a7add8) > reproduce (this is a W=1 build): > wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross > chmod +x ~/bin/make.cross > # install x86_64 cross compiling tool for clang build > # apt-get install binutils-x86-64-linux-gnu > # save the attached .config to linux build tree > COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=x86_64 > > If you fix the issue, kindly add following tag as appropriate > Reported-by: kernel test robot <lkp@intel.com> > > All error/warnings (new ones prefixed by >>): > > In file included from <built-in>:1: > In file included from ./usr/include/linux/btrfs_tree.h:5: > >> ./usr/include/linux/btrfs.h:35:15: error: expected parameter declarator > static_assert(sizeof(struct btrfs_ioctl_vol_args) == 4096); > ^ Does that mean that clang (11.0) does not support static_assert? We aren't doing anything special here, include only the standard kernel headers and use macros as intended.
On Tue, Jul 14, 2020 at 01:20:53PM +0200, David Sterba wrote: > On Tue, Jul 14, 2020 at 05:01:21AM +0800, kernel test robot wrote: > > Hi Johannes, > > > > I love your patch! Yet something to improve: > > > > [auto build test ERROR on v5.8-rc5] > > [cannot apply to kdave/for-next btrfs/next next-20200713] > > [If your patch is applied to the wrong git tree, kindly drop us a note. > > And when submitting patch, we suggest to use as documented in > > https://git-scm.com/docs/git-format-patch] > > > > url: https://github.com/0day-ci/linux/commits/Johannes-Thumshirn/Two-furhter-additions-for-fsinfo-ioctl/20200713-203321 > > base: 11ba468877bb23f28956a35e896356252d63c983 > > config: x86_64-randconfig-a016-20200713 (attached as .config) > > compiler: clang version 11.0.0 (https://github.com/llvm/llvm-project 02946de3802d3bc65bc9f2eb9b8d4969b5a7add8) > > reproduce (this is a W=1 build): > > wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross > > chmod +x ~/bin/make.cross > > # install x86_64 cross compiling tool for clang build > > # apt-get install binutils-x86-64-linux-gnu > > # save the attached .config to linux build tree > > COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=x86_64 > > > > If you fix the issue, kindly add following tag as appropriate > > Reported-by: kernel test robot <lkp@intel.com> > > > > All error/warnings (new ones prefixed by >>): > > > > In file included from <built-in>:1: > > In file included from ./usr/include/linux/btrfs_tree.h:5: > > >> ./usr/include/linux/btrfs.h:35:15: error: expected parameter declarator > > static_assert(sizeof(struct btrfs_ioctl_vol_args) == 4096); > > ^ > > Does that mean that clang (11.0) does not support static_assert? We > aren't doing anything special here, include only the standard kernel > headers and use macros as intended. Never mind, Johanness fixed it in v2, the macro is not defined for non-kernel build.
diff --git a/include/uapi/linux/btrfs.h b/include/uapi/linux/btrfs.h index 69b88f6cb57f..d3c363398e10 100644 --- a/include/uapi/linux/btrfs.h +++ b/include/uapi/linux/btrfs.h @@ -32,6 +32,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 @@ -190,6 +191,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 @@ -242,6 +244,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 +273,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 @@ -453,6 +456,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 +464,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 +480,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 +559,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 */ @@ -710,6 +717,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
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> --- include/uapi/linux/btrfs.h | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-)