* [PATCH v1] mm: recheck PageCompound() before printing out info of head page
@ 2022-04-14 23:59 Naoya Horiguchi
2022-04-15 1:08 ` John Hubbard
0 siblings, 1 reply; 4+ messages in thread
From: Naoya Horiguchi @ 2022-04-14 23:59 UTC (permalink / raw)
To: Matthew Wilcox, linux-mm
Cc: Christoph Hellwig, Andrew Morton, John Hubbard, Jason Gunthorpe,
William Kucharski, Naoya Horiguchi, linux-kernel
From: Naoya Horiguchi <naoya.horiguchi@nec.com>
The following VM_BUG_ON_FOLIO() is triggered when memory error event
happens on the (thp/folio) pages which are about to be freed:
[ 1160.232771] page:00000000b36a8a0f refcount:1 mapcount:0 mapping:0000000000000000 index:0x1 pfn:0x16a000
[ 1160.236916] page:00000000b36a8a0f refcount:0 mapcount:0 mapping:0000000000000000 index:0x1 pfn:0x16a000
[ 1160.240684] flags: 0x57ffffc0800000(hwpoison|node=1|zone=2|lastcpupid=0x1fffff)
[ 1160.243458] raw: 0057ffffc0800000 dead000000000100 dead000000000122 0000000000000000
[ 1160.246268] raw: 0000000000000001 0000000000000000 00000000ffffffff 0000000000000000
[ 1160.249197] page dumped because: VM_BUG_ON_FOLIO(!folio_test_large(folio))
[ 1160.251815] ------------[ cut here ]------------
[ 1160.253438] kernel BUG at include/linux/mm.h:788!
[ 1160.256162] invalid opcode: 0000 [#1] PREEMPT SMP PTI
[ 1160.258172] CPU: 2 PID: 115368 Comm: mceinj.sh Tainted: G E 5.18.0-rc1-v5.18-rc1-220404-2353-005-g83111+ #3
[ 1160.262049] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.15.0-1.fc35 04/01/2014
[ 1160.265103] RIP: 0010:dump_page.cold+0x27e/0x2bd
[ 1160.266757] Code: fe ff ff 48 c7 c6 81 f1 5a 98 e9 4c fe ff ff 48 c7 c6 a1 95 59 98 e9 40 fe ff ff 48 c7 c6 50 bf 5a 98 48 89 ef e8 9d 04 6d ff <0f> 0b 41 f7 c4 ff 0f 00 00 0f 85 9f fd ff ff 49 8b 04 24 a9 00 00
[ 1160.273180] RSP: 0018:ffffaa2c4d59fd18 EFLAGS: 00010292
[ 1160.274969] RAX: 000000000000003e RBX: 0000000000000001 RCX: 0000000000000000
[ 1160.277263] RDX: 0000000000000001 RSI: ffffffff985995a1 RDI: 00000000ffffffff
[ 1160.279571] RBP: ffffdc9c45a80000 R08: 0000000000000000 R09: 00000000ffffdfff
[ 1160.281794] R10: ffffaa2c4d59fb08 R11: ffffffff98940d08 R12: ffffdc9c45a80000
[ 1160.283920] R13: ffffffff985b6f94 R14: 0000000000000000 R15: ffffdc9c45a80000
[ 1160.286641] FS: 00007eff54ce1740(0000) GS:ffff99c67bd00000(0000) knlGS:0000000000000000
[ 1160.289498] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 1160.291106] CR2: 00005628381a5f68 CR3: 0000000104712003 CR4: 0000000000170ee0
[ 1160.293031] Call Trace:
[ 1160.293724] <TASK>
[ 1160.294334] get_hwpoison_page+0x47d/0x570
[ 1160.295474] memory_failure+0x106/0xaa0
[ 1160.296474] ? security_capable+0x36/0x50
[ 1160.297524] hard_offline_page_store+0x43/0x80
[ 1160.298684] kernfs_fop_write_iter+0x11c/0x1b0
[ 1160.299829] new_sync_write+0xf9/0x160
[ 1160.300810] vfs_write+0x209/0x290
[ 1160.301835] ksys_write+0x4f/0xc0
[ 1160.302718] do_syscall_64+0x3b/0x90
[ 1160.303664] entry_SYSCALL_64_after_hwframe+0x44/0xae
[ 1160.304981] RIP: 0033:0x7eff54b018b7
As shown in the RIP address, this VM_BUG_ON in folio_entire_mapcount() is
called from dump_page("hwpoison: unhandlable page") in get_any_page().
The below explains the mechanism of the race:
CPU 0 CPU 1
memory_failure
get_hwpoison_page
get_any_page
dump_page
compound = PageCompound
free_pages_prepare
page->flags &= ~PAGE_FLAGS_CHECK_AT_PREP
folio_entire_mapcount
VM_BUG_ON_FOLIO(!folio_test_large(folio))
So in order to avoid the BUG_ON, recheck PageCompound() before __dump_page()
prints out information about the head page. Precisely saying, this does
not close up the race, but mitigates the impact to just inaccurate info,
which should be acceptable.
Fixes: 74e8ee4708a8 ("mm: Turn head_compound_mapcount() into folio_entire_mapcount()")
Signed-off-by: Naoya Horiguchi <naoya.horiguchi@nec.com>
---
I'm actually not sure how widely the race condition could affect.
If we can consider this as a bug specific to memory_failure(), simply
replacing the dump_page() with simple printk() might be a proper fix
(I confirmed that that fix also works). But if any other dump_page()'s
caller could race with freeing thp/folio, the more generic fix
(I'm suggesting in this patch) would be desirable.
---
mm/debug.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/mm/debug.c b/mm/debug.c
index bef329bf28f0..fce63b200bea 100644
--- a/mm/debug.c
+++ b/mm/debug.c
@@ -93,7 +93,7 @@ static void __dump_page(struct page *page)
pr_warn("page:%p refcount:%d mapcount:%d mapping:%p index:%#lx pfn:%#lx\n",
page, page_ref_count(head), mapcount, mapping,
page_to_pgoff(page), page_to_pfn(page));
- if (compound) {
+ if (compound && PageCompound(page)) {
pr_warn("head:%p order:%u compound_mapcount:%d compound_pincount:%d\n",
head, compound_order(head),
folio_entire_mapcount(folio),
--
2.25.1
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH v1] mm: recheck PageCompound() before printing out info of head page
2022-04-14 23:59 [PATCH v1] mm: recheck PageCompound() before printing out info of head page Naoya Horiguchi
@ 2022-04-15 1:08 ` John Hubbard
2022-04-16 1:56 ` Miaohe Lin
0 siblings, 1 reply; 4+ messages in thread
From: John Hubbard @ 2022-04-15 1:08 UTC (permalink / raw)
To: Naoya Horiguchi, Matthew Wilcox, linux-mm
Cc: Christoph Hellwig, Andrew Morton, Jason Gunthorpe,
William Kucharski, Naoya Horiguchi, linux-kernel
On 4/14/22 16:59, Naoya Horiguchi wrote:
> From: Naoya Horiguchi <naoya.horiguchi@nec.com>
>
> The following VM_BUG_ON_FOLIO() is triggered when memory error event
> happens on the (thp/folio) pages which are about to be freed:
>
> [ 1160.232771] page:00000000b36a8a0f refcount:1 mapcount:0 mapping:0000000000000000 index:0x1 pfn:0x16a000
> [ 1160.236916] page:00000000b36a8a0f refcount:0 mapcount:0 mapping:0000000000000000 index:0x1 pfn:0x16a000
> [ 1160.240684] flags: 0x57ffffc0800000(hwpoison|node=1|zone=2|lastcpupid=0x1fffff)
> [ 1160.243458] raw: 0057ffffc0800000 dead000000000100 dead000000000122 0000000000000000
> [ 1160.246268] raw: 0000000000000001 0000000000000000 00000000ffffffff 0000000000000000
> [ 1160.249197] page dumped because: VM_BUG_ON_FOLIO(!folio_test_large(folio))
> [ 1160.251815] ------------[ cut here ]------------
> [ 1160.253438] kernel BUG at include/linux/mm.h:788!
> [ 1160.256162] invalid opcode: 0000 [#1] PREEMPT SMP PTI
> [ 1160.258172] CPU: 2 PID: 115368 Comm: mceinj.sh Tainted: G E 5.18.0-rc1-v5.18-rc1-220404-2353-005-g83111+ #3
> [ 1160.262049] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.15.0-1.fc35 04/01/2014
> [ 1160.265103] RIP: 0010:dump_page.cold+0x27e/0x2bd
> [ 1160.266757] Code: fe ff ff 48 c7 c6 81 f1 5a 98 e9 4c fe ff ff 48 c7 c6 a1 95 59 98 e9 40 fe ff ff 48 c7 c6 50 bf 5a 98 48 89 ef e8 9d 04 6d ff <0f> 0b 41 f7 c4 ff 0f 00 00 0f 85 9f fd ff ff 49 8b 04 24 a9 00 00
> [ 1160.273180] RSP: 0018:ffffaa2c4d59fd18 EFLAGS: 00010292
> [ 1160.274969] RAX: 000000000000003e RBX: 0000000000000001 RCX: 0000000000000000
> [ 1160.277263] RDX: 0000000000000001 RSI: ffffffff985995a1 RDI: 00000000ffffffff
> [ 1160.279571] RBP: ffffdc9c45a80000 R08: 0000000000000000 R09: 00000000ffffdfff
> [ 1160.281794] R10: ffffaa2c4d59fb08 R11: ffffffff98940d08 R12: ffffdc9c45a80000
> [ 1160.283920] R13: ffffffff985b6f94 R14: 0000000000000000 R15: ffffdc9c45a80000
> [ 1160.286641] FS: 00007eff54ce1740(0000) GS:ffff99c67bd00000(0000) knlGS:0000000000000000
> [ 1160.289498] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [ 1160.291106] CR2: 00005628381a5f68 CR3: 0000000104712003 CR4: 0000000000170ee0
> [ 1160.293031] Call Trace:
> [ 1160.293724] <TASK>
> [ 1160.294334] get_hwpoison_page+0x47d/0x570
> [ 1160.295474] memory_failure+0x106/0xaa0
> [ 1160.296474] ? security_capable+0x36/0x50
> [ 1160.297524] hard_offline_page_store+0x43/0x80
> [ 1160.298684] kernfs_fop_write_iter+0x11c/0x1b0
> [ 1160.299829] new_sync_write+0xf9/0x160
> [ 1160.300810] vfs_write+0x209/0x290
> [ 1160.301835] ksys_write+0x4f/0xc0
> [ 1160.302718] do_syscall_64+0x3b/0x90
> [ 1160.303664] entry_SYSCALL_64_after_hwframe+0x44/0xae
> [ 1160.304981] RIP: 0033:0x7eff54b018b7
>
> As shown in the RIP address, this VM_BUG_ON in folio_entire_mapcount() is
> called from dump_page("hwpoison: unhandlable page") in get_any_page().
> The below explains the mechanism of the race:
>
> CPU 0 CPU 1
>
> memory_failure
> get_hwpoison_page
> get_any_page
> dump_page
> compound = PageCompound
> free_pages_prepare
> page->flags &= ~PAGE_FLAGS_CHECK_AT_PREP
> folio_entire_mapcount
> VM_BUG_ON_FOLIO(!folio_test_large(folio))
>
> So in order to avoid the BUG_ON, recheck PageCompound() before __dump_page()
> prints out information about the head page. Precisely saying, this does
> not close up the race, but mitigates the impact to just inaccurate info,
> which should be acceptable.
Actually, I don't think it really closes the race even to that extent,
because a machine check (MCE, on x86) could come in at any point, so
rechecking (without locks) won't really completely fix such a bug, right?
>
> Fixes: 74e8ee4708a8 ("mm: Turn head_compound_mapcount() into folio_entire_mapcount()")
> Signed-off-by: Naoya Horiguchi <naoya.horiguchi@nec.com>
> ---
> I'm actually not sure how widely the race condition could affect.
> If we can consider this as a bug specific to memory_failure(), simply
> replacing the dump_page() with simple printk() might be a proper fix
I think we should have separate, limited, special case functions that
are safe for use by exotic things like the MCE handler. And so just
doing a printk there seems better to me.
> (I confirmed that that fix also works). But if any other dump_page()'s
> caller could race with freeing thp/folio, the more generic fix
> (I'm suggesting in this patch) would be desirable.
*If* there are other races, they should be addressed separately, IMHO.
Because MCE should be a special case.
> ---
> mm/debug.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/mm/debug.c b/mm/debug.c
> index bef329bf28f0..fce63b200bea 100644
> --- a/mm/debug.c
> +++ b/mm/debug.c
> @@ -93,7 +93,7 @@ static void __dump_page(struct page *page)
> pr_warn("page:%p refcount:%d mapcount:%d mapping:%p index:%#lx pfn:%#lx\n",
> page, page_ref_count(head), mapcount, mapping,
> page_to_pgoff(page), page_to_pfn(page));
> - if (compound) {
> + if (compound && PageCompound(page)) {
If people decide to keep this approach, then I'd go with something
that makes it clear, like:
/* Re-check, in a semi-doomed attempt to avoid racing with MCE: */
compound = PageCompound(page);
:)
thanks,
--
John Hubbard
NVIDIA
> pr_warn("head:%p order:%u compound_mapcount:%d compound_pincount:%d\n",
> head, compound_order(head),
> folio_entire_mapcount(folio),
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v1] mm: recheck PageCompound() before printing out info of head page
2022-04-15 1:08 ` John Hubbard
@ 2022-04-16 1:56 ` Miaohe Lin
2022-04-16 4:25 ` Naoya Horiguchi
0 siblings, 1 reply; 4+ messages in thread
From: Miaohe Lin @ 2022-04-16 1:56 UTC (permalink / raw)
To: John Hubbard, Naoya Horiguchi, Matthew Wilcox
Cc: Christoph Hellwig, Andrew Morton, Jason Gunthorpe,
William Kucharski, Naoya Horiguchi, linux-kernel, Linux-MM
On 2022/4/15 9:08, John Hubbard wrote:
> On 4/14/22 16:59, Naoya Horiguchi wrote:
>> From: Naoya Horiguchi <naoya.horiguchi@nec.com>
>>
>> The following VM_BUG_ON_FOLIO() is triggered when memory error event
>> happens on the (thp/folio) pages which are about to be freed:
>>
>> [ 1160.232771] page:00000000b36a8a0f refcount:1 mapcount:0 mapping:0000000000000000 index:0x1 pfn:0x16a000
>> [ 1160.236916] page:00000000b36a8a0f refcount:0 mapcount:0 mapping:0000000000000000 index:0x1 pfn:0x16a000
>> [ 1160.240684] flags: 0x57ffffc0800000(hwpoison|node=1|zone=2|lastcpupid=0x1fffff)
>> [ 1160.243458] raw: 0057ffffc0800000 dead000000000100 dead000000000122 0000000000000000
>> [ 1160.246268] raw: 0000000000000001 0000000000000000 00000000ffffffff 0000000000000000
>> [ 1160.249197] page dumped because: VM_BUG_ON_FOLIO(!folio_test_large(folio))
>> [ 1160.251815] ------------[ cut here ]------------
>> [ 1160.253438] kernel BUG at include/linux/mm.h:788!
>> [ 1160.256162] invalid opcode: 0000 [#1] PREEMPT SMP PTI
>> [ 1160.258172] CPU: 2 PID: 115368 Comm: mceinj.sh Tainted: G E 5.18.0-rc1-v5.18-rc1-220404-2353-005-g83111+ #3
>> [ 1160.262049] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.15.0-1.fc35 04/01/2014
>> [ 1160.265103] RIP: 0010:dump_page.cold+0x27e/0x2bd
>> [ 1160.266757] Code: fe ff ff 48 c7 c6 81 f1 5a 98 e9 4c fe ff ff 48 c7 c6 a1 95 59 98 e9 40 fe ff ff 48 c7 c6 50 bf 5a 98 48 89 ef e8 9d 04 6d ff <0f> 0b 41 f7 c4 ff 0f 00 00 0f 85 9f fd ff ff 49 8b 04 24 a9 00 00
>> [ 1160.273180] RSP: 0018:ffffaa2c4d59fd18 EFLAGS: 00010292
>> [ 1160.274969] RAX: 000000000000003e RBX: 0000000000000001 RCX: 0000000000000000
>> [ 1160.277263] RDX: 0000000000000001 RSI: ffffffff985995a1 RDI: 00000000ffffffff
>> [ 1160.279571] RBP: ffffdc9c45a80000 R08: 0000000000000000 R09: 00000000ffffdfff
>> [ 1160.281794] R10: ffffaa2c4d59fb08 R11: ffffffff98940d08 R12: ffffdc9c45a80000
>> [ 1160.283920] R13: ffffffff985b6f94 R14: 0000000000000000 R15: ffffdc9c45a80000
>> [ 1160.286641] FS: 00007eff54ce1740(0000) GS:ffff99c67bd00000(0000) knlGS:0000000000000000
>> [ 1160.289498] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
>> [ 1160.291106] CR2: 00005628381a5f68 CR3: 0000000104712003 CR4: 0000000000170ee0
>> [ 1160.293031] Call Trace:
>> [ 1160.293724] <TASK>
>> [ 1160.294334] get_hwpoison_page+0x47d/0x570
>> [ 1160.295474] memory_failure+0x106/0xaa0
>> [ 1160.296474] ? security_capable+0x36/0x50
>> [ 1160.297524] hard_offline_page_store+0x43/0x80
>> [ 1160.298684] kernfs_fop_write_iter+0x11c/0x1b0
>> [ 1160.299829] new_sync_write+0xf9/0x160
>> [ 1160.300810] vfs_write+0x209/0x290
>> [ 1160.301835] ksys_write+0x4f/0xc0
>> [ 1160.302718] do_syscall_64+0x3b/0x90
>> [ 1160.303664] entry_SYSCALL_64_after_hwframe+0x44/0xae
>> [ 1160.304981] RIP: 0033:0x7eff54b018b7
>>
>> As shown in the RIP address, this VM_BUG_ON in folio_entire_mapcount() is
>> called from dump_page("hwpoison: unhandlable page") in get_any_page().
>> The below explains the mechanism of the race:
>>
>> CPU 0 CPU 1
>>
>> memory_failure
>> get_hwpoison_page
>> get_any_page
>> dump_page
>> compound = PageCompound
>> free_pages_prepare
>> page->flags &= ~PAGE_FLAGS_CHECK_AT_PREP
>> folio_entire_mapcount
>> VM_BUG_ON_FOLIO(!folio_test_large(folio))
>>
>> So in order to avoid the BUG_ON, recheck PageCompound() before __dump_page()
>> prints out information about the head page. Precisely saying, this does
>> not close up the race, but mitigates the impact to just inaccurate info,
>> which should be acceptable.
>
> Actually, I don't think it really closes the race even to that extent,
> because a machine check (MCE, on x86) could come in at any point, so
> rechecking (without locks) won't really completely fix such a bug, right?
It might also be unsafe to do dump_mapping or dump_page_owner when page could
be freed at any time or we might come across some use-after-free issues?
>
>>
>> Fixes: 74e8ee4708a8 ("mm: Turn head_compound_mapcount() into folio_entire_mapcount()")
>> Signed-off-by: Naoya Horiguchi <naoya.horiguchi@nec.com>
>> ---
>> I'm actually not sure how widely the race condition could affect.
>> If we can consider this as a bug specific to memory_failure(), simply
>> replacing the dump_page() with simple printk() might be a proper fix
>
> I think we should have separate, limited, special case functions that
> are safe for use by exotic things like the MCE handler. And so just
> doing a printk there seems better to me.
>
>> (I confirmed that that fix also works). But if any other dump_page()'s
>> caller could race with freeing thp/folio, the more generic fix
>> (I'm suggesting in this patch) would be desirable.
>
> *If* there are other races, they should be addressed separately, IMHO.
> Because MCE should be a special case.
>
>> ---
>> mm/debug.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/mm/debug.c b/mm/debug.c
>> index bef329bf28f0..fce63b200bea 100644
>> --- a/mm/debug.c
>> +++ b/mm/debug.c
>> @@ -93,7 +93,7 @@ static void __dump_page(struct page *page)
>> pr_warn("page:%p refcount:%d mapcount:%d mapping:%p index:%#lx pfn:%#lx\n",
>> page, page_ref_count(head), mapcount, mapping,
>> page_to_pgoff(page), page_to_pfn(page));
>> - if (compound) {
>> + if (compound && PageCompound(page)) {
>
> If people decide to keep this approach, then I'd go with something
> that makes it clear, like:
>
> /* Re-check, in a semi-doomed attempt to avoid racing with MCE: */
> compound = PageCompound(page);
Agree. A comment will be needed or the code might look obscure.
Thanks!
>
> :)
>
> thanks,
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v1] mm: recheck PageCompound() before printing out info of head page
2022-04-16 1:56 ` Miaohe Lin
@ 2022-04-16 4:25 ` Naoya Horiguchi
0 siblings, 0 replies; 4+ messages in thread
From: Naoya Horiguchi @ 2022-04-16 4:25 UTC (permalink / raw)
To: John Hubbard, Miaohe Lin
Cc: Matthew Wilcox, Christoph Hellwig, Andrew Morton,
Jason Gunthorpe, William Kucharski, Naoya Horiguchi,
linux-kernel, Linux-MM
On Sat, Apr 16, 2022 at 09:56:41AM +0800, Miaohe Lin wrote:
> On 2022/4/15 9:08, John Hubbard wrote:
> > On 4/14/22 16:59, Naoya Horiguchi wrote:
> >> From: Naoya Horiguchi <naoya.horiguchi@nec.com>
> >>
> >> The following VM_BUG_ON_FOLIO() is triggered when memory error event
> >> happens on the (thp/folio) pages which are about to be freed:
> >>
> >> [ 1160.232771] page:00000000b36a8a0f refcount:1 mapcount:0 mapping:0000000000000000 index:0x1 pfn:0x16a000
> >> [ 1160.236916] page:00000000b36a8a0f refcount:0 mapcount:0 mapping:0000000000000000 index:0x1 pfn:0x16a000
> >> [ 1160.240684] flags: 0x57ffffc0800000(hwpoison|node=1|zone=2|lastcpupid=0x1fffff)
> >> [ 1160.243458] raw: 0057ffffc0800000 dead000000000100 dead000000000122 0000000000000000
> >> [ 1160.246268] raw: 0000000000000001 0000000000000000 00000000ffffffff 0000000000000000
> >> [ 1160.249197] page dumped because: VM_BUG_ON_FOLIO(!folio_test_large(folio))
> >> [ 1160.251815] ------------[ cut here ]------------
> >> [ 1160.253438] kernel BUG at include/linux/mm.h:788!
> >> [ 1160.256162] invalid opcode: 0000 [#1] PREEMPT SMP PTI
> >> [ 1160.258172] CPU: 2 PID: 115368 Comm: mceinj.sh Tainted: G E 5.18.0-rc1-v5.18-rc1-220404-2353-005-g83111+ #3
> >> [ 1160.262049] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.15.0-1.fc35 04/01/2014
> >> [ 1160.265103] RIP: 0010:dump_page.cold+0x27e/0x2bd
> >> [ 1160.266757] Code: fe ff ff 48 c7 c6 81 f1 5a 98 e9 4c fe ff ff 48 c7 c6 a1 95 59 98 e9 40 fe ff ff 48 c7 c6 50 bf 5a 98 48 89 ef e8 9d 04 6d ff <0f> 0b 41 f7 c4 ff 0f 00 00 0f 85 9f fd ff ff 49 8b 04 24 a9 00 00
> >> [ 1160.273180] RSP: 0018:ffffaa2c4d59fd18 EFLAGS: 00010292
> >> [ 1160.274969] RAX: 000000000000003e RBX: 0000000000000001 RCX: 0000000000000000
> >> [ 1160.277263] RDX: 0000000000000001 RSI: ffffffff985995a1 RDI: 00000000ffffffff
> >> [ 1160.279571] RBP: ffffdc9c45a80000 R08: 0000000000000000 R09: 00000000ffffdfff
> >> [ 1160.281794] R10: ffffaa2c4d59fb08 R11: ffffffff98940d08 R12: ffffdc9c45a80000
> >> [ 1160.283920] R13: ffffffff985b6f94 R14: 0000000000000000 R15: ffffdc9c45a80000
> >> [ 1160.286641] FS: 00007eff54ce1740(0000) GS:ffff99c67bd00000(0000) knlGS:0000000000000000
> >> [ 1160.289498] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> >> [ 1160.291106] CR2: 00005628381a5f68 CR3: 0000000104712003 CR4: 0000000000170ee0
> >> [ 1160.293031] Call Trace:
> >> [ 1160.293724] <TASK>
> >> [ 1160.294334] get_hwpoison_page+0x47d/0x570
> >> [ 1160.295474] memory_failure+0x106/0xaa0
> >> [ 1160.296474] ? security_capable+0x36/0x50
> >> [ 1160.297524] hard_offline_page_store+0x43/0x80
> >> [ 1160.298684] kernfs_fop_write_iter+0x11c/0x1b0
> >> [ 1160.299829] new_sync_write+0xf9/0x160
> >> [ 1160.300810] vfs_write+0x209/0x290
> >> [ 1160.301835] ksys_write+0x4f/0xc0
> >> [ 1160.302718] do_syscall_64+0x3b/0x90
> >> [ 1160.303664] entry_SYSCALL_64_after_hwframe+0x44/0xae
> >> [ 1160.304981] RIP: 0033:0x7eff54b018b7
> >>
> >> As shown in the RIP address, this VM_BUG_ON in folio_entire_mapcount() is
> >> called from dump_page("hwpoison: unhandlable page") in get_any_page().
> >> The below explains the mechanism of the race:
> >>
> >> CPU 0 CPU 1
> >>
> >> memory_failure
> >> get_hwpoison_page
> >> get_any_page
> >> dump_page
> >> compound = PageCompound
> >> free_pages_prepare
> >> page->flags &= ~PAGE_FLAGS_CHECK_AT_PREP
> >> folio_entire_mapcount
> >> VM_BUG_ON_FOLIO(!folio_test_large(folio))
> >>
> >> So in order to avoid the BUG_ON, recheck PageCompound() before __dump_page()
> >> prints out information about the head page. Precisely saying, this does
> >> not close up the race, but mitigates the impact to just inaccurate info,
> >> which should be acceptable.
> >
> > Actually, I don't think it really closes the race even to that extent,
> > because a machine check (MCE, on x86) could come in at any point, so
> > rechecking (without locks) won't really completely fix such a bug, right?
>
> It might also be unsafe to do dump_mapping or dump_page_owner when page could
> be freed at any time or we might come across some use-after-free issues?
OK, so memory_failure() simply shouldn't call dump_page() before taking page refcnt.
>
> >
> >>
> >> Fixes: 74e8ee4708a8 ("mm: Turn head_compound_mapcount() into folio_entire_mapcount()")
> >> Signed-off-by: Naoya Horiguchi <naoya.horiguchi@nec.com>
> >> ---
> >> I'm actually not sure how widely the race condition could affect.
> >> If we can consider this as a bug specific to memory_failure(), simply
> >> replacing the dump_page() with simple printk() might be a proper fix
> >
> > I think we should have separate, limited, special case functions that
> > are safe for use by exotic things like the MCE handler. And so just
> > doing a printk there seems better to me.
Thanks, so now I lean to the printk-approach.
> >
> >> (I confirmed that that fix also works). But if any other dump_page()'s
> >> caller could race with freeing thp/folio, the more generic fix
> >> (I'm suggesting in this patch) would be desirable.
> >
> > *If* there are other races, they should be addressed separately, IMHO.
> > Because MCE should be a special case.
I agree.
Thanks,
Naoya Horiguchi
> >
> >> ---
> >> mm/debug.c | 2 +-
> >> 1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/mm/debug.c b/mm/debug.c
> >> index bef329bf28f0..fce63b200bea 100644
> >> --- a/mm/debug.c
> >> +++ b/mm/debug.c
> >> @@ -93,7 +93,7 @@ static void __dump_page(struct page *page)
> >> pr_warn("page:%p refcount:%d mapcount:%d mapping:%p index:%#lx pfn:%#lx\n",
> >> page, page_ref_count(head), mapcount, mapping,
> >> page_to_pgoff(page), page_to_pfn(page));
> >> - if (compound) {
> >> + if (compound && PageCompound(page)) {
> >
> > If people decide to keep this approach, then I'd go with something
> > that makes it clear, like:
> >
> > /* Re-check, in a semi-doomed attempt to avoid racing with MCE: */
> > compound = PageCompound(page);
>
> Agree. A comment will be needed or the code might look obscure.
>
> Thanks!
>
> >
> > :)
> >
> > thanks,
>
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2022-04-16 4:25 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-04-14 23:59 [PATCH v1] mm: recheck PageCompound() before printing out info of head page Naoya Horiguchi
2022-04-15 1:08 ` John Hubbard
2022-04-16 1:56 ` Miaohe Lin
2022-04-16 4:25 ` Naoya Horiguchi
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).