linux-kernel - Re: [PATCH 4/4] btrfs: Fix transaction abort during failure in del_balance

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 linux-cve-announce 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]

Message-ID: <CAL3q7H7BS6juCS0eRdo6sqM4jzeMMi1o=huG38wgKYumD7qmmw@mail.gmail.com>
Date: Tue, 8 Apr 2025 15:53:04 +0100
From: Filipe Manana <fdmanana@...nel.org>
To: Yangtao Li <frank.li@...o.com>
Cc: clm@...com, josef@...icpanda.com, dsterba@...e.com, 
	linux-btrfs@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 4/4] btrfs: Fix transaction abort during failure in del_balance_item()

On Tue, Apr 8, 2025 at 1:19 PM Yangtao Li <frank.li@...o.com> wrote:
>
> Handle errors by adding explicit btrfs_abort_transaction
> and btrfs_end_transaction calls.

Again, like in the previous patch, why?
This provides no reason at all why we should abort.
And the same comment below.

>
> Signed-off-by: Yangtao Li <frank.li@...o.com>
> ---
>  fs/btrfs/volumes.c | 11 +++++++----
>  1 file changed, 7 insertions(+), 4 deletions(-)
>
> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> index 347c475028e0..23739d18d833 100644
> --- a/fs/btrfs/volumes.c
> +++ b/fs/btrfs/volumes.c
> @@ -3777,7 +3777,7 @@ static int del_balance_item(struct btrfs_fs_info *fs_info)
>         struct btrfs_trans_handle *trans;
>         BTRFS_PATH_AUTO_FREE(path);
>         struct btrfs_key key;
> -       int ret, err;
> +       int ret;
>
>         path = btrfs_alloc_path();
>         if (!path)
> @@ -3800,10 +3800,13 @@ static int del_balance_item(struct btrfs_fs_info *fs_info)
>         }
>
>         ret = btrfs_del_item(trans, root, path);
> +       if (ret)
> +               goto out;
> +
> +       return btrfs_commit_transaction(trans);
>  out:
> -       err = btrfs_commit_transaction(trans);
> -       if (err && !ret)
> -               ret = err;
> +       btrfs_abort_transaction(trans, ret);
> +       btrfs_end_transaction(trans);

A transaction abort will turn the fs into RO mode, and it's meant to
be used when we can't proceed with changes to the fs after we did
partial changes, to avoid leaving things in an inconsistent state.
Here we don't abort because we haven't done any changes before using
the transaction handle, so an abort is pointless and will turn the fs
into RO mode unnecessarily.

Thanks.

>         return ret;
>  }
>
> --
> 2.39.0
>
>