[LTP] [bug] problems with migration of huge pages with v4.20-10214-ge1ef035d272e

[LTP] [bug] problems with migration of huge pages with v4.20-10214-ge1ef035d272e

[Jan Stancek]

Hi,

LTP move_pages12 [1] started failing recently.

The test maps/unmaps some anonymous private huge pages
and migrates them between 2 nodes. This now reliably
hits NULL ptr deref:

[  194.819357] BUG: unable to handle kernel NULL pointer dereference at 0000000000000030
[  194.864410] #PF error: [WRITE]
[  194.881502] PGD 22c758067 P4D 22c758067 PUD 235177067 PMD 0
[  194.913833] Oops: 0002 [#1] SMP NOPTI
[  194.935062] CPU: 0 PID: 865 Comm: move_pages12 Not tainted 4.20.0+ #1
[  194.972993] Hardware name: HP ProLiant SL335s G7/, BIOS A24 12/08/2012
[  195.005359] RIP: 0010:down_write+0x1b/0x40
[  195.028257] Code: 00 5c 01 00 48 83 c8 03 48 89 43 20 5b c3 90 0f 1f 44 00 00 53 48 89 fb e8 d2 d7 ff ff 48 89 d8 48 ba 01 00 00 00 ff ff
ff ff <f0> 48 0f c1 10 85 d2 74 05 e8 07 26 ff ff 65 48 8b 04 25 00 5c 01
[  195.121836] RSP: 0018:ffffb87e4224fd00 EFLAGS: 00010246
[  195.147097] RAX: 0000000000000030 RBX: 0000000000000030 RCX: 0000000000000000
[  195.185096] RDX: ffffffff00000001 RSI: ffffffffa69d30f0 RDI: 0000000000000030
[  195.219251] RBP: 0000000000000030 R08: ffffe7d4889d8008 R09: 0000000000000003
[  195.258291] R10: 000000000000000f R11: ffffe7d4889d8008 R12: ffffe7d4889d0008
[  195.294547] R13: ffffe7d490b78000 R14: ffffe7d4889d0000 R15: ffff8be9b2ba4580
[  195.332532] FS:  00007f1670112b80(0000) GS:ffff8be9b7a00000(0000) knlGS:0000000000000000
[  195.373888] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[  195.405938] CR2: 0000000000000030 CR3: 000000023477e000 CR4: 00000000000006f0
[  195.443579] Call Trace:
[  195.456876]  migrate_pages+0x833/0xcb0
[  195.478070]  ? __ia32_compat_sys_migrate_pages+0x20/0x20
[  195.506027]  do_move_pages_to_node.isra.63.part.64+0x2a/0x50
[  195.536963]  kernel_move_pages+0x667/0x8c0
[  195.559616]  ? __handle_mm_fault+0xb95/0x1370
[  195.588765]  __x64_sys_move_pages+0x24/0x30
[  195.611439]  do_syscall_64+0x5b/0x160
[  195.631901]  entry_SYSCALL_64_after_hwframe+0x44/0xa9
[  195.657790] RIP: 0033:0x7f166f5ff959
[  195.676365] Code: 00 c3 66 2e 0f 1f 84 00 00 00 00 00 0f 1f 44 00 00 48 89 f8 48 89 f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08
0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 8b 0d 17 45 2c 00 f7 d8 64 89 01 48
[  195.772938] RSP: 002b:00007ffd8d77bb48 EFLAGS: 00000246 ORIG_RAX: 0000000000000117
[  195.810207] RAX: ffffffffffffffda RBX: 0000000000000400 RCX: 00007f166f5ff959
[  195.847522] RDX: 0000000002303400 RSI: 0000000000000400 RDI: 0000000000000360
[  195.882327] RBP: 0000000000000400 R08: 0000000002306420 R09: 0000000000000004
[  195.920017] R10: 0000000002305410 R11: 0000000000000246 R12: 0000000002303400
[  195.958053] R13: 0000000002305410 R14: 0000000002306420 R15: 0000000000000003
[  195.997028] Modules linked in: sunrpc amd64_edac_mod ipmi_ssif edac_mce_amd kvm_amd ipmi_si igb ipmi_devintf k10temp kvm pcspkr ipmi_msgha
ndler joydev irqbypass sp5100_tco dca hpwdt hpilo i2c_piix4 xfs libcrc32c radeon i2c_algo_bit drm_kms_helper ttm ata_generic pata_acpi drm se
rio_raw pata_atiixp
[  196.134162] CR2: 0000000000000030
[  196.152788] ---[ end trace 4420ea5061342d3e ]---

Suspected commit is:
  b43a99900559 ("hugetlbfs: use i_mmap_rwsem for more pmd sharing synchronization")
which adds to unmap_and_move_huge_page():
+               struct address_space *mapping = page_mapping(hpage);
+
+               /*
+                * try_to_unmap could potentially call huge_pmd_unshare.
+                * Because of this, take semaphore in write mode here and
+                * set TTU_RMAP_LOCKED to let lower levels know we have
+                * taken the lock.
+                */
+               i_mmap_lock_write(mapping);

If I'm reading this right, 'mapping' will be NULL for anon mappings.

Running same test with s/MAP_PRIVATE/MAP_SHARED/ leads to user-space
hanging at:

# cat /proc/23654/stack
[<0>] io_schedule+0x12/0x40
[<0>] __lock_page+0x13c/0x200
[<0>] remove_inode_hugepages+0x275/0x300
[<0>] hugetlbfs_evict_inode+0x2e/0x60
[<0>] evict+0xcb/0x190
[<0>] __dentry_kill+0xce/0x160
[<0>] dentry_kill+0x47/0x170
[<0>] dput.part.33+0xc6/0x100
[<0>] __fput+0x105/0x230
[<0>] task_work_run+0x84/0xa0
[<0>] exit_to_usermode_loop+0xd3/0xe0
[<0>] do_syscall_64+0x14d/0x160
[<0>] entry_SYSCALL_64_after_hwframe+0x44/0xa9
[<0>] 0xffffffffffffffff

# cat /proc/23655/stack
[<0>] call_rwsem_down_read_failed+0x14/0x30
[<0>] rmap_walk_file+0x1c1/0x2f0
[<0>] remove_migration_ptes+0x6d/0x80
[<0>] migrate_pages+0x86a/0xcb0
[<0>] do_move_pages_to_node.isra.63.part.64+0x2a/0x50
[<0>] kernel_move_pages+0x667/0x8c0
[<0>] __x64_sys_move_pages+0x24/0x30
[<0>] do_syscall_64+0x5b/0x160
[<0>] entry_SYSCALL_64_after_hwframe+0x44/0xa9
[<0>] 0xffffffffffffffff

Regards,
Jan

[1] https://github.com/linux-test-project/ltp/blob/master/testcases/kernel/syscalls/move_pages/move_pages12.c

[bug] problems with migration of huge pages with v4.20-10214-ge1ef035d272e

[Jan Stancek]

Hi,

LTP move_pages12 [1] started failing recently.

The test maps/unmaps some anonymous private huge pages
and migrates them between 2 nodes. This now reliably
hits NULL ptr deref:

[  194.819357] BUG: unable to handle kernel NULL pointer dereference at 0000000000000030
[  194.864410] #PF error: [WRITE]
[  194.881502] PGD 22c758067 P4D 22c758067 PUD 235177067 PMD 0
[  194.913833] Oops: 0002 [#1] SMP NOPTI
[  194.935062] CPU: 0 PID: 865 Comm: move_pages12 Not tainted 4.20.0+ #1
[  194.972993] Hardware name: HP ProLiant SL335s G7/, BIOS A24 12/08/2012
[  195.005359] RIP: 0010:down_write+0x1b/0x40
[  195.028257] Code: 00 5c 01 00 48 83 c8 03 48 89 43 20 5b c3 90 0f 1f 44 00 00 53 48 89 fb e8 d2 d7 ff ff 48 89 d8 48 ba 01 00 00 00 ff ff
ff ff <f0> 48 0f c1 10 85 d2 74 05 e8 07 26 ff ff 65 48 8b 04 25 00 5c 01
[  195.121836] RSP: 0018:ffffb87e4224fd00 EFLAGS: 00010246
[  195.147097] RAX: 0000000000000030 RBX: 0000000000000030 RCX: 0000000000000000
[  195.185096] RDX: ffffffff00000001 RSI: ffffffffa69d30f0 RDI: 0000000000000030
[  195.219251] RBP: 0000000000000030 R08: ffffe7d4889d8008 R09: 0000000000000003
[  195.258291] R10: 000000000000000f R11: ffffe7d4889d8008 R12: ffffe7d4889d0008
[  195.294547] R13: ffffe7d490b78000 R14: ffffe7d4889d0000 R15: ffff8be9b2ba4580
[  195.332532] FS:  00007f1670112b80(0000) GS:ffff8be9b7a00000(0000) knlGS:0000000000000000
[  195.373888] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[  195.405938] CR2: 0000000000000030 CR3: 000000023477e000 CR4: 00000000000006f0
[  195.443579] Call Trace:
[  195.456876]  migrate_pages+0x833/0xcb0
[  195.478070]  ? __ia32_compat_sys_migrate_pages+0x20/0x20
[  195.506027]  do_move_pages_to_node.isra.63.part.64+0x2a/0x50
[  195.536963]  kernel_move_pages+0x667/0x8c0
[  195.559616]  ? __handle_mm_fault+0xb95/0x1370
[  195.588765]  __x64_sys_move_pages+0x24/0x30
[  195.611439]  do_syscall_64+0x5b/0x160
[  195.631901]  entry_SYSCALL_64_after_hwframe+0x44/0xa9
[  195.657790] RIP: 0033:0x7f166f5ff959
[  195.676365] Code: 00 c3 66 2e 0f 1f 84 00 00 00 00 00 0f 1f 44 00 00 48 89 f8 48 89 f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08
0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 8b 0d 17 45 2c 00 f7 d8 64 89 01 48
[  195.772938] RSP: 002b:00007ffd8d77bb48 EFLAGS: 00000246 ORIG_RAX: 0000000000000117
[  195.810207] RAX: ffffffffffffffda RBX: 0000000000000400 RCX: 00007f166f5ff959
[  195.847522] RDX: 0000000002303400 RSI: 0000000000000400 RDI: 0000000000000360
[  195.882327] RBP: 0000000000000400 R08: 0000000002306420 R09: 0000000000000004
[  195.920017] R10: 0000000002305410 R11: 0000000000000246 R12: 0000000002303400
[  195.958053] R13: 0000000002305410 R14: 0000000002306420 R15: 0000000000000003
[  195.997028] Modules linked in: sunrpc amd64_edac_mod ipmi_ssif edac_mce_amd kvm_amd ipmi_si igb ipmi_devintf k10temp kvm pcspkr ipmi_msgha
ndler joydev irqbypass sp5100_tco dca hpwdt hpilo i2c_piix4 xfs libcrc32c radeon i2c_algo_bit drm_kms_helper ttm ata_generic pata_acpi drm se
rio_raw pata_atiixp
[  196.134162] CR2: 0000000000000030
[  196.152788] ---[ end trace 4420ea5061342d3e ]---

Suspected commit is:
  b43a99900559 ("hugetlbfs: use i_mmap_rwsem for more pmd sharing synchronization")
which adds to unmap_and_move_huge_page():
+               struct address_space *mapping = page_mapping(hpage);
+
+               /*
+                * try_to_unmap could potentially call huge_pmd_unshare.
+                * Because of this, take semaphore in write mode here and
+                * set TTU_RMAP_LOCKED to let lower levels know we have
+                * taken the lock.
+                */
+               i_mmap_lock_write(mapping);

If I'm reading this right, 'mapping' will be NULL for anon mappings.

Running same test with s/MAP_PRIVATE/MAP_SHARED/ leads to user-space
hanging at:

# cat /proc/23654/stack
[<0>] io_schedule+0x12/0x40
[<0>] __lock_page+0x13c/0x200
[<0>] remove_inode_hugepages+0x275/0x300
[<0>] hugetlbfs_evict_inode+0x2e/0x60
[<0>] evict+0xcb/0x190
[<0>] __dentry_kill+0xce/0x160
[<0>] dentry_kill+0x47/0x170
[<0>] dput.part.33+0xc6/0x100
[<0>] __fput+0x105/0x230
[<0>] task_work_run+0x84/0xa0
[<0>] exit_to_usermode_loop+0xd3/0xe0
[<0>] do_syscall_64+0x14d/0x160
[<0>] entry_SYSCALL_64_after_hwframe+0x44/0xa9
[<0>] 0xffffffffffffffff

# cat /proc/23655/stack
[<0>] call_rwsem_down_read_failed+0x14/0x30
[<0>] rmap_walk_file+0x1c1/0x2f0
[<0>] remove_migration_ptes+0x6d/0x80
[<0>] migrate_pages+0x86a/0xcb0
[<0>] do_move_pages_to_node.isra.63.part.64+0x2a/0x50
[<0>] kernel_move_pages+0x667/0x8c0
[<0>] __x64_sys_move_pages+0x24/0x30
[<0>] do_syscall_64+0x5b/0x160
[<0>] entry_SYSCALL_64_after_hwframe+0x44/0xa9
[<0>] 0xffffffffffffffff

Regards,
Jan

[1] https://github.com/linux-test-project/ltp/blob/master/testcases/kernel/syscalls/move_pages/move_pages12.c

[LTP] [bug] problems with migration of huge pages with v4.20-10214-ge1ef035d272e

[Mike Kravetz]

On 1/2/19 12:30 PM, Jan Stancek wrote:
> Hi,
> 
> LTP move_pages12 [1] started failing recently.
> 
> The test maps/unmaps some anonymous private huge pages
> and migrates them between 2 nodes. This now reliably
> hits NULL ptr deref:
> 
> [  194.819357] BUG: unable to handle kernel NULL pointer dereference at 0000000000000030
> [  194.864410] #PF error: [WRITE]
> [  194.881502] PGD 22c758067 P4D 22c758067 PUD 235177067 PMD 0
> [  194.913833] Oops: 0002 [#1] SMP NOPTI
> [  194.935062] CPU: 0 PID: 865 Comm: move_pages12 Not tainted 4.20.0+ #1
> [  194.972993] Hardware name: HP ProLiant SL335s G7/, BIOS A24 12/08/2012
> [  195.005359] RIP: 0010:down_write+0x1b/0x40
> [  195.028257] Code: 00 5c 01 00 48 83 c8 03 48 89 43 20 5b c3 90 0f 1f 44 00 00 53 48 89 fb e8 d2 d7 ff ff 48 89 d8 48 ba 01 00 00 00 ff ff
> ff ff <f0> 48 0f c1 10 85 d2 74 05 e8 07 26 ff ff 65 48 8b 04 25 00 5c 01
> [  195.121836] RSP: 0018:ffffb87e4224fd00 EFLAGS: 00010246
> [  195.147097] RAX: 0000000000000030 RBX: 0000000000000030 RCX: 0000000000000000
> [  195.185096] RDX: ffffffff00000001 RSI: ffffffffa69d30f0 RDI: 0000000000000030
> [  195.219251] RBP: 0000000000000030 R08: ffffe7d4889d8008 R09: 0000000000000003
> [  195.258291] R10: 000000000000000f R11: ffffe7d4889d8008 R12: ffffe7d4889d0008
> [  195.294547] R13: ffffe7d490b78000 R14: ffffe7d4889d0000 R15: ffff8be9b2ba4580
> [  195.332532] FS:  00007f1670112b80(0000) GS:ffff8be9b7a00000(0000) knlGS:0000000000000000
> [  195.373888] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [  195.405938] CR2: 0000000000000030 CR3: 000000023477e000 CR4: 00000000000006f0
> [  195.443579] Call Trace:
> [  195.456876]  migrate_pages+0x833/0xcb0
> [  195.478070]  ? __ia32_compat_sys_migrate_pages+0x20/0x20
> [  195.506027]  do_move_pages_to_node.isra.63.part.64+0x2a/0x50
> [  195.536963]  kernel_move_pages+0x667/0x8c0
> [  195.559616]  ? __handle_mm_fault+0xb95/0x1370
> [  195.588765]  __x64_sys_move_pages+0x24/0x30
> [  195.611439]  do_syscall_64+0x5b/0x160
> [  195.631901]  entry_SYSCALL_64_after_hwframe+0x44/0xa9
> [  195.657790] RIP: 0033:0x7f166f5ff959
> [  195.676365] Code: 00 c3 66 2e 0f 1f 84 00 00 00 00 00 0f 1f 44 00 00 48 89 f8 48 89 f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08
> 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 8b 0d 17 45 2c 00 f7 d8 64 89 01 48
> [  195.772938] RSP: 002b:00007ffd8d77bb48 EFLAGS: 00000246 ORIG_RAX: 0000000000000117
> [  195.810207] RAX: ffffffffffffffda RBX: 0000000000000400 RCX: 00007f166f5ff959
> [  195.847522] RDX: 0000000002303400 RSI: 0000000000000400 RDI: 0000000000000360
> [  195.882327] RBP: 0000000000000400 R08: 0000000002306420 R09: 0000000000000004
> [  195.920017] R10: 0000000002305410 R11: 0000000000000246 R12: 0000000002303400
> [  195.958053] R13: 0000000002305410 R14: 0000000002306420 R15: 0000000000000003
> [  195.997028] Modules linked in: sunrpc amd64_edac_mod ipmi_ssif edac_mce_amd kvm_amd ipmi_si igb ipmi_devintf k10temp kvm pcspkr ipmi_msgha
> ndler joydev irqbypass sp5100_tco dca hpwdt hpilo i2c_piix4 xfs libcrc32c radeon i2c_algo_bit drm_kms_helper ttm ata_generic pata_acpi drm se
> rio_raw pata_atiixp
> [  196.134162] CR2: 0000000000000030
> [  196.152788] ---[ end trace 4420ea5061342d3e ]---
> 
> Suspected commit is:
>   b43a99900559 ("hugetlbfs: use i_mmap_rwsem for more pmd sharing synchronization")
> which adds to unmap_and_move_huge_page():
> +               struct address_space *mapping = page_mapping(hpage);
> +
> +               /*
> +                * try_to_unmap could potentially call huge_pmd_unshare.
> +                * Because of this, take semaphore in write mode here and
> +                * set TTU_RMAP_LOCKED to let lower levels know we have
> +                * taken the lock.
> +                */
> +               i_mmap_lock_write(mapping);
> 
> If I'm reading this right, 'mapping' will be NULL for anon mappings.

Not exactly.

In the anon case mapping will point to a 'struct anon_vma *'.  But,
i_mmap_lock_write expects a 'struct address_space *'.  I believe this
is source of the NULL pointer dereference in down_write.

Not sure what is happening in the MAP_SHARED case.  My test which does
something similar does not have issues.

In any case, the commit is bad.  I will investagate further.
-- 
Mike Kravetz

> Running same test with s/MAP_PRIVATE/MAP_SHARED/ leads to user-space
> hanging at:
> 
> # cat /proc/23654/stack
> [<0>] io_schedule+0x12/0x40
> [<0>] __lock_page+0x13c/0x200
> [<0>] remove_inode_hugepages+0x275/0x300
> [<0>] hugetlbfs_evict_inode+0x2e/0x60
> [<0>] evict+0xcb/0x190
> [<0>] __dentry_kill+0xce/0x160
> [<0>] dentry_kill+0x47/0x170
> [<0>] dput.part.33+0xc6/0x100
> [<0>] __fput+0x105/0x230
> [<0>] task_work_run+0x84/0xa0
> [<0>] exit_to_usermode_loop+0xd3/0xe0
> [<0>] do_syscall_64+0x14d/0x160
> [<0>] entry_SYSCALL_64_after_hwframe+0x44/0xa9
> [<0>] 0xffffffffffffffff
> 
> # cat /proc/23655/stack
> [<0>] call_rwsem_down_read_failed+0x14/0x30
> [<0>] rmap_walk_file+0x1c1/0x2f0
> [<0>] remove_migration_ptes+0x6d/0x80
> [<0>] migrate_pages+0x86a/0xcb0
> [<0>] do_move_pages_to_node.isra.63.part.64+0x2a/0x50
> [<0>] kernel_move_pages+0x667/0x8c0
> [<0>] __x64_sys_move_pages+0x24/0x30
> [<0>] do_syscall_64+0x5b/0x160
> [<0>] entry_SYSCALL_64_after_hwframe+0x44/0xa9
> [<0>] 0xffffffffffffffff
> 
> Regards,
> Jan
> 
> [1] https://github.com/linux-test-project/ltp/blob/master/testcases/kernel/syscalls/move_pages/move_pages12.c
> 

Re: [bug] problems with migration of huge pages with v4.20-10214-ge1ef035d272e

[Mike Kravetz]

On 1/2/19 12:30 PM, Jan Stancek wrote:
> Hi,
> 
> LTP move_pages12 [1] started failing recently.
> 
> The test maps/unmaps some anonymous private huge pages
> and migrates them between 2 nodes. This now reliably
> hits NULL ptr deref:
> 
> [  194.819357] BUG: unable to handle kernel NULL pointer dereference at 0000000000000030
> [  194.864410] #PF error: [WRITE]
> [  194.881502] PGD 22c758067 P4D 22c758067 PUD 235177067 PMD 0
> [  194.913833] Oops: 0002 [#1] SMP NOPTI
> [  194.935062] CPU: 0 PID: 865 Comm: move_pages12 Not tainted 4.20.0+ #1
> [  194.972993] Hardware name: HP ProLiant SL335s G7/, BIOS A24 12/08/2012
> [  195.005359] RIP: 0010:down_write+0x1b/0x40
> [  195.028257] Code: 00 5c 01 00 48 83 c8 03 48 89 43 20 5b c3 90 0f 1f 44 00 00 53 48 89 fb e8 d2 d7 ff ff 48 89 d8 48 ba 01 00 00 00 ff ff
> ff ff <f0> 48 0f c1 10 85 d2 74 05 e8 07 26 ff ff 65 48 8b 04 25 00 5c 01
> [  195.121836] RSP: 0018:ffffb87e4224fd00 EFLAGS: 00010246
> [  195.147097] RAX: 0000000000000030 RBX: 0000000000000030 RCX: 0000000000000000
> [  195.185096] RDX: ffffffff00000001 RSI: ffffffffa69d30f0 RDI: 0000000000000030
> [  195.219251] RBP: 0000000000000030 R08: ffffe7d4889d8008 R09: 0000000000000003
> [  195.258291] R10: 000000000000000f R11: ffffe7d4889d8008 R12: ffffe7d4889d0008
> [  195.294547] R13: ffffe7d490b78000 R14: ffffe7d4889d0000 R15: ffff8be9b2ba4580
> [  195.332532] FS:  00007f1670112b80(0000) GS:ffff8be9b7a00000(0000) knlGS:0000000000000000
> [  195.373888] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [  195.405938] CR2: 0000000000000030 CR3: 000000023477e000 CR4: 00000000000006f0
> [  195.443579] Call Trace:
> [  195.456876]  migrate_pages+0x833/0xcb0
> [  195.478070]  ? __ia32_compat_sys_migrate_pages+0x20/0x20
> [  195.506027]  do_move_pages_to_node.isra.63.part.64+0x2a/0x50
> [  195.536963]  kernel_move_pages+0x667/0x8c0
> [  195.559616]  ? __handle_mm_fault+0xb95/0x1370
> [  195.588765]  __x64_sys_move_pages+0x24/0x30
> [  195.611439]  do_syscall_64+0x5b/0x160
> [  195.631901]  entry_SYSCALL_64_after_hwframe+0x44/0xa9
> [  195.657790] RIP: 0033:0x7f166f5ff959
> [  195.676365] Code: 00 c3 66 2e 0f 1f 84 00 00 00 00 00 0f 1f 44 00 00 48 89 f8 48 89 f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08
> 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 8b 0d 17 45 2c 00 f7 d8 64 89 01 48
> [  195.772938] RSP: 002b:00007ffd8d77bb48 EFLAGS: 00000246 ORIG_RAX: 0000000000000117
> [  195.810207] RAX: ffffffffffffffda RBX: 0000000000000400 RCX: 00007f166f5ff959
> [  195.847522] RDX: 0000000002303400 RSI: 0000000000000400 RDI: 0000000000000360
> [  195.882327] RBP: 0000000000000400 R08: 0000000002306420 R09: 0000000000000004
> [  195.920017] R10: 0000000002305410 R11: 0000000000000246 R12: 0000000002303400
> [  195.958053] R13: 0000000002305410 R14: 0000000002306420 R15: 0000000000000003
> [  195.997028] Modules linked in: sunrpc amd64_edac_mod ipmi_ssif edac_mce_amd kvm_amd ipmi_si igb ipmi_devintf k10temp kvm pcspkr ipmi_msgha
> ndler joydev irqbypass sp5100_tco dca hpwdt hpilo i2c_piix4 xfs libcrc32c radeon i2c_algo_bit drm_kms_helper ttm ata_generic pata_acpi drm se
> rio_raw pata_atiixp
> [  196.134162] CR2: 0000000000000030
> [  196.152788] ---[ end trace 4420ea5061342d3e ]---
> 
> Suspected commit is:
>   b43a99900559 ("hugetlbfs: use i_mmap_rwsem for more pmd sharing synchronization")
> which adds to unmap_and_move_huge_page():
> +               struct address_space *mapping = page_mapping(hpage);
> +
> +               /*
> +                * try_to_unmap could potentially call huge_pmd_unshare.
> +                * Because of this, take semaphore in write mode here and
> +                * set TTU_RMAP_LOCKED to let lower levels know we have
> +                * taken the lock.
> +                */
> +               i_mmap_lock_write(mapping);
> 
> If I'm reading this right, 'mapping' will be NULL for anon mappings.

Not exactly.

In the anon case mapping will point to a 'struct anon_vma *'.  But,
i_mmap_lock_write expects a 'struct address_space *'.  I believe this
is source of the NULL pointer dereference in down_write.

Not sure what is happening in the MAP_SHARED case.  My test which does
something similar does not have issues.

In any case, the commit is bad.  I will investagate further.
-- 
Mike Kravetz

> Running same test with s/MAP_PRIVATE/MAP_SHARED/ leads to user-space
> hanging at:
> 
> # cat /proc/23654/stack
> [<0>] io_schedule+0x12/0x40
> [<0>] __lock_page+0x13c/0x200
> [<0>] remove_inode_hugepages+0x275/0x300
> [<0>] hugetlbfs_evict_inode+0x2e/0x60
> [<0>] evict+0xcb/0x190
> [<0>] __dentry_kill+0xce/0x160
> [<0>] dentry_kill+0x47/0x170
> [<0>] dput.part.33+0xc6/0x100
> [<0>] __fput+0x105/0x230
> [<0>] task_work_run+0x84/0xa0
> [<0>] exit_to_usermode_loop+0xd3/0xe0
> [<0>] do_syscall_64+0x14d/0x160
> [<0>] entry_SYSCALL_64_after_hwframe+0x44/0xa9
> [<0>] 0xffffffffffffffff
> 
> # cat /proc/23655/stack
> [<0>] call_rwsem_down_read_failed+0x14/0x30
> [<0>] rmap_walk_file+0x1c1/0x2f0
> [<0>] remove_migration_ptes+0x6d/0x80
> [<0>] migrate_pages+0x86a/0xcb0
> [<0>] do_move_pages_to_node.isra.63.part.64+0x2a/0x50
> [<0>] kernel_move_pages+0x667/0x8c0
> [<0>] __x64_sys_move_pages+0x24/0x30
> [<0>] do_syscall_64+0x5b/0x160
> [<0>] entry_SYSCALL_64_after_hwframe+0x44/0xa9
> [<0>] 0xffffffffffffffff
> 
> Regards,
> Jan
> 
> [1] https://github.com/linux-test-project/ltp/blob/master/testcases/kernel/syscalls/move_pages/move_pages12.c
> 

[LTP] [bug] problems with migration of huge pages with v4.20-10214-ge1ef035d272e

[Mike Kravetz]

On 1/2/19 1:24 PM, Mike Kravetz wrote:
> On 1/2/19 12:30 PM, Jan Stancek wrote:
>> Hi,
>>
>> LTP move_pages12 [1] started failing recently.
>>
>> The test maps/unmaps some anonymous private huge pages
>> and migrates them between 2 nodes. This now reliably
>> hits NULL ptr deref:
>>
>> [  194.819357] BUG: unable to handle kernel NULL pointer dereference at 0000000000000030
>> [  194.864410] #PF error: [WRITE]
>> [  194.881502] PGD 22c758067 P4D 22c758067 PUD 235177067 PMD 0
>> [  194.913833] Oops: 0002 [#1] SMP NOPTI
>> [  194.935062] CPU: 0 PID: 865 Comm: move_pages12 Not tainted 4.20.0+ #1
>> [  194.972993] Hardware name: HP ProLiant SL335s G7/, BIOS A24 12/08/2012
>> [  195.005359] RIP: 0010:down_write+0x1b/0x40
>> [  195.028257] Code: 00 5c 01 00 48 83 c8 03 48 89 43 20 5b c3 90 0f 1f 44 00 00 53 48 89 fb e8 d2 d7 ff ff 48 89 d8 48 ba 01 00 00 00 ff ff
>> ff ff <f0> 48 0f c1 10 85 d2 74 05 e8 07 26 ff ff 65 48 8b 04 25 00 5c 01
>> [  195.121836] RSP: 0018:ffffb87e4224fd00 EFLAGS: 00010246
>> [  195.147097] RAX: 0000000000000030 RBX: 0000000000000030 RCX: 0000000000000000
>> [  195.185096] RDX: ffffffff00000001 RSI: ffffffffa69d30f0 RDI: 0000000000000030
>> [  195.219251] RBP: 0000000000000030 R08: ffffe7d4889d8008 R09: 0000000000000003
>> [  195.258291] R10: 000000000000000f R11: ffffe7d4889d8008 R12: ffffe7d4889d0008
>> [  195.294547] R13: ffffe7d490b78000 R14: ffffe7d4889d0000 R15: ffff8be9b2ba4580
>> [  195.332532] FS:  00007f1670112b80(0000) GS:ffff8be9b7a00000(0000) knlGS:0000000000000000
>> [  195.373888] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
>> [  195.405938] CR2: 0000000000000030 CR3: 000000023477e000 CR4: 00000000000006f0
>> [  195.443579] Call Trace:
>> [  195.456876]  migrate_pages+0x833/0xcb0
>> [  195.478070]  ? __ia32_compat_sys_migrate_pages+0x20/0x20
>> [  195.506027]  do_move_pages_to_node.isra.63.part.64+0x2a/0x50
>> [  195.536963]  kernel_move_pages+0x667/0x8c0
>> [  195.559616]  ? __handle_mm_fault+0xb95/0x1370
>> [  195.588765]  __x64_sys_move_pages+0x24/0x30
>> [  195.611439]  do_syscall_64+0x5b/0x160
>> [  195.631901]  entry_SYSCALL_64_after_hwframe+0x44/0xa9
>> [  195.657790] RIP: 0033:0x7f166f5ff959
>> [  195.676365] Code: 00 c3 66 2e 0f 1f 84 00 00 00 00 00 0f 1f 44 00 00 48 89 f8 48 89 f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08
>> 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 8b 0d 17 45 2c 00 f7 d8 64 89 01 48
>> [  195.772938] RSP: 002b:00007ffd8d77bb48 EFLAGS: 00000246 ORIG_RAX: 0000000000000117
>> [  195.810207] RAX: ffffffffffffffda RBX: 0000000000000400 RCX: 00007f166f5ff959
>> [  195.847522] RDX: 0000000002303400 RSI: 0000000000000400 RDI: 0000000000000360
>> [  195.882327] RBP: 0000000000000400 R08: 0000000002306420 R09: 0000000000000004
>> [  195.920017] R10: 0000000002305410 R11: 0000000000000246 R12: 0000000002303400
>> [  195.958053] R13: 0000000002305410 R14: 0000000002306420 R15: 0000000000000003
>> [  195.997028] Modules linked in: sunrpc amd64_edac_mod ipmi_ssif edac_mce_amd kvm_amd ipmi_si igb ipmi_devintf k10temp kvm pcspkr ipmi_msgha
>> ndler joydev irqbypass sp5100_tco dca hpwdt hpilo i2c_piix4 xfs libcrc32c radeon i2c_algo_bit drm_kms_helper ttm ata_generic pata_acpi drm se
>> rio_raw pata_atiixp
>> [  196.134162] CR2: 0000000000000030
>> [  196.152788] ---[ end trace 4420ea5061342d3e ]---
>>
>> Suspected commit is:
>>   b43a99900559 ("hugetlbfs: use i_mmap_rwsem for more pmd sharing synchronization")
>> which adds to unmap_and_move_huge_page():
>> +               struct address_space *mapping = page_mapping(hpage);
>> +
>> +               /*
>> +                * try_to_unmap could potentially call huge_pmd_unshare.
>> +                * Because of this, take semaphore in write mode here and
>> +                * set TTU_RMAP_LOCKED to let lower levels know we have
>> +                * taken the lock.
>> +                */
>> +               i_mmap_lock_write(mapping);
>>
>> If I'm reading this right, 'mapping' will be NULL for anon mappings.
> 
> Not exactly.
> 

Well, yes exactly.  Sorry, I was confusing this with something else.

That commit does cause BUGs for migration and page poisoning of anon huge
pages.  The patch was trying to take care of i_mmap_rwsem locking outside
try_to_unmap infrastructure.  This is because try_to_unmap will take the
semaphore in read mode (for file mappings) and we really need it to be
taken in write mode.

The patch below continues to take the semaphore outside try_to_unmap for
the file mapping case.  For anon mappings, the locking is done as a special
case in try_to_unmap_one.  This is something I was trying to avoid as it
it harder to follow/understand.  Any suggestions on how to restructure this
or make it more clear are welcome.

Adding Andrew on Cc as he already sent the commit causing the BUGs upstream.

From: Mike Kravetz <mike.kravetz@oracle.com>

hugetlbfs: fix migration and poisoning of anon huge pages

Expanded use of i_mmap_rwsem for pmd sharing synchronization incorrectly
used page_mapping() of anon huge pages to get to address_space
i_mmap_rwsem.  Since page_mapping() is NULL for pages of anon mappings,
an "unable to handle kernel NULL pointer" BUG would occur with stack
similar to:

RIP: 0010:down_write+0x1b/0x40
Call Trace:
 migrate_pages+0x81f/0xb90
 __ia32_compat_sys_migrate_pages+0x190/0x190
 do_move_pages_to_node.isra.53.part.54+0x2a/0x50
 kernel_move_pages+0x566/0x7b0
 __x64_sys_move_pages+0x24/0x30
 do_syscall_64+0x5b/0x180
 entry_SYSCALL_64_after_hwframe+0x44/0xa9

To fix, only use page_mapping() for non-anon or file pages.  For anon
pages wait until we find a vma in which the page is mapped and get the
address_space from vm_file.

Fixes: b43a99900559 ("hugetlbfs: use i_mmap_rwsem for more pmd sharing
synchronization")
Signed-off-by: Mike Kravetz <mike.kravetz@oracle.com>
---
 mm/memory-failure.c | 15 +++++++++++----
 mm/migrate.c        | 34 +++++++++++++++++++++++-----------
 mm/rmap.c           | 15 ++++++++++++---
 3 files changed, 46 insertions(+), 18 deletions(-)

diff --git a/mm/memory-failure.c b/mm/memory-failure.c
index 93558fb981fb..f229cbd0b347 100644
--- a/mm/memory-failure.c
+++ b/mm/memory-failure.c
@@ -1032,14 +1032,21 @@ static bool hwpoison_user_mappings(struct page *p,
unsigned long pfn,
 		unmap_success = try_to_unmap(hpage, ttu);
 	} else if (mapping) {
 		/*
-		 * For hugetlb pages, try_to_unmap could potentially call
-		 * huge_pmd_unshare.  Because of this, take semaphore in
-		 * write mode here and set TTU_RMAP_LOCKED to indicate we
-		 * have taken the lock at this higer level.
+		 * For file mappings, take semaphore in write mode here and
+		 * set TTU_RMAP_LOCKED to let lower levels know we have taken
+		 * the lock.  This is in case lower levels call
+		 * huge_pmd_unshare.  Without this, try_to_unmap would only
+		 * take the semaphore in read mode.
 		 */
 		i_mmap_lock_write(mapping);
 		unmap_success = try_to_unmap(hpage, ttu|TTU_RMAP_LOCKED);
 		i_mmap_unlock_write(mapping);
+	} else {
+		/*
+		 * For huge page anon mappings, try_to_unmap_one will take the
+		 * i_mmap_rwsem before calling huge_pmd_unshare if necessary.
+		 */
+		unmap_success = try_to_unmap(hpage, ttu);
 	}
 	if (!unmap_success)
 		pr_err("Memory failure: %#lx: failed to unmap page (mapcount=%d)\n",
diff --git a/mm/migrate.c b/mm/migrate.c
index 725edaef238a..45d7dd0c9479 100644
--- a/mm/migrate.c
+++ b/mm/migrate.c
@@ -1309,17 +1309,29 @@ static int unmap_and_move_huge_page(new_page_t get_new_page,
 	if (page_mapped(hpage)) {
 		struct address_space *mapping = page_mapping(hpage);

-		/*
-		 * try_to_unmap could potentially call huge_pmd_unshare.
-		 * Because of this, take semaphore in write mode here and
-		 * set TTU_RMAP_LOCKED to let lower levels know we have
-		 * taken the lock.
-		 */
-		i_mmap_lock_write(mapping);
-		try_to_unmap(hpage,
-			TTU_MIGRATION|TTU_IGNORE_MLOCK|TTU_IGNORE_ACCESS|
-			TTU_RMAP_LOCKED);
-		i_mmap_unlock_write(mapping);
+		if (mapping) {
+			/*
+			 * For file mappings, take semaphore in write mode here
+			 * and set TTU_RMAP_LOCKED to let lower levels know we
+			 * have taken the lock.  This is in case lower levels
+			 * call huge_pmd_unshare.  Without this, try_to_unmap
+			 * would only take the semaphore in read mode.
+			 */
+			i_mmap_lock_write(mapping);
+			try_to_unmap(hpage,
+				TTU_MIGRATION|TTU_IGNORE_MLOCK|
+				TTU_IGNORE_ACCESS|TTU_RMAP_LOCKED);
+			i_mmap_unlock_write(mapping);
+		} else {
+			/*
+			 * For anon mappings, try_to_unmap_one will take the
+			 * i_mmap_rwsem before calling huge_pmd_unshare if
+			 * necessary.
+			 */
+			try_to_unmap(hpage,
+				TTU_MIGRATION|TTU_IGNORE_MLOCK|
+				TTU_IGNORE_ACCESS);
+		}
 		page_was_mapped = 1;
 	}

diff --git a/mm/rmap.c b/mm/rmap.c
index c566bd552535..b267cc084f92 100644
--- a/mm/rmap.c
+++ b/mm/rmap.c
@@ -1375,11 +1375,17 @@ static bool try_to_unmap_one(struct page *page, struct
vm_area_struct *vma,
 		/*
 		 * If sharing is possible, start and end will be adjusted
 		 * accordingly.
-		 *
-		 * If called for a huge page, caller must hold i_mmap_rwsem
-		 * in write mode as it is possible to call huge_pmd_unshare.
 		 */
 		adjust_range_if_pmd_sharing_possible(vma, &start, &end);
+
+		/*
+		 * If called for a huge page file mapping, caller will hold
+		 * i_mmap_rwsem in write mode.  For anon mappings, we must
+		 * take the semaphore here.  All this is necessary because
+		 * it is possible huge_pmd_unshare will 'unshare' a pmd.
+		 */
+		if (PageAnon(page))
+			i_mmap_lock_write(vma->vm_file->f_mapping);
 	}
 	mmu_notifier_invalidate_range_start(vma->vm_mm, start, end);

@@ -1655,6 +1661,9 @@ static bool try_to_unmap_one(struct page *page, struct
vm_area_struct *vma,
 	}

 	mmu_notifier_invalidate_range_end(vma->vm_mm, start, end);
+	/* For anon huge pages, we must unlock. */
+	if (PageHuge(page) && PageAnon(page))
+		i_mmap_unlock_write(vma->vm_file->f_mapping);

 	return ret;
 }
-- 
2.17.2

Re: [bug] problems with migration of huge pages with v4.20-10214-ge1ef035d272e

[Mike Kravetz]

On 1/2/19 1:24 PM, Mike Kravetz wrote:
> On 1/2/19 12:30 PM, Jan Stancek wrote:
>> Hi,
>>
>> LTP move_pages12 [1] started failing recently.
>>
>> The test maps/unmaps some anonymous private huge pages
>> and migrates them between 2 nodes. This now reliably
>> hits NULL ptr deref:
>>
>> [  194.819357] BUG: unable to handle kernel NULL pointer dereference at 0000000000000030
>> [  194.864410] #PF error: [WRITE]
>> [  194.881502] PGD 22c758067 P4D 22c758067 PUD 235177067 PMD 0
>> [  194.913833] Oops: 0002 [#1] SMP NOPTI
>> [  194.935062] CPU: 0 PID: 865 Comm: move_pages12 Not tainted 4.20.0+ #1
>> [  194.972993] Hardware name: HP ProLiant SL335s G7/, BIOS A24 12/08/2012
>> [  195.005359] RIP: 0010:down_write+0x1b/0x40
>> [  195.028257] Code: 00 5c 01 00 48 83 c8 03 48 89 43 20 5b c3 90 0f 1f 44 00 00 53 48 89 fb e8 d2 d7 ff ff 48 89 d8 48 ba 01 00 00 00 ff ff
>> ff ff <f0> 48 0f c1 10 85 d2 74 05 e8 07 26 ff ff 65 48 8b 04 25 00 5c 01
>> [  195.121836] RSP: 0018:ffffb87e4224fd00 EFLAGS: 00010246
>> [  195.147097] RAX: 0000000000000030 RBX: 0000000000000030 RCX: 0000000000000000
>> [  195.185096] RDX: ffffffff00000001 RSI: ffffffffa69d30f0 RDI: 0000000000000030
>> [  195.219251] RBP: 0000000000000030 R08: ffffe7d4889d8008 R09: 0000000000000003
>> [  195.258291] R10: 000000000000000f R11: ffffe7d4889d8008 R12: ffffe7d4889d0008
>> [  195.294547] R13: ffffe7d490b78000 R14: ffffe7d4889d0000 R15: ffff8be9b2ba4580
>> [  195.332532] FS:  00007f1670112b80(0000) GS:ffff8be9b7a00000(0000) knlGS:0000000000000000
>> [  195.373888] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
>> [  195.405938] CR2: 0000000000000030 CR3: 000000023477e000 CR4: 00000000000006f0
>> [  195.443579] Call Trace:
>> [  195.456876]  migrate_pages+0x833/0xcb0
>> [  195.478070]  ? __ia32_compat_sys_migrate_pages+0x20/0x20
>> [  195.506027]  do_move_pages_to_node.isra.63.part.64+0x2a/0x50
>> [  195.536963]  kernel_move_pages+0x667/0x8c0
>> [  195.559616]  ? __handle_mm_fault+0xb95/0x1370
>> [  195.588765]  __x64_sys_move_pages+0x24/0x30
>> [  195.611439]  do_syscall_64+0x5b/0x160
>> [  195.631901]  entry_SYSCALL_64_after_hwframe+0x44/0xa9
>> [  195.657790] RIP: 0033:0x7f166f5ff959
>> [  195.676365] Code: 00 c3 66 2e 0f 1f 84 00 00 00 00 00 0f 1f 44 00 00 48 89 f8 48 89 f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08
>> 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 8b 0d 17 45 2c 00 f7 d8 64 89 01 48
>> [  195.772938] RSP: 002b:00007ffd8d77bb48 EFLAGS: 00000246 ORIG_RAX: 0000000000000117
>> [  195.810207] RAX: ffffffffffffffda RBX: 0000000000000400 RCX: 00007f166f5ff959
>> [  195.847522] RDX: 0000000002303400 RSI: 0000000000000400 RDI: 0000000000000360
>> [  195.882327] RBP: 0000000000000400 R08: 0000000002306420 R09: 0000000000000004
>> [  195.920017] R10: 0000000002305410 R11: 0000000000000246 R12: 0000000002303400
>> [  195.958053] R13: 0000000002305410 R14: 0000000002306420 R15: 0000000000000003
>> [  195.997028] Modules linked in: sunrpc amd64_edac_mod ipmi_ssif edac_mce_amd kvm_amd ipmi_si igb ipmi_devintf k10temp kvm pcspkr ipmi_msgha
>> ndler joydev irqbypass sp5100_tco dca hpwdt hpilo i2c_piix4 xfs libcrc32c radeon i2c_algo_bit drm_kms_helper ttm ata_generic pata_acpi drm se
>> rio_raw pata_atiixp
>> [  196.134162] CR2: 0000000000000030
>> [  196.152788] ---[ end trace 4420ea5061342d3e ]---
>>
>> Suspected commit is:
>>   b43a99900559 ("hugetlbfs: use i_mmap_rwsem for more pmd sharing synchronization")
>> which adds to unmap_and_move_huge_page():
>> +               struct address_space *mapping = page_mapping(hpage);
>> +
>> +               /*
>> +                * try_to_unmap could potentially call huge_pmd_unshare.
>> +                * Because of this, take semaphore in write mode here and
>> +                * set TTU_RMAP_LOCKED to let lower levels know we have
>> +                * taken the lock.
>> +                */
>> +               i_mmap_lock_write(mapping);
>>
>> If I'm reading this right, 'mapping' will be NULL for anon mappings.
> 
> Not exactly.
> 

Well, yes exactly.  Sorry, I was confusing this with something else.

That commit does cause BUGs for migration and page poisoning of anon huge
pages.  The patch was trying to take care of i_mmap_rwsem locking outside
try_to_unmap infrastructure.  This is because try_to_unmap will take the
semaphore in read mode (for file mappings) and we really need it to be
taken in write mode.

The patch below continues to take the semaphore outside try_to_unmap for
the file mapping case.  For anon mappings, the locking is done as a special
case in try_to_unmap_one.  This is something I was trying to avoid as it
it harder to follow/understand.  Any suggestions on how to restructure this
or make it more clear are welcome.

Adding Andrew on Cc as he already sent the commit causing the BUGs upstream.

From: Mike Kravetz <mike.kravetz@oracle.com>

hugetlbfs: fix migration and poisoning of anon huge pages

Expanded use of i_mmap_rwsem for pmd sharing synchronization incorrectly
used page_mapping() of anon huge pages to get to address_space
i_mmap_rwsem.  Since page_mapping() is NULL for pages of anon mappings,
an "unable to handle kernel NULL pointer" BUG would occur with stack
similar to:

RIP: 0010:down_write+0x1b/0x40
Call Trace:
 migrate_pages+0x81f/0xb90
 __ia32_compat_sys_migrate_pages+0x190/0x190
 do_move_pages_to_node.isra.53.part.54+0x2a/0x50
 kernel_move_pages+0x566/0x7b0
 __x64_sys_move_pages+0x24/0x30
 do_syscall_64+0x5b/0x180
 entry_SYSCALL_64_after_hwframe+0x44/0xa9

To fix, only use page_mapping() for non-anon or file pages.  For anon
pages wait until we find a vma in which the page is mapped and get the
address_space from vm_file.

Fixes: b43a99900559 ("hugetlbfs: use i_mmap_rwsem for more pmd sharing
synchronization")
Signed-off-by: Mike Kravetz <mike.kravetz@oracle.com>
---
 mm/memory-failure.c | 15 +++++++++++----
 mm/migrate.c        | 34 +++++++++++++++++++++++-----------
 mm/rmap.c           | 15 ++++++++++++---
 3 files changed, 46 insertions(+), 18 deletions(-)

diff --git a/mm/memory-failure.c b/mm/memory-failure.c
index 93558fb981fb..f229cbd0b347 100644
--- a/mm/memory-failure.c
+++ b/mm/memory-failure.c
@@ -1032,14 +1032,21 @@ static bool hwpoison_user_mappings(struct page *p,
unsigned long pfn,
 		unmap_success = try_to_unmap(hpage, ttu);
 	} else if (mapping) {
 		/*
-		 * For hugetlb pages, try_to_unmap could potentially call
-		 * huge_pmd_unshare.  Because of this, take semaphore in
-		 * write mode here and set TTU_RMAP_LOCKED to indicate we
-		 * have taken the lock at this higer level.
+		 * For file mappings, take semaphore in write mode here and
+		 * set TTU_RMAP_LOCKED to let lower levels know we have taken
+		 * the lock.  This is in case lower levels call
+		 * huge_pmd_unshare.  Without this, try_to_unmap would only
+		 * take the semaphore in read mode.
 		 */
 		i_mmap_lock_write(mapping);
 		unmap_success = try_to_unmap(hpage, ttu|TTU_RMAP_LOCKED);
 		i_mmap_unlock_write(mapping);
+	} else {
+		/*
+		 * For huge page anon mappings, try_to_unmap_one will take the
+		 * i_mmap_rwsem before calling huge_pmd_unshare if necessary.
+		 */
+		unmap_success = try_to_unmap(hpage, ttu);
 	}
 	if (!unmap_success)
 		pr_err("Memory failure: %#lx: failed to unmap page (mapcount=%d)\n",
diff --git a/mm/migrate.c b/mm/migrate.c
index 725edaef238a..45d7dd0c9479 100644
--- a/mm/migrate.c
+++ b/mm/migrate.c
@@ -1309,17 +1309,29 @@ static int unmap_and_move_huge_page(new_page_t get_new_page,
 	if (page_mapped(hpage)) {
 		struct address_space *mapping = page_mapping(hpage);

-		/*
-		 * try_to_unmap could potentially call huge_pmd_unshare.
-		 * Because of this, take semaphore in write mode here and
-		 * set TTU_RMAP_LOCKED to let lower levels know we have
-		 * taken the lock.
-		 */
-		i_mmap_lock_write(mapping);
-		try_to_unmap(hpage,
-			TTU_MIGRATION|TTU_IGNORE_MLOCK|TTU_IGNORE_ACCESS|
-			TTU_RMAP_LOCKED);
-		i_mmap_unlock_write(mapping);
+		if (mapping) {
+			/*
+			 * For file mappings, take semaphore in write mode here
+			 * and set TTU_RMAP_LOCKED to let lower levels know we
+			 * have taken the lock.  This is in case lower levels
+			 * call huge_pmd_unshare.  Without this, try_to_unmap
+			 * would only take the semaphore in read mode.
+			 */
+			i_mmap_lock_write(mapping);
+			try_to_unmap(hpage,
+				TTU_MIGRATION|TTU_IGNORE_MLOCK|
+				TTU_IGNORE_ACCESS|TTU_RMAP_LOCKED);
+			i_mmap_unlock_write(mapping);
+		} else {
+			/*
+			 * For anon mappings, try_to_unmap_one will take the
+			 * i_mmap_rwsem before calling huge_pmd_unshare if
+			 * necessary.
+			 */
+			try_to_unmap(hpage,
+				TTU_MIGRATION|TTU_IGNORE_MLOCK|
+				TTU_IGNORE_ACCESS);
+		}
 		page_was_mapped = 1;
 	}

diff --git a/mm/rmap.c b/mm/rmap.c
index c566bd552535..b267cc084f92 100644
--- a/mm/rmap.c
+++ b/mm/rmap.c
@@ -1375,11 +1375,17 @@ static bool try_to_unmap_one(struct page *page, struct
vm_area_struct *vma,
 		/*
 		 * If sharing is possible, start and end will be adjusted
 		 * accordingly.
-		 *
-		 * If called for a huge page, caller must hold i_mmap_rwsem
-		 * in write mode as it is possible to call huge_pmd_unshare.
 		 */
 		adjust_range_if_pmd_sharing_possible(vma, &start, &end);
+
+		/*
+		 * If called for a huge page file mapping, caller will hold
+		 * i_mmap_rwsem in write mode.  For anon mappings, we must
+		 * take the semaphore here.  All this is necessary because
+		 * it is possible huge_pmd_unshare will 'unshare' a pmd.
+		 */
+		if (PageAnon(page))
+			i_mmap_lock_write(vma->vm_file->f_mapping);
 	}
 	mmu_notifier_invalidate_range_start(vma->vm_mm, start, end);

@@ -1655,6 +1661,9 @@ static bool try_to_unmap_one(struct page *page, struct
vm_area_struct *vma,
 	}

 	mmu_notifier_invalidate_range_end(vma->vm_mm, start, end);
+	/* For anon huge pages, we must unlock. */
+	if (PageHuge(page) && PageAnon(page))
+		i_mmap_unlock_write(vma->vm_file->f_mapping);

 	return ret;
 }
-- 
2.17.2

[LTP] [bug] problems with migration of huge pages with v4.20-10214-ge1ef035d272e

[Jan Stancek]

----- Original Message -----
> On 1/2/19 1:24 PM, Mike Kravetz wrote:
> > On 1/2/19 12:30 PM, Jan Stancek wrote:
> >> Hi,
> >>
> >> LTP move_pages12 [1] started failing recently.
> >>
> >> The test maps/unmaps some anonymous private huge pages
> >> and migrates them between 2 nodes. This now reliably
> >> hits NULL ptr deref:
> >>
> >> [  194.819357] BUG: unable to handle kernel NULL pointer dereference at
> >> 0000000000000030
> >> [  194.864410] #PF error: [WRITE]
> >> [  194.881502] PGD 22c758067 P4D 22c758067 PUD 235177067 PMD 0
> >> [  194.913833] Oops: 0002 [#1] SMP NOPTI
> >> [  194.935062] CPU: 0 PID: 865 Comm: move_pages12 Not tainted 4.20.0+ #1
> >> [  194.972993] Hardware name: HP ProLiant SL335s G7/, BIOS A24 12/08/2012
> >> [  195.005359] RIP: 0010:down_write+0x1b/0x40
> >> [  195.028257] Code: 00 5c 01 00 48 83 c8 03 48 89 43 20 5b c3 90 0f 1f 44
> >> 00 00 53 48 89 fb e8 d2 d7 ff ff 48 89 d8 48 ba 01 00 00 00 ff ff
> >> ff ff <f0> 48 0f c1 10 85 d2 74 05 e8 07 26 ff ff 65 48 8b 04 25 00 5c 01
> >> [  195.121836] RSP: 0018:ffffb87e4224fd00 EFLAGS: 00010246
> >> [  195.147097] RAX: 0000000000000030 RBX: 0000000000000030 RCX:
> >> 0000000000000000
> >> [  195.185096] RDX: ffffffff00000001 RSI: ffffffffa69d30f0 RDI:
> >> 0000000000000030
> >> [  195.219251] RBP: 0000000000000030 R08: ffffe7d4889d8008 R09:
> >> 0000000000000003
> >> [  195.258291] R10: 000000000000000f R11: ffffe7d4889d8008 R12:
> >> ffffe7d4889d0008
> >> [  195.294547] R13: ffffe7d490b78000 R14: ffffe7d4889d0000 R15:
> >> ffff8be9b2ba4580
> >> [  195.332532] FS:  00007f1670112b80(0000) GS:ffff8be9b7a00000(0000)
> >> knlGS:0000000000000000
> >> [  195.373888] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> >> [  195.405938] CR2: 0000000000000030 CR3: 000000023477e000 CR4:
> >> 00000000000006f0
> >> [  195.443579] Call Trace:
> >> [  195.456876]  migrate_pages+0x833/0xcb0
> >> [  195.478070]  ? __ia32_compat_sys_migrate_pages+0x20/0x20
> >> [  195.506027]  do_move_pages_to_node.isra.63.part.64+0x2a/0x50
> >> [  195.536963]  kernel_move_pages+0x667/0x8c0
> >> [  195.559616]  ? __handle_mm_fault+0xb95/0x1370
> >> [  195.588765]  __x64_sys_move_pages+0x24/0x30
> >> [  195.611439]  do_syscall_64+0x5b/0x160
> >> [  195.631901]  entry_SYSCALL_64_after_hwframe+0x44/0xa9
> >> [  195.657790] RIP: 0033:0x7f166f5ff959
> >> [  195.676365] Code: 00 c3 66 2e 0f 1f 84 00 00 00 00 00 0f 1f 44 00 00 48
> >> 89 f8 48 89 f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08
> >> 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 8b 0d 17 45 2c 00 f7 d8 64 89 01 48
> >> [  195.772938] RSP: 002b:00007ffd8d77bb48 EFLAGS: 00000246 ORIG_RAX:
> >> 0000000000000117
> >> [  195.810207] RAX: ffffffffffffffda RBX: 0000000000000400 RCX:
> >> 00007f166f5ff959
> >> [  195.847522] RDX: 0000000002303400 RSI: 0000000000000400 RDI:
> >> 0000000000000360
> >> [  195.882327] RBP: 0000000000000400 R08: 0000000002306420 R09:
> >> 0000000000000004
> >> [  195.920017] R10: 0000000002305410 R11: 0000000000000246 R12:
> >> 0000000002303400
> >> [  195.958053] R13: 0000000002305410 R14: 0000000002306420 R15:
> >> 0000000000000003
> >> [  195.997028] Modules linked in: sunrpc amd64_edac_mod ipmi_ssif
> >> edac_mce_amd kvm_amd ipmi_si igb ipmi_devintf k10temp kvm pcspkr
> >> ipmi_msgha
> >> ndler joydev irqbypass sp5100_tco dca hpwdt hpilo i2c_piix4 xfs libcrc32c
> >> radeon i2c_algo_bit drm_kms_helper ttm ata_generic pata_acpi drm se
> >> rio_raw pata_atiixp
> >> [  196.134162] CR2: 0000000000000030
> >> [  196.152788] ---[ end trace 4420ea5061342d3e ]---
> >>
> >> Suspected commit is:
> >>   b43a99900559 ("hugetlbfs: use i_mmap_rwsem for more pmd sharing
> >>   synchronization")
> >> which adds to unmap_and_move_huge_page():
> >> +               struct address_space *mapping = page_mapping(hpage);
> >> +
> >> +               /*
> >> +                * try_to_unmap could potentially call huge_pmd_unshare.
> >> +                * Because of this, take semaphore in write mode here and
> >> +                * set TTU_RMAP_LOCKED to let lower levels know we have
> >> +                * taken the lock.
> >> +                */
> >> +               i_mmap_lock_write(mapping);
> >>
> >> If I'm reading this right, 'mapping' will be NULL for anon mappings.
> > 
> > Not exactly.
> > 
> 
> Well, yes exactly.  Sorry, I was confusing this with something else.
> 
> That commit does cause BUGs for migration and page poisoning of anon huge
> pages.  The patch was trying to take care of i_mmap_rwsem locking outside
> try_to_unmap infrastructure.  This is because try_to_unmap will take the
> semaphore in read mode (for file mappings) and we really need it to be
> taken in write mode.
> 
> The patch below continues to take the semaphore outside try_to_unmap for
> the file mapping case.  For anon mappings, the locking is done as a special
> case in try_to_unmap_one.  This is something I was trying to avoid as it
> it harder to follow/understand.  Any suggestions on how to restructure this
> or make it more clear are welcome.
> 
> Adding Andrew on Cc as he already sent the commit causing the BUGs upstream.
> 
> From: Mike Kravetz <mike.kravetz@oracle.com>
> 
> hugetlbfs: fix migration and poisoning of anon huge pages
> 
> Expanded use of i_mmap_rwsem for pmd sharing synchronization incorrectly
> used page_mapping() of anon huge pages to get to address_space
> i_mmap_rwsem.  Since page_mapping() is NULL for pages of anon mappings,
> an "unable to handle kernel NULL pointer" BUG would occur with stack
> similar to:
> 
> RIP: 0010:down_write+0x1b/0x40
> Call Trace:
>  migrate_pages+0x81f/0xb90
>  __ia32_compat_sys_migrate_pages+0x190/0x190
>  do_move_pages_to_node.isra.53.part.54+0x2a/0x50
>  kernel_move_pages+0x566/0x7b0
>  __x64_sys_move_pages+0x24/0x30
>  do_syscall_64+0x5b/0x180
>  entry_SYSCALL_64_after_hwframe+0x44/0xa9
> 
> To fix, only use page_mapping() for non-anon or file pages.  For anon
> pages wait until we find a vma in which the page is mapped and get the
> address_space from vm_file.
> 
> Fixes: b43a99900559 ("hugetlbfs: use i_mmap_rwsem for more pmd sharing
> synchronization")
> Signed-off-by: Mike Kravetz <mike.kravetz@oracle.com>

Mike,

1) with LTP move_pages12 (MAP_PRIVATE version of reproducer)
Patch below fixes the panic for me.
It didn't apply cleanly to latest master, but conflicts were easy to resolve.

2) with MAP_SHARED version of reproducer
It still hangs in user-space.
v4.19 kernel appears to work fine so I've started a bisect.

Regards,
Jan

> ---
>  mm/memory-failure.c | 15 +++++++++++----
>  mm/migrate.c        | 34 +++++++++++++++++++++++-----------
>  mm/rmap.c           | 15 ++++++++++++---
>  3 files changed, 46 insertions(+), 18 deletions(-)
> 
> diff --git a/mm/memory-failure.c b/mm/memory-failure.c
> index 93558fb981fb..f229cbd0b347 100644
> --- a/mm/memory-failure.c
> +++ b/mm/memory-failure.c
> @@ -1032,14 +1032,21 @@ static bool hwpoison_user_mappings(struct page *p,
> unsigned long pfn,
>  		unmap_success = try_to_unmap(hpage, ttu);
>  	} else if (mapping) {
>  		/*
> -		 * For hugetlb pages, try_to_unmap could potentially call
> -		 * huge_pmd_unshare.  Because of this, take semaphore in
> -		 * write mode here and set TTU_RMAP_LOCKED to indicate we
> -		 * have taken the lock at this higer level.
> +		 * For file mappings, take semaphore in write mode here and
> +		 * set TTU_RMAP_LOCKED to let lower levels know we have taken
> +		 * the lock.  This is in case lower levels call
> +		 * huge_pmd_unshare.  Without this, try_to_unmap would only
> +		 * take the semaphore in read mode.
>  		 */
>  		i_mmap_lock_write(mapping);
>  		unmap_success = try_to_unmap(hpage, ttu|TTU_RMAP_LOCKED);
>  		i_mmap_unlock_write(mapping);
> +	} else {
> +		/*
> +		 * For huge page anon mappings, try_to_unmap_one will take the
> +		 * i_mmap_rwsem before calling huge_pmd_unshare if necessary.
> +		 */
> +		unmap_success = try_to_unmap(hpage, ttu);
>  	}
>  	if (!unmap_success)
>  		pr_err("Memory failure: %#lx: failed to unmap page (mapcount=%d)\n",
> diff --git a/mm/migrate.c b/mm/migrate.c
> index 725edaef238a..45d7dd0c9479 100644
> --- a/mm/migrate.c
> +++ b/mm/migrate.c
> @@ -1309,17 +1309,29 @@ static int unmap_and_move_huge_page(new_page_t
> get_new_page,
>  	if (page_mapped(hpage)) {
>  		struct address_space *mapping = page_mapping(hpage);
> 
> -		/*
> -		 * try_to_unmap could potentially call huge_pmd_unshare.
> -		 * Because of this, take semaphore in write mode here and
> -		 * set TTU_RMAP_LOCKED to let lower levels know we have
> -		 * taken the lock.
> -		 */
> -		i_mmap_lock_write(mapping);
> -		try_to_unmap(hpage,
> -			TTU_MIGRATION|TTU_IGNORE_MLOCK|TTU_IGNORE_ACCESS|
> -			TTU_RMAP_LOCKED);
> -		i_mmap_unlock_write(mapping);
> +		if (mapping) {
> +			/*
> +			 * For file mappings, take semaphore in write mode here
> +			 * and set TTU_RMAP_LOCKED to let lower levels know we
> +			 * have taken the lock.  This is in case lower levels
> +			 * call huge_pmd_unshare.  Without this, try_to_unmap
> +			 * would only take the semaphore in read mode.
> +			 */
> +			i_mmap_lock_write(mapping);
> +			try_to_unmap(hpage,
> +				TTU_MIGRATION|TTU_IGNORE_MLOCK|
> +				TTU_IGNORE_ACCESS|TTU_RMAP_LOCKED);
> +			i_mmap_unlock_write(mapping);
> +		} else {
> +			/*
> +			 * For anon mappings, try_to_unmap_one will take the
> +			 * i_mmap_rwsem before calling huge_pmd_unshare if
> +			 * necessary.
> +			 */
> +			try_to_unmap(hpage,
> +				TTU_MIGRATION|TTU_IGNORE_MLOCK|
> +				TTU_IGNORE_ACCESS);
> +		}
>  		page_was_mapped = 1;
>  	}
> 
> diff --git a/mm/rmap.c b/mm/rmap.c
> index c566bd552535..b267cc084f92 100644
> --- a/mm/rmap.c
> +++ b/mm/rmap.c
> @@ -1375,11 +1375,17 @@ static bool try_to_unmap_one(struct page *page,
> struct
> vm_area_struct *vma,
>  		/*
>  		 * If sharing is possible, start and end will be adjusted
>  		 * accordingly.
> -		 *
> -		 * If called for a huge page, caller must hold i_mmap_rwsem
> -		 * in write mode as it is possible to call huge_pmd_unshare.
>  		 */
>  		adjust_range_if_pmd_sharing_possible(vma, &start, &end);
> +
> +		/*
> +		 * If called for a huge page file mapping, caller will hold
> +		 * i_mmap_rwsem in write mode.  For anon mappings, we must
> +		 * take the semaphore here.  All this is necessary because
> +		 * it is possible huge_pmd_unshare will 'unshare' a pmd.
> +		 */
> +		if (PageAnon(page))
> +			i_mmap_lock_write(vma->vm_file->f_mapping);
>  	}
>  	mmu_notifier_invalidate_range_start(vma->vm_mm, start, end);
> 
> @@ -1655,6 +1661,9 @@ static bool try_to_unmap_one(struct page *page, struct
> vm_area_struct *vma,
>  	}
> 
>  	mmu_notifier_invalidate_range_end(vma->vm_mm, start, end);
> +	/* For anon huge pages, we must unlock. */
> +	if (PageHuge(page) && PageAnon(page))
> +		i_mmap_unlock_write(vma->vm_file->f_mapping);
> 
>  	return ret;
>  }
> --
> 2.17.2
> 
> 

Re: [bug] problems with migration of huge pages with v4.20-10214-ge1ef035d272e

[Jan Stancek]

----- Original Message -----
> On 1/2/19 1:24 PM, Mike Kravetz wrote:
> > On 1/2/19 12:30 PM, Jan Stancek wrote:
> >> Hi,
> >>
> >> LTP move_pages12 [1] started failing recently.
> >>
> >> The test maps/unmaps some anonymous private huge pages
> >> and migrates them between 2 nodes. This now reliably
> >> hits NULL ptr deref:
> >>
> >> [  194.819357] BUG: unable to handle kernel NULL pointer dereference at
> >> 0000000000000030
> >> [  194.864410] #PF error: [WRITE]
> >> [  194.881502] PGD 22c758067 P4D 22c758067 PUD 235177067 PMD 0
> >> [  194.913833] Oops: 0002 [#1] SMP NOPTI
> >> [  194.935062] CPU: 0 PID: 865 Comm: move_pages12 Not tainted 4.20.0+ #1
> >> [  194.972993] Hardware name: HP ProLiant SL335s G7/, BIOS A24 12/08/2012
> >> [  195.005359] RIP: 0010:down_write+0x1b/0x40
> >> [  195.028257] Code: 00 5c 01 00 48 83 c8 03 48 89 43 20 5b c3 90 0f 1f 44
> >> 00 00 53 48 89 fb e8 d2 d7 ff ff 48 89 d8 48 ba 01 00 00 00 ff ff
> >> ff ff <f0> 48 0f c1 10 85 d2 74 05 e8 07 26 ff ff 65 48 8b 04 25 00 5c 01
> >> [  195.121836] RSP: 0018:ffffb87e4224fd00 EFLAGS: 00010246
> >> [  195.147097] RAX: 0000000000000030 RBX: 0000000000000030 RCX:
> >> 0000000000000000
> >> [  195.185096] RDX: ffffffff00000001 RSI: ffffffffa69d30f0 RDI:
> >> 0000000000000030
> >> [  195.219251] RBP: 0000000000000030 R08: ffffe7d4889d8008 R09:
> >> 0000000000000003
> >> [  195.258291] R10: 000000000000000f R11: ffffe7d4889d8008 R12:
> >> ffffe7d4889d0008
> >> [  195.294547] R13: ffffe7d490b78000 R14: ffffe7d4889d0000 R15:
> >> ffff8be9b2ba4580
> >> [  195.332532] FS:  00007f1670112b80(0000) GS:ffff8be9b7a00000(0000)
> >> knlGS:0000000000000000
> >> [  195.373888] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> >> [  195.405938] CR2: 0000000000000030 CR3: 000000023477e000 CR4:
> >> 00000000000006f0
> >> [  195.443579] Call Trace:
> >> [  195.456876]  migrate_pages+0x833/0xcb0
> >> [  195.478070]  ? __ia32_compat_sys_migrate_pages+0x20/0x20
> >> [  195.506027]  do_move_pages_to_node.isra.63.part.64+0x2a/0x50
> >> [  195.536963]  kernel_move_pages+0x667/0x8c0
> >> [  195.559616]  ? __handle_mm_fault+0xb95/0x1370
> >> [  195.588765]  __x64_sys_move_pages+0x24/0x30
> >> [  195.611439]  do_syscall_64+0x5b/0x160
> >> [  195.631901]  entry_SYSCALL_64_after_hwframe+0x44/0xa9
> >> [  195.657790] RIP: 0033:0x7f166f5ff959
> >> [  195.676365] Code: 00 c3 66 2e 0f 1f 84 00 00 00 00 00 0f 1f 44 00 00 48
> >> 89 f8 48 89 f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08
> >> 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 8b 0d 17 45 2c 00 f7 d8 64 89 01 48
> >> [  195.772938] RSP: 002b:00007ffd8d77bb48 EFLAGS: 00000246 ORIG_RAX:
> >> 0000000000000117
> >> [  195.810207] RAX: ffffffffffffffda RBX: 0000000000000400 RCX:
> >> 00007f166f5ff959
> >> [  195.847522] RDX: 0000000002303400 RSI: 0000000000000400 RDI:
> >> 0000000000000360
> >> [  195.882327] RBP: 0000000000000400 R08: 0000000002306420 R09:
> >> 0000000000000004
> >> [  195.920017] R10: 0000000002305410 R11: 0000000000000246 R12:
> >> 0000000002303400
> >> [  195.958053] R13: 0000000002305410 R14: 0000000002306420 R15:
> >> 0000000000000003
> >> [  195.997028] Modules linked in: sunrpc amd64_edac_mod ipmi_ssif
> >> edac_mce_amd kvm_amd ipmi_si igb ipmi_devintf k10temp kvm pcspkr
> >> ipmi_msgha
> >> ndler joydev irqbypass sp5100_tco dca hpwdt hpilo i2c_piix4 xfs libcrc32c
> >> radeon i2c_algo_bit drm_kms_helper ttm ata_generic pata_acpi drm se
> >> rio_raw pata_atiixp
> >> [  196.134162] CR2: 0000000000000030
> >> [  196.152788] ---[ end trace 4420ea5061342d3e ]---
> >>
> >> Suspected commit is:
> >>   b43a99900559 ("hugetlbfs: use i_mmap_rwsem for more pmd sharing
> >>   synchronization")
> >> which adds to unmap_and_move_huge_page():
> >> +               struct address_space *mapping = page_mapping(hpage);
> >> +
> >> +               /*
> >> +                * try_to_unmap could potentially call huge_pmd_unshare.
> >> +                * Because of this, take semaphore in write mode here and
> >> +                * set TTU_RMAP_LOCKED to let lower levels know we have
> >> +                * taken the lock.
> >> +                */
> >> +               i_mmap_lock_write(mapping);
> >>
> >> If I'm reading this right, 'mapping' will be NULL for anon mappings.
> > 
> > Not exactly.
> > 
> 
> Well, yes exactly.  Sorry, I was confusing this with something else.
> 
> That commit does cause BUGs for migration and page poisoning of anon huge
> pages.  The patch was trying to take care of i_mmap_rwsem locking outside
> try_to_unmap infrastructure.  This is because try_to_unmap will take the
> semaphore in read mode (for file mappings) and we really need it to be
> taken in write mode.
> 
> The patch below continues to take the semaphore outside try_to_unmap for
> the file mapping case.  For anon mappings, the locking is done as a special
> case in try_to_unmap_one.  This is something I was trying to avoid as it
> it harder to follow/understand.  Any suggestions on how to restructure this
> or make it more clear are welcome.
> 
> Adding Andrew on Cc as he already sent the commit causing the BUGs upstream.
> 
> From: Mike Kravetz <mike.kravetz@oracle.com>
> 
> hugetlbfs: fix migration and poisoning of anon huge pages
> 
> Expanded use of i_mmap_rwsem for pmd sharing synchronization incorrectly
> used page_mapping() of anon huge pages to get to address_space
> i_mmap_rwsem.  Since page_mapping() is NULL for pages of anon mappings,
> an "unable to handle kernel NULL pointer" BUG would occur with stack
> similar to:
> 
> RIP: 0010:down_write+0x1b/0x40
> Call Trace:
>  migrate_pages+0x81f/0xb90
>  __ia32_compat_sys_migrate_pages+0x190/0x190
>  do_move_pages_to_node.isra.53.part.54+0x2a/0x50
>  kernel_move_pages+0x566/0x7b0
>  __x64_sys_move_pages+0x24/0x30
>  do_syscall_64+0x5b/0x180
>  entry_SYSCALL_64_after_hwframe+0x44/0xa9
> 
> To fix, only use page_mapping() for non-anon or file pages.  For anon
> pages wait until we find a vma in which the page is mapped and get the
> address_space from vm_file.
> 
> Fixes: b43a99900559 ("hugetlbfs: use i_mmap_rwsem for more pmd sharing
> synchronization")
> Signed-off-by: Mike Kravetz <mike.kravetz@oracle.com>

Mike,

1) with LTP move_pages12 (MAP_PRIVATE version of reproducer)
Patch below fixes the panic for me.
It didn't apply cleanly to latest master, but conflicts were easy to resolve.

2) with MAP_SHARED version of reproducer
It still hangs in user-space.
v4.19 kernel appears to work fine so I've started a bisect.

Regards,
Jan

> ---
>  mm/memory-failure.c | 15 +++++++++++----
>  mm/migrate.c        | 34 +++++++++++++++++++++++-----------
>  mm/rmap.c           | 15 ++++++++++++---
>  3 files changed, 46 insertions(+), 18 deletions(-)
> 
> diff --git a/mm/memory-failure.c b/mm/memory-failure.c
> index 93558fb981fb..f229cbd0b347 100644
> --- a/mm/memory-failure.c
> +++ b/mm/memory-failure.c
> @@ -1032,14 +1032,21 @@ static bool hwpoison_user_mappings(struct page *p,
> unsigned long pfn,
>  		unmap_success = try_to_unmap(hpage, ttu);
>  	} else if (mapping) {
>  		/*
> -		 * For hugetlb pages, try_to_unmap could potentially call
> -		 * huge_pmd_unshare.  Because of this, take semaphore in
> -		 * write mode here and set TTU_RMAP_LOCKED to indicate we
> -		 * have taken the lock at this higer level.
> +		 * For file mappings, take semaphore in write mode here and
> +		 * set TTU_RMAP_LOCKED to let lower levels know we have taken
> +		 * the lock.  This is in case lower levels call
> +		 * huge_pmd_unshare.  Without this, try_to_unmap would only
> +		 * take the semaphore in read mode.
>  		 */
>  		i_mmap_lock_write(mapping);
>  		unmap_success = try_to_unmap(hpage, ttu|TTU_RMAP_LOCKED);
>  		i_mmap_unlock_write(mapping);
> +	} else {
> +		/*
> +		 * For huge page anon mappings, try_to_unmap_one will take the
> +		 * i_mmap_rwsem before calling huge_pmd_unshare if necessary.
> +		 */
> +		unmap_success = try_to_unmap(hpage, ttu);
>  	}
>  	if (!unmap_success)
>  		pr_err("Memory failure: %#lx: failed to unmap page (mapcount=%d)\n",
> diff --git a/mm/migrate.c b/mm/migrate.c
> index 725edaef238a..45d7dd0c9479 100644
> --- a/mm/migrate.c
> +++ b/mm/migrate.c
> @@ -1309,17 +1309,29 @@ static int unmap_and_move_huge_page(new_page_t
> get_new_page,
>  	if (page_mapped(hpage)) {
>  		struct address_space *mapping = page_mapping(hpage);
> 
> -		/*
> -		 * try_to_unmap could potentially call huge_pmd_unshare.
> -		 * Because of this, take semaphore in write mode here and
> -		 * set TTU_RMAP_LOCKED to let lower levels know we have
> -		 * taken the lock.
> -		 */
> -		i_mmap_lock_write(mapping);
> -		try_to_unmap(hpage,
> -			TTU_MIGRATION|TTU_IGNORE_MLOCK|TTU_IGNORE_ACCESS|
> -			TTU_RMAP_LOCKED);
> -		i_mmap_unlock_write(mapping);
> +		if (mapping) {
> +			/*
> +			 * For file mappings, take semaphore in write mode here
> +			 * and set TTU_RMAP_LOCKED to let lower levels know we
> +			 * have taken the lock.  This is in case lower levels
> +			 * call huge_pmd_unshare.  Without this, try_to_unmap
> +			 * would only take the semaphore in read mode.
> +			 */
> +			i_mmap_lock_write(mapping);
> +			try_to_unmap(hpage,
> +				TTU_MIGRATION|TTU_IGNORE_MLOCK|
> +				TTU_IGNORE_ACCESS|TTU_RMAP_LOCKED);
> +			i_mmap_unlock_write(mapping);
> +		} else {
> +			/*
> +			 * For anon mappings, try_to_unmap_one will take the
> +			 * i_mmap_rwsem before calling huge_pmd_unshare if
> +			 * necessary.
> +			 */
> +			try_to_unmap(hpage,
> +				TTU_MIGRATION|TTU_IGNORE_MLOCK|
> +				TTU_IGNORE_ACCESS);
> +		}
>  		page_was_mapped = 1;
>  	}
> 
> diff --git a/mm/rmap.c b/mm/rmap.c
> index c566bd552535..b267cc084f92 100644
> --- a/mm/rmap.c
> +++ b/mm/rmap.c
> @@ -1375,11 +1375,17 @@ static bool try_to_unmap_one(struct page *page,
> struct
> vm_area_struct *vma,
>  		/*
>  		 * If sharing is possible, start and end will be adjusted
>  		 * accordingly.
> -		 *
> -		 * If called for a huge page, caller must hold i_mmap_rwsem
> -		 * in write mode as it is possible to call huge_pmd_unshare.
>  		 */
>  		adjust_range_if_pmd_sharing_possible(vma, &start, &end);
> +
> +		/*
> +		 * If called for a huge page file mapping, caller will hold
> +		 * i_mmap_rwsem in write mode.  For anon mappings, we must
> +		 * take the semaphore here.  All this is necessary because
> +		 * it is possible huge_pmd_unshare will 'unshare' a pmd.
> +		 */
> +		if (PageAnon(page))
> +			i_mmap_lock_write(vma->vm_file->f_mapping);
>  	}
>  	mmu_notifier_invalidate_range_start(vma->vm_mm, start, end);
> 
> @@ -1655,6 +1661,9 @@ static bool try_to_unmap_one(struct page *page, struct
> vm_area_struct *vma,
>  	}
> 
>  	mmu_notifier_invalidate_range_end(vma->vm_mm, start, end);
> +	/* For anon huge pages, we must unlock. */
> +	if (PageHuge(page) && PageAnon(page))
> +		i_mmap_unlock_write(vma->vm_file->f_mapping);
> 
>  	return ret;
>  }
> --
> 2.17.2
> 
> 

[LTP] [bug] problems with migration of huge pages with v4.20-10214-ge1ef035d272e

[Jan Stancek]

----- Original Message -----
<snip>

> > That commit does cause BUGs for migration and page poisoning of anon huge
> > pages.  The patch was trying to take care of i_mmap_rwsem locking outside
> > try_to_unmap infrastructure.  This is because try_to_unmap will take the
> > semaphore in read mode (for file mappings) and we really need it to be
> > taken in write mode.
> > 
> > The patch below continues to take the semaphore outside try_to_unmap for
> > the file mapping case.  For anon mappings, the locking is done as a special
> > case in try_to_unmap_one.  This is something I was trying to avoid as it
> > it harder to follow/understand.  Any suggestions on how to restructure this
> > or make it more clear are welcome.
> > 
> > Adding Andrew on Cc as he already sent the commit causing the BUGs
> > upstream.
> > 
> > From: Mike Kravetz <mike.kravetz@oracle.com>
> > 
> > hugetlbfs: fix migration and poisoning of anon huge pages
> > 
> > Expanded use of i_mmap_rwsem for pmd sharing synchronization incorrectly
> > used page_mapping() of anon huge pages to get to address_space
> > i_mmap_rwsem.  Since page_mapping() is NULL for pages of anon mappings,
> > an "unable to handle kernel NULL pointer" BUG would occur with stack
> > similar to:
> > 
> > RIP: 0010:down_write+0x1b/0x40
> > Call Trace:
> >  migrate_pages+0x81f/0xb90
> >  __ia32_compat_sys_migrate_pages+0x190/0x190
> >  do_move_pages_to_node.isra.53.part.54+0x2a/0x50
> >  kernel_move_pages+0x566/0x7b0
> >  __x64_sys_move_pages+0x24/0x30
> >  do_syscall_64+0x5b/0x180
> >  entry_SYSCALL_64_after_hwframe+0x44/0xa9
> > 
> > To fix, only use page_mapping() for non-anon or file pages.  For anon
> > pages wait until we find a vma in which the page is mapped and get the
> > address_space from vm_file.
> > 
> > Fixes: b43a99900559 ("hugetlbfs: use i_mmap_rwsem for more pmd sharing
> > synchronization")
> > Signed-off-by: Mike Kravetz <mike.kravetz@oracle.com>
> 
> Mike,
> 
> 1) with LTP move_pages12 (MAP_PRIVATE version of reproducer)
> Patch below fixes the panic for me.
> It didn't apply cleanly to latest master, but conflicts were easy to resolve.
> 
> 2) with MAP_SHARED version of reproducer
> It still hangs in user-space.
> v4.19 kernel appears to work fine so I've started a bisect.

My bisect with MAP_SHARED version arrived at same 2 commits:
  c86aa7bbfd55 hugetlbfs: Use i_mmap_rwsem to fix page fault/truncate race
  b43a99900559 hugetlbfs: use i_mmap_rwsem for more pmd sharing synchronization

Maybe a deadlock between page lock and mapping->i_mmap_rwsem?

thread1:
  hugetlbfs_evict_inode
    i_mmap_lock_write(mapping);
    remove_inode_hugepages
      lock_page(page);

thread2:
  __unmap_and_move
    trylock_page(page) / lock_page(page)
      remove_migration_ptes
        rmap_walk_file
          i_mmap_lock_read(mapping);

Here's strace output:
<snip>
1196  11:27:16 mmap(NULL, 4194304, PROT_READ|PROT_WRITE, MAP_SHARED|MAP_ANONYMOUS|MAP_HUGETLB, -1, 0) = 0x7f646c400000
1197  11:27:16 set_robust_list(0x7f646d5b0e60, 24) = 0
1197  11:27:16 getppid()                = 1196
1197  11:27:16 move_pages(1196, 1024, [0x7f646c400000, 0x7f646c401000, 0x7f646c402000, 0x7f646c403000, 0x7f646c404000, 0x7f646c405000, 0x7f646c406000, 0x7f646c407000, 0x7f646c408000, 0x7f646c409000, 0x7f646c40a000, 0x7f646c40b000, 0x7f646c40c000, 0x7f646c40d000, 0x7f646c40e000, 0x7f646c40f000, 0x7f646c410000, 0x7f646c411000, 0x7f646c412000, 0x7f646c413000, 0x7f646c414000, 0x7f646c415000, 0x7f646c416000, 0x7f646c417000, 0x7f646c418000, 0x7f646c419000, 0x7f646c41a000, 0x7f646c41b000, 0x7f646c41c000, 0x7f646c41d000, 0x7f646c41e000, 0x7f646c41f000, ...], [0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, ...], [-ENOENT, -ENOENT, -ENOENT, -ENOENT, -ENOENT, -ENOENT, -ENOENT, -ENOENT, -ENOENT, -ENOENT, -ENOENT, -ENOENT, -ENOENT, -ENOENT, -ENOENT, -ENOENT, -ENOENT, -ENOENT, -ENOENT, -ENOENT, -ENOENT, -ENOENT, -ENOENT, -ENOENT, -ENOENT, -ENOENT, -ENOENT, -ENOENT, -ENOENT, -ENOENT, -ENOENT, -ENOENT, ...], MPOL_MF_MOVE_ALL) = 0
1197  11:27:16 move_pages(1196, 1024, [0x7f646c400000, 0x7f646c401000, 0x7f646c402000, 0x7f646c403000, 0x7f646c404000, 0x7f646c405000, 0x7f646c406000, 0x7f646c407000, 0x7f646c408000, 0x7f646c409000, 0x7f646c40a000, 0x7f646c40b000, 0x7f646c40c000, 0x7f646c40d000, 0x7f646c40e000, 0x7f646c40f000, 0x7f646c410000, 0x7f646c411000, 0x7f646c412000, 0x7f646c413000, 0x7f646c414000, 0x7f646c415000, 0x7f646c416000, 0x7f646c417000, 0x7f646c418000, 0x7f646c419000, 0x7f646c41a000, 0x7f646c41b000, 0x7f646c41c000, 0x7f646c41d000, 0x7f646c41e000, 0x7f646c41f000, ...], [1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, ...], [1, -EACCES, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, ...], MPOL_MF_MOVE_ALL) = 0
1197  11:27:16 move_pages(1196, 1024, [0x7f646c400000, 0x7f646c401000, 0x7f646c402000, 0x7f646c403000, 0x7f646c404000, 0x7f646c405000, 0x7f646c406000, 0x7f646c407000, 0x7f646c408000, 0x7f646c409000, 0x7f646c40a000, 0x7f646c40b000, 0x7f646c40c000, 0x7f646c40d000, 0x7f646c40e000, 0x7f646c40f000, 0x7f646c410000, 0x7f646c411000, 0x7f646c412000, 0x7f646c413000, 0x7f646c414000, 0x7f646c415000, 0x7f646c416000, 0x7f646c417000, 0x7f646c418000, 0x7f646c419000, 0x7f646c41a000, 0x7f646c41b000, 0x7f646c41c000, 0x7f646c41d000, 0x7f646c41e000, 0x7f646c41f000, ...], [0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, ...],  <unfinished ...>
1196  11:27:16 munmap(0x7f646c400000, 4194304 <unfinished ...>
<hangs>

Regards,
Jan

Re: [bug] problems with migration of huge pages with v4.20-10214-ge1ef035d272e

[Jan Stancek]

----- Original Message -----
<snip>

> > That commit does cause BUGs for migration and page poisoning of anon huge
> > pages.  The patch was trying to take care of i_mmap_rwsem locking outside
> > try_to_unmap infrastructure.  This is because try_to_unmap will take the
> > semaphore in read mode (for file mappings) and we really need it to be
> > taken in write mode.
> > 
> > The patch below continues to take the semaphore outside try_to_unmap for
> > the file mapping case.  For anon mappings, the locking is done as a special
> > case in try_to_unmap_one.  This is something I was trying to avoid as it
> > it harder to follow/understand.  Any suggestions on how to restructure this
> > or make it more clear are welcome.
> > 
> > Adding Andrew on Cc as he already sent the commit causing the BUGs
> > upstream.
> > 
> > From: Mike Kravetz <mike.kravetz@oracle.com>
> > 
> > hugetlbfs: fix migration and poisoning of anon huge pages
> > 
> > Expanded use of i_mmap_rwsem for pmd sharing synchronization incorrectly
> > used page_mapping() of anon huge pages to get to address_space
> > i_mmap_rwsem.  Since page_mapping() is NULL for pages of anon mappings,
> > an "unable to handle kernel NULL pointer" BUG would occur with stack
> > similar to:
> > 
> > RIP: 0010:down_write+0x1b/0x40
> > Call Trace:
> >  migrate_pages+0x81f/0xb90
> >  __ia32_compat_sys_migrate_pages+0x190/0x190
> >  do_move_pages_to_node.isra.53.part.54+0x2a/0x50
> >  kernel_move_pages+0x566/0x7b0
> >  __x64_sys_move_pages+0x24/0x30
> >  do_syscall_64+0x5b/0x180
> >  entry_SYSCALL_64_after_hwframe+0x44/0xa9
> > 
> > To fix, only use page_mapping() for non-anon or file pages.  For anon
> > pages wait until we find a vma in which the page is mapped and get the
> > address_space from vm_file.
> > 
> > Fixes: b43a99900559 ("hugetlbfs: use i_mmap_rwsem for more pmd sharing
> > synchronization")
> > Signed-off-by: Mike Kravetz <mike.kravetz@oracle.com>
> 
> Mike,
> 
> 1) with LTP move_pages12 (MAP_PRIVATE version of reproducer)
> Patch below fixes the panic for me.
> It didn't apply cleanly to latest master, but conflicts were easy to resolve.
> 
> 2) with MAP_SHARED version of reproducer
> It still hangs in user-space.
> v4.19 kernel appears to work fine so I've started a bisect.

My bisect with MAP_SHARED version arrived at same 2 commits:
  c86aa7bbfd55 hugetlbfs: Use i_mmap_rwsem to fix page fault/truncate race
  b43a99900559 hugetlbfs: use i_mmap_rwsem for more pmd sharing synchronization

Maybe a deadlock between page lock and mapping->i_mmap_rwsem?

thread1:
  hugetlbfs_evict_inode
    i_mmap_lock_write(mapping);
    remove_inode_hugepages
      lock_page(page);

thread2:
  __unmap_and_move
    trylock_page(page) / lock_page(page)
      remove_migration_ptes
        rmap_walk_file
          i_mmap_lock_read(mapping);

Here's strace output:
<snip>
1196  11:27:16 mmap(NULL, 4194304, PROT_READ|PROT_WRITE, MAP_SHARED|MAP_ANONYMOUS|MAP_HUGETLB, -1, 0) = 0x7f646c400000
1197  11:27:16 set_robust_list(0x7f646d5b0e60, 24) = 0
1197  11:27:16 getppid()                = 1196
1197  11:27:16 move_pages(1196, 1024, [0x7f646c400000, 0x7f646c401000, 0x7f646c402000, 0x7f646c403000, 0x7f646c404000, 0x7f646c405000, 0x7f646c406000, 0x7f646c407000, 0x7f646c408000, 0x7f646c409000, 0x7f646c40a000, 0x7f646c40b000, 0x7f646c40c000, 0x7f646c40d000, 0x7f646c40e000, 0x7f646c40f000, 0x7f646c410000, 0x7f646c411000, 0x7f646c412000, 0x7f646c413000, 0x7f646c414000, 0x7f646c415000, 0x7f646c416000, 0x7f646c417000, 0x7f646c418000, 0x7f646c419000, 0x7f646c41a000, 0x7f646c41b000, 0x7f646c41c000, 0x7f646c41d000, 0x7f646c41e000, 0x7f646c41f000, ...], [0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, ...], [-ENOENT, -ENOENT, -ENOENT, -ENOENT, -ENOENT, -ENOENT, -ENOENT, -ENOENT, -ENOENT, -ENOENT, -ENOENT, -ENOENT, -ENOENT, -ENOENT, -ENOENT, -ENOENT, -ENOENT, -ENOENT, -ENOENT, -ENOENT, -ENOENT, -ENOENT, -ENOENT, -ENOENT, -ENOENT, -ENOENT, -ENOENT, -ENOENT, -ENOENT, -ENOENT, -ENOENT, -ENOENT, ...], MPOL_MF_MOVE_ALL) = 0
1197  11:27:16 move_pages(1196, 1024, [0x7f646c400000, 0x7f646c401000, 0x7f646c402000, 0x7f646c403000, 0x7f646c404000, 0x7f646c405000, 0x7f646c406000, 0x7f646c407000, 0x7f646c408000, 0x7f646c409000, 0x7f646c40a000, 0x7f646c40b000, 0x7f646c40c000, 0x7f646c40d000, 0x7f646c40e000, 0x7f646c40f000, 0x7f646c410000, 0x7f646c411000, 0x7f646c412000, 0x7f646c413000, 0x7f646c414000, 0x7f646c415000, 0x7f646c416000, 0x7f646c417000, 0x7f646c418000, 0x7f646c419000, 0x7f646c41a000, 0x7f646c41b000, 0x7f646c41c000, 0x7f646c41d000, 0x7f646c41e000, 0x7f646c41f000, ...], [1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, ...], [1, -EACCES, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, ...], MPOL_MF_MOVE_ALL) = 0
1197  11:27:16 move_pages(1196, 1024, [0x7f646c400000, 0x7f646c401000, 0x7f646c402000, 0x7f646c403000, 0x7f646c404000, 0x7f646c405000, 0x7f646c406000, 0x7f646c407000, 0x7f646c408000, 0x7f646c409000, 0x7f646c40a000, 0x7f646c40b000, 0x7f646c40c000, 0x7f646c40d000, 0x7f646c40e000, 0x7f646c40f000, 0x7f646c410000, 0x7f646c411000, 0x7f646c412000, 0x7f646c413000, 0x7f646c414000, 0x7f646c415000, 0x7f646c416000, 0x7f646c417000, 0x7f646c418000, 0x7f646c419000, 0x7f646c41a000, 0x7f646c41b000, 0x7f646c41c000, 0x7f646c41d000, 0x7f646c41e000, 0x7f646c41f000, ...], [0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, ...],  <unfinished ...>
1196  11:27:16 munmap(0x7f646c400000, 4194304 <unfinished ...>
<hangs>

Regards,
Jan

[LTP] [bug] problems with migration of huge pages with v4.20-10214-ge1ef035d272e

[Mike Kravetz]

On 1/3/19 9:06 AM, Jan Stancek wrote:
<snip>
>> 1) with LTP move_pages12 (MAP_PRIVATE version of reproducer)
>> Patch below fixes the panic for me.
>> It didn't apply cleanly to latest master, but conflicts were easy to resolve.
>>
>> 2) with MAP_SHARED version of reproducer
>> It still hangs in user-space.
>> v4.19 kernel appears to work fine so I've started a bisect.
> 
> My bisect with MAP_SHARED version arrived at same 2 commits:
>   c86aa7bbfd55 hugetlbfs: Use i_mmap_rwsem to fix page fault/truncate race
>   b43a99900559 hugetlbfs: use i_mmap_rwsem for more pmd sharing synchronization
> 
> Maybe a deadlock between page lock and mapping->i_mmap_rwsem?
> 
> thread1:
>   hugetlbfs_evict_inode
>     i_mmap_lock_write(mapping);
>     remove_inode_hugepages
>       lock_page(page);
> 
> thread2:
>   __unmap_and_move
>     trylock_page(page) / lock_page(page)
>       remove_migration_ptes
>         rmap_walk_file
>           i_mmap_lock_read(mapping);

Thanks Jan!  That is an ABBA deadlock. :(

Commit c86aa7bbfd55 ("Use i_mmap_rwsem to fix page fault/truncate race") is
the patch which causes remove_inode_hugepages to be called with i_mmap_rwsem
held in write mode.  Clearly, i_mmap_rwsem should not be held when calling
remove_inode_hugepages.  If you back out that patch, then the deadlock will
go away.

But, the whole point of that patch is to expand the locking so that
remove_inode_hugepages can not race with a page fault.  If they do race, then
hugetlbfs specific metadata becomes inconsistent.  With some tweaks to
c86aa7bbfd55, I think we could make truncate/page fault races safe.  However,
the issue would still exist for hole punch/page fault races.  We need some
way to prevent page faults while in remove_inode_hugepages.

Andrew, it might be best to revert these patches.  I am not sure if all the
issues with this approach to synchronization can be fixed.  To do so would
likely require more 'special case' conditions to code paths.  The code is
already difficult to understand.  I'd like to step back and take another look
at the best way to fix these problems.  As mentioned before, the issues these
patches address have existed for at least 10 years.  AFAIK, they have not been
seen in real world use cases.  They were discovered via code inspection and
can only be reproduced with highly targeted test programs.  So, waiting for
another release cycle to get a better solution might be the best approach.
I will continue to work this, but if you agree that backing out is the best
approach for now please let me know the process.  Do I simply send a 'revert'
patch to you and the list?

-- 
Mike Kravetz

Re: [bug] problems with migration of huge pages with v4.20-10214-ge1ef035d272e

[Mike Kravetz]

On 1/3/19 9:06 AM, Jan Stancek wrote:
<snip>
>> 1) with LTP move_pages12 (MAP_PRIVATE version of reproducer)
>> Patch below fixes the panic for me.
>> It didn't apply cleanly to latest master, but conflicts were easy to resolve.
>>
>> 2) with MAP_SHARED version of reproducer
>> It still hangs in user-space.
>> v4.19 kernel appears to work fine so I've started a bisect.
> 
> My bisect with MAP_SHARED version arrived at same 2 commits:
>   c86aa7bbfd55 hugetlbfs: Use i_mmap_rwsem to fix page fault/truncate race
>   b43a99900559 hugetlbfs: use i_mmap_rwsem for more pmd sharing synchronization
> 
> Maybe a deadlock between page lock and mapping->i_mmap_rwsem?
> 
> thread1:
>   hugetlbfs_evict_inode
>     i_mmap_lock_write(mapping);
>     remove_inode_hugepages
>       lock_page(page);
> 
> thread2:
>   __unmap_and_move
>     trylock_page(page) / lock_page(page)
>       remove_migration_ptes
>         rmap_walk_file
>           i_mmap_lock_read(mapping);

Thanks Jan!  That is an ABBA deadlock. :(

Commit c86aa7bbfd55 ("Use i_mmap_rwsem to fix page fault/truncate race") is
the patch which causes remove_inode_hugepages to be called with i_mmap_rwsem
held in write mode.  Clearly, i_mmap_rwsem should not be held when calling
remove_inode_hugepages.  If you back out that patch, then the deadlock will
go away.

But, the whole point of that patch is to expand the locking so that
remove_inode_hugepages can not race with a page fault.  If they do race, then
hugetlbfs specific metadata becomes inconsistent.  With some tweaks to
c86aa7bbfd55, I think we could make truncate/page fault races safe.  However,
the issue would still exist for hole punch/page fault races.  We need some
way to prevent page faults while in remove_inode_hugepages.

Andrew, it might be best to revert these patches.  I am not sure if all the
issues with this approach to synchronization can be fixed.  To do so would
likely require more 'special case' conditions to code paths.  The code is
already difficult to understand.  I'd like to step back and take another look
at the best way to fix these problems.  As mentioned before, the issues these
patches address have existed for at least 10 years.  AFAIK, they have not been
seen in real world use cases.  They were discovered via code inspection and
can only be reproduced with highly targeted test programs.  So, waiting for
another release cycle to get a better solution might be the best approach.
I will continue to work this, but if you agree that backing out is the best
approach for now please let me know the process.  Do I simply send a 'revert'
patch to you and the list?

-- 
Mike Kravetz

[LTP] [bug] problems with migration of huge pages with v4.20-10214-ge1ef035d272e

[Andrew Morton]

On Thu, 3 Jan 2019 13:44:20 -0800 Mike Kravetz <mike.kravetz@oracle.com> wrote:

> Do I simply send a 'revert'
> patch to you and the list?

Probably that would be best, to give us a changelog, to make sure that
the correct things are reverted and so we have something which you
tested.

Re: [bug] problems with migration of huge pages with v4.20-10214-ge1ef035d272e

[Andrew Morton]

On Thu, 3 Jan 2019 13:44:20 -0800 Mike Kravetz <mike.kravetz@oracle.com> wrote:

> Do I simply send a 'revert'
> patch to you and the list?

Probably that would be best, to give us a changelog, to make sure that
the correct things are reverted and so we have something which you
tested.