linux-kernel - Re: [PATCH 3/4] btrfs: Fix transaction abort during failure in insert_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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAL3q7H7crVYyQrffLWO6Fr8QpFS2B0BjURqrpTYDYbX3ki0imA@mail.gmail.com>
Date: Tue, 8 Apr 2025 15:50:33 +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 3/4] btrfs: Fix transaction abort during failure in insert_balance_item()

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

But why should we do it?
More below.

>
> Signed-off-by: Yangtao Li <frank.li@...o.com>
> ---
>  fs/btrfs/volumes.c | 16 ++++++++--------
>  1 file changed, 8 insertions(+), 8 deletions(-)
>
> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> index 375d92720e17..347c475028e0 100644
> --- a/fs/btrfs/volumes.c
> +++ b/fs/btrfs/volumes.c
> @@ -3733,7 +3733,7 @@ static int insert_balance_item(struct btrfs_fs_info *fs_info,
>         BTRFS_PATH_AUTO_FREE(path);
>         struct extent_buffer *leaf;
>         struct btrfs_key key;
> -       int ret, err;
> +       int ret;
>
>         path = btrfs_alloc_path();
>         if (!path)
> @@ -3749,8 +3749,11 @@ static int insert_balance_item(struct btrfs_fs_info *fs_info,
>
>         ret = btrfs_insert_empty_item(trans, root, path, &key,
>                                       sizeof(*item));
> -       if (ret)
> -               goto out;
> +       if (ret) {
> +               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;
> +       }
>
>         leaf = path->nodes[0];
>         item = btrfs_item_ptr(leaf, path->slots[0], struct btrfs_balance_item);
> @@ -3764,11 +3767,8 @@ static int insert_balance_item(struct btrfs_fs_info *fs_info,
>         btrfs_cpu_balance_args_to_disk(&disk_bargs, &bctl->sys);
>         btrfs_set_balance_sys(leaf, item, &disk_bargs);
>         btrfs_set_balance_flags(leaf, item, bctl->flags);
> -out:
> -       err = btrfs_commit_transaction(trans);
> -       if (err && !ret)
> -               ret = err;
> -       return ret;
> +
> +       return btrfs_commit_transaction(trans);
>  }
>
>  static int del_balance_item(struct btrfs_fs_info *fs_info)
> --
> 2.39.0
>
>