linux-kernel - Re: [PATCH] btrfs: ioctl: fix inaccurate determination of exclusive

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20230331174533.GZ10580@twin.jikos.cz>
Date:   Fri, 31 Mar 2023 19:45:33 +0200
From:   David Sterba <dsterba@...e.cz>
To:     xiaoshoukui <xiaoshoukui@...il.com>
Cc:     dsterba@...e.cz, clm@...com, josef@...icpanda.com,
        dsterba@...e.com, linux-btrfs@...r.kernel.org,
        linux-kernel@...r.kernel.org, xiaoshoukui@...jie.com.cn
Subject: Re: [PATCH] btrfs: ioctl: fix inaccurate determination of
 exclusive_operation

On Tue, Mar 28, 2023 at 05:43:35AM -0400, xiaoshoukui wrote:
> > 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.
> 
> Yes, there is a racy bewteen btrfs_exclop_balance and btrfs_exclop_finish
> in btrfs_ioctl_add_dev, when cocurrently adding multiple devices to the
> same mnt point. That will cause the assertion in btrfs_exclop_balance to fail.
> 
> > void btrfs_exclop_balance(struct btrfs_fs_info *fs_info,
> > 			  enum btrfs_exclusive_operation op)
> > {
> > 	switch (op) {
> > 	case BTRFS_EXCLOP_BALANCE_PAUSED:
> > 		spin_lock(&fs_info->super_lock);
> > 		ASSERT(fs_info->exclusive_operation == BTRFS_EXCLOP_BALANCE ||
> > 		       fs_info->exclusive_operation == BTRFS_EXCLOP_DEV_ADD);
> 
> when btrfs_exclop_finish function was executed before the ASSERT, the
> fs_info->exclusive_operation will change to BTRFS_EXCLOP_NONE. So this
> assert will failed.
>
> Please review whether we should patch the assert to add BTRFS_EXCLOP_NONE condtion.
> I'll post a patch if needed. thx.

Yeah I think the assertion should also check for NONE status. The paused
balance makes the state tracking harder but in user-started (manual or
scripted) commands it's typically not racing.

btrfs_exclop_start_try_lock does not allow to do the change from
none -> op mandating an explicit btrfs_exclop_start first but the
assertions do not care about that.