linux-kernel - Re: [PATCH v2] btrfs: put all allocated extent buffer folios in failure case

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: <20250424182154.GB3340135@zen.localdomain>
Date: Thu, 24 Apr 2025 11:21:54 -0700
From: Boris Burkov <boris@....io>
To: Daniel Vacek <neelx@...e.com>
Cc: Chris Mason <clm@...com>, Josef Bacik <josef@...icpanda.com>,
	David Sterba <dsterba@...e.com>, linux-btrfs@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] btrfs: put all allocated extent buffer folios in
 failure case

On Thu, Apr 24, 2025 at 05:08:08PM +0200, Daniel Vacek wrote:
> When attaching a folio fails, for example if another one is already mapped,
> we need to put all newly allocated folios. And as a consequence we do not
> need to flag the eb UNMAPPED anymore.
> 
> Signed-off-by: Daniel Vacek <neelx@...e.com>

Reviewed-by: Boris Burkov <boris@....io>

> ---
>  fs/btrfs/extent_io.c | 32 ++++++++++++++------------------
>  1 file changed, 14 insertions(+), 18 deletions(-)
> 
> diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
> index 197f5e51c4744..7023dd527d3e7 100644
> --- a/fs/btrfs/extent_io.c
> +++ b/fs/btrfs/extent_io.c
> @@ -3385,30 +3385,26 @@ struct extent_buffer *alloc_extent_buffer(struct btrfs_fs_info *fs_info,
>  	 * we'll lookup the folio for that index, and grab that EB.  We do not
>  	 * want that to grab this eb, as we're getting ready to free it.  So we
>  	 * have to detach it first and then unlock it.
> -	 *
> -	 * We have to drop our reference and NULL it out here because in the
> -	 * subpage case detaching does a btrfs_folio_dec_eb_refs() for our eb.
> -	 * Below when we call btrfs_release_extent_buffer() we will call
> -	 * detach_extent_buffer_folio() on our remaining pages in the !subpage
> -	 * case.  If we left eb->folios[i] populated in the subpage case we'd
> -	 * double put our reference and be super sad.
>  	 */
> -	for (int i = 0; i < attached; i++) {
> -		ASSERT(eb->folios[i]);
> -		detach_extent_buffer_folio(eb, eb->folios[i]);
> -		folio_unlock(eb->folios[i]);
> -		folio_put(eb->folios[i]);
> +	for (int i = 0; i < num_extent_pages(eb); i++) {
> +		struct folio *folio = eb->folios[i];
> +
> +		if (i < attached) {
> +			ASSERT(folio);
> +			detach_extent_buffer_folio(eb, folio);
> +			folio_unlock(folio);
> +		} else if (!folio)
> +			continue;
> +
> +		ASSERT(!folio_test_private(folio));
> +		folio_put(folio);
>  		eb->folios[i] = NULL;
>  	}
> -	/*
> -	 * Now all pages of that extent buffer is unmapped, set UNMAPPED flag,
> -	 * so it can be cleaned up without utilizing folio->mapping.
> -	 */
> -	set_bit(EXTENT_BUFFER_UNMAPPED, &eb->bflags);
> -
>  	btrfs_release_extent_buffer(eb);
> +
>  	if (ret < 0)
>  		return ERR_PTR(ret);
> +
>  	ASSERT(existing_eb);
>  	return existing_eb;
>  }
> -- 
> 2.47.2
>