Re: [PATCH] btrfs: convert ioctl handlers to AUTO_KFREE

[Qu Wenruo <wqu@suse.com>]

在 2026/4/15 06:42, David Sterba 写道:
> Many ioctl handlers are suitable for the AUTO_KFREE conversions as the
> data are temporary and short lived. The conversions are trivial or the
> collateral changes are straightforward.
> 
> A kfree() preceding mnt_drop_write_file() is slightly more efficient but
> in the reverse order (i.e. the automatic kfree) does not cause any
> significant change as the write drop does only a few simple operations.
> 
> Signed-off-by: David Sterba <dsterba@suse.com>
> ---
>   fs/btrfs/ioctl.c | 289 +++++++++++++++++------------------------------
>   1 file changed, 104 insertions(+), 185 deletions(-)
> 
> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
> index b2e447f5005c16..17f72b3a1c43d6 100644
> --- a/fs/btrfs/ioctl.c
> +++ b/fs/btrfs/ioctl.c
> @@ -707,7 +707,7 @@ static int create_snapshot(struct btrfs_root *root, struct inode *dir,
>   {
>   	struct btrfs_fs_info *fs_info = inode_to_fs_info(dir);
>   	struct inode *inode;
> -	struct btrfs_pending_snapshot *pending_snapshot;
> +	struct btrfs_pending_snapshot AUTO_KFREE(pending_snapshot);

Checkpatch is reporting warnings about it;

WARNING:FUNCTION_ARGUMENTS: function definition argument 
'pending_snapshot' should also have an identifier name

It doesn't recognize the expanded macro here.

Not sure if the plain __free() method will solve the false alerts.


>   	unsigned int trans_num_items;
>   	struct btrfs_trans_handle *trans;
>   	struct btrfs_block_rsv *block_rsv;
> @@ -816,7 +816,6 @@ static int create_snapshot(struct btrfs_root *root, struct inode *dir,
>   		free_anon_bdev(pending_snapshot->anon_dev);
>   	kfree(pending_snapshot->root_item);
>   	btrfs_free_path(pending_snapshot->path);
> -	kfree(pending_snapshot);
>   
>   	return ret;
>   }
> @@ -961,7 +960,7 @@ static noinline int btrfs_ioctl_resize(struct file *file,
>   	u64 new_size;
>   	u64 old_size;
>   	u64 devid = 1;
> -	struct btrfs_ioctl_vol_args *vol_args;
> +	struct btrfs_ioctl_vol_args AUTO_KFREE(vol_args);

@vol_args is later called with memdup_user(), which can return error 
pointers, and unfortunately kfree() doesn't have any special checks on 
error pointer.

[...]
>   static noinline int btrfs_ioctl_subvol_getflags(struct btrfs_inode *inode,
> @@ -1865,7 +1846,7 @@ static int btrfs_search_path_in_tree_user(struct mnt_idmap *idmap,
>   static noinline int btrfs_ioctl_ino_lookup(struct btrfs_root *root,
>   					   void __user *argp)
>   {
> -	struct btrfs_ioctl_ino_lookup_args *args;
> +	struct btrfs_ioctl_ino_lookup_args AUTO_KFREE(args);
>   	int ret = 0;
>   
>   	args = memdup_user(argp, sizeof(*args));

The same error pointer on error cases.

Unfortunately there are too many memdup_user() callers, and they all 
share the same problem.

> @@ -1895,10 +1876,9 @@ static noinline int btrfs_ioctl_ino_lookup(struct btrfs_root *root,
>   
>   out:
>   	if (ret == 0 && copy_to_user(argp, args, sizeof(*args)))
> -		ret = -EFAULT;
> +		return -EFAULT;
>   
> -	kfree(args);
> -	return ret;
> +	return 0;

The original error is now discarded.

Thanks,
Qu