Re: [PATCH] btrfs: fix fallocate to use file_modified to update permissions consistently

[Filipe Manana <fdmanana@kernel.org>]

On Thu, Mar 10, 2022 at 11:22:45AM -0800, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> Since the initial introduction of (posix) fallocate back at the turn of
> the century, it has been possible to use this syscall to change the
> user-visible contents of files.  This can happen by extending the file
> size during a preallocation, or through any of the newer modes (punch,
> zero range).  Because the call can be used to change file contents, we
> should treat it like we do any other modification to a file -- update
> the mtime, and drop set[ug]id privileges/capabilities.
> 
> The VFS function file_modified() does all this for us if pass it a
> locked inode, so let's make fallocate drop permissions correctly.
> 
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> ---
> Note: I plan to add fstests to test this behavior, but after the
> generic/673 mess, I'm holding back on them until I can fix the three
> major filesystems and clean up the xfs setattr_copy code.
> 
> https://lore.kernel.org/linux-ext4/20220310174410.GB8172@magnolia/T/#u
> ---
>  fs/btrfs/file.c |   23 +++++++++++++++++++----
>  1 file changed, 19 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/btrfs/file.c b/fs/btrfs/file.c
> index a0179cc62913..79e61c88b9e7 100644
> --- a/fs/btrfs/file.c
> +++ b/fs/btrfs/file.c
> @@ -2918,8 +2918,9 @@ int btrfs_replace_file_extents(struct btrfs_inode *inode,
>  	return ret;
>  }
>  
> -static int btrfs_punch_hole(struct inode *inode, loff_t offset, loff_t len)
> +static int btrfs_punch_hole(struct file *file, loff_t offset, loff_t len)
>  {
> +	struct inode *inode = file_inode(file);
>  	struct btrfs_fs_info *fs_info = btrfs_sb(inode->i_sb);
>  	struct btrfs_root *root = BTRFS_I(inode)->root;
>  	struct extent_state *cached_state = NULL;
> @@ -2951,6 +2952,10 @@ static int btrfs_punch_hole(struct inode *inode, loff_t offset, loff_t len)
>  		goto out_only_mutex;
>  	}
>  
> +	ret = file_modified(file);
> +	if (ret)
> +		goto out_only_mutex;
> +
>  	lockstart = round_up(offset, btrfs_inode_sectorsize(BTRFS_I(inode)));
>  	lockend = round_down(offset + len,
>  			     btrfs_inode_sectorsize(BTRFS_I(inode))) - 1;
> @@ -3177,11 +3182,12 @@ static int btrfs_zero_range_check_range_boundary(struct btrfs_inode *inode,
>  	return ret;
>  }
>  
> -static int btrfs_zero_range(struct inode *inode,
> +static int btrfs_zero_range(struct file *file,
>  			    loff_t offset,
>  			    loff_t len,
>  			    const int mode)
>  {
> +	struct inode *inode = file_inode(file);
>  	struct btrfs_fs_info *fs_info = BTRFS_I(inode)->root->fs_info;
>  	struct extent_map *em;
>  	struct extent_changeset *data_reserved = NULL;
> @@ -3202,6 +3208,12 @@ static int btrfs_zero_range(struct inode *inode,
>  		goto out;
>  	}
>  
> +	ret = file_modified(file);
> +	if (ret) {
> +		free_extent_map(em);
> +		goto out;
> +	}
> +

Could be done before getting the extent map, to make the code a bit shorter, or
see the comment below.

>  	/*
>  	 * Avoid hole punching and extent allocation for some cases. More cases
>  	 * could be considered, but these are unlikely common and we keep things
> @@ -3391,7 +3403,7 @@ static long btrfs_fallocate(struct file *file, int mode,
>  		return -EOPNOTSUPP;
>  
>  	if (mode & FALLOC_FL_PUNCH_HOLE)
> -		return btrfs_punch_hole(inode, offset, len);
> +		return btrfs_punch_hole(file, offset, len);
>  
>  	/*
>  	 * Only trigger disk allocation, don't trigger qgroup reserve
> @@ -3446,7 +3458,7 @@ static long btrfs_fallocate(struct file *file, int mode,
>  		goto out;
>  
>  	if (mode & FALLOC_FL_ZERO_RANGE) {
> -		ret = btrfs_zero_range(inode, offset, len, mode);
> +		ret = btrfs_zero_range(file, offset, len, mode);
>  		btrfs_inode_unlock(inode, BTRFS_ILOCK_MMAP);
>  		return ret;
>  	}
> @@ -3528,6 +3540,9 @@ static long btrfs_fallocate(struct file *file, int mode,
>  		cur_offset = last_byte;
>  	}
>  
> +	if (!ret)
> +		ret = file_modified(file);

If call file_modified() before the if that checks for the zero range case,
then we avoid having to call file_modified() at btrfs_zero_range() too,
and get the behaviour for both plain fallocate and zero range.

Otherwise, it looks good.

Thanks for doing this, I had it on my todo list since I noticed the generic/673
failure with reflinks and the suid/sgid bits.

> +
>  	/*
>  	 * If ret is still 0, means we're OK to fallocate.
>  	 * Or just cleanup the list and exit.