* [PATCH] btrfs: unlock all extent buffer folios in failure case @ 2025-04-22 12:57 Daniel Vacek 2025-04-24 11:09 ` Klara Modin 2025-04-24 15:08 ` [PATCH v2] btrfs: put all allocated " Daniel Vacek 0 siblings, 2 replies; 7+ messages in thread From: Daniel Vacek @ 2025-04-22 12:57 UTC (permalink / raw) To: Chris Mason, Josef Bacik, David Sterba Cc: Daniel Vacek, linux-btrfs, linux-kernel 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 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] btrfs: unlock all extent buffer folios in failure case 2025-04-22 12:57 [PATCH] btrfs: unlock all extent buffer folios in failure case Daniel Vacek @ 2025-04-24 11:09 ` Klara Modin 2025-04-24 11:15 ` Klara Modin 2025-04-24 15:08 ` [PATCH v2] btrfs: put all allocated " Daniel Vacek 1 sibling, 1 reply; 7+ messages in thread From: Klara Modin @ 2025-04-24 11:09 UTC (permalink / raw) To: Daniel Vacek Cc: Chris Mason, Josef Bacik, David Sterba, linux-btrfs, linux-kernel [-- 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 --] ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH] btrfs: unlock all extent buffer folios in failure case 2025-04-24 11:09 ` Klara Modin @ 2025-04-24 11:15 ` Klara Modin 2025-04-24 11:57 ` David Sterba 0 siblings, 1 reply; 7+ messages in thread From: Klara Modin @ 2025-04-24 11:15 UTC (permalink / raw) To: Daniel Vacek Cc: Chris Mason, Josef Bacik, David Sterba, linux-btrfs, linux-kernel [-- 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 --] ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] btrfs: unlock all extent buffer folios in failure case 2025-04-24 11:15 ` Klara Modin @ 2025-04-24 11:57 ` David Sterba 0 siblings, 0 replies; 7+ messages in thread From: David Sterba @ 2025-04-24 11:57 UTC (permalink / raw) To: Klara Modin Cc: Daniel Vacek, Chris Mason, Josef Bacik, David Sterba, linux-btrfs, linux-kernel 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. ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH v2] btrfs: put all allocated extent buffer folios in failure case 2025-04-22 12:57 [PATCH] btrfs: unlock all extent buffer folios in failure case Daniel Vacek 2025-04-24 11:09 ` Klara Modin @ 2025-04-24 15:08 ` Daniel Vacek 2025-04-24 18:21 ` Boris Burkov 2025-04-24 18:49 ` Klara Modin 1 sibling, 2 replies; 7+ messages in thread From: Daniel Vacek @ 2025-04-24 15:08 UTC (permalink / raw) To: Chris Mason, Josef Bacik, David Sterba Cc: Daniel Vacek, linux-btrfs, linux-kernel 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 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH v2] btrfs: put all allocated extent buffer folios in failure case 2025-04-24 15:08 ` [PATCH v2] btrfs: put all allocated " Daniel Vacek @ 2025-04-24 18:21 ` Boris Burkov 2025-04-24 18:49 ` Klara Modin 1 sibling, 0 replies; 7+ messages in thread From: Boris Burkov @ 2025-04-24 18:21 UTC (permalink / raw) To: Daniel Vacek Cc: Chris Mason, Josef Bacik, David Sterba, linux-btrfs, linux-kernel 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 > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2] btrfs: put all allocated extent buffer folios in failure case 2025-04-24 15:08 ` [PATCH v2] btrfs: put all allocated " Daniel Vacek 2025-04-24 18:21 ` Boris Burkov @ 2025-04-24 18:49 ` Klara Modin 1 sibling, 0 replies; 7+ messages in thread From: Klara Modin @ 2025-04-24 18:49 UTC (permalink / raw) To: Daniel Vacek, Chris Mason, Josef Bacik, David Sterba Cc: linux-btrfs, linux-kernel 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; > } ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2025-04-24 18:49 UTC | newest] Thread overview: 7+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2025-04-22 12:57 [PATCH] btrfs: unlock all extent buffer folios in failure case Daniel Vacek 2025-04-24 11:09 ` Klara Modin 2025-04-24 11:15 ` Klara Modin 2025-04-24 11:57 ` David Sterba 2025-04-24 15:08 ` [PATCH v2] btrfs: put all allocated " Daniel Vacek 2025-04-24 18:21 ` Boris Burkov 2025-04-24 18:49 ` Klara Modin
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox