Re: Patch "btrfs: fix a potential hole punching failure" has been added to the 5.4-stable tree

[Greg KH]

On Sat, May 08, 2021 at 12:10:13PM -0400, Sasha Levin wrote:
> This is a note to let you know that I've just added the patch titled
> 
>     btrfs: fix a potential hole punching failure
> 
> to the 5.4-stable tree which can be found at:
>     http://www.kernel.org/git/?p=linux/kernel/git/stable/stable-queue.git;a=summary
> 
> The filename of the patch is:
>      btrfs-fix-a-potential-hole-punching-failure.patch
> and it can be found in the queue-5.4 subdirectory.
> 
> If you, or anyone else, feels it should not be added to the stable tree,
> please let <stable@xxxxxxxxxxxxxxx> know about it.
> 
> 
> 
> commit 3744bdfd37eaa635ddb70f15fe5d3bde233e54f0
> Author: BingJing Chang <bingjingc@xxxxxxxxxxxx>
> Date:   Thu Mar 25 09:56:22 2021 +0800
> 
>     btrfs: fix a potential hole punching failure
>     
>     [ Upstream commit 3227788cd369d734d2d3cd94f8af7536b60fa552 ]
>     
>     In commit d77815461f04 ("btrfs: Avoid trucating page or punching hole
>     in a already existed hole."), existing holes can be skipped by calling
>     find_first_non_hole() to adjust start and len. However, if the given len
>     is invalid and large, when an EXTENT_MAP_HOLE extent is found, len will
>     not be set to zero because (em->start + em->len) is less than
>     (start + len). Then the ret will be 1 but len will not be set to 0.
>     The propagated non-zero ret will result in fallocate failure.
>     
>     In the while-loop of btrfs_replace_file_extents(), len is not updated
>     every time before it calls find_first_non_hole(). That is, after
>     btrfs_drop_extents() successfully drops the last non-hole file extent,
>     it may fail with ENOSPC when attempting to drop a file extent item
>     representing a hole. The problem can happen. After it calls
>     find_first_non_hole(), the cur_offset will be adjusted to be larger
>     than or equal to end. However, since the len is not set to zero, the
>     break-loop condition (ret && !len) will not be met. After it leaves the
>     while-loop, fallocate will return 1, which is an unexpected return
>     value.
>     
>     We're not able to construct a reproducible way to let
>     btrfs_drop_extents() fail with ENOSPC after it drops the last non-hole
>     file extent but with remaining holes left. However, it's quite easy to
>     fix. We just need to update and check the len every time before we call
>     find_first_non_hole(). To make the while loop more readable, we also
>     pull the variable updates to the bottom of loop like this:
>       while (cur_offset < end) {
>               ...
>               // update cur_offset & len
>               // advance cur_offset & len in hole-punching case if needed
>       }
>     
>     Reported-by: Robbie Ko <robbieko@xxxxxxxxxxxx>
>     Fixes: d77815461f04 ("btrfs: Avoid trucating page or punching hole in a already existed hole.")
>     CC: stable@xxxxxxxxxxxxxxx # 4.4+
>     Reviewed-by: Robbie Ko <robbieko@xxxxxxxxxxxx>
>     Reviewed-by: Chung-Chiang Cheng <cccheng@xxxxxxxxxxxx>
>     Reviewed-by: Filipe Manana <fdmanana@xxxxxxxx>
>     Signed-off-by: BingJing Chang <bingjingc@xxxxxxxxxxxx>
>     Signed-off-by: David Sterba <dsterba@xxxxxxxx>
>     Signed-off-by: Sasha Levin <sashal@xxxxxxxxxx>
> 
> diff --git a/fs/btrfs/file.c b/fs/btrfs/file.c
> index f8e5c47b95e4..dc6f4bfd9a45 100644
> --- a/fs/btrfs/file.c
> +++ b/fs/btrfs/file.c
> @@ -2642,8 +2642,6 @@ int btrfs_punch_hole_range(struct inode *inode, struct btrfs_path *path,
>  			clone_info->file_offset += clone_len;
>  		}
>  
> -		cur_offset = drop_end;
> -
>  		ret = btrfs_update_inode(trans, root, inode);
>  		if (ret)
>  			break;
> @@ -2663,7 +2661,9 @@ int btrfs_punch_hole_range(struct inode *inode, struct btrfs_path *path,
>  		BUG_ON(ret);	/* shouldn't happen */
>  		trans->block_rsv = rsv;
>  
> -		if (!clone_info) {
> +		cur_offset = drop_args.drop_end;
> +                len = end - cur_offset;
> +		if (!clone_info && len) {
>  			ret = find_first_non_hole(inode, &cur_offset, &len);
>  			if (unlikely(ret < 0))
>  				break;

This broke the build, so I'm dropping it :(