Re: [PATCH 4/5] btrfs: fail transaction when a setget bounds check failure is detected

[Filipe Manana <fdmanana@kernel.org>]

On Thu, Feb 03, 2022 at 06:26:31PM +0100, David Sterba wrote:
> As the setget check only sets the bit, we need to use it in the
> transaction:
> 
> - when attempting to start a new one, fail with EROFS as if would be
>   aborted in another way already
> 
> - in should_end_transaction
> 
> - when transaction is about to end, insert an explicit abort
> 
> Signed-off-by: David Sterba <dsterba@suse.com>
> ---
>  fs/btrfs/transaction.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/fs/btrfs/transaction.c b/fs/btrfs/transaction.c
> index 6db634ebae17..f48194df6c33 100644
> --- a/fs/btrfs/transaction.c
> +++ b/fs/btrfs/transaction.c
> @@ -591,6 +591,9 @@ start_transaction(struct btrfs_root *root, unsigned int num_items,
>  	if (BTRFS_FS_ERROR(fs_info))
>  		return ERR_PTR(-EROFS);
>  
> +	if (test_bit(BTRFS_FS_SETGET_COMPLAINS, &fs_info->flags))
> +		return ERR_PTR(-EROFS);
> +
>  	if (current->journal_info) {
>  		WARN_ON(type & TRANS_EXTWRITERS);
>  		h = current->journal_info;
> @@ -924,6 +927,9 @@ static bool should_end_transaction(struct btrfs_trans_handle *trans)
>  {
>  	struct btrfs_fs_info *fs_info = trans->fs_info;
>  
> +	if (test_bit(BTRFS_FS_SETGET_COMPLAINS, &fs_info->flags))
> +		return true;
> +
>  	if (btrfs_check_space_for_delayed_refs(fs_info))
>  		return true;
>  
> @@ -969,6 +975,11 @@ static int __btrfs_end_transaction(struct btrfs_trans_handle *trans,
>  	struct btrfs_transaction *cur_trans = trans->transaction;
>  	int err = 0;
>  
> +	/* If a serious error was detected abort the transaction early */
> +	if (!TRANS_ABORTED(trans) &&
> +	    test_bit(BTRFS_FS_SETGET_COMPLAINS, &info->flags))
> +		btrfs_abort_transaction(trans, -EIO);

Instead of sprinkling the test for BTRFS_FS_SETGET_COMPLAINS in all
these places, it seems to me it could be included in BTRFS_FS_ERROR().
And then having check_setget_bounds() call btrfs_handle_fs_error().

That would remove the need for all this code. Wouldn't it?

> +
>  	if (refcount_read(&trans->use_count) > 1) {
>  		refcount_dec(&trans->use_count);
>  		trans->block_rsv = trans->orig_rsv;

This misses one important case:

  task starts/joins/attaches a transaction

  fails one of the bounds check when accessing some extent buffer

  calls btrfs_commit_transaction()

The transaction ends up committed.

So a check and abort in the commit path, right before writing the super blocks,
should be in place.

With the above suggestions for check_setget_bounds() and BTRFS_FS_ERROR(),
this case would be handled automatically like the others, so no need for
sprinkling the checks and aborts in several places.

Thanks.

> -- 
> 2.34.1
>