Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Tue, 28 Mar 2023 01:05:53 +0200
From:   David Sterba <dsterba@suse.cz>
To:     xiaoshoukui <xiaoshoukui@gmail.com>
Cc:     clm@fb.com, josef@toxicpanda.com, dsterba@suse.com,
        linux-btrfs@vger.kernel.org, linux-kernel@vger.kernel.org,
        xiaoshoukui <xiaoshoukui@ruijie.com.cn>
Subject: Re: [PATCH] btrfs: ioctl: fix inaccurate determination of
 exclusive_operation
Message-ID: <20230327230553.GJ10580@twin.jikos.cz>
Reply-To: dsterba@suse.cz
References: <20230324031611.98986-1-xiaoshoukui@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230324031611.98986-1-xiaoshoukui@gmail.com>
User-Agent: Mutt/1.5.23.1-rc1 (2014-03-12)
Precedence: bulk

On Thu, Mar 23, 2023 at 11:16:11PM -0400, xiaoshoukui wrote:
> with fs_info->exclusive_operation == BTRFS_EXCLOP_DEV_ADD enter
> btrfs_ioctl_add_dev function , exclusive_operation will be classified
> as in paused balance operation. After return from btrfs_ioctl_add_dev,
> exclusive_operation will be restore to BTRFS_EXCLOP_BALANCE_PAUSED which
> is not its original state.

Sorry, I don't understand what you mean. The paused balance and 'device
add' are supposed to be compatible exclusive operations (see commit
a174c0a2e857 ("btrfs: allow device add if balance is paused")).

Have you found some bug with the above or is there other combination of
the exclusive operations that should not work? The changes to the state
values are the same, besides the wrong locking.

> Signed-off-by: xiaoshoukui <xiaoshoukui@ruijie.com.cn>
> ---
>  fs/btrfs/ioctl.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
> index a0ef1a1784c7..aab5fdb9445c 100644
> --- a/fs/btrfs/ioctl.c
> +++ b/fs/btrfs/ioctl.c
> @@ -2629,7 +2629,7 @@ static long btrfs_ioctl_add_dev(struct btrfs_fs_info *fs_info, void __user *arg)
>  	}
>  
>  	if (!btrfs_exclop_start(fs_info, BTRFS_EXCLOP_DEV_ADD)) {
> -		if (!btrfs_exclop_start_try_lock(fs_info, BTRFS_EXCLOP_DEV_ADD))
> +		if (fs_info->exclusive_operation != BTRFS_EXCLOP_BALANCE_PAUSED)

This is removing the atomicity of the check so it's racy and could
forcibly overwrite the exclusive operation to BTRFS_EXCLOP_DEV_ADD
without the protecting the whole critical section.

>  			return BTRFS_ERROR_DEV_EXCL_RUN_IN_PROGRESS;
>  
>  		/*
> @@ -2637,8 +2637,9 @@ static long btrfs_ioctl_add_dev(struct btrfs_fs_info *fs_info, void __user *arg)
>  		 * change the exclusive op type and remember we should bring
>  		 * back the paused balance
>  		 */
> +		spin_lock(&fs_info->super_lock);

What if there's another exclusive operation started before this lock is
taken?

>  		fs_info->exclusive_operation = BTRFS_EXCLOP_DEV_ADD;
> -		btrfs_exclop_start_unlock(fs_info);
> +		spin_unlock(&fs_info->super_lock);
>  		restore_op = true;
>  	}