[PATCH] btrfs: unlock all extent buffer folios in failure case

[PATCH] btrfs: unlock all extent buffer folios in failure case

[Daniel Vacek]

When attaching a folio fails, for example if another one is already mapped,
we need to unlock all newly allocated folios before putting them. And as a
consequence we do not need to flag the eb UNMAPPED anymore.

Signed-off-by: Daniel Vacek <neelx@suse.com>
---
 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_folios(eb); i++) {
+		struct folio *folio = eb->folios[i];
+
+		if (i < attached) {
+			ASSERT(folio);
+			detach_extent_buffer_folio(eb, folio);
+		} else if (!folio)
+			continue;
+
+		ASSERT(!folio_test_private(folio));
+		folio_unlock(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

Re: [PATCH] btrfs: unlock all extent buffer folios in failure case

[Klara Modin]

[-- Attachment #1: Type: text/plain, Size: 9929 bytes --]

Hi,

On Tue, Apr 22, 2025 at 02:57:01PM +0200, Daniel Vacek wrote:
> When attaching a folio fails, for example if another one is already mapped,
> we need to unlock all newly allocated folios before putting them. And as a
> consequence we do not need to flag the eb UNMAPPED anymore.
> 
> Signed-off-by: Daniel Vacek <neelx@suse.com>

I hit a null pointer dereference in next-20250424 which seems to point
to this commit. Reverting it resolves the issue for me (did not bisect).

Please let me know if there's anything else you need.

Regrads,
Klara Modin

BUG: kernel NULL pointer dereference, address: 0000000000000000
nct6683 nct6683.2592: NCT6687D EC firmware version 1.0 build 05/07/20
#PF: supervisor read access in kernel mode
#PF: error_code(0x0000) - not-present page
PGD 0 P4D 0
Oops: Oops: 0000 [#1] SMP NOPTI
CPU: 19 UID: 0 PID: 1138 Comm: (udev-worker) Not tainted 6.15.0-rc3-next-20250424 #474 PREEMPTLAZY
Hardware name: Micro-Star International Co., Ltd. MS-7C91/MAG B550 TOMAHAWK (MS-7C91), BIOS A.G0 03/12/2024
RIP: 0010:alloc_extent_buffer (arch/x86/include/asm/bitops.h:206 arch/x86/include/asm/bitops.h:238 include/asm-generic/bitops/instrumented-non-atomic.h:142 include/linux/page-flags.h:860 include/linux/page-flags.h:881 include/linux/mm.h:994 fs/btrfs/extent_io.h:290 fs/btrfs/extent_io.c:3360) 
Code: ad 8f cb ff f0 ff 4b 34 0f 84 9a 01 00 00 4b c7 84 f4 a8 00 00 00 00 00 00 00 41 8b 74 24 08 49 8b 8c 24 a8 00 00 00 41 ff c5 <48> 8b 01 a8 40 74 0b 80 79 40 00 b8 01 00 00 00 75 0d 89 f0 ba 01
All code
========
   0:	ad                   	lods   %ds:(%rsi),%eax
   1:	8f                   	(bad)
   2:	cb                   	lret
   3:	ff f0                	push   %rax
   5:	ff 4b 34             	decl   0x34(%rbx)
   8:	0f 84 9a 01 00 00    	je     0x1a8
   e:	4b c7 84 f4 a8 00 00 	movq   $0x0,0xa8(%r12,%r14,8)
  15:	00 00 00 00 00 
  1a:	41 8b 74 24 08       	mov    0x8(%r12),%esi
  1f:	49 8b 8c 24 a8 00 00 	mov    0xa8(%r12),%rcx
  26:	00 
  27:	41 ff c5             	inc    %r13d
  2a:*	48 8b 01             	mov    (%rcx),%rax		<-- trapping instruction
  2d:	a8 40                	test   $0x40,%al
  2f:	74 0b                	je     0x3c
  31:	80 79 40 00          	cmpb   $0x0,0x40(%rcx)
  35:	b8 01 00 00 00       	mov    $0x1,%eax
  3a:	75 0d                	jne    0x49
  3c:	89 f0                	mov    %esi,%eax
  3e:	ba                   	.byte 0xba
  3f:	01                   	.byte 0x1

Code starting with the faulting instruction
===========================================
   0:	48 8b 01             	mov    (%rcx),%rax
   3:	a8 40                	test   $0x40,%al
   5:	74 0b                	je     0x12
   7:	80 79 40 00          	cmpb   $0x0,0x40(%rcx)
   b:	b8 01 00 00 00       	mov    $0x1,%eax
  10:	75 0d                	jne    0x1f
  12:	89 f0                	mov    %esi,%eax
  14:	ba                   	.byte 0xba
  15:	01                   	.byte 0x1
RSP: 0018:ffffac790119b740 EFLAGS: 00010202
RAX: 0000000000000013 RBX: fffff9cd44725bc0 RCX: 0000000000000000
RDX: 00000000000003a4 RSI: 0000000000004000 RDI: ffff95ca3efd3040
RBP: 0000000000000000 R08: 0000000000000000 R09: 00000000000094b5
R10: 0000000000000000 R11: 00000000000005df R12: ffff95c351019848
R13: 0000000000000001 R14: 0000000000000000 R15: ffff95c34b438250
FS:  00007fd880dad840(0000) GS:ffff95caa9f0e000(0000) knlGS:0000000000000000
CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 0000000000000000 CR3: 0000000108412000 CR4: 0000000000350ef0
Call Trace:
 <TASK>
read_block_for_search (fs/btrfs/ctree.c:1533) 
btrfs_search_slot (fs/btrfs/ctree.c:2173 (discriminator 1)) 
? srso_return_thunk (arch/x86/lib/retpoline.S:224) 
? mempool_alloc_noprof (mm/mempool.c:402) 
btrfs_lookup_csum (fs/btrfs/file-item.c:249) 
? btrfs_csum_root (fs/btrfs/disk-io.c:828) 
btrfs_lookup_bio_sums (fs/btrfs/file-item.c:312 (discriminator 1) fs/btrfs/file-item.c:406 (discriminator 1)) 
btrfs_submit_chunk (fs/btrfs/bio.c:717) 
? btrfs_clear_extent_bit_changeset (fs/btrfs/extent-io-tree.c:751) 
? srso_untrain_ret (arch/x86/lib/retpoline.S:209) 
? btrfs_clear_extent_bit_changeset (fs/btrfs/extent-io-tree.c:751) 
btrfs_submit_bbio (fs/btrfs/bio.c:791 (discriminator 2)) 
submit_one_bio (fs/btrfs/extent_io.c:132) 
btrfs_readahead (fs/btrfs/extent_io.c:2535) 
? srso_return_thunk (arch/x86/lib/retpoline.S:224) 
? __pfx_end_bbio_data_read (fs/btrfs/extent_io.c:502) 
read_pages (include/linux/pagemap.h:1400 include/linux/pagemap.h:1426 mm/readahead.c:162) 
page_cache_ra_unbounded (include/linux/fs.h:933 mm/readahead.c:298) 
filemap_get_pages (mm/filemap.c:2592) 
? srso_return_thunk (arch/x86/lib/retpoline.S:224) 
? dput (fs/dcache.c:858 fs/dcache.c:896) 
? srso_return_thunk (arch/x86/lib/retpoline.S:224) 
filemap_read (mm/filemap.c:2702) 
? srso_return_thunk (arch/x86/lib/retpoline.S:224) 
? do_filp_open (fs/namei.c:4074 (discriminator 2)) 
? srso_return_thunk (arch/x86/lib/retpoline.S:224) 
? __pfx_page_put_link (fs/namei.c:5454) 
? kmem_cache_alloc_noprof (arch/x86/include/asm/jump_label.h:46 include/linux/memcontrol.h:1696 mm/slub.c:2190 mm/slub.c:4174 mm/slub.c:4213 mm/slub.c:4220) 
vfs_read (fs/read_write.c:489 fs/read_write.c:570) 
ksys_read (fs/read_write.c:714) 
do_syscall_64 (arch/x86/entry/syscall_64.c:63 (discriminator 1) arch/x86/entry/syscall_64.c:94 (discriminator 1)) 
entry_SYSCALL_64_after_hwframe (arch/x86/entry/entry_64.S:130) 
RIP: 0033:0x7fd880694207
Code: 00 49 89 d0 48 89 fa 4c 89 df e8 74 b8 00 00 8b 93 08 03 00 00 59 5e 48 83 f8 fc 74 16 5b c3 0f 1f 40 00 48 8b 44 24 10 0f 05 <5b> c3 0f 1f 80 00 00 00 00 83 e2 39 83 fa 08 75 e2 e8 23 ff ff ff
All code
========
   0:	00 49 89             	add    %cl,-0x77(%rcx)
   3:	d0 48 89             	rorb   $1,-0x77(%rax)
   6:	fa                   	cli
   7:	4c 89 df             	mov    %r11,%rdi
   a:	e8 74 b8 00 00       	call   0xb883
   f:	8b 93 08 03 00 00    	mov    0x308(%rbx),%edx
  15:	59                   	pop    %rcx
  16:	5e                   	pop    %rsi
  17:	48 83 f8 fc          	cmp    $0xfffffffffffffffc,%rax
  1b:	74 16                	je     0x33
  1d:	5b                   	pop    %rbx
  1e:	c3                   	ret
  1f:	0f 1f 40 00          	nopl   0x0(%rax)
  23:	48 8b 44 24 10       	mov    0x10(%rsp),%rax
  28:	0f 05                	syscall
  2a:*	5b                   	pop    %rbx		<-- trapping instruction
  2b:	c3                   	ret
  2c:	0f 1f 80 00 00 00 00 	nopl   0x0(%rax)
  33:	83 e2 39             	and    $0x39,%edx
  36:	83 fa 08             	cmp    $0x8,%edx
  39:	75 e2                	jne    0x1d
  3b:	e8 23 ff ff ff       	call   0xffffffffffffff63

Code starting with the faulting instruction
===========================================
   0:	5b                   	pop    %rbx
   1:	c3                   	ret
   2:	0f 1f 80 00 00 00 00 	nopl   0x0(%rax)
   9:	83 e2 39             	and    $0x39,%edx
   c:	83 fa 08             	cmp    $0x8,%edx
   f:	75 e2                	jne    0xfffffffffffffff3
  11:	e8 23 ff ff ff       	call   0xffffffffffffff39
RSP: 002b:00007ffc7372ce60 EFLAGS: 00000202 ORIG_RAX: 0000000000000000
RAX: ffffffffffffffda RBX: 00007fd880dad840 RCX: 00007fd880694207
RDX: 0000000000000006 RSI: 00007ffc7372cf01 RDI: 0000000000000049
RBP: 0000000000000000 R08: 0000000000000000 R09: 0000000000000000
R10: 0000000000000000 R11: 0000000000000202 R12: 00007ffc7372cf01
R13: 00007ffc7372cf01 R14: 0000000000000049 R15: 00007ffc7372cf01
 </TASK>
Modules linked in: drm_exec btbcm iwlwifi(+) nct6683 gpu_sched snd_hwdep msi_ec(-) drm_suballoc_helper evdev kvm ee1004 bluetooth battery joydev mac_hid snd_hda_core video irqbypass cfg80211 drm_panel_backlight_quirks ghash_clmulni_intel snd_pcm cec sha512_ssse3 sp5100_tco sha256_ssse3 drm_buddy snd_timer sha1_ssse3 watchdog drm_display_helper ccp snd tpm_crb aesni_intel rfkill soundcore pcspkr tpm_tis tpm_tis_core i2c_piix4 k10temp i2c_smbus tpm libaescfb ecdh_generic gpio_amdpt ecc gpio_generic button wmi
CR2: 0000000000000000
---[ end trace 0000000000000000 ]---

> ---
>  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_folios(eb); i++) {
> +		struct folio *folio = eb->folios[i];
> +
> +		if (i < attached) {
> +			ASSERT(folio);
> +			detach_extent_buffer_folio(eb, folio);
> +		} else if (!folio)
> +			continue;
> +
> +		ASSERT(!folio_test_private(folio));
> +		folio_unlock(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
> 

[-- Attachment #2: btrfs_oops3_decoded.gz --]
[-- Type: application/gzip, Size: 19018 bytes --]

[-- Attachment #3: config.gz --]
[-- Type: application/gzip, Size: 45147 bytes --]

[PATCH] btrfs: unlock all extent buffer folios in failure case

[Klara Modin]

[-- Attachment #1: Type: text/plain, Size: 10002 bytes --]


Hi,

On Tue, Apr 22, 2025 at 02:57:01PM +0200, Daniel Vacek wrote:
> When attaching a folio fails, for example if another one is already mapped,
> we need to unlock all newly allocated folios before putting them. And as a
> consequence we do not need to flag the eb UNMAPPED anymore.
> 
> Signed-off-by: Daniel Vacek <neelx@suse.com>

I hit a null pointer dereference in next-20250424 which seems to point
to this commit. Reverting it resolves the issue for me (did not bisect).

Please let me know if there's anything else you need.

Regards,
Klara Modin

BUG: kernel NULL pointer dereference, address: 0000000000000000
nct6683 nct6683.2592: NCT6687D EC firmware version 1.0 build 05/07/20
#PF: supervisor read access in kernel mode
#PF: error_code(0x0000) - not-present page
PGD 0 P4D 0
Oops: Oops: 0000 [#1] SMP NOPTI
CPU: 19 UID: 0 PID: 1138 Comm: (udev-worker) Not tainted 
6.15.0-rc3-next-20250424 #474 PREEMPTLAZY
Hardware name: Micro-Star International Co., Ltd. MS-7C91/MAG B550 
TOMAHAWK (MS-7C91), BIOS A.G0 03/12/2024
RIP: 0010:alloc_extent_buffer (arch/x86/include/asm/bitops.h:206 
arch/x86/include/asm/bitops.h:238 
include/asm-generic/bitops/instrumented-non-atomic.h:142 
include/linux/page-flags.h:860 include/linux/page-flags.h:881 
include/linux/mm.h:994 fs/btrfs/extent_io.h:290 
fs/btrfs/extent_io.c:3360) Code: ad 8f cb ff f0 ff 4b 34 0f 84 9a 01 00 
00 4b c7 84 f4 a8 00 00 00 00 00 00 00 41 8b 74 24 08 49 8b 8c 24 a8 00 
00 00 41 ff c5 <48> 8b 01 a8 40 74 0b 80 79 40 00 b8 01 00 00 00 75 0d 
89 f0 ba 01
All code
========
    0:	ad                   	lods   %ds:(%rsi),%eax
    1:	8f                   	(bad)
    2:	cb                   	lret
    3:	ff f0                	push   %rax
    5:	ff 4b 34             	decl   0x34(%rbx)
    8:	0f 84 9a 01 00 00    	je     0x1a8
    e:	4b c7 84 f4 a8 00 00 	movq   $0x0,0xa8(%r12,%r14,8)
   15:	00 00 00 00 00   1a:	41 8b 74 24 08       	mov    0x8(%r12),%esi
   1f:	49 8b 8c 24 a8 00 00 	mov    0xa8(%r12),%rcx
   26:	00   27:	41 ff c5             	inc    %r13d
   2a:*	48 8b 01             	mov    (%rcx),%rax		<-- trapping instruction
   2d:	a8 40                	test   $0x40,%al
   2f:	74 0b                	je     0x3c
   31:	80 79 40 00          	cmpb   $0x0,0x40(%rcx)
   35:	b8 01 00 00 00       	mov    $0x1,%eax
   3a:	75 0d                	jne    0x49
   3c:	89 f0                	mov    %esi,%eax
   3e:	ba                   	.byte 0xba
   3f:	01                   	.byte 0x1

Code starting with the faulting instruction
===========================================
    0:	48 8b 01             	mov    (%rcx),%rax
    3:	a8 40                	test   $0x40,%al
    5:	74 0b                	je     0x12
    7:	80 79 40 00          	cmpb   $0x0,0x40(%rcx)
    b:	b8 01 00 00 00       	mov    $0x1,%eax
   10:	75 0d                	jne    0x1f
   12:	89 f0                	mov    %esi,%eax
   14:	ba                   	.byte 0xba
   15:	01                   	.byte 0x1
RSP: 0018:ffffac790119b740 EFLAGS: 00010202
RAX: 0000000000000013 RBX: fffff9cd44725bc0 RCX: 0000000000000000
RDX: 00000000000003a4 RSI: 0000000000004000 RDI: ffff95ca3efd3040
RBP: 0000000000000000 R08: 0000000000000000 R09: 00000000000094b5
R10: 0000000000000000 R11: 00000000000005df R12: ffff95c351019848
R13: 0000000000000001 R14: 0000000000000000 R15: ffff95c34b438250
FS:  00007fd880dad840(0000) GS:ffff95caa9f0e000(0000) knlGS:0000000000000000
CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 0000000000000000 CR3: 0000000108412000 CR4: 0000000000350ef0
Call Trace:
  <TASK>
read_block_for_search (fs/btrfs/ctree.c:1533) btrfs_search_slot 
(fs/btrfs/ctree.c:2173 (discriminator 1)) ? srso_return_thunk 
(arch/x86/lib/retpoline.S:224) ? mempool_alloc_noprof (mm/mempool.c:402) 
btrfs_lookup_csum (fs/btrfs/file-item.c:249) ? btrfs_csum_root 
(fs/btrfs/disk-io.c:828) btrfs_lookup_bio_sums (fs/btrfs/file-item.c:312 
(discriminator 1) fs/btrfs/file-item.c:406 (discriminator 1)) 
btrfs_submit_chunk (fs/btrfs/bio.c:717) ? 
btrfs_clear_extent_bit_changeset (fs/btrfs/extent-io-tree.c:751) ? 
srso_untrain_ret (arch/x86/lib/retpoline.S:209) ? 
btrfs_clear_extent_bit_changeset (fs/btrfs/extent-io-tree.c:751) 
btrfs_submit_bbio (fs/btrfs/bio.c:791 (discriminator 2)) submit_one_bio 
(fs/btrfs/extent_io.c:132) btrfs_readahead (fs/btrfs/extent_io.c:2535) ? 
srso_return_thunk (arch/x86/lib/retpoline.S:224) ? 
__pfx_end_bbio_data_read (fs/btrfs/extent_io.c:502) read_pages 
(include/linux/pagemap.h:1400 include/linux/pagemap.h:1426 
mm/readahead.c:162) page_cache_ra_unbounded (include/linux/fs.h:933 
mm/readahead.c:298) filemap_get_pages (mm/filemap.c:2592) ? 
srso_return_thunk (arch/x86/lib/retpoline.S:224) ? dput (fs/dcache.c:858 
fs/dcache.c:896) ? srso_return_thunk (arch/x86/lib/retpoline.S:224) 
filemap_read (mm/filemap.c:2702) ? srso_return_thunk 
(arch/x86/lib/retpoline.S:224) ? do_filp_open (fs/namei.c:4074 
(discriminator 2)) ? srso_return_thunk (arch/x86/lib/retpoline.S:224) ? 
__pfx_page_put_link (fs/namei.c:5454) ? kmem_cache_alloc_noprof 
(arch/x86/include/asm/jump_label.h:46 include/linux/memcontrol.h:1696 
mm/slub.c:2190 mm/slub.c:4174 mm/slub.c:4213 mm/slub.c:4220) vfs_read 
(fs/read_write.c:489 fs/read_write.c:570) ksys_read 
(fs/read_write.c:714) do_syscall_64 (arch/x86/entry/syscall_64.c:63 
(discriminator 1) arch/x86/entry/syscall_64.c:94 (discriminator 1)) 
entry_SYSCALL_64_after_hwframe (arch/x86/entry/entry_64.S:130) RIP: 
0033:0x7fd880694207
Code: 00 49 89 d0 48 89 fa 4c 89 df e8 74 b8 00 00 8b 93 08 03 00 00 59 
5e 48 83 f8 fc 74 16 5b c3 0f 1f 40 00 48 8b 44 24 10 0f 05 <5b> c3 0f 
1f 80 00 00 00 00 83 e2 39 83 fa 08 75 e2 e8 23 ff ff ff
All code
========
    0:	00 49 89             	add    %cl,-0x77(%rcx)
    3:	d0 48 89             	rorb   $1,-0x77(%rax)
    6:	fa                   	cli
    7:	4c 89 df             	mov    %r11,%rdi
    a:	e8 74 b8 00 00       	call   0xb883
    f:	8b 93 08 03 00 00    	mov    0x308(%rbx),%edx
   15:	59                   	pop    %rcx
   16:	5e                   	pop    %rsi
   17:	48 83 f8 fc          	cmp    $0xfffffffffffffffc,%rax
   1b:	74 16                	je     0x33
   1d:	5b                   	pop    %rbx
   1e:	c3                   	ret
   1f:	0f 1f 40 00          	nopl   0x0(%rax)
   23:	48 8b 44 24 10       	mov    0x10(%rsp),%rax
   28:	0f 05                	syscall
   2a:*	5b                   	pop    %rbx		<-- trapping instruction
   2b:	c3                   	ret
   2c:	0f 1f 80 00 00 00 00 	nopl   0x0(%rax)
   33:	83 e2 39             	and    $0x39,%edx
   36:	83 fa 08             	cmp    $0x8,%edx
   39:	75 e2                	jne    0x1d
   3b:	e8 23 ff ff ff       	call   0xffffffffffffff63

Code starting with the faulting instruction
===========================================
    0:	5b                   	pop    %rbx
    1:	c3                   	ret
    2:	0f 1f 80 00 00 00 00 	nopl   0x0(%rax)
    9:	83 e2 39             	and    $0x39,%edx
    c:	83 fa 08             	cmp    $0x8,%edx
    f:	75 e2                	jne    0xfffffffffffffff3
   11:	e8 23 ff ff ff       	call   0xffffffffffffff39
RSP: 002b:00007ffc7372ce60 EFLAGS: 00000202 ORIG_RAX: 0000000000000000
RAX: ffffffffffffffda RBX: 00007fd880dad840 RCX: 00007fd880694207
RDX: 0000000000000006 RSI: 00007ffc7372cf01 RDI: 0000000000000049
RBP: 0000000000000000 R08: 0000000000000000 R09: 0000000000000000
R10: 0000000000000000 R11: 0000000000000202 R12: 00007ffc7372cf01
R13: 00007ffc7372cf01 R14: 0000000000000049 R15: 00007ffc7372cf01
  </TASK>
Modules linked in: drm_exec btbcm iwlwifi(+) nct6683 gpu_sched snd_hwdep 
msi_ec(-) drm_suballoc_helper evdev kvm ee1004 bluetooth battery joydev 
mac_hid snd_hda_core video irqbypass cfg80211 drm_panel_backlight_quirks 
ghash_clmulni_intel snd_pcm cec sha512_ssse3 sp5100_tco sha256_ssse3 
drm_buddy snd_timer sha1_ssse3 watchdog drm_display_helper ccp snd 
tpm_crb aesni_intel rfkill soundcore pcspkr tpm_tis tpm_tis_core 
i2c_piix4 k10temp i2c_smbus tpm libaescfb ecdh_generic gpio_amdpt ecc 
gpio_generic button wmi
CR2: 0000000000000000
---[ end trace 0000000000000000 ]---

> ---
>  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_folios(eb); i++) {
> +		struct folio *folio = eb->folios[i];
> +
> +		if (i < attached) {
> +			ASSERT(folio);
> +			detach_extent_buffer_folio(eb, folio);
> +		} else if (!folio)
> +			continue;
> +
> +		ASSERT(!folio_test_private(folio));
> +		folio_unlock(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
> 

[-- Attachment #2: btrfs_oops3_decoded.gz --]
[-- Type: application/gzip, Size: 19018 bytes --]

[-- Attachment #3: config.gz --]
[-- Type: application/gzip, Size: 45147 bytes --]

Re: [PATCH] btrfs: unlock all extent buffer folios in failure case

[David Sterba]

On Thu, Apr 24, 2025 at 01:15:24PM +0200, Klara Modin wrote:
> 
> Hi,
> 
> On Tue, Apr 22, 2025 at 02:57:01PM +0200, Daniel Vacek wrote:
> > When attaching a folio fails, for example if another one is already mapped,
> > we need to unlock all newly allocated folios before putting them. And as a
> > consequence we do not need to flag the eb UNMAPPED anymore.
> > 
> > Signed-off-by: Daniel Vacek <neelx@suse.com>
> 
> I hit a null pointer dereference in next-20250424 which seems to point
> to this commit. Reverting it resolves the issue for me (did not bisect).

Thanks for the report, the patch has been removed from linux-next
branches.

[PATCH v2] btrfs: put all allocated extent buffer folios in failure case

[Daniel Vacek]

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@suse.com>
---
 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

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

[Boris Burkov]

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@suse.com>

Reviewed-by: Boris Burkov <boris@bur.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
> 

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

[Klara Modin]

Hi,

On 2025-04-24 17:08, 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@suse.com>

This version did not trigger an oops for me.

Thanks,
Tested-by: Klara Modin <klarasmodin@gmail.com>

> ---
>   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;
>   }