linux-kernel - Re: [PATCH] btrfs: push memalloc_nofs_save/restore() out of alloc

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: <CAL3q7H6BQpWAtfkA+tnW-fBkdqSDb4udAeJvdgHfKSKfpYWAOA@mail.gmail.com>
Date: Thu, 2 Oct 2025 13:06:15 +0100
From: Filipe Manana <fdmanana@...nel.org>
To: rtapadia730@...il.com
Cc: dsterba@...e.com, clm@...com, fdmanana@...e.com, 
	linux-btrfs@...r.kernel.org, linux-kernel@...r.kernel.org, 
	skhan@...uxfoundation.org, khalid@...nel.org, david.hunter.linux@...il.com
Subject: Re: [PATCH] btrfs: push memalloc_nofs_save/restore() out of alloc_bitmap()

On Thu, Oct 2, 2025 at 12:55 PM <rtapadia730@...il.com> wrote:
>
> From: Rajeev Tapadia <rtapadia730@...il.com>
>
> alloc_bitmap() currently wraps its allocation in memalloc_nofs_save
> /restore(), but this hides allocation context from callers. GFP_NOFS is
> required to avoid recursion into the filesystem during transaction commits,

Not just during transaction commits, but because in every context we
call alloc_bitmap() we are holding a transaction handle open, so if
the memory allocation recurses into the filesystem and triggers the
current transaction's commit, we deadlock - that's why we need NOFS.

> but the correct place to enforce that is at the call sites where we know
> recursion is unsafe.
>
> So now alloc_bitmap() just allocates a bitmap. Also completing the TODO
> comment.

No. Think for a moment...

alloc_bitmap() only has two callers, and in both of them we need a
NOFS allocation since we are holding a transaction handle...
So it only makes sense to leave the setup of NOFS in alloc_bitmap() -
do not move it to the callers and duplicate code...

Just fix the comment, removing the TODO and mention that the reason we
can't recurse is because we are holding a transaction handle open - it
doesn't matter if we are in critical section of a transaction commit
or not - all it matters is that we are holding a transaction handle
open, so a GFP_KERNEL allocation results in a deadlock if it recurses
to the filesystem to commit the transaction.


>
> Signed-off-by: Rajeev Tapadia <rtapadia730@...il.com>
> ---
>
> The patch was tested by enabling CONFIG_BTRFS_FS_RUN_SANITY_TESTS
> All tests passed while booting the kernel in qemu.

We test btrfs with fstests. The sanity tests aren't enough, they are
mostly to test internal functions.

>
>  fs/btrfs/free-space-tree.c | 16 ++++++++--------
>  1 file changed, 8 insertions(+), 8 deletions(-)
>
> diff --git a/fs/btrfs/free-space-tree.c b/fs/btrfs/free-space-tree.c
> index dad0b492a663..abdbdc74edf8 100644
> --- a/fs/btrfs/free-space-tree.c
> +++ b/fs/btrfs/free-space-tree.c
> @@ -159,8 +159,6 @@ static inline u32 free_space_bitmap_size(const struct btrfs_fs_info *fs_info,
>
>  static unsigned long *alloc_bitmap(u32 bitmap_size)
>  {
> -       unsigned long *ret;
> -       unsigned int nofs_flag;
>         u32 bitmap_rounded_size = round_up(bitmap_size, sizeof(unsigned long));
>
>         /*
> @@ -168,13 +166,11 @@ static unsigned long *alloc_bitmap(u32 bitmap_size)
>          * into the filesystem as the free space bitmap can be modified in the
>          * critical section of a transaction commit.
>          *
> -        * TODO: push the memalloc_nofs_{save,restore}() to the caller where we
> -        * know that recursion is unsafe.
> +        * This function's caller is responsible for setting the appropriate
> +        * allocation context (e.g., using memalloc_nofs_save/restore())
> +        * to prevent recursion.
>          */
> -       nofs_flag = memalloc_nofs_save();
> -       ret = kvzalloc(bitmap_rounded_size, GFP_KERNEL);
> -       memalloc_nofs_restore(nofs_flag);
> -       return ret;
> +       return kvzalloc(bitmap_rounded_size, GFP_KERNEL);
>  }
>
>  static void le_bitmap_set(unsigned long *map, unsigned int start, int len)
> @@ -217,7 +213,9 @@ int btrfs_convert_free_space_to_bitmaps(struct btrfs_trans_handle *trans,
>         int ret;
>
>         bitmap_size = free_space_bitmap_size(fs_info, block_group->length);
> +       unsigned int nofs_flag = memalloc_nofs_save();

Please don't declare variables in the middle of the code.
We always declare variables at the top of the current scope.

>         bitmap = alloc_bitmap(bitmap_size);
> +       memalloc_nofs_restore(nofs_flag);
>         if (unlikely(!bitmap)) {
>                 ret = -ENOMEM;
>                 btrfs_abort_transaction(trans, ret);
> @@ -360,7 +358,9 @@ int btrfs_convert_free_space_to_extents(struct btrfs_trans_handle *trans,
>         int ret;
>
>         bitmap_size = free_space_bitmap_size(fs_info, block_group->length);
> +       unsigned int nofs_flag = memalloc_nofs_save();
>         bitmap = alloc_bitmap(bitmap_size);
> +       memalloc_nofs_restore(nofs_flag);
>         if (unlikely(!bitmap)) {
>                 ret = -ENOMEM;
>                 btrfs_abort_transaction(trans, ret);
> --
> 2.51.0
>
>