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

[Filipe Manana <fdmanana@kernel.org>]

On Thu, Feb 10, 2022 at 06:50:17PM +0100, David Sterba wrote:
> 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.

Well, btrfs_handle_fs_error() is handy when we don't have access to a
transaction and we need to prevent a future transaction from starting.

Another alternative, instead of adding that new bit, simply doing something
like the following at check_setget_bounds():

    if (current->journal_info)
        btrfs_abort_transaction(current->journal_info, -EUCLEAN);

For a task doing reads only, and that nevers joins/starts a transaction,
in case it calls a getter that does an out of bounds access, there's no
way to return an error back to user space anyway.

There's always the case a task may do such a bad get and later join/start
a transaction and call a setter with a value computed on top of a bad
value returned by the bad getter.

I still think btrfs_handle_fs_error() is the best to use here. It fits
perfectly for this scenario, without neither the need to add a new bit
nor to sprinkle more logic into transaction.c