diff mbox series

[3/7] btrfs: Allocate btrfs_ioctl_get_subvol_info_args on stack

Message ID 2200f9340973d627ee304ac4470f5921061266f9.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_get_subvol_info_args,
allocate btrfs_ioctl_get_subvol_info_args on stack.

sizeof(btrfs_ioctl_get_subvol_info_args) = 504

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

Comments

Anand Jain July 28, 2021, 5:59 a.m. UTC | #1 
On 28/07/2021 05:17, Goldwyn Rodrigues wrote:
> From: Goldwyn Rodrigues <rgoldwyn@suse.com>
> 
> Instead of using kmalloc() to allocate btrfs_ioctl_get_subvol_info_args,
> allocate btrfs_ioctl_get_subvol_info_args on stack.
> 
> sizeof(btrfs_ioctl_get_subvol_info_args) = 504
> 
> Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com>


Looks good.

Reviewed-by: Anand Jain <anand.jain@oracle.com>


> ---
>   fs/btrfs/ioctl.c | 55 +++++++++++++++++++++---------------------------
>   1 file changed, 24 insertions(+), 31 deletions(-)
> 
> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
> index 0ba98e08a029..90b134b5a653 100644
> --- a/fs/btrfs/ioctl.c
> +++ b/fs/btrfs/ioctl.c
> @@ -2681,7 +2681,7 @@ static int btrfs_ioctl_ino_lookup_user(struct file *file, void __user *argp)
>   /* Get the subvolume information in BTRFS_ROOT_ITEM and BTRFS_ROOT_BACKREF */
>   static int btrfs_ioctl_get_subvol_info(struct file *file, void __user *argp)
>   {
> -	struct btrfs_ioctl_get_subvol_info_args *subvol_info;
> +	struct btrfs_ioctl_get_subvol_info_args subvol_info = {0};
>   	struct btrfs_fs_info *fs_info;
>   	struct btrfs_root *root;
>   	struct btrfs_path *path;
> @@ -2699,12 +2699,6 @@ static int btrfs_ioctl_get_subvol_info(struct file *file, void __user *argp)
>   	if (!path)
>   		return -ENOMEM;
>   
> -	subvol_info = kzalloc(sizeof(*subvol_info), GFP_KERNEL);
> -	if (!subvol_info) {
> -		btrfs_free_path(path);
> -		return -ENOMEM;
> -	}
> -
>   	inode = file_inode(file);
>   	fs_info = BTRFS_I(inode)->root->fs_info;
>   
> @@ -2717,32 +2711,32 @@ static int btrfs_ioctl_get_subvol_info(struct file *file, void __user *argp)
>   	}
>   	root_item = &root->root_item;
>   
> -	subvol_info->treeid = key.objectid;
> +	subvol_info.treeid = key.objectid;
>   
> -	subvol_info->generation = btrfs_root_generation(root_item);
> -	subvol_info->flags = btrfs_root_flags(root_item);
> +	subvol_info.generation = btrfs_root_generation(root_item);
> +	subvol_info.flags = btrfs_root_flags(root_item);
>   
> -	memcpy(subvol_info->uuid, root_item->uuid, BTRFS_UUID_SIZE);
> -	memcpy(subvol_info->parent_uuid, root_item->parent_uuid,
> +	memcpy(subvol_info.uuid, root_item->uuid, BTRFS_UUID_SIZE);
> +	memcpy(subvol_info.parent_uuid, root_item->parent_uuid,
>   						    BTRFS_UUID_SIZE);
> -	memcpy(subvol_info->received_uuid, root_item->received_uuid,
> +	memcpy(subvol_info.received_uuid, root_item->received_uuid,
>   						    BTRFS_UUID_SIZE);
>   
> -	subvol_info->ctransid = btrfs_root_ctransid(root_item);
> -	subvol_info->ctime.sec = btrfs_stack_timespec_sec(&root_item->ctime);
> -	subvol_info->ctime.nsec = btrfs_stack_timespec_nsec(&root_item->ctime);
> +	subvol_info.ctransid = btrfs_root_ctransid(root_item);
> +	subvol_info.ctime.sec = btrfs_stack_timespec_sec(&root_item->ctime);
> +	subvol_info.ctime.nsec = btrfs_stack_timespec_nsec(&root_item->ctime);
>   
> -	subvol_info->otransid = btrfs_root_otransid(root_item);
> -	subvol_info->otime.sec = btrfs_stack_timespec_sec(&root_item->otime);
> -	subvol_info->otime.nsec = btrfs_stack_timespec_nsec(&root_item->otime);
> +	subvol_info.otransid = btrfs_root_otransid(root_item);
> +	subvol_info.otime.sec = btrfs_stack_timespec_sec(&root_item->otime);
> +	subvol_info.otime.nsec = btrfs_stack_timespec_nsec(&root_item->otime);
>   
> -	subvol_info->stransid = btrfs_root_stransid(root_item);
> -	subvol_info->stime.sec = btrfs_stack_timespec_sec(&root_item->stime);
> -	subvol_info->stime.nsec = btrfs_stack_timespec_nsec(&root_item->stime);
> +	subvol_info.stransid = btrfs_root_stransid(root_item);
> +	subvol_info.stime.sec = btrfs_stack_timespec_sec(&root_item->stime);
> +	subvol_info.stime.nsec = btrfs_stack_timespec_nsec(&root_item->stime);
>   
> -	subvol_info->rtransid = btrfs_root_rtransid(root_item);
> -	subvol_info->rtime.sec = btrfs_stack_timespec_sec(&root_item->rtime);
> -	subvol_info->rtime.nsec = btrfs_stack_timespec_nsec(&root_item->rtime);
> +	subvol_info.rtransid = btrfs_root_rtransid(root_item);
> +	subvol_info.rtime.sec = btrfs_stack_timespec_sec(&root_item->rtime);
> +	subvol_info.rtime.nsec = btrfs_stack_timespec_nsec(&root_item->rtime);
>   
>   	if (key.objectid != BTRFS_FS_TREE_OBJECTID) {
>   		/* Search root tree for ROOT_BACKREF of this subvolume */
> @@ -2765,18 +2759,18 @@ static int btrfs_ioctl_get_subvol_info(struct file *file, void __user *argp)
>   		leaf = path->nodes[0];
>   		slot = path->slots[0];
>   		btrfs_item_key_to_cpu(leaf, &key, slot);
> -		if (key.objectid == subvol_info->treeid &&
> +		if (key.objectid == subvol_info.treeid &&
>   		    key.type == BTRFS_ROOT_BACKREF_KEY) {
> -			subvol_info->parent_id = key.offset;
> +			subvol_info.parent_id = key.offset;
>   
>   			rref = btrfs_item_ptr(leaf, slot, struct btrfs_root_ref);
> -			subvol_info->dirid = btrfs_root_ref_dirid(leaf, rref);
> +			subvol_info.dirid = btrfs_root_ref_dirid(leaf, rref);
>   
>   			item_off = btrfs_item_ptr_offset(leaf, slot)
>   					+ sizeof(struct btrfs_root_ref);
>   			item_len = btrfs_item_size_nr(leaf, slot)
>   					- sizeof(struct btrfs_root_ref);
> -			read_extent_buffer(leaf, subvol_info->name,
> +			read_extent_buffer(leaf, subvol_info.name,
>   					   item_off, item_len);
>   		} else {
>   			ret = -ENOENT;
> @@ -2784,14 +2778,13 @@ static int btrfs_ioctl_get_subvol_info(struct file *file, void __user *argp)
>   		}
>   	}
>   
> -	if (copy_to_user(argp, subvol_info, sizeof(*subvol_info)))
> +	if (copy_to_user(argp, &subvol_info, sizeof(subvol_info)))
>   		ret = -EFAULT;
>   
>   out:
>   	btrfs_put_root(root);
>   out_free:
>   	btrfs_free_path(path);
> -	kfree(subvol_info);
>   	return ret;
>   }
>   
>
David Sterba July 29, 2021, 5:08 p.m. UTC | #2 
On Tue, Jul 27, 2021 at 04:17:27PM -0500, Goldwyn Rodrigues wrote:
> From: Goldwyn Rodrigues <rgoldwyn@suse.com>
> 
> Instead of using kmalloc() to allocate btrfs_ioctl_get_subvol_info_args,
> allocate btrfs_ioctl_get_subvol_info_args on stack.
> 
> sizeof(btrfs_ioctl_get_subvol_info_args) = 504
> 
> Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com>
> ---
>  fs/btrfs/ioctl.c | 55 +++++++++++++++++++++---------------------------
>  1 file changed, 24 insertions(+), 31 deletions(-)
> 
> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
> index 0ba98e08a029..90b134b5a653 100644
> --- a/fs/btrfs/ioctl.c
> +++ b/fs/btrfs/ioctl.c
> @@ -2681,7 +2681,7 @@ static int btrfs_ioctl_ino_lookup_user(struct file *file, void __user *argp)
>  /* Get the subvolume information in BTRFS_ROOT_ITEM and BTRFS_ROOT_BACKREF */
>  static int btrfs_ioctl_get_subvol_info(struct file *file, void __user *argp)
>  {
> -	struct btrfs_ioctl_get_subvol_info_args *subvol_info;
> +	struct btrfs_ioctl_get_subvol_info_args subvol_info = {0};

There are concerns [1] if the {0} initialization zeroes all the bytes
entirely, including padding between members and at the end, but as I
understand it, this should be safe.

[1] long thread, https://lore.kernel.org/lkml/20210727205855.411487-20-keescook@chromium.org/T/#m7e4e04af9664f204232a569390c3f3dc2e4bf907

If it turns out {0} is not sufficient, we'll have to add explicit
memset() in case the on-stack structure is then copied back with
copy_to_user.
David Sterba July 29, 2021, 5:22 p.m. UTC | #3 
On Tue, Jul 27, 2021 at 04:17:27PM -0500, Goldwyn Rodrigues wrote:
> From: Goldwyn Rodrigues <rgoldwyn@suse.com>
> 
> Instead of using kmalloc() to allocate btrfs_ioctl_get_subvol_info_args,
> allocate btrfs_ioctl_get_subvol_info_args on stack.
> 
> sizeof(btrfs_ioctl_get_subvol_info_args) = 504

I'm not sure about this one, it's a lot and it's not a perf critical
ioctl. Reading information about a subvolume can potentially trigger a
lot of reads in a cold state, thus triggering all the deep call chains.
diff mbox series

Patch

diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
index 0ba98e08a029..90b134b5a653 100644
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -2681,7 +2681,7 @@  static int btrfs_ioctl_ino_lookup_user(struct file *file, void __user *argp)
 /* Get the subvolume information in BTRFS_ROOT_ITEM and BTRFS_ROOT_BACKREF */
 static int btrfs_ioctl_get_subvol_info(struct file *file, void __user *argp)
 {
-	struct btrfs_ioctl_get_subvol_info_args *subvol_info;
+	struct btrfs_ioctl_get_subvol_info_args subvol_info = {0};
 	struct btrfs_fs_info *fs_info;
 	struct btrfs_root *root;
 	struct btrfs_path *path;
@@ -2699,12 +2699,6 @@  static int btrfs_ioctl_get_subvol_info(struct file *file, void __user *argp)
 	if (!path)
 		return -ENOMEM;
 
-	subvol_info = kzalloc(sizeof(*subvol_info), GFP_KERNEL);
-	if (!subvol_info) {
-		btrfs_free_path(path);
-		return -ENOMEM;
-	}
-
 	inode = file_inode(file);
 	fs_info = BTRFS_I(inode)->root->fs_info;
 
@@ -2717,32 +2711,32 @@  static int btrfs_ioctl_get_subvol_info(struct file *file, void __user *argp)
 	}
 	root_item = &root->root_item;
 
-	subvol_info->treeid = key.objectid;
+	subvol_info.treeid = key.objectid;
 
-	subvol_info->generation = btrfs_root_generation(root_item);
-	subvol_info->flags = btrfs_root_flags(root_item);
+	subvol_info.generation = btrfs_root_generation(root_item);
+	subvol_info.flags = btrfs_root_flags(root_item);
 
-	memcpy(subvol_info->uuid, root_item->uuid, BTRFS_UUID_SIZE);
-	memcpy(subvol_info->parent_uuid, root_item->parent_uuid,
+	memcpy(subvol_info.uuid, root_item->uuid, BTRFS_UUID_SIZE);
+	memcpy(subvol_info.parent_uuid, root_item->parent_uuid,
 						    BTRFS_UUID_SIZE);
-	memcpy(subvol_info->received_uuid, root_item->received_uuid,
+	memcpy(subvol_info.received_uuid, root_item->received_uuid,
 						    BTRFS_UUID_SIZE);
 
-	subvol_info->ctransid = btrfs_root_ctransid(root_item);
-	subvol_info->ctime.sec = btrfs_stack_timespec_sec(&root_item->ctime);
-	subvol_info->ctime.nsec = btrfs_stack_timespec_nsec(&root_item->ctime);
+	subvol_info.ctransid = btrfs_root_ctransid(root_item);
+	subvol_info.ctime.sec = btrfs_stack_timespec_sec(&root_item->ctime);
+	subvol_info.ctime.nsec = btrfs_stack_timespec_nsec(&root_item->ctime);
 
-	subvol_info->otransid = btrfs_root_otransid(root_item);
-	subvol_info->otime.sec = btrfs_stack_timespec_sec(&root_item->otime);
-	subvol_info->otime.nsec = btrfs_stack_timespec_nsec(&root_item->otime);
+	subvol_info.otransid = btrfs_root_otransid(root_item);
+	subvol_info.otime.sec = btrfs_stack_timespec_sec(&root_item->otime);
+	subvol_info.otime.nsec = btrfs_stack_timespec_nsec(&root_item->otime);
 
-	subvol_info->stransid = btrfs_root_stransid(root_item);
-	subvol_info->stime.sec = btrfs_stack_timespec_sec(&root_item->stime);
-	subvol_info->stime.nsec = btrfs_stack_timespec_nsec(&root_item->stime);
+	subvol_info.stransid = btrfs_root_stransid(root_item);
+	subvol_info.stime.sec = btrfs_stack_timespec_sec(&root_item->stime);
+	subvol_info.stime.nsec = btrfs_stack_timespec_nsec(&root_item->stime);
 
-	subvol_info->rtransid = btrfs_root_rtransid(root_item);
-	subvol_info->rtime.sec = btrfs_stack_timespec_sec(&root_item->rtime);
-	subvol_info->rtime.nsec = btrfs_stack_timespec_nsec(&root_item->rtime);
+	subvol_info.rtransid = btrfs_root_rtransid(root_item);
+	subvol_info.rtime.sec = btrfs_stack_timespec_sec(&root_item->rtime);
+	subvol_info.rtime.nsec = btrfs_stack_timespec_nsec(&root_item->rtime);
 
 	if (key.objectid != BTRFS_FS_TREE_OBJECTID) {
 		/* Search root tree for ROOT_BACKREF of this subvolume */
@@ -2765,18 +2759,18 @@  static int btrfs_ioctl_get_subvol_info(struct file *file, void __user *argp)
 		leaf = path->nodes[0];
 		slot = path->slots[0];
 		btrfs_item_key_to_cpu(leaf, &key, slot);
-		if (key.objectid == subvol_info->treeid &&
+		if (key.objectid == subvol_info.treeid &&
 		    key.type == BTRFS_ROOT_BACKREF_KEY) {
-			subvol_info->parent_id = key.offset;
+			subvol_info.parent_id = key.offset;
 
 			rref = btrfs_item_ptr(leaf, slot, struct btrfs_root_ref);
-			subvol_info->dirid = btrfs_root_ref_dirid(leaf, rref);
+			subvol_info.dirid = btrfs_root_ref_dirid(leaf, rref);
 
 			item_off = btrfs_item_ptr_offset(leaf, slot)
 					+ sizeof(struct btrfs_root_ref);
 			item_len = btrfs_item_size_nr(leaf, slot)
 					- sizeof(struct btrfs_root_ref);
-			read_extent_buffer(leaf, subvol_info->name,
+			read_extent_buffer(leaf, subvol_info.name,
 					   item_off, item_len);
 		} else {
 			ret = -ENOENT;
@@ -2784,14 +2778,13 @@  static int btrfs_ioctl_get_subvol_info(struct file *file, void __user *argp)
 		}
 	}
 
-	if (copy_to_user(argp, subvol_info, sizeof(*subvol_info)))
+	if (copy_to_user(argp, &subvol_info, sizeof(subvol_info)))
 		ret = -EFAULT;
 
 out:
 	btrfs_put_root(root);
 out_free:
 	btrfs_free_path(path);
-	kfree(subvol_info);
 	return ret;
 }