Re: [PATCH 1/2] btrfs: Abort transaction if btrfs_update_root fails in btrfs_ioctl_subvol_setflags

[Nikolay Borisov <nborisov@suse.com>]

On 26.09.2017 20:39, David Sterba wrote:
> On Tue, Sep 26, 2017 at 05:27:41PM +0300, Nikolay Borisov wrote:
>> btrfs_update_root can fail for any number of reasons, however the only error
>> handling we do is to revert the modified flags, yet we do not abort the
>> transaction but proceed to commit it.
> 
> AFAICS btrfs_update_root aborts the transaction itself, so it's not
> needed in the caller.
> 
>> Fix this by explicitly checking if the
>> update root routine has failed and abort the transaction.
>>
>> Fixes: 0caa102da827 ("Btrfs: Add BTRFS_IOC_SUBVOL_GETFLAGS/SETFLAGS ioctls")
>> Signed-off-by: Nikolay Borisov <nborisov@suse.com>
>> ---
>>  fs/btrfs/ioctl.c | 3 +++
>>  1 file changed, 3 insertions(+)
>>
>> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
>> index d6715c2bcdc4..09fcd51f0e8c 100644
>> --- a/fs/btrfs/ioctl.c
>> +++ b/fs/btrfs/ioctl.c
>> @@ -1842,8 +1842,11 @@ static noinline int btrfs_ioctl_subvol_setflags(struct file *file,
>>  
>>  	ret = btrfs_update_root(trans, fs_info->tree_root,
>>  				&root->root_key, &root->root_item);
>> +	if (ret < 0)
>> +		btrfs_abort_transaction(trans, ret);
>>  
>>  	btrfs_commit_transaction(trans);
> 
> I think the error from commit_transaction should be returned as well if
> we get here.
> 
> So:
> 
> ret = btrfs_update_root();
> 
> if (ret < 0) {
> 	end_transaction();
> 	goto out_reset;
> }
> 
> ret = btrfs_commit_transaction();
> 
> out_reset:
> /* and then as it is */
> if (ret)
> 	btrfs_set_root_flags(...);
> 
> etc.

I think even this is not needed, since btrfs_commit_transaction actually
handles aborted transaction by calling btrfs_end_transaction and
returning the error with which the trans was aborted. So I will just
drop this patch


> 
> 
> 
>> +
>>  out_reset:
>>  	if (ret)
>>  		btrfs_set_root_flags(&root->root_item, root_flags);
>> -- 
>> 2.7.4
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html