[PATCH] Btrfs: fix access to available allocation bits when starting balance

[PATCH] Btrfs: fix access to available allocation bits when starting balance

[fdmanana]

From: Filipe Manana <fdmanana@suse.com>

The available allocation bits members from struct btrfs_fs_info are
protected by a sequence lock, and when starting balance we access them
incorrectly in two different ways:

1) In the read sequence lock loop at btrfs_balance() we use the values we
   read from fs_info->avail_*_alloc_bits and we can immediately do actions
   that have side effects and can not be undone (printing a message and
   jumping to a label). This is wrong because a retry might be needed, so
   our actions must not have side effects and must be repeatable as long
   as read_seqretry() returns a non-zero value. In other words, we were
   essentially ignoring the sequence lock;

2) Right below the read sequence lock loop, we were reading the values
   from avail_metadata_alloc_bits and avail_data_alloc_bits without any
   protection from concurrent writers, that is, reading them outside of
   the read sequence lock critical section.

So fix this by making sure we only read the available allocation bits
while in a read sequence lock critical section and that what we do in the
critical section is repeatable (has nothing that can not be undone) so
that any eventual retry that is needed is handled properly.

Fixes: de98ced9e743 ("Btrfs: use seqlock to protect fs_info->avail_{data, metadata, system}_alloc_bits")
Fixes: 14506127979a ("btrfs: fix a bogus warning when converting only data or metadata")
Signed-off-by: Filipe Manana <fdmanana@suse.com>
---
 fs/btrfs/volumes.c | 39 +++++++++++++++++++++++----------------
 1 file changed, 23 insertions(+), 16 deletions(-)

diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index f4405e430da6..223334f08530 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -3712,6 +3712,7 @@ int btrfs_balance(struct btrfs_fs_info *fs_info,
 	int ret;
 	u64 num_devices;
 	unsigned seq;
+	bool reducing_integrity;
 
 	if (btrfs_fs_closing(fs_info) ||
 	    atomic_read(&fs_info->balance_pause_req) ||
@@ -3796,24 +3797,30 @@ int btrfs_balance(struct btrfs_fs_info *fs_info,
 		     !(bctl->sys.target & allowed)) ||
 		    ((bctl->meta.flags & BTRFS_BALANCE_ARGS_CONVERT) &&
 		     (fs_info->avail_metadata_alloc_bits & allowed) &&
-		     !(bctl->meta.target & allowed))) {
-			if (bctl->flags & BTRFS_BALANCE_FORCE) {
-				btrfs_info(fs_info,
-				"balance: force reducing metadata integrity");
-			} else {
-				btrfs_err(fs_info,
-	"balance: reduces metadata integrity, use --force if you want this");
-				ret = -EINVAL;
-				goto out;
-			}
-		}
+		     !(bctl->meta.target & allowed)))
+			reducing_integrity = true;
+		else
+			reducing_integrity = false;
+
+		/* if we're not converting, the target field is uninitialized */
+		meta_target = (bctl->meta.flags & BTRFS_BALANCE_ARGS_CONVERT) ?
+			bctl->meta.target : fs_info->avail_metadata_alloc_bits;
+		data_target = (bctl->data.flags & BTRFS_BALANCE_ARGS_CONVERT) ?
+			bctl->data.target : fs_info->avail_data_alloc_bits;
 	} while (read_seqretry(&fs_info->profiles_lock, seq));
 
-	/* if we're not converting, the target field is uninitialized */
-	meta_target = (bctl->meta.flags & BTRFS_BALANCE_ARGS_CONVERT) ?
-		bctl->meta.target : fs_info->avail_metadata_alloc_bits;
-	data_target = (bctl->data.flags & BTRFS_BALANCE_ARGS_CONVERT) ?
-		bctl->data.target : fs_info->avail_data_alloc_bits;
+	if (reducing_integrity) {
+		if (bctl->flags & BTRFS_BALANCE_FORCE) {
+			btrfs_info(fs_info,
+				   "balance: force reducing metadata integrity");
+		} else {
+			btrfs_err(fs_info,
+	  "balance: reduces metadata integrity, use --force if you want this");
+			ret = -EINVAL;
+			goto out;
+		}
+	}
+
 	if (btrfs_get_num_tolerated_disk_barrier_failures(meta_target) <
 		btrfs_get_num_tolerated_disk_barrier_failures(data_target)) {
 		int meta_index = btrfs_bg_flags_to_raid_index(meta_target);
-- 
2.11.0

Re: [PATCH] Btrfs: fix access to available allocation bits when starting balance

[Nikolay Borisov]

On 19.11.18 г. 11:48 ч., fdmanana@kernel.org wrote:
> From: Filipe Manana <fdmanana@suse.com>
> 
> The available allocation bits members from struct btrfs_fs_info are
> protected by a sequence lock, and when starting balance we access them
> incorrectly in two different ways:
> 
> 1) In the read sequence lock loop at btrfs_balance() we use the values we
>    read from fs_info->avail_*_alloc_bits and we can immediately do actions
>    that have side effects and can not be undone (printing a message and
>    jumping to a label). This is wrong because a retry might be needed, so
>    our actions must not have side effects and must be repeatable as long
>    as read_seqretry() returns a non-zero value. In other words, we were
>    essentially ignoring the sequence lock;
> 
> 2) Right below the read sequence lock loop, we were reading the values
>    from avail_metadata_alloc_bits and avail_data_alloc_bits without any
>    protection from concurrent writers, that is, reading them outside of
>    the read sequence lock critical section.
> 
> So fix this by making sure we only read the available allocation bits
> while in a read sequence lock critical section and that what we do in the
> critical section is repeatable (has nothing that can not be undone) so
> that any eventual retry that is needed is handled properly.
> 
> Fixes: de98ced9e743 ("Btrfs: use seqlock to protect fs_info->avail_{data, metadata, system}_alloc_bits")
> Fixes: 14506127979a ("btrfs: fix a bogus warning when converting only data or metadata")
> Signed-off-by: Filipe Manana <fdmanana@suse.com>

Reviewed-by: Nikolay Borisov <nborisov@suse.com>


> ---
>  fs/btrfs/volumes.c | 39 +++++++++++++++++++++++----------------
>  1 file changed, 23 insertions(+), 16 deletions(-)
> 
> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> index f4405e430da6..223334f08530 100644
> --- a/fs/btrfs/volumes.c
> +++ b/fs/btrfs/volumes.c
> @@ -3712,6 +3712,7 @@ int btrfs_balance(struct btrfs_fs_info *fs_info,
>  	int ret;
>  	u64 num_devices;
>  	unsigned seq;
> +	bool reducing_integrity;
>  
>  	if (btrfs_fs_closing(fs_info) ||
>  	    atomic_read(&fs_info->balance_pause_req) ||
> @@ -3796,24 +3797,30 @@ int btrfs_balance(struct btrfs_fs_info *fs_info,
>  		     !(bctl->sys.target & allowed)) ||
>  		    ((bctl->meta.flags & BTRFS_BALANCE_ARGS_CONVERT) &&
>  		     (fs_info->avail_metadata_alloc_bits & allowed) &&
> -		     !(bctl->meta.target & allowed))) {
> -			if (bctl->flags & BTRFS_BALANCE_FORCE) {
> -				btrfs_info(fs_info,
> -				"balance: force reducing metadata integrity");
> -			} else {
> -				btrfs_err(fs_info,
> -	"balance: reduces metadata integrity, use --force if you want this");
> -				ret = -EINVAL;
> -				goto out;
> -			}
> -		}
> +		     !(bctl->meta.target & allowed)))
> +			reducing_integrity = true;
> +		else
> +			reducing_integrity = false;
> +
> +		/* if we're not converting, the target field is uninitialized */
> +		meta_target = (bctl->meta.flags & BTRFS_BALANCE_ARGS_CONVERT) ?
> +			bctl->meta.target : fs_info->avail_metadata_alloc_bits;
> +		data_target = (bctl->data.flags & BTRFS_BALANCE_ARGS_CONVERT) ?
> +			bctl->data.target : fs_info->avail_data_alloc_bits;
>  	} while (read_seqretry(&fs_info->profiles_lock, seq));
>  
> -	/* if we're not converting, the target field is uninitialized */
> -	meta_target = (bctl->meta.flags & BTRFS_BALANCE_ARGS_CONVERT) ?
> -		bctl->meta.target : fs_info->avail_metadata_alloc_bits;
> -	data_target = (bctl->data.flags & BTRFS_BALANCE_ARGS_CONVERT) ?
> -		bctl->data.target : fs_info->avail_data_alloc_bits;
> +	if (reducing_integrity) {
> +		if (bctl->flags & BTRFS_BALANCE_FORCE) {
> +			btrfs_info(fs_info,
> +				   "balance: force reducing metadata integrity");
> +		} else {
> +			btrfs_err(fs_info,
> +	  "balance: reduces metadata integrity, use --force if you want this");
> +			ret = -EINVAL;
> +			goto out;
> +		}
> +	}
> +
>  	if (btrfs_get_num_tolerated_disk_barrier_failures(meta_target) <
>  		btrfs_get_num_tolerated_disk_barrier_failures(data_target)) {
>  		int meta_index = btrfs_bg_flags_to_raid_index(meta_target);
> 

Re: [PATCH] Btrfs: fix access to available allocation bits when starting balance

[David Sterba]

On Mon, Nov 19, 2018 at 09:48:12AM +0000, fdmanana@kernel.org wrote:
> From: Filipe Manana <fdmanana@suse.com>
> 
> The available allocation bits members from struct btrfs_fs_info are
> protected by a sequence lock, and when starting balance we access them
> incorrectly in two different ways:
> 
> 1) In the read sequence lock loop at btrfs_balance() we use the values we
>    read from fs_info->avail_*_alloc_bits and we can immediately do actions
>    that have side effects and can not be undone (printing a message and
>    jumping to a label). This is wrong because a retry might be needed, so
>    our actions must not have side effects and must be repeatable as long
>    as read_seqretry() returns a non-zero value. In other words, we were
>    essentially ignoring the sequence lock;
> 
> 2) Right below the read sequence lock loop, we were reading the values
>    from avail_metadata_alloc_bits and avail_data_alloc_bits without any
>    protection from concurrent writers, that is, reading them outside of
>    the read sequence lock critical section.
> 
> So fix this by making sure we only read the available allocation bits
> while in a read sequence lock critical section and that what we do in the
> critical section is repeatable (has nothing that can not be undone) so
> that any eventual retry that is needed is handled properly.
> 
> Fixes: de98ced9e743 ("Btrfs: use seqlock to protect fs_info->avail_{data, metadata, system}_alloc_bits")
> Fixes: 14506127979a ("btrfs: fix a bogus warning when converting only data or metadata")
> Signed-off-by: Filipe Manana <fdmanana@suse.com>

Added to misc-next, thanks.