diff mbox series

[4/7] btrfs: Allocate btrfs_ioctl_balance_args on stack

Message ID 320216bed8e0c28e9235571db1962cbb1e18366a.1627418762.git.rgoldwyn@suse.com (mailing list archive)
State New, archived
Headers show
Series Allocate structures on stack instead of kmalloc() | expand

Commit Message

Goldwyn Rodrigues July 27, 2021, 9:17 p.m. UTC 
From: Goldwyn Rodrigues <rgoldwyn@suse.com>

Instead of using kmalloc() to allocate btrfs_ioctl_balance_args, allocate
btrfs_ioctl_balance_args on stack.

sizeof(btrfs_ioctl_balance_args) = 1024

Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com>
---
 fs/btrfs/ioctl.c | 13 +++----------
 1 file changed, 3 insertions(+), 10 deletions(-)

Comments

Darrick J. Wong July 28, 2021, 12:02 a.m. UTC | #1 
On Tue, Jul 27, 2021 at 04:17:28PM -0500, Goldwyn Rodrigues wrote:
> From: Goldwyn Rodrigues <rgoldwyn@suse.com>
> 
> Instead of using kmalloc() to allocate btrfs_ioctl_balance_args, allocate
> btrfs_ioctl_balance_args on stack.
> 
> sizeof(btrfs_ioctl_balance_args) = 1024

That's a pretty big addition to the stack frame.  Aren't some of the
kbuild robots configured to whinge about functions that eat more than
1100 bytes or so?

--D

> 
> Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com>
> ---
>  fs/btrfs/ioctl.c | 13 +++----------
>  1 file changed, 3 insertions(+), 10 deletions(-)
> 
> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
> index 90b134b5a653..9c3acc539052 100644
> --- a/fs/btrfs/ioctl.c
> +++ b/fs/btrfs/ioctl.c
> @@ -4141,7 +4141,7 @@ static long btrfs_ioctl_balance_ctl(struct btrfs_fs_info *fs_info, int cmd)
>  static long btrfs_ioctl_balance_progress(struct btrfs_fs_info *fs_info,
>  					 void __user *arg)
>  {
> -	struct btrfs_ioctl_balance_args *bargs;
> +	struct btrfs_ioctl_balance_args bargs = {0};
>  	int ret = 0;
>  
>  	if (!capable(CAP_SYS_ADMIN))
> @@ -4153,18 +4153,11 @@ static long btrfs_ioctl_balance_progress(struct btrfs_fs_info *fs_info,
>  		goto out;
>  	}
>  
> -	bargs = kzalloc(sizeof(*bargs), GFP_KERNEL);
> -	if (!bargs) {
> -		ret = -ENOMEM;
> -		goto out;
> -	}
> -
> -	btrfs_update_ioctl_balance_args(fs_info, bargs);
> +	btrfs_update_ioctl_balance_args(fs_info, &bargs);
>  
> -	if (copy_to_user(arg, bargs, sizeof(*bargs)))
> +	if (copy_to_user(arg, &bargs, sizeof(bargs)))
>  		ret = -EFAULT;
>  
> -	kfree(bargs);
>  out:
>  	mutex_unlock(&fs_info->balance_mutex);
>  	return ret;
> -- 
> 2.32.0
>
kernel test robot July 28, 2021, 1:19 a.m. UTC | #2 
Hi Goldwyn,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on kdave/for-next]
[also build test WARNING on v5.14-rc3 next-20210727]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Goldwyn-Rodrigues/btrfs-Allocate-walk_control-on-stack/20210728-061756
base:   https://git.kernel.org/pub/scm/linux/kernel/git/kdave/linux.git for-next
config: xtensa-allyesconfig (attached as .config)
compiler: xtensa-linux-gcc (GCC) 10.3.0
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
        # https://github.com/0day-ci/linux/commit/d5153c5e6009b09ae3916c2d693a0a609ec75cac
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Goldwyn-Rodrigues/btrfs-Allocate-walk_control-on-stack/20210728-061756
        git checkout d5153c5e6009b09ae3916c2d693a0a609ec75cac
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-10.3.0 make.cross ARCH=xtensa 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

   fs/btrfs/ioctl.c: In function 'btrfs_ioctl_balance_progress':
>> fs/btrfs/ioctl.c:4177:1: warning: the frame size of 1040 bytes is larger than 1024 bytes [-Wframe-larger-than=]
    4177 | }
         | ^


vim +4177 fs/btrfs/ioctl.c

837d5b6e46d1a4 Ilya Dryomov      2012-01-16  4153  
2ff7e61e0d30ff Jeff Mahoney      2016-06-22  4154  static long btrfs_ioctl_balance_progress(struct btrfs_fs_info *fs_info,
19a39dce3b9bf0 Ilya Dryomov      2012-01-16  4155  					 void __user *arg)
19a39dce3b9bf0 Ilya Dryomov      2012-01-16  4156  {
d5153c5e6009b0 Goldwyn Rodrigues 2021-07-27  4157  	struct btrfs_ioctl_balance_args bargs = {0};
19a39dce3b9bf0 Ilya Dryomov      2012-01-16  4158  	int ret = 0;
19a39dce3b9bf0 Ilya Dryomov      2012-01-16  4159  
19a39dce3b9bf0 Ilya Dryomov      2012-01-16  4160  	if (!capable(CAP_SYS_ADMIN))
19a39dce3b9bf0 Ilya Dryomov      2012-01-16  4161  		return -EPERM;
19a39dce3b9bf0 Ilya Dryomov      2012-01-16  4162  
19a39dce3b9bf0 Ilya Dryomov      2012-01-16  4163  	mutex_lock(&fs_info->balance_mutex);
19a39dce3b9bf0 Ilya Dryomov      2012-01-16  4164  	if (!fs_info->balance_ctl) {
19a39dce3b9bf0 Ilya Dryomov      2012-01-16  4165  		ret = -ENOTCONN;
19a39dce3b9bf0 Ilya Dryomov      2012-01-16  4166  		goto out;
19a39dce3b9bf0 Ilya Dryomov      2012-01-16  4167  	}
19a39dce3b9bf0 Ilya Dryomov      2012-01-16  4168  
d5153c5e6009b0 Goldwyn Rodrigues 2021-07-27  4169  	btrfs_update_ioctl_balance_args(fs_info, &bargs);
19a39dce3b9bf0 Ilya Dryomov      2012-01-16  4170  
d5153c5e6009b0 Goldwyn Rodrigues 2021-07-27  4171  	if (copy_to_user(arg, &bargs, sizeof(bargs)))
19a39dce3b9bf0 Ilya Dryomov      2012-01-16  4172  		ret = -EFAULT;
19a39dce3b9bf0 Ilya Dryomov      2012-01-16  4173  
19a39dce3b9bf0 Ilya Dryomov      2012-01-16  4174  out:
19a39dce3b9bf0 Ilya Dryomov      2012-01-16  4175  	mutex_unlock(&fs_info->balance_mutex);
19a39dce3b9bf0 Ilya Dryomov      2012-01-16  4176  	return ret;
19a39dce3b9bf0 Ilya Dryomov      2012-01-16 @4177  }
19a39dce3b9bf0 Ilya Dryomov      2012-01-16  4178  

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
Goldwyn Rodrigues July 28, 2021, 2:04 a.m. UTC | #3 
On 17:02 27/07, Darrick J. Wong wrote:
> On Tue, Jul 27, 2021 at 04:17:28PM -0500, Goldwyn Rodrigues wrote:
> > From: Goldwyn Rodrigues <rgoldwyn@suse.com>
> > 
> > Instead of using kmalloc() to allocate btrfs_ioctl_balance_args, allocate
> > btrfs_ioctl_balance_args on stack.
> > 
> > sizeof(btrfs_ioctl_balance_args) = 1024
> 
> That's a pretty big addition to the stack frame.  Aren't some of the
> kbuild robots configured to whinge about functions that eat more than
> 1100 bytes or so?

Apparently you are faster than the bot to detect this ;)
I got the warning mail from the kbuild bot and the limit is 1024, so it
would not fit in the frame. We can reject this patch.
diff mbox series

Patch

diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
index 90b134b5a653..9c3acc539052 100644
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -4141,7 +4141,7 @@  static long btrfs_ioctl_balance_ctl(struct btrfs_fs_info *fs_info, int cmd)
 static long btrfs_ioctl_balance_progress(struct btrfs_fs_info *fs_info,
 					 void __user *arg)
 {
-	struct btrfs_ioctl_balance_args *bargs;
+	struct btrfs_ioctl_balance_args bargs = {0};
 	int ret = 0;
 
 	if (!capable(CAP_SYS_ADMIN))
@@ -4153,18 +4153,11 @@  static long btrfs_ioctl_balance_progress(struct btrfs_fs_info *fs_info,
 		goto out;
 	}
 
-	bargs = kzalloc(sizeof(*bargs), GFP_KERNEL);
-	if (!bargs) {
-		ret = -ENOMEM;
-		goto out;
-	}
-
-	btrfs_update_ioctl_balance_args(fs_info, bargs);
+	btrfs_update_ioctl_balance_args(fs_info, &bargs);
 
-	if (copy_to_user(arg, bargs, sizeof(*bargs)))
+	if (copy_to_user(arg, &bargs, sizeof(bargs)))
 		ret = -EFAULT;
 
-	kfree(bargs);
 out:
 	mutex_unlock(&fs_info->balance_mutex);
 	return ret;