mm: khugepaged: Fix kernel BUG in hpage_collapse_scan_file
Subsystems (2)
PATCH v2 1/1 mm: khugepaged: Fix kernel BUG in hpage_collapse_scan_file
Syzkaller reported the following issue:
kernel BUG at mm/khugepaged.c:1823!
invalid opcode: 0000 [#1] PREEMPT SMP KASAN
CPU: 1 PID: 5097 Comm: syz-executor220 Not tainted 6.2.0-syzkaller-13154-g857f1268a591 #0 Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 02/16/2023 RIP: 0010:collapse_file mm/khugepaged.c:1823 [inline] RIP: 0010:hpage_collapse_scan_file+0x67c8/0x7580 mm/khugepaged.c:2233 Code: 00 00 89 de e8 c9 66 a3 ff 31 ff 89 de e8 c0 66 a3 ff 45 84 f6 0f 85 28 0d 00 00 e8 22 64 a3 ff e9 dc f7 ff ff e8 18 64 a3 ff <0f> 0b f3 0f 1e fa e8 0d 64 a3 ff e9 93 f6 ff ff f3 0f 1e fa 4c 89 RSP: 0018:ffffc90003dff4e0 EFLAGS: 00010093 RAX: ffffffff81e95988 RBX: 00000000000001c1 RCX: ffff8880205b3a80 RDX: 0000000000000000 RSI: 00000000000001c0 RDI: 00000000000001c1 RBP: ffffc90003dff830 R08: ffffffff81e90e67 R09: fffffbfff1a433c3 R10: 0000000000000000 R11: dffffc0000000001 R12: 0000000000000000 R13: ffffc90003dff6c0 R14: 00000000000001c0 R15: 0000000000000000 FS: 00007fdbae5ee700(0000) GS:ffff8880b9900000(0000) knlGS:0000000000000000 CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 CR2: 00007fdbae6901e0 CR3: 000000007b2dd000 CR4: 00000000003506e0 DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000 DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400 Call Trace: <TASK> madvise_collapse+0x721/0xf50 mm/khugepaged.c:2693 madvise_vma_behavior mm/madvise.c:1086 [inline] madvise_walk_vmas mm/madvise.c:1260 [inline] do_madvise+0x9e5/0x4680 mm/madvise.c:1439 __do_sys_madvise mm/madvise.c:1452 [inline] __se_sys_madvise mm/madvise.c:1450 [inline] __x64_sys_madvise+0xa5/0xb0 mm/madvise.c:1450 do_syscall_x64 arch/x86/entry/common.c:50 [inline] do_syscall_64+0x41/0xc0 arch/x86/entry/common.c:80 entry_SYSCALL_64_after_hwframe+0x63/0xcd
The 'xas_store' call during page cache scanning can potentially translate 'xas' into the error state (with the reproducer provided by the syzkaller the error code is -ENOMEM). However, there are no further checks after the 'xas_store', and the next call of 'xas_next' at the start of the scanning cycle doesn't increase the xa_index, and the issue occurs.
This patch will add the xarray state error checking after the 'xas_store' and the corresponding result error code. It will also add xarray state error checking via WARN_ON_ONCE macros, to be sure that ENOMEM or other possible errors don't occur at the places they shouldn't.
Tested via syzbot.
---
V1 -> V2: Add WARN_ON_ONCE error checking and comments
mm/khugepaged.c | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+)
mm/khugepaged.c +20 -0
@@ -55,6 +55,7 @@ enum scan_result {SCAN_CGROUP_CHARGE_FAIL ,SCAN_TRUNCATED ,SCAN_PAGE_HAS_PRIVATE , + SCAN_STORE_FAILED, };#define CREATE_TRACE_POINTS
@@ -1840,6 +1841,15 @@ static int collapse_file(struct mm_struct *mm, unsigned long addr, goto xa_locked; }xas_store (&xas, hpage); +if (xas_error (&xas)) { +/* revert shmem_charge performed +* in the previous condition +*/ + mapping->nrpages--; +shmem_uncharge (mapping->host,1 ); + result =SCAN_STORE_FAILED ; + goto xa_locked; + } nr_none++;continue ; }
@@ -1992,6 +2002,11 @@ static int collapse_file(struct mm_struct *mm, unsigned long addr,/* Finally, replace with the new page. */ xas_store (&xas, hpage); +/* We can't get an ENOMEM here (because the allocation happened before) +* but let's check for errors (XArray implementation can be +* changed in the future) +*/ +WARN_ON_ONCE (xas_error (&xas));continue ; out_unlock:unlock_page (page);
@@ -2029,6 +2044,11 @@ static int collapse_file(struct mm_struct *mm, unsigned long addr,/* Join all the small entries into a single multi-index entry */ xas_set_order (&xas, start,HPAGE_PMD_ORDER );xas_store (&xas, hpage); +/* Here we can't get an ENOMEM (because entries were +* previously allocated) But let's check for errors +* (XArray implementation can be changed in the future) +*/ +WARN_ON_ONCE (xas_error (&xas)); xa_locked:xas_unlock_irq (&xas); xa_unlocked:
Signature · 1 line
2.34.1
Quoted 24 lines · Ivan Orlov
Syzkaller reported the following issue:
kernel BUG at
mm/khugepaged.c:1823!
invalid opcode: 0000 [#1] PREEMPT SMP KASANCPU: 1 PID: 5097 Comm: syz-executor220 Not tainted 6.2.0-syzkaller-13154-g857f1268a591 #0 Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 02/16/2023 RIP: 0010:collapse_file mm/khugepaged.c:1823 [inline] RIP: 0010:hpage_collapse_scan_file+0x67c8/0x7580 mm/khugepaged.c:2233 Code: 00 00 89 de e8 c9 66 a3 ff 31 ff 89 de e8 c0 66 a3 ff 45 84 f6 0f 85 28 0d 00 00 e8 22 64 a3 ff e9 dc f7 ff ff e8 18 64 a3 ff <0f> 0b f3 0f 1e fa e8 0d 64 a3 ff e9 93 f6 ff ff f3 0f 1e fa 4c 89 RSP: 0018:ffffc90003dff4e0 EFLAGS: 00010093 RAX: ffffffff81e95988 RBX: 00000000000001c1 RCX: ffff8880205b3a80 RDX: 0000000000000000 RSI: 00000000000001c0 RDI: 00000000000001c1 RBP: ffffc90003dff830 R08: ffffffff81e90e67 R09: fffffbfff1a433c3 R10: 0000000000000000 R11: dffffc0000000001 R12: 0000000000000000 R13: ffffc90003dff6c0 R14: 00000000000001c0 R15: 0000000000000000 FS: 00007fdbae5ee700(0000) GS:ffff8880b9900000(0000) knlGS:0000000000000000 CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 CR2: 00007fdbae6901e0 CR3: 000000007b2dd000 CR4: 00000000003506e0 DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000 DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400 Call Trace: <TASK> madvise_collapse+0x721/0xf50 mm/khugepaged.c:2693 madvise_vma_behavior mm/madvise.c:1086 [inline]
Quoted 9 lines
madvise_walk_vmas mm/madvise.c:1260 [inline] do_madvise+0x9e5/0x4680 mm/madvise.c:1439 __do_sys_madvise mm/madvise.c:1452 [inline] __se_sys_madvise mm/madvise.c:1450 [inline] __x64_sys_madvise+0xa5/0xb0 mm/madvise.c:1450 do_syscall_x64 arch/x86/entry/common.c:50 [inline] do_syscall_64+0x41/0xc0 arch/x86/entry/common.c:80 entry_SYSCALL_64_after_hwframe+0x63/0xcd
Thanks, Ivan.
In the process of reviewing this, I starting thinking if the !shmem case was also susceptible to a similar race, and I *think* it might be. Unfortunately, my time has ran out, and I haven't been able to validate ; I'm less familiar with the file-side of things.
The underlying problem is race with truncation/hole-punch under OOM condition. The nice do-while loop near the top of collapse_file() attempts to avoid this scenario by making sure enough slots are available. However, when we drop xarray lock, we open ourselves up to concurrent removal + slot deletion. Those slots then need to be allocated again -- which under OOM condition is failable.
The syzbot reproducer picks on shmem, but I think this can occur for file as well. If we find a hole, we unlock the xarray and call page_cache_sync_readahead(), which if it succeeds, IIUC, will have allocated a new slot in our mapping pointing to the new page. We *then* locks the page. Only after the page is locked are we protected from concurrent removal (Note: this is what provides us protection in many of the xas_store() cases ; we've held the slot's contained page-lock since verifying the slot exists, protecting us from removal / reallocation races).
Maybe I'm just low on caffeine at the end of the day, and am missing something, but if I had more time, I'd be looking into the file-side some more to verify. Apologies that hasn't occurred to me until now ; I was looking at one of your comments and double-checked why I *thought* we were safe.
Anyways, irrespective of that looming issues, some more notes to follow:
Quoted 7 lines
The '
xas_store' call during page cache scanning can potentially translate 'xas' into the error state (with the reproducer provided by the syzkaller the error code is-ENOMEM). However, there are no further checks after the 'xas_store', and the next call of 'xas_next' at the start of the scanning cycle doesn't increase thexa_index, and the issue occurs.
Quoted 5 lines
This patch will add the xarray state error checking after the '
xas_store' and the corresponding result error code. It will also add xarray state error checking viaWARN_ON_ONCEmacros, to be sure that ENOMEM or other possible errors don't occur at the places they shouldn't.
Thanks for the additions here. I think it's worthwhile providing even more details about the specifics of the race we are fixing and/or guarding against to help ppl understand how that -ENOMEM comes about if the do-while loop has "Ensured" we have slots available (additionally, I think that comment can be augmented).
Quoted 16 lines
Tested via syzbot.
Reported-by [email protected]Signed-off-by Ivan Orlov <[email protected]>---
V1 -> V2: AddWARN_ON_ONCEerror checking and commentsmm/khugepaged.c | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+)mm/khugepaged.c +0
-0
mm/khugepaged.c enum scan_result { · line 59
Quoted 5 lines
mm/khugepaged.c +1
-0@@ -55,6 +55,7 @@ enum scan_result {SCAN_CGROUP_CHARGE_FAIL ,SCAN_TRUNCATED ,SCAN_PAGE_HAS_PRIVATE , + SCAN_STORE_FAILED, };
I'm still reluctant to add a new error code for this as this seems like quite a rare race that requires OOM to trigger. I'd be happier just reusing SCAN_FAIL, or, something we might get some millage out of later: SCAN_OOM.
Also, a reminder to update include/trace/events/huge_memory.h, if you go that route.
mm/khugepaged.c static int collapse_file(struct mm_struct *mm, unsigned long addr, · line 1847
Quoted 10 lines
mm/khugepaged.c +4
-0#define CREATE_TRACE_POINTS @@ -1840,6 +1841,15 @@ static int collapse_file(struct mm_struct *mm, unsigned long addr, goto xa_locked; }xas_store (&xas, hpage); +if (xas_error (&xas)) { +/* revert shmem_charge performed +* in the previous condition +*/
Nit: Here, and following, I think standard convention for multiline comment is to have an empty first and last line, eg:
+ /*
+ * revert shmem_charge performed
+ * in the previous condition
+ */
Though, checkpatch.pl --strict didn't seem to care.
Quoted 9 lines
mm/khugepaged.c +5
-0+ mapping->nrpages--; +shmem_uncharge (mapping->host,1 ); + result =SCAN_STORE_FAILED ; + goto xa_locked; + } nr_none++;continue ; }
mm/khugepaged.c static int collapse_file(struct mm_struct *mm, unsigned long addr, · line 2009
Quoted 8 lines
mm/khugepaged.c +5
-0@@ -1992,6 +2002,11 @@ static int collapse_file(struct mm_struct *mm, unsigned long addr,/* Finally, replace with the new page. */ xas_store (&xas, hpage); +/* We can't get an ENOMEM here (because the allocation happened before) +* but let's check for errors (XArray implementation can be +* changed in the future) +*/ +WARN_ON_ONCE (xas_error (&xas));
Nit: it's not just that allocation happened before -- need some guarantee we've been protected from concurrent removal. This is what made me look at the file side.
mm/khugepaged.c static int collapse_file(struct mm_struct *mm, unsigned long addr, · line 2051
Quoted 12 lines
mm/khugepaged.c +5
-0continue ; out_unlock:unlock_page (page);@@ -2029,6 +2044,11 @@ static int collapse_file(struct mm_struct *mm, unsigned long addr,/* Join all the small entries into a single multi-index entry */ xas_set_order (&xas, start,HPAGE_PMD_ORDER );xas_store (&xas, hpage); +/* Here we can't get an ENOMEM (because entries were +* previously allocated) But let's check for errors +* (XArray implementation can be changed in the future) +*/ +WARN_ON_ONCE (xas_error (&xas));
Ditto.
Apologies I won't be around to see this change through -- I'm just out of time, and will be shutting my computer down tomorrow for 3 months. Sorry for the poor timing, for raising issues, then disappearing. Hopefully I'm wrong and the file-side isn't a concern.
Best,
Zach
Quoted 6 lines
mm/khugepaged.c +0
-0xa_locked:xas_unlock_irq (&xas); xa_unlocked:Signature · 1 line
2.34.1
_______________________________________________
Linux-kernel-mentees mailing list
[email protected]
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees
Quoted 135 lines · Zach O'Keefe
Thanks, Ivan.
In the process of reviewing this, I starting thinking if the !shmem case was also susceptible to a similar race, and I *think* it might be. Unfortunately, my time has ran out, and I haven't been able to validate ; I'm less familiar with the file-side of things.
The underlying problem is race with truncation/hole-punch under OOM condition. The nice do-while loop near the top of
collapse_file()attempts to avoid this scenario by making sure enough slots are available. However, when we drop xarray lock, we open ourselves up to concurrent removal + slot deletion. Those slots then need to be allocated again -- which under OOM condition is failable.The syzbot reproducer picks on shmem, but I think this can occur for file as well. If we find a hole, we unlock the xarray and call
page_cache_sync_readahead(), which if it succeeds, IIUC, will have allocated a new slot in our mapping pointing to the new page. We *then* locks the page. Only after the page is locked are we protected from concurrent removal (Note: this is what provides us protection in many of thexas_store()cases ; we've held the slot's contained page-lock since verifying the slot exists, protecting us from removal / reallocation races).Maybe I'm just low on caffeine at the end of the day, and am missing something, but if I had more time, I'd be looking into the file-side some more to verify. Apologies that hasn't occurred to me until now ; I was looking at one of your comments and double-checked why I *thought* we were safe.
Anyways, irrespective of that looming issues, some more notes to follow:
Quoted 7 lines
The '
xas_store' call during page cache scanning can potentially translate 'xas' into the error state (with the reproducer provided by the syzkaller the error code is-ENOMEM). However, there are no further checks after the 'xas_store', and the next call of 'xas_next' at the start of the scanning cycle doesn't increase thexa_index, and the issue occurs.Quoted 5 lines
This patch will add the xarray state error checking after the '
xas_store' and the corresponding result error code. It will also add xarray state error checking viaWARN_ON_ONCEmacros, to be sure that ENOMEM or other possible errors don't occur at the places they shouldn't.Thanks for the additions here. I think it's worthwhile providing even more details about the specifics of the race we are fixing and/or guarding against to help ppl understand how that
-ENOMEMcomes about if the do-while loop has "Ensured" we have slots available (additionally, I think that comment can be augmented).Quoted 16 lines
Tested via syzbot.
Reported-by [email protected]Signed-off-by Ivan Orlov <[email protected]>---
V1 -> V2: AddWARN_ON_ONCEerror checking and commentsmm/khugepaged.c | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+)mm/khugepaged.c +0
-0mm/khugepaged.c enum scan_result { · line 59
Quoted 5 lines
mm/khugepaged.c +1
-0@@ -55,6 +55,7 @@ enum scan_result {SCAN_CGROUP_CHARGE_FAIL ,SCAN_TRUNCATED ,SCAN_PAGE_HAS_PRIVATE , + SCAN_STORE_FAILED, };I'm still reluctant to add a new error code for this as this seems like quite a rare race that requires OOM to trigger. I'd be happier just reusing
SCAN_FAIL, or, something we might get some millage out of later:SCAN_OOM.Also, a reminder to update
include/trace/events/huge_memory.h, if you go that route.mm/khugepaged.c static int collapse_file(struct mm_struct *mm, unsigned long addr, · line 1847
Quoted 10 lines
mm/khugepaged.c +4
-0#define CREATE_TRACE_POINTS @@ -1840,6 +1841,15 @@ static int collapse_file(struct mm_struct *mm, unsigned long addr, goto xa_locked; }xas_store (&xas, hpage); +if (xas_error (&xas)) { +/* revert shmem_charge performed +* in the previous condition +*/ Nit: Here, and following, I think standard convention for multiline comment is to have an empty first and last line, eg:
+ /*
+ * revertshmem_chargeperformed
+ * in the previous condition
+ */Though, checkpatch.pl --strict didn't seem to care.
Quoted 9 lines
mm/khugepaged.c +5
-0+ mapping->nrpages--; +shmem_uncharge (mapping->host,1 ); + result =SCAN_STORE_FAILED ; + goto xa_locked; + } nr_none++;continue ; }mm/khugepaged.c static int collapse_file(struct mm_struct *mm, unsigned long addr, · line 2009
Quoted 8 lines
mm/khugepaged.c +5
-0@@ -1992,6 +2002,11 @@ static int collapse_file(struct mm_struct *mm, unsigned long addr,/* Finally, replace with the new page. */ xas_store (&xas, hpage); +/* We can't get an ENOMEM here (because the allocation happened before) +* but let's check for errors (XArray implementation can be +* changed in the future) +*/ +WARN_ON_ONCE (xas_error (&xas));Nit: it's not just that allocation happened before -- need some guarantee we've been protected from concurrent removal. This is what made me look at the file side.
mm/khugepaged.c static int collapse_file(struct mm_struct *mm, unsigned long addr, · line 2051
Quoted 12 lines
mm/khugepaged.c +5
-0continue ; out_unlock:unlock_page (page);@@ -2029,6 +2044,11 @@ static int collapse_file(struct mm_struct *mm, unsigned long addr,/* Join all the small entries into a single multi-index entry */ xas_set_order (&xas, start,HPAGE_PMD_ORDER );xas_store (&xas, hpage); +/* Here we can't get an ENOMEM (because entries were +* previously allocated) But let's check for errors +* (XArray implementation can be changed in the future) +*/ +WARN_ON_ONCE (xas_error (&xas));Ditto.
Quoted 14 lines
Apologies I won't be around to see this change through -- I'm just out of time, and will be shutting my computer down tomorrow for 3 months. Sorry for the poor timing, for raising issues, then disappearing. Hopefully I'm wrong and the file-side isn't a concern.
Best,
ZachQuoted 6 lines
mm/khugepaged.c +0
-0xa_locked:xas_unlock_irq (&xas); xa_unlocked:Signature · 1 line
2.34.1
Hello, Zach! Thank you very much for the detailed review! I thought that locking is our guarantee to not get an -ENOMEM, but I didn't have the deep understanding of the problem's cause, due to the fact I'm just starting my memory management journey :) In any case, I will do a research about this problem, and hopefully after you get out of the vacation you will see a new patch fully fixes this problem. Have a nice vacation! Thanks again!
Quoted 49 lines · Zach O'Keefe
Quoted 24 lines · Ivan Orlov
Syzkaller reported the following issue:
kernel BUG at
mm/khugepaged.c:1823!
invalid opcode: 0000 [#1] PREEMPT SMP KASANCPU: 1 PID: 5097 Comm: syz-executor220 Not tainted 6.2.0-syzkaller-13154-g857f1268a591 #0 Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 02/16/2023 RIP: 0010:collapse_file mm/khugepaged.c:1823 [inline] RIP: 0010:hpage_collapse_scan_file+0x67c8/0x7580 mm/khugepaged.c:2233 Code: 00 00 89 de e8 c9 66 a3 ff 31 ff 89 de e8 c0 66 a3 ff 45 84 f6 0f 85 28 0d 00 00 e8 22 64 a3 ff e9 dc f7 ff ff e8 18 64 a3 ff <0f> 0b f3 0f 1e fa e8 0d 64 a3 ff e9 93 f6 ff ff f3 0f 1e fa 4c 89 RSP: 0018:ffffc90003dff4e0 EFLAGS: 00010093 RAX: ffffffff81e95988 RBX: 00000000000001c1 RCX: ffff8880205b3a80 RDX: 0000000000000000 RSI: 00000000000001c0 RDI: 00000000000001c1 RBP: ffffc90003dff830 R08: ffffffff81e90e67 R09: fffffbfff1a433c3 R10: 0000000000000000 R11: dffffc0000000001 R12: 0000000000000000 R13: ffffc90003dff6c0 R14: 00000000000001c0 R15: 0000000000000000 FS: 00007fdbae5ee700(0000) GS:ffff8880b9900000(0000) knlGS:0000000000000000 CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 CR2: 00007fdbae6901e0 CR3: 000000007b2dd000 CR4: 00000000003506e0 DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000 DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400 Call Trace: <TASK> madvise_collapse+0x721/0xf50 mm/khugepaged.c:2693 madvise_vma_behavior mm/madvise.c:1086 [inline]Quoted 9 lines
madvise_walk_vmas mm/madvise.c:1260 [inline] do_madvise+0x9e5/0x4680 mm/madvise.c:1439 __do_sys_madvise mm/madvise.c:1452 [inline] __se_sys_madvise mm/madvise.c:1450 [inline] __x64_sys_madvise+0xa5/0xb0 mm/madvise.c:1450 do_syscall_x64 arch/x86/entry/common.c:50 [inline] do_syscall_64+0x41/0xc0 arch/x86/entry/common.c:80 entry_SYSCALL_64_after_hwframe+0x63/0xcdThanks, Ivan.
In the process of reviewing this, I starting thinking if the !shmem case was also susceptible to a similar race, and I *think* it might be. Unfortunately, my time has ran out, and I haven't been able to validate ; I'm less familiar with the file-side of things.
The underlying problem is race with truncation/hole-punch under OOM condition. The nice do-while loop near the top of
collapse_file()attempts to avoid this scenario by making sure enough slots are available. However, when we drop xarray lock, we open ourselves up to concurrent removal + slot deletion. Those slots then need to be allocated again -- which under OOM condition is failable.
Quoted 8 lines
The syzbot reproducer picks on shmem, but I think this can occur for file as well. If we find a hole, we unlock the xarray and call
page_cache_sync_readahead(), which if it succeeds, IIUC, will have allocated a new slot in our mapping pointing to the new page. We *then* locks the page. Only after the page is locked are we protected from concurrent removal (Note: this is what provides us protection in many of thexas_store()cases ; we've held the slot's contained page-lock since verifying the slot exists, protecting us from removal / reallocation races).
IIUC, the find_lock_page() should be able to handle the race. It does check whether the page is truncated or not after locking the page.
Quoted 128 lines
Maybe I'm just low on caffeine at the end of the day, and am missing something, but if I had more time, I'd be looking into the file-side some more to verify. Apologies that hasn't occurred to me until now ; I was looking at one of your comments and double-checked why I *thought* we were safe.
Anyways, irrespective of that looming issues, some more notes to follow:
Quoted 7 lines
The '
xas_store' call during page cache scanning can potentially translate 'xas' into the error state (with the reproducer provided by the syzkaller the error code is-ENOMEM). However, there are no further checks after the 'xas_store', and the next call of 'xas_next' at the start of the scanning cycle doesn't increase thexa_index, and the issue occurs.Quoted 5 lines
This patch will add the xarray state error checking after the '
xas_store' and the corresponding result error code. It will also add xarray state error checking viaWARN_ON_ONCEmacros, to be sure that ENOMEM or other possible errors don't occur at the places they shouldn't.Thanks for the additions here. I think it's worthwhile providing even more details about the specifics of the race we are fixing and/or guarding against to help ppl understand how that
-ENOMEMcomes about if the do-while loop has "Ensured" we have slots available (additionally, I think that comment can be augmented).Quoted 16 lines
Tested via syzbot.
Reported-by [email protected]Signed-off-by Ivan Orlov <[email protected]>---
V1 -> V2: AddWARN_ON_ONCEerror checking and commentsmm/khugepaged.c | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+)mm/khugepaged.c +0
-0mm/khugepaged.c enum scan_result { · line 59
Quoted 5 lines
mm/khugepaged.c +1
-0@@ -55,6 +55,7 @@ enum scan_result {SCAN_CGROUP_CHARGE_FAIL ,SCAN_TRUNCATED ,SCAN_PAGE_HAS_PRIVATE , + SCAN_STORE_FAILED, };I'm still reluctant to add a new error code for this as this seems like quite a rare race that requires OOM to trigger. I'd be happier just reusing
SCAN_FAIL, or, something we might get some millage out of later:SCAN_OOM.Also, a reminder to update
include/trace/events/huge_memory.h, if you go that route.mm/khugepaged.c static int collapse_file(struct mm_struct *mm, unsigned long addr, · line 1847
Quoted 10 lines
mm/khugepaged.c +4
-0#define CREATE_TRACE_POINTS @@ -1840,6 +1841,15 @@ static int collapse_file(struct mm_struct *mm, unsigned long addr, goto xa_locked; }xas_store (&xas, hpage); +if (xas_error (&xas)) { +/* revert shmem_charge performed +* in the previous condition +*/ Nit: Here, and following, I think standard convention for multiline comment is to have an empty first and last line, eg:
+ /*
+ * revert shmem_charge performed + * in the previous condition + */Though, checkpatch.pl --strict didn't seem to care.
Quoted 9 lines
mm/khugepaged.c +5
-0+ mapping->nrpages--; +shmem_uncharge (mapping->host,1 ); + result =SCAN_STORE_FAILED ; + goto xa_locked; + } nr_none++;continue ; }mm/khugepaged.c static int collapse_file(struct mm_struct *mm, unsigned long addr, · line 2009
Quoted 8 lines
mm/khugepaged.c +5
-0@@ -1992,6 +2002,11 @@ static int collapse_file(struct mm_struct *mm, unsigned long addr,/* Finally, replace with the new page. */ xas_store (&xas, hpage); +/* We can't get an ENOMEM here (because the allocation happened before) +* but let's check for errors (XArray implementation can be +* changed in the future) +*/ +WARN_ON_ONCE (xas_error (&xas));Nit: it's not just that allocation happened before -- need some guarantee we've been protected from concurrent removal. This is what made me look at the file side.
mm/khugepaged.c static int collapse_file(struct mm_struct *mm, unsigned long addr, · line 2051
Quoted 12 lines
mm/khugepaged.c +5
-0continue ; out_unlock:unlock_page (page);@@ -2029,6 +2044,11 @@ static int collapse_file(struct mm_struct *mm, unsigned long addr,/* Join all the small entries into a single multi-index entry */ xas_set_order (&xas, start,HPAGE_PMD_ORDER );xas_store (&xas, hpage); +/* Here we can't get an ENOMEM (because entries were +* previously allocated) But let's check for errors +* (XArray implementation can be changed in the future) +*/ +WARN_ON_ONCE (xas_error (&xas));Ditto.
Apologies I won't be around to see this change through -- I'm just out of time, and will be shutting my computer down tomorrow for 3 months. Sorry for the poor timing, for raising issues, then disappearing. Hopefully I'm wrong and the file-side isn't a concern.
Best,
ZachQuoted 6 lines
mm/khugepaged.c +0
-1xa_locked:xas_unlock_irq (&xas); xa_unlocked: --2.34.1
Quoted 71 lines · Yang Shi
Quoted 7 lines · Zach O'Keefe
Quoted 5 lines · Ivan Orlov
Syzkaller reported the following issue:
kernel BUG at
mm/khugepaged.c:1823!
invalid opcode: 0000 [#1] PREEMPT SMP KASANCPU: 1 PID: 5097 Comm: syz-executor220 Not tainted6.2.0-syzkaller-13154-g857f1268a591 #0
Quoted 1 line
Quoted 1 line
Hardware name: Google Google Compute Engine/Google Compute Engine,BIOS Google 02/16/2023
Quoted 3 lines
Quoted 3 lines
RIP: 0010:collapse_file mm/khugepaged.c:1823 [inline] RIP: 0010:hpage_collapse_scan_file+0x67c8/0x7580 mm/khugepaged.c:2233 Code: 00 00 89 de e8 c9 66 a3 ff 31 ff 89 de e8 c0 66 a3 ff 45 84 f60f 85 28 0d 00 00 e8 22 64 a3 ff e9 dc f7 ff ff e8 18 64 a3 ff <0f> 0b f3 0f 1e fa e8 0d 64 a3 ff e9 93 f6 ff ff f3 0f 1e fa 4c 89
Quoted 7 lines
Quoted 7 lines
RSP: 0018:ffffc90003dff4e0 EFLAGS: 00010093 RAX: ffffffff81e95988 RBX: 00000000000001c1 RCX: ffff8880205b3a80 RDX: 0000000000000000 RSI: 00000000000001c0 RDI: 00000000000001c1 RBP: ffffc90003dff830 R08: ffffffff81e90e67 R09: fffffbfff1a433c3 R10: 0000000000000000 R11: dffffc0000000001 R12: 0000000000000000 R13: ffffc90003dff6c0 R14: 00000000000001c0 R15: 0000000000000000 FS: 00007fdbae5ee700(0000) GS:ffff8880b9900000(0000)knlGS:0000000000000000
Quoted 18 lines
Quoted 8 lines
CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 CR2: 00007fdbae6901e0 CR3: 000000007b2dd000 CR4: 00000000003506e0 DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000 DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400 Call Trace: <TASK> madvise_collapse+0x721/0xf50 mm/khugepaged.c:2693 madvise_vma_behavior mm/madvise.c:1086 [inline]Quoted 9 lines
madvise_walk_vmas mm/madvise.c:1260 [inline] do_madvise+0x9e5/0x4680 mm/madvise.c:1439 __do_sys_madvise mm/madvise.c:1452 [inline] __se_sys_madvise mm/madvise.c:1450 [inline] __x64_sys_madvise+0xa5/0xb0 mm/madvise.c:1450 do_syscall_x64 arch/x86/entry/common.c:50 [inline] do_syscall_64+0x41/0xc0 arch/x86/entry/common.c:80 entry_SYSCALL_64_after_hwframe+0x63/0xcdQuoted 3 lines
Thanks, Ivan.
In the process of reviewing this, I starting thinking if the !shmem case
was
Quoted 1 line
also susceptible to a similar race, and I *think* it might be.
Unfortunately, my
Quoted 1 line
time has ran out, and I haven't been able to validate ; I'm less
familiar with
Quoted 3 lines
the file-side of things.
The underlying problem is race with truncation/hole-punch under OOM
condition.
Quoted 1 line
The nice do-while loop near the top of
collapse_file()attempts to avoidthis
Quoted 1 line
scenario by making sure enough slots are available. However, when we
drop xarray
Quoted 1 line
lock, we open ourselves up to concurrent removal + slot deletion. Those
slots
Quoted 3 lines
then need to be allocated again -- which under OOM condition is failable.
The syzbot reproducer picks on shmem, but I think this can occur for
file as
Quoted 2 lines
well. If we find a hole, we unlock the xarray and call
page_cache_sync_readahead(), which if it succeeds, IIUC, will haveallocated a
Quoted 1 line
new slot in our mapping pointing to the new page. We *then* locks the
page. Only
Quoted 1 line
after the page is locked are we protected from concurrent removal (Note:
this is
Quoted 9 lines
Quoted 1 line
what provides us protection in many of the
xas_store()cases ; we'veheld the
Quoted 1 line
slot's contained page-lock since verifying the slot exists, protecting
us from
Quoted 1 line
removal / reallocation races).
IIUC, the
find_lock_page()should be able to handle the race. It does check whether the page is truncated or not after locking the page.
Ugh. Duh. You’re right, and I’m not sure how I missed that implication; we *did* succeed in locking the page in page cache. Well, the above rant just looks foolish now.
Anyways, sorry for the false alarm, Ivan, and thanks for keeping me honest, Yang!
Quoted 151 lines
Quoted 2 lines
Maybe I'm just low on caffeine at the end of the day, and am missing
something,
Quoted 1 line
but if I had more time, I'd be looking into the file-side some more to
verify.
Quoted 1 line
Apologies that hasn't occurred to me until now ; I was looking at one of
your
Quoted 11 lines
comments and double-checked why I *thought* we were safe.
Anyways, irrespective of that looming issues, some more notes to follow:
Quoted 7 lines
The '
xas_store' call during page cache scanning can potentially translate 'xas' into the error state (with the reproducer provided by the syzkaller the error code is-ENOMEM). However, there are no further checks after the 'xas_store', and the next call of 'xas_next' at the start of the scanning cycle doesn't increase thexa_index, and the issue occurs.Quoted 7 lines
Quoted 5 lines
This patch will add the xarray state error checking after the '
xas_store' and the corresponding result error code. It will also add xarray state error checking viaWARN_ON_ONCEmacros, to be sure that ENOMEM or other possible errors don't occur at the places they shouldn't.Thanks for the additions here. I think it's worthwhile providing even
more
Quoted 1 line
details about the specifics of the race we are fixing and/or guarding
against to
Quoted 2 lines
help ppl understand how that
-ENOMEMcomes about if the do-while loop has "Ensured" we have slots available (additionally, I think that commentcan be
Quoted 6 lines
augmented).
Quoted 4 lines
Tested via syzbot.
Reported-by [email protected]Link:
https://syzkaller.appspot.com/bug?id=7d6bb3760e026ece7524500fe44fb024a0e959fc
Quoted 7 lines
Quoted 7 lines
Signed-off-by Ivan Orlov <[email protected]>---
V1 -> V2: AddWARN_ON_ONCEerror checking and commentsmm/khugepaged.c | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+)Quoted 12 lines
mm/khugepaged.c enum scan_result { · line 59
Quoted 10 lines
mm/khugepaged.c +1
-0@@ -55,6 +55,7 @@ enum scan_result {SCAN_CGROUP_CHARGE_FAIL ,SCAN_TRUNCATED ,SCAN_PAGE_HAS_PRIVATE , + SCAN_STORE_FAILED, };I'm still reluctant to add a new error code for this as this seems like
quite a
Quoted 1 line
rare race that requires OOM to trigger. I'd be happier just reusing
SCAN_FAIL,Quoted 3 lines
or, something we might get some millage out of later:
SCAN_OOM.Also, a reminder to update
include/trace/events/huge_memory.h, if you gothat
Quoted 5 lines
route.
mm/khugepaged.c static int collapse_file(struct mm_struct *mm,
Quoted 3 lines
mm/khugepaged.c +0
-0#define CREATE_TRACE_POINTS @@ -1840,6 +1841,15 @@ static int collapse_file(struct mm_struct *mm,unsigned long addr,
Quoted 9 lines
mm/khugepaged.c static int collapse_file(struct mm_struct *mm, unsigned long addr, · line 1847
Quoted 7 lines
mm/khugepaged.c +4
-0goto xa_locked; }xas_store (&xas, hpage); +if (xas_error (&xas)) { +/* revert shmem_charge performed +* in the previous condition +*/ Nit: Here, and following, I think standard convention for multiline
comment is
Quoted 9 lines
to have an empty first and last line, eg:
+ /*
+ * revert shmem_charge performed + * in the previous condition + */Though, checkpatch.pl --strict didn't seem to care.
Quoted 9 lines
mm/khugepaged.c static int collapse_file(struct mm_struct *mm,
Quoted 9 lines
mm/khugepaged.c +5
-0+ mapping->nrpages--; +shmem_uncharge (mapping->host,1 ); + result =SCAN_STORE_FAILED ; + goto xa_locked; + } nr_none++;continue ; }@@ -1992,6 +2002,11 @@ static int collapse_file(struct mm_struct *mm,unsigned long addr,
Quoted 4 lines
mm/khugepaged.c static int collapse_file(struct mm_struct *mm, unsigned long addr,
Quoted 4 lines
mm/khugepaged.c +1
-0/* Finally, replace with the new page. */ xas_store (&xas, hpage); +/* We can't get an ENOMEM here (because the allocation happened before)
Quoted 1 line
mm/khugepaged.c static int collapse_file(struct mm_struct *mm, · line 2005
Quoted 1 line
mm/khugepaged.c +1
-0+* but let's check for errors (XArray implementation can be
Quoted 5 lines
mm/khugepaged.c static int collapse_file(struct mm_struct *mm, unsigned long addr, · line 2009
Quoted 3 lines
mm/khugepaged.c +3
-0+* changed in the future) +*/ +WARN_ON_ONCE (xas_error (&xas));Nit: it's not just that allocation happened before -- need some
guarantee we've
Quoted 1 line
been protected from concurrent removal. This is what made me look at the
file
Quoted 6 lines
side.
mm/khugepaged.c static int collapse_file(struct mm_struct *mm,
Quoted 4 lines
mm/khugepaged.c +0
-0continue ; out_unlock:unlock_page (page);@@ -2029,6 +2044,11 @@ static int collapse_file(struct mm_struct *mm,unsigned long addr,
Quoted 9 lines
mm/khugepaged.c static int collapse_file(struct mm_struct *mm, unsigned long addr, · line 2051
Quoted 8 lines
mm/khugepaged.c +5
-0/* Join all the small entries into a single multi-index entry */ xas_set_order (&xas, start,HPAGE_PMD_ORDER );xas_store (&xas, hpage); +/* Here we can't get an ENOMEM (because entries were +* previously allocated) But let's check for errors +* (XArray implementation can be changed in the future) +*/ +WARN_ON_ONCE (xas_error (&xas));Quoted 3 lines
Ditto.
Apologies I won't be around to see this change through -- I'm just out
of time,
Quoted 1 line
and will be shutting my computer down tomorrow for 3 months. Sorry for
the poor
Quoted 1 line
timing, for raising issues, then disappearing. Hopefully I'm wrong and
the
Quoted 11 lines
file-side isn't a concern.
Best,
ZachQuoted 6 lines
mm/khugepaged.c +0
-1xa_locked:xas_unlock_irq (&xas); xa_unlocked: --2.34.1
Andrew Morton
maintainerCircling back to this fix...
The BUG() is obviously real. We're unsure that Ivan's fix is the best one. We haven't identified a Fixes:, and as this report is against the 6.2 kernel, a cc:stable will be needed.
According to the sysbot bisection
(https://syzkaller.appspot.com/bug?id=7d6bb3760e026ece7524500fe44fb024a0e959fc),
this is present in linux-5.19, so it might predate Zach's 58ac9a8993a13ebc changes. But that bisection claim might be misleading.
And Zach is offline for a few months. So can people please take a look and see if we can get this wrapped up?
Matthew, the assertion failure is in the
VM_BUG_ON(index != xas.xa_index);
which was added in 77da9389b9d5f, so perhaps you could take a look?
Thanks.
Quoted 16 lines · Andrew Morton
Circling back to this fix...
The
BUG()is obviously real. We're unsure that Ivan's fix is the best one. We haven't identified a Fixes:, and as this report is against the 6.2 kernel, a cc:stable will be needed.According to the sysbot bisection
(https://syzkaller.appspot.com/bug?id=7d6bb3760e026ece7524500fe44fb024a0e959fc),
this is present in linux-5.19, so it might predate Zach's58ac9a8993a13ebcchanges. But that bisection claim might be misleading.And Zach is offline for a few months. So can people please take a look and see if we can get this wrapped up?
Quoted 7 lines
Matthew, the assertion failure is in the
VM_BUG_ON(index != xas.xa_index);which was added in
77da9389b9d5f, so perhaps you could take a look?Thanks.
Hello, Andrew!
I have been unable to reproduce the problem with any of the existing reproducers on the 3d7cb6b04c3f3115719235cc6866b10326de34cd commit, which was detected by the syzkaller bisection. I also tried to test if the problem is reproducible on this commit via syzbot, but it did not detect the problem. It's possible that the bisection claim is misleading, as the issue may not be consistently reproducible.
Why did you mention the 58ac9a8993a13ebc commit? I thought 99cb0dbd47a15 was considered as a "Fixes". 99cb0dbd47a15 is older than 3d7cb6b04c3f3, and the problematic code appeared there, so probably the problem could appear in 99cb0dbd47a15 as well.
Please, correct me if I'm missing something.
Kind regards,
Ivan Orlov.
Quoted 16 lines · Andrew Morton
Circling back to this fix...
The
BUG()is obviously real. We're unsure that Ivan's fix is the best one. We haven't identified a Fixes:, and as this report is against the 6.2 kernel, a cc:stable will be needed.According to the sysbot bisection
(https://syzkaller.appspot.com/bug?id=7d6bb3760e026ece7524500fe44fb024a0e959fc),
this is present in linux-5.19, so it might predate Zach's58ac9a8993a13ebcchanges. But that bisection claim might be misleading.And Zach is offline for a few months. So can people please take a look and see if we can get this wrapped up?
Quoted 7 lines
Matthew, the assertion failure is in the
VM_BUG_ON(index != xas.xa_index);which was added in
77da9389b9d5f, so perhaps you could take a look?Thanks.
I tested the reproducers on the 99cb0dbd47a15 commit, and they do not trigger the problematic condition of shared memory truncation or hole-punching. I will investigate further, as there have been many changes in khugepaged since the 99cb0dbd47a15 commit that could potentially affect its behavior.
Quoted 4 lines · Andrew Morton
Circling back to this fix...
The
BUG()is obviously real.
It's worth remembering that syzbot's reproducer involves artificially injecting page allocation failures. So although the bug may be "real", it is rather in the theoretical category, way down on my own list to look at, and I'd say not at all urgent to fix.
Hugh
Quoted 20 lines
We're unsure that Ivan's fix is the best
one. We haven't identified a Fixes:, and as this report is against the 6.2 kernel, a cc:stable will be needed.According to the sysbot bisection
(https://syzkaller.appspot.com/bug?id=7d6bb3760e026ece7524500fe44fb024a0e959fc),
this is present in linux-5.19, so it might predate Zach's58ac9a8993a13ebcchanges. But that bisection claim might be misleading.And Zach is offline for a few months. So can people please take a look and see if we can get this wrapped up?
Matthew, the assertion failure is in the
VM_BUG_ON(index != xas.xa_index);which was added in
77da9389b9d5f, so perhaps you could take a look?Thanks.
Quoted 16 lines · Andrew Morton
Circling back to this fix...
The
BUG()is obviously real. We're unsure that Ivan's fix is the best one. We haven't identified a Fixes:, and as this report is against the 6.2 kernel, a cc:stable will be needed.According to the sysbot bisection
(https://syzkaller.appspot.com/bug?id=7d6bb3760e026ece7524500fe44fb024a0e959fc),
this is present in linux-5.19, so it might predate Zach's58ac9a8993a13ebcchanges. But that bisection claim might be misleading.And Zach is offline for a few months. So can people please take a look and see if we can get this wrapped up?
Quoted 7 lines
Matthew, the assertion failure is in the
VM_BUG_ON(index != xas.xa_index);which was added in
77da9389b9d5f, so perhaps you could take a look?Thanks.
Tested today on the latest rc7, the bug still exists.
Kind regards,
Ivan Orlov.
Andrew Morton
maintainerQuoted 1 line · On Mon, 17 Apr 2023 22:28:51 +0400 Ivan Orlov <[email protected]> wrote:
Tested today on the latest rc7, the bug still exists.
OK, thanks. I'll send this patch upstream during the next merge window. If you, Zach or someone else decides to remove SCAN_STORE_FAILED and perhaps those WARN_ON_ONCE()es then we can address these things later.