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

[David Sterba <dsterba@suse.cz>]

On Fri, Feb 04, 2022 at 11:29:51AM +0000, Filipe Manana wrote:
> 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().

Yeah that's a good idea.

> And then having check_setget_bounds() call btrfs_handle_fs_error().

btrfs_handle_fs_error is a bit heavyweight for all the potential cases
where the eb member check could happen.

> 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.

Agreed with the BTRFS_FS_ERROR part, I'm not sure about calling the
btrfs_handle_fs_error. The function was introduced before the
transaction abort mechanism, which builds on top of it, but there are
still calls to btrfs_handle_fs_error that seem to substitute abort.
Conversions like ba51e2a11e38 ("btrfs: change handle_fs_error in
recover_log_trees to aborts") need to happen, there are still like 30 of
them.