linux-kernel - Re: [PATCH] fs: Fix freeze_bdev()/thaw_bdev() accounting of bd_fsfreeze

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 PHC
Open Source and information security mailing list archives

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

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

Date:   Mon, 4 Jan 2021 13:58:02 -0800
From:   "Darrick J. Wong" <darrick.wong@...cle.com>
To:     Satya Tangirala <satyat@...gle.com>
Cc:     Alexander Viro <viro@...iv.linux.org.uk>,
        Christoph Hellwig <hch@....de>, linux-fsdevel@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] fs: Fix freeze_bdev()/thaw_bdev() accounting of
 bd_fsfreeze_sb

On Thu, Dec 24, 2020 at 04:49:54AM +0000, Satya Tangirala wrote:
> freeze/thaw_bdev() currently use bdev->bd_fsfreeze_count to infer
> whether or not bdev->bd_fsfreeze_sb is valid (it's valid iff
> bd_fsfreeze_count is non-zero). thaw_bdev() doesn't nullify
> bd_fsfreeze_sb.
> 
> But this means a freeze_bdev() call followed by a thaw_bdev() call can
> leave bd_fsfreeze_sb with a non-null value, while bd_fsfreeze_count is
> zero. If freeze_bdev() is called again, and this time
> get_active_super() returns NULL (e.g. because the FS is unmounted),
> we'll end up with bd_fsfreeze_count > 0, but bd_fsfreeze_sb is
> *untouched* - it stays the same (now garbage) value. A subsequent
> thaw_bdev() will decide that the bd_fsfreeze_sb value is legitimate
> (since bd_fsfreeze_count > 0), and attempt to use it.
> 
> Fix this by always setting bd_fsfreeze_sb to NULL when
> bd_fsfreeze_count is successfully decremented to 0 in thaw_sb().
> Alternatively, we could set bd_fsfreeze_sb to whatever
> get_active_super() returns in freeze_bdev() whenever bd_fsfreeze_count
> is successfully incremented to 1 from 0 (which can be achieved cleanly
> by moving the line currently setting bd_fsfreeze_sb to immediately
> after the "sync:" label, but it might be a little too subtle/easily
> overlooked in future).
> 
> This fixes the currently panicking xfstests generic/085.
> 
> Fixes: 040f04bd2e82 ("fs: simplify freeze_bdev/thaw_bdev")
> Signed-off-by: Satya Tangirala <satyat@...gle.com>

I came up with the same solution to the same crash, so:

Reviewed-by: Darrick J. Wong <darrick.wong@...cle.com>

--D

> ---
>  fs/block_dev.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/fs/block_dev.c b/fs/block_dev.c
> index 9e56ee1f2652..12a811a9ae4b 100644
> --- a/fs/block_dev.c
> +++ b/fs/block_dev.c
> @@ -606,6 +606,8 @@ int thaw_bdev(struct block_device *bdev)
>  		error = thaw_super(sb);
>  	if (error)
>  		bdev->bd_fsfreeze_count++;
> +	else
> +		bdev->bd_fsfreeze_sb = NULL;
>  out:
>  	mutex_unlock(&bdev->bd_fsfreeze_mutex);
>  	return error;
> -- 
> 2.29.2.729.g45daf8777d-goog
>