shmem folio changes have broken linux-next

shmem folio changes have broken linux-next

[Ryan Roberts]

Hi Matthew,

Our CI is reporting an oops during boot on linux-next (next-20240806) on arm64. Bisect tells me that it is due to your commit cdc4ad36a871b ("fs: Convert aops->write_begin to take a folio"), but there is no link to a mail thread on the patch and I can't find it in lore.

Anyway, I believe the issue is that you are doing this in shmem_write_begin():

  if (folio_test_has_hwpoisoned(folio)) {

But folio could be small and I think that function is only safe for large folios? (AFAICT it is unconditionally looking at the flags in the second page?).

Elsewhere in the file, this pattern is used:

  if (folio_test_hwpoison(folio) ||
      (folio_test_large(folio) && folio_test_has_hwpoisoned(folio))) {


Here is the oops (pretty much as soon as we get into user space):

[    0.623253] page: refcount:3 mapcount:0 mapping:00000000eebcb8cf index:0x0 pfn:0x18cc07
[    0.624212] memcg:ffff000142023000
[    0.624617] aops:shmem_aops ino:800 dentry name:"memfd:snapd-env-generator"
[    0.625444] flags: 0xbfffe0000040005(locked|referenced|swapbacked|node=0|zone=2|lastcpupid=0x1ffff)
[    0.626532] raw: 0bfffe0000040005 0000000000000000 dead000000000122 ffff000181dd0ac0
[    0.627442] raw: 0000000000000000 0000000000000000 00000003ffffffff ffff000142023000
[    0.628331] page dumped because: VM_BUG_ON_PAGE(n > 0 && !((__builtin_constant_p(PG_head) && __builtin_constant_p((uintptr_t)(&page->flags) != (uintptr_t)((void *)0)) && (uintptr_t)(&page->flags) != (uintptr_t)((void *)0) && __builtin_constant_p(*(const unsigned long *)(&page->flags))) ? const_test_bit(PG_head, &page->flags) : generic_test_bit(PG_head, &page->flags)))
[    0.632106] ------------[ cut here ]------------
[    0.632630] kernel BUG at include/linux/page-flags.h:308!
[    0.633269] Internal error: Oops - BUG: 00000000f2000800 [#1] PREEMPT SMP
[    0.634046] Modules linked in:
[    0.634412] CPU: 2 UID: 0 PID: 109 Comm: snapd-env-gener Not tainted 6.10.0-12082-gcdc4ad36a871 #11
[    0.635523] Hardware name: linux,dummy-virt (DT)
[    0.636117] pstate: 60400005 (nZCv daif +PAN -UAO -TCO -DIT -SSBS BTYPE=--)
[    0.636994] pc : const_folio_flags+0x60/0x70
[    0.637520] lr : const_folio_flags+0x60/0x70
[    0.638066] sp : ffff80008389bbf0
[    0.638487] x29: ffff80008389bbf0 x28: ffffba16d28a6540 x27: 000000000000004c
[    0.639383] x26: ffff000181b4c500 x25: 0000000000000000 x24: 0000000000000000
[    0.640252] x23: 0000000000200000 x22: ffff80008389bd60 x21: fffffdffc53301c0
[    0.641278] x20: ffff80008389bcc8 x19: 0000000000000000 x18: ffffffffffffffff
[    0.642162] x17: 3130303066666666 x16: 2066666666666666 x15: 0720072007200729
[    0.643048] x14: 0729072907730767 x13: 0720072007200729 x12: 0729072907730767
[    0.643916] x11: 0720072007200720 x10: ffffba16d43ef600 x9 : ffffba16d1532b3c
[    0.644808] x8 : 00000000ffffefff x7 : ffffba16d43ef600 x6 : 0000000000000000
[    0.645680] x5 : 80000000fffff000 x4 : 0000000000000fff x3 : 0000000000000000
[    0.646569] x2 : 0000000000000000 x1 : ffff000181e39180 x0 : 0000000000000166
[    0.647443] Call trace:
[    0.647753]  const_folio_flags+0x60/0x70
[    0.648247]  shmem_write_begin+0xa4/0x170
[    0.648755]  generic_perform_write+0xd8/0x2d0
[    0.649294]  shmem_file_write_iter+0xa8/0xb8
[    0.649835]  vfs_write+0x2b8/0x370
[    0.650259]  ksys_write+0x70/0x108
[    0.650676]  __arm64_sys_write+0x24/0x38
[    0.651141]  invoke_syscall+0x50/0x120
[    0.651606]  el0_svc_common+0x44/0xf8
[    0.652051]  do_el0_svc+0x28/0x40
[    0.652447]  el0_svc+0x34/0xe0
[    0.652821]  el0t_64_sync_handler+0x13c/0x158
[    0.653346]  el0t_64_sync+0x190/0x198
[    0.653810] Code: 54ffff21 b000dd41 912c0021 94009c7b (d4210000) 
[    0.654579] ---[ end trace 0000000000000000 ]---
[    0.655157] note: snapd-env-gener[109] exited with irqs disabled
[    0.655932] note: snapd-env-gener[109] exited with preempt_count 1
[    0.656724] ------------[ cut here ]------------
[    0.656788] systemd[108]: /usr/lib/systemd/system-environment-generators/snapd-env-generator terminated by signal SEGV.
[    0.657281] WARNING: CPU: 2 PID: 0 at kernel/context_tracking.c:128 ct_kernel_exit.constprop.0+0x100/0x118
[    0.659817] Modules linked in:
[    0.660194] CPU: 2 UID: 0 PID: 0 Comm: swapper/2 Tainted: G      D            6.10.0-12082-gcdc4ad36a871 #11
[    0.661435] Tainted: [D]=DIE
[    0.661796] Hardware name: linux,dummy-virt (DT)
[    0.662351] pstate: 204003c5 (nzCv DAIF +PAN -UAO -TCO -DIT -SSBS BTYPE=--)
[    0.663209] pc : ct_kernel_exit.constprop.0+0x100/0x118
[    0.663860] lr : ct_idle_enter+0x10/0x20
[    0.664357] sp : ffff8000801bbdc0
[    0.664776] x29: ffff8000801bbdc0 x28: 0000000000000000 x27: 0000000000000000
[    0.665655] x26: 0000000000000000 x25: ffff0001420b0000 x24: 0000000000000000
[    0.666533] x23: 0000000000000000 x22: ffffba16d4369dd0 x21: ffffba16d380ccf0
[    0.667398] x20: ffffba16d4369c70 x19: ffff00017ffc8fe8 x18: 0000000000000001
[    0.668291] x17: 0000000000000000 x16: 1fffe000284881a1 x15: 0000ffff9eccdfff
[    0.669171] x14: ffff80008389b758 x13: 0000000000000000 x12: ffffba16d2e5e880
[    0.670073] x11: 0000008bef90e7c6 x10: 0000000000000ad0 x9 : ffffba16d158cbf8
[    0.670965] x8 : ffff0001420b0b30 x7 : ffff45eaac7bf000 x6 : 00000000ffffffff
[    0.671871] x5 : 4000000000000002 x4 : ffff45eaac7bf000 x3 : ffff8000801bbdc0
[    0.672750] x2 : ffffba16d3809fe8 x1 : ffffba16d3809fe8 x0 : 4000000000000000
[    0.673636] Call trace:
[    0.673949]  ct_kernel_exit.constprop.0+0x100/0x118
[    0.674543]  ct_idle_enter+0x10/0x20
[    0.675009]  default_idle_call+0x24/0x148
[    0.675487]  do_idle+0x20c/0x270
[    0.675879]  cpu_startup_entry+0x40/0x50
[    0.676347]  secondary_start_kernel+0x138/0x160
[    0.676899]  __secondary_switched+0xb8/0xc0
[    0.677412] ---[ end trace 0000000000000000 ]---


And here is the bisect log:

# bad: [1e391b34f6aa043c7afa40a2103163a0ef06d179] Add linux-next specific files for 20240806
git bisect bad 1e391b34f6aa043c7afa40a2103163a0ef06d179
# good: [de9c2c66ad8e787abec7c9d7eff4f8c3cdd28aed] Linux 6.11-rc2
git bisect good de9c2c66ad8e787abec7c9d7eff4f8c3cdd28aed
# bad: [01c2d56f2c52e8af01dfd91af1fe9affc76c4c9e] Merge branch 'for-next' of git://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git
git bisect bad 01c2d56f2c52e8af01dfd91af1fe9affc76c4c9e
# good: [3610638e967f32f02c56c7cc8f7d6a815972f8c2] Merge branch 'for-linux-next' of git://git.kernel.org/pub/scm/linux/kernel/git/sudeep.holla/linux.git
git bisect good 3610638e967f32f02c56c7cc8f7d6a815972f8c2
# bad: [1b3d9c66aa0d64054a8f9fa1c14db3ee78156a50] Merge branch 'for-next' of git://git.kernel.org/pub/scm/linux/kernel/git/printk/linux.git
git bisect bad 1b3d9c66aa0d64054a8f9fa1c14db3ee78156a50
# bad: [1b3d9c66aa0d64054a8f9fa1c14db3ee78156a50] Merge branch 'for-next' of git://git.kernel.org/pub/scm/linux/kernel/git/printk/linux.git
git bisect bad 1b3d9c66aa0d64054a8f9fa1c14db3ee78156a50
# bad: [1b3d9c66aa0d64054a8f9fa1c14db3ee78156a50] Merge branch 'for-next' of git://git.kernel.org/pub/scm/linux/kernel/git/printk/linux.git
git bisect bad 1b3d9c66aa0d64054a8f9fa1c14db3ee78156a50
# bad: [d15fe7f84765bc7905e3128bfbce964e5b77164e] Merge branch 'vfs.all' of git://git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git
git bisect bad d15fe7f84765bc7905e3128bfbce964e5b77164e
# bad: [dc811e7b74eb4e5ae5fae7ef9e2e88c518bd9499] Merge branch 'work.write.end'
git bisect bad dc811e7b74eb4e5ae5fae7ef9e2e88c518bd9499
# bad: [c1dbed4c3c12f54037bd418b5c46a3a10d507cb5] Merge branch 'write-end' of git://git.infradead.org/users/willy/pagecache
git bisect bad c1dbed4c3c12f54037bd418b5c46a3a10d507cb5
# good: [753b3c028d7ee9a47b13bb4ee70a34a70b647cec] minixfs: Convert minix_delete_entry() to work on a folio
git bisect good 753b3c028d7ee9a47b13bb4ee70a34a70b647cec
# good: [ef2c557265b13f52f845655f7df1f8a9fc44ed72] f2fs: Convert f2fs_write_end() to use a folio
git bisect good ef2c557265b13f52f845655f7df1f8a9fc44ed72
# good: [4fc11ea804155651591518a011494598086a6a27] orangefs: Convert orangefs_write_end() to use a folio
git bisect good 4fc11ea804155651591518a011494598086a6a27
# good: [300dd0fa8e20eca38c251fbe4b0b373f4b53c1b4] fs: Convert aops->write_end to take a folio
git bisect good 300dd0fa8e20eca38c251fbe4b0b373f4b53c1b4
# bad: [d5dd3891b9ec273f12c5d0e147d365956fc24b8f] ocfs2: Convert ocfs2_write_zero_page to use a folio
git bisect bad d5dd3891b9ec273f12c5d0e147d365956fc24b8f
# bad: [cdc4ad36a871b7ac43fcc6b2891058d332ce60ce] fs: Convert aops->write_begin to take a folio
git bisect bad cdc4ad36a871b7ac43fcc6b2891058d332ce60ce
# first bad commit: [cdc4ad36a871b7ac43fcc6b2891058d332ce60ce] fs: Convert aops->write_begin to take a folio


Thanks,
Ryan

Re: shmem folio changes have broken linux-next

[Matthew Wilcox]

On Tue, Aug 06, 2024 at 09:47:19AM +0100, Ryan Roberts wrote:
> Our CI is reporting an oops during boot on linux-next (next-20240806) on arm64. Bisect tells me that it is due to your commit cdc4ad36a871b ("fs: Convert aops->write_begin to take a folio"), but there is no link to a mail thread on the patch and I can't find it in lore.

You're looking in the wrong place ;-)

https://lore.kernel.org/linux-fsdevel/20240717154716.237943-22-willy@infradead.org/#Z31mm:shmem.c

> Anyway, I believe the issue is that you are doing this in shmem_write_begin():
> 
>   if (folio_test_has_hwpoisoned(folio)) {
> 
> But folio could be small and I think that function is only safe for large folios? (AFAICT it is unconditionally looking at the flags in the second page?).
> 
> Elsewhere in the file, this pattern is used:
> 
>   if (folio_test_hwpoison(folio) ||
>       (folio_test_large(folio) && folio_test_has_hwpoisoned(folio))) {

Ugh.  The hwpoison stuff is too complicated.  Because that's wrong too.
It should be ...

if (folio_test_large(folio) && folio_test_has_hwpoisoned(folio) ||
    !folio_test_large(folio) && folio_test_hwpoison(folio))

right?  But that's a mouthful to write.  I'm tempted to rip it all out
and start again ...

> Here is the oops (pretty much as soon as we get into user space):
> 
> [    0.623253] page: refcount:3 mapcount:0 mapping:00000000eebcb8cf index:0x0 pfn:0x18cc07
> [    0.624212] memcg:ffff000142023000
> [    0.624617] aops:shmem_aops ino:800 dentry name:"memfd:snapd-env-generator"
> [    0.625444] flags: 0xbfffe0000040005(locked|referenced|swapbacked|node=0|zone=2|lastcpupid=0x1ffff)
> [    0.626532] raw: 0bfffe0000040005 0000000000000000 dead000000000122 ffff000181dd0ac0
> [    0.627442] raw: 0000000000000000 0000000000000000 00000003ffffffff ffff000142023000
> [    0.628331] page dumped because: VM_BUG_ON_PAGE(n > 0 && !((__builtin_constant_p(PG_head) && __builtin_constant_p((uintptr_t)(&page->flags) != (uintptr_t)((void *)0)) && (uintptr_t)(&page->flags) != (uintptr_t)((void *)0) && __builtin_constant_p(*(const unsigned long *)(&page->flags))) ? const_test_bit(PG_head, &page->flags) : generic_test_bit(PG_head, &page->flags)))
> [    0.632106] ------------[ cut here ]------------
> [    0.632630] kernel BUG at include/linux/page-flags.h:308!

I'm glad I made it so noisy instead of silently checking something
that's not the flag we thought it was ...

Re: shmem folio changes have broken linux-next

[Ryan Roberts]

On 06/08/2024 16:16, Matthew Wilcox wrote:
> On Tue, Aug 06, 2024 at 09:47:19AM +0100, Ryan Roberts wrote:
>> Our CI is reporting an oops during boot on linux-next (next-20240806) on arm64. Bisect tells me that it is due to your commit cdc4ad36a871b ("fs: Convert aops->write_begin to take a folio"), but there is no link to a mail thread on the patch and I can't find it in lore.
> 
> You're looking in the wrong place ;-)
> 
> https://lore.kernel.org/linux-fsdevel/20240717154716.237943-22-willy@infradead.org/#Z31mm:shmem.c

Ahha, cheers!

> 
>> Anyway, I believe the issue is that you are doing this in shmem_write_begin():
>>
>>   if (folio_test_has_hwpoisoned(folio)) {
>>
>> But folio could be small and I think that function is only safe for large folios? (AFAICT it is unconditionally looking at the flags in the second page?).
>>
>> Elsewhere in the file, this pattern is used:
>>
>>   if (folio_test_hwpoison(folio) ||
>>       (folio_test_large(folio) && folio_test_has_hwpoisoned(folio))) {
> 
> Ugh.  The hwpoison stuff is too complicated.  Because that's wrong too.
> It should be ...
> 
> if (folio_test_large(folio) && folio_test_has_hwpoisoned(folio) ||
>     !folio_test_large(folio) && folio_test_hwpoison(folio))

Err... I clearly don't understand it properly. I guess you sometimes want to
know if any page in the folio is poisioned, and sometimes if a specific page is
poisoned? So

// returns true if any page in the folio is hwpoisoned.
// works for any folio (large or small).
folio_test_hwpoison(folio);

// returns true if the page at index within folio is hwpoisoned.
// works for any folio (large or small).
// BUGs if index out of range.
folio_test_hwpoison_page(folio, index);

Why isn't this the right interface? Why do we have a function that takes a folio
but is only correct to call if the folio is large?

> 
> right?  But that's a mouthful to write.  I'm tempted to rip it all out
> and start again ...
> 
>> Here is the oops (pretty much as soon as we get into user space):
>>
>> [    0.623253] page: refcount:3 mapcount:0 mapping:00000000eebcb8cf index:0x0 pfn:0x18cc07
>> [    0.624212] memcg:ffff000142023000
>> [    0.624617] aops:shmem_aops ino:800 dentry name:"memfd:snapd-env-generator"
>> [    0.625444] flags: 0xbfffe0000040005(locked|referenced|swapbacked|node=0|zone=2|lastcpupid=0x1ffff)
>> [    0.626532] raw: 0bfffe0000040005 0000000000000000 dead000000000122 ffff000181dd0ac0
>> [    0.627442] raw: 0000000000000000 0000000000000000 00000003ffffffff ffff000142023000
>> [    0.628331] page dumped because: VM_BUG_ON_PAGE(n > 0 && !((__builtin_constant_p(PG_head) && __builtin_constant_p((uintptr_t)(&page->flags) != (uintptr_t)((void *)0)) && (uintptr_t)(&page->flags) != (uintptr_t)((void *)0) && __builtin_constant_p(*(const unsigned long *)(&page->flags))) ? const_test_bit(PG_head, &page->flags) : generic_test_bit(PG_head, &page->flags)))
>> [    0.632106] ------------[ cut here ]------------
>> [    0.632630] kernel BUG at include/linux/page-flags.h:308!
> 
> I'm glad I made it so noisy instead of silently checking something
> that's not the flag we thought it was ...