diff mbox series

[v1,01/12] mm/gup: reject FOLL_SPLIT_PMD with hugetlb VMAs

Message ID 20250129115411.2077152-2-david@redhat.com (mailing list archive)
State New
Headers show
Series mm: fixes for device-exclusive entries (hmm) | expand

Commit Message

David Hildenbrand Jan. 29, 2025, 11:53 a.m. UTC 
We only have two FOLL_SPLIT_PMD users. While uprobe refuses hugetlb
early, make_device_exclusive_range() can end up getting called on
hugetlb VMAs.

Right now, this means that with a PMD-sized hugetlb page, we can end
up calling split_huge_pmd(), because pmd_trans_huge() also succeeds
with hugetlb PMDs.

For example, using a modified hmm-test selftest one can trigger:

[  207.017134][T14945] ------------[ cut here ]------------
[  207.018614][T14945] kernel BUG at mm/page_table_check.c:87!
[  207.019716][T14945] Oops: invalid opcode: 0000 [#1] PREEMPT SMP KASAN NOPTI
[  207.021072][T14945] CPU: 3 UID: 0 PID: ...
[  207.023036][T14945] Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 1.16.3-2.fc40 04/01/2014
[  207.024834][T14945] RIP: 0010:page_table_check_clear.part.0+0x488/0x510
[  207.026128][T14945] Code: ...
[  207.029965][T14945] RSP: 0018:ffffc9000cb8f348 EFLAGS: 00010293
[  207.031139][T14945] RAX: 0000000000000000 RBX: 00000000ffffffff RCX: ffffffff8249a0cd
[  207.032649][T14945] RDX: ffff88811e883c80 RSI: ffffffff8249a357 RDI: ffff88811e883c80
[  207.034183][T14945] RBP: ffff888105c0a050 R08: 0000000000000005 R09: 0000000000000000
[  207.035688][T14945] R10: 00000000ffffffff R11: 0000000000000003 R12: 0000000000000001
[  207.037203][T14945] R13: 0000000000000200 R14: 0000000000000001 R15: dffffc0000000000
[  207.038711][T14945] FS:  00007f2783275740(0000) GS:ffff8881f4980000(0000) knlGS:0000000000000000
[  207.040407][T14945] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[  207.041660][T14945] CR2: 00007f2782c00000 CR3: 0000000132356000 CR4: 0000000000750ef0
[  207.043196][T14945] PKRU: 55555554
[  207.043880][T14945] Call Trace:
[  207.044506][T14945]  <TASK>
[  207.045086][T14945]  ? __die+0x51/0x92
[  207.045864][T14945]  ? die+0x29/0x50
[  207.046596][T14945]  ? do_trap+0x250/0x320
[  207.047430][T14945]  ? do_error_trap+0xe7/0x220
[  207.048346][T14945]  ? page_table_check_clear.part.0+0x488/0x510
[  207.049535][T14945]  ? handle_invalid_op+0x34/0x40
[  207.050494][T14945]  ? page_table_check_clear.part.0+0x488/0x510
[  207.051681][T14945]  ? exc_invalid_op+0x2e/0x50
[  207.052589][T14945]  ? asm_exc_invalid_op+0x1a/0x20
[  207.053596][T14945]  ? page_table_check_clear.part.0+0x1fd/0x510
[  207.054790][T14945]  ? page_table_check_clear.part.0+0x487/0x510
[  207.055993][T14945]  ? page_table_check_clear.part.0+0x488/0x510
[  207.057195][T14945]  ? page_table_check_clear.part.0+0x487/0x510
[  207.058384][T14945]  __page_table_check_pmd_clear+0x34b/0x5a0
[  207.059524][T14945]  ? __pfx___page_table_check_pmd_clear+0x10/0x10
[  207.060775][T14945]  ? __pfx___mutex_unlock_slowpath+0x10/0x10
[  207.061940][T14945]  ? __pfx___lock_acquire+0x10/0x10
[  207.062967][T14945]  pmdp_huge_clear_flush+0x279/0x360
[  207.064024][T14945]  split_huge_pmd_locked+0x82b/0x3750
...

Before commit 9cb28da54643 ("mm/gup: handle hugetlb in the generic
follow_page_mask code"), we would have ignored the flag; instead, let's
simply refuse the combination completely in check_vma_flags(): the
caller is likely not prepared to handle any hugetlb folios.

We'll teach make_device_exclusive_range() separately to ignore any hugetlb
folios as a future-proof safety net.

Fixes: 9cb28da54643 ("mm/gup: handle hugetlb in the generic follow_page_mask code")
Cc: <stable@vger.kernel.org>
Signed-off-by: David Hildenbrand <david@redhat.com>
---
 mm/gup.c | 3 +++
 1 file changed, 3 insertions(+)

Comments

John Hubbard Jan. 29, 2025, 9:42 p.m. UTC | #1 
On 1/29/25 3:53 AM, David Hildenbrand wrote:
> We only have two FOLL_SPLIT_PMD users. While uprobe refuses hugetlb
> early, make_device_exclusive_range() can end up getting called on
> hugetlb VMAs.
> 
> Right now, this means that with a PMD-sized hugetlb page, we can end
> up calling split_huge_pmd(), because pmd_trans_huge() also succeeds
> with hugetlb PMDs.
> 
> For example, using a modified hmm-test selftest one can trigger:
> 
> [  207.017134][T14945] ------------[ cut here ]------------
> [  207.018614][T14945] kernel BUG at mm/page_table_check.c:87!
> [  207.019716][T14945] Oops: invalid opcode: 0000 [#1] PREEMPT SMP KASAN NOPTI
> [  207.021072][T14945] CPU: 3 UID: 0 PID: ...
> [  207.023036][T14945] Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 1.16.3-2.fc40 04/01/2014
> [  207.024834][T14945] RIP: 0010:page_table_check_clear.part.0+0x488/0x510
> [  207.026128][T14945] Code: ...
> [  207.029965][T14945] RSP: 0018:ffffc9000cb8f348 EFLAGS: 00010293
> [  207.031139][T14945] RAX: 0000000000000000 RBX: 00000000ffffffff RCX: ffffffff8249a0cd
> [  207.032649][T14945] RDX: ffff88811e883c80 RSI: ffffffff8249a357 RDI: ffff88811e883c80
> [  207.034183][T14945] RBP: ffff888105c0a050 R08: 0000000000000005 R09: 0000000000000000
> [  207.035688][T14945] R10: 00000000ffffffff R11: 0000000000000003 R12: 0000000000000001
> [  207.037203][T14945] R13: 0000000000000200 R14: 0000000000000001 R15: dffffc0000000000
> [  207.038711][T14945] FS:  00007f2783275740(0000) GS:ffff8881f4980000(0000) knlGS:0000000000000000
> [  207.040407][T14945] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [  207.041660][T14945] CR2: 00007f2782c00000 CR3: 0000000132356000 CR4: 0000000000750ef0
> [  207.043196][T14945] PKRU: 55555554
> [  207.043880][T14945] Call Trace:
> [  207.044506][T14945]  <TASK>
> [  207.045086][T14945]  ? __die+0x51/0x92
> [  207.045864][T14945]  ? die+0x29/0x50
> [  207.046596][T14945]  ? do_trap+0x250/0x320
> [  207.047430][T14945]  ? do_error_trap+0xe7/0x220
> [  207.048346][T14945]  ? page_table_check_clear.part.0+0x488/0x510
> [  207.049535][T14945]  ? handle_invalid_op+0x34/0x40
> [  207.050494][T14945]  ? page_table_check_clear.part.0+0x488/0x510
> [  207.051681][T14945]  ? exc_invalid_op+0x2e/0x50
> [  207.052589][T14945]  ? asm_exc_invalid_op+0x1a/0x20
> [  207.053596][T14945]  ? page_table_check_clear.part.0+0x1fd/0x510
> [  207.054790][T14945]  ? page_table_check_clear.part.0+0x487/0x510
> [  207.055993][T14945]  ? page_table_check_clear.part.0+0x488/0x510
> [  207.057195][T14945]  ? page_table_check_clear.part.0+0x487/0x510
> [  207.058384][T14945]  __page_table_check_pmd_clear+0x34b/0x5a0
> [  207.059524][T14945]  ? __pfx___page_table_check_pmd_clear+0x10/0x10
> [  207.060775][T14945]  ? __pfx___mutex_unlock_slowpath+0x10/0x10
> [  207.061940][T14945]  ? __pfx___lock_acquire+0x10/0x10
> [  207.062967][T14945]  pmdp_huge_clear_flush+0x279/0x360
> [  207.064024][T14945]  split_huge_pmd_locked+0x82b/0x3750
> ...
> 
> Before commit 9cb28da54643 ("mm/gup: handle hugetlb in the generic
> follow_page_mask code"), we would have ignored the flag; instead, let's

...and so after that commit (which doesn't touch FOLL_SPLIT_PMD, we no
longer ignore the flag? At a first look at that commit, I don't quite
understand the connection, can you clarify just a bit for me?

> simply refuse the combination completely in check_vma_flags(): the
> caller is likely not prepared to handle any hugetlb folios.

Yes.

> 
> We'll teach make_device_exclusive_range() separately to ignore any hugetlb
> folios as a future-proof safety net.
> 
> Fixes: 9cb28da54643 ("mm/gup: handle hugetlb in the generic follow_page_mask code")
> Cc: <stable@vger.kernel.org>
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>   mm/gup.c | 3 +++
>   1 file changed, 3 insertions(+)
> 
> diff --git a/mm/gup.c b/mm/gup.c
> index 3883b307780e..61e751baf862 100644
> --- a/mm/gup.c
> +++ b/mm/gup.c
> @@ -1283,6 +1283,9 @@ static int check_vma_flags(struct vm_area_struct *vma, unsigned long gup_flags)
>   	if ((gup_flags & FOLL_LONGTERM) && vma_is_fsdax(vma))
>   		return -EOPNOTSUPP;
>   
> +	if ((gup_flags & FOLL_SPLIT_PMD) && is_vm_hugetlb_page(vma))
> +		return -EOPNOTSUPP;
> +

This seems correct by inspection, as one cannot split a hugetlbfs page, so:

Reviewed-by: John Hubbard <jhubbard@nvidia.com>


thanks,
Alistair Popple Jan. 30, 2025, 5:46 a.m. UTC | #2 
On Wed, Jan 29, 2025 at 12:53:59PM +0100, David Hildenbrand wrote:
> We only have two FOLL_SPLIT_PMD users. While uprobe refuses hugetlb
> early, make_device_exclusive_range() can end up getting called on
> hugetlb VMAs.
> 
> Right now, this means that with a PMD-sized hugetlb page, we can end
> up calling split_huge_pmd(), because pmd_trans_huge() also succeeds
> with hugetlb PMDs.
> 
> For example, using a modified hmm-test selftest one can trigger:

I haven't explicitly tested this with nouveau but I can't see anything that
would prevent this being called on a hugetlb vma there either.

Reviewed-by: Alistair Popple <apopple@nvidia.com>

> [  207.017134][T14945] ------------[ cut here ]------------
> [  207.018614][T14945] kernel BUG at mm/page_table_check.c:87!
> [  207.019716][T14945] Oops: invalid opcode: 0000 [#1] PREEMPT SMP KASAN NOPTI
> [  207.021072][T14945] CPU: 3 UID: 0 PID: ...
> [  207.023036][T14945] Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 1.16.3-2.fc40 04/01/2014
> [  207.024834][T14945] RIP: 0010:page_table_check_clear.part.0+0x488/0x510
> [  207.026128][T14945] Code: ...
> [  207.029965][T14945] RSP: 0018:ffffc9000cb8f348 EFLAGS: 00010293
> [  207.031139][T14945] RAX: 0000000000000000 RBX: 00000000ffffffff RCX: ffffffff8249a0cd
> [  207.032649][T14945] RDX: ffff88811e883c80 RSI: ffffffff8249a357 RDI: ffff88811e883c80
> [  207.034183][T14945] RBP: ffff888105c0a050 R08: 0000000000000005 R09: 0000000000000000
> [  207.035688][T14945] R10: 00000000ffffffff R11: 0000000000000003 R12: 0000000000000001
> [  207.037203][T14945] R13: 0000000000000200 R14: 0000000000000001 R15: dffffc0000000000
> [  207.038711][T14945] FS:  00007f2783275740(0000) GS:ffff8881f4980000(0000) knlGS:0000000000000000
> [  207.040407][T14945] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [  207.041660][T14945] CR2: 00007f2782c00000 CR3: 0000000132356000 CR4: 0000000000750ef0
> [  207.043196][T14945] PKRU: 55555554
> [  207.043880][T14945] Call Trace:
> [  207.044506][T14945]  <TASK>
> [  207.045086][T14945]  ? __die+0x51/0x92
> [  207.045864][T14945]  ? die+0x29/0x50
> [  207.046596][T14945]  ? do_trap+0x250/0x320
> [  207.047430][T14945]  ? do_error_trap+0xe7/0x220
> [  207.048346][T14945]  ? page_table_check_clear.part.0+0x488/0x510
> [  207.049535][T14945]  ? handle_invalid_op+0x34/0x40
> [  207.050494][T14945]  ? page_table_check_clear.part.0+0x488/0x510
> [  207.051681][T14945]  ? exc_invalid_op+0x2e/0x50
> [  207.052589][T14945]  ? asm_exc_invalid_op+0x1a/0x20
> [  207.053596][T14945]  ? page_table_check_clear.part.0+0x1fd/0x510
> [  207.054790][T14945]  ? page_table_check_clear.part.0+0x487/0x510
> [  207.055993][T14945]  ? page_table_check_clear.part.0+0x488/0x510
> [  207.057195][T14945]  ? page_table_check_clear.part.0+0x487/0x510
> [  207.058384][T14945]  __page_table_check_pmd_clear+0x34b/0x5a0
> [  207.059524][T14945]  ? __pfx___page_table_check_pmd_clear+0x10/0x10
> [  207.060775][T14945]  ? __pfx___mutex_unlock_slowpath+0x10/0x10
> [  207.061940][T14945]  ? __pfx___lock_acquire+0x10/0x10
> [  207.062967][T14945]  pmdp_huge_clear_flush+0x279/0x360
> [  207.064024][T14945]  split_huge_pmd_locked+0x82b/0x3750
> ...
> 
> Before commit 9cb28da54643 ("mm/gup: handle hugetlb in the generic
> follow_page_mask code"), we would have ignored the flag; instead, let's
> simply refuse the combination completely in check_vma_flags(): the
> caller is likely not prepared to handle any hugetlb folios.
> 
> We'll teach make_device_exclusive_range() separately to ignore any hugetlb
> folios as a future-proof safety net.
> 
> Fixes: 9cb28da54643 ("mm/gup: handle hugetlb in the generic follow_page_mask code")
> Cc: <stable@vger.kernel.org>
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>  mm/gup.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/mm/gup.c b/mm/gup.c
> index 3883b307780e..61e751baf862 100644
> --- a/mm/gup.c
> +++ b/mm/gup.c
> @@ -1283,6 +1283,9 @@ static int check_vma_flags(struct vm_area_struct *vma, unsigned long gup_flags)
>  	if ((gup_flags & FOLL_LONGTERM) && vma_is_fsdax(vma))
>  		return -EOPNOTSUPP;
>  
> +	if ((gup_flags & FOLL_SPLIT_PMD) && is_vm_hugetlb_page(vma))
> +		return -EOPNOTSUPP;
> +
>  	if (vma_is_secretmem(vma))
>  		return -EFAULT;
>  
> -- 
> 2.48.1
>
David Hildenbrand Jan. 30, 2025, 8:56 a.m. UTC | #3 
On 29.01.25 22:42, John Hubbard wrote:
> On 1/29/25 3:53 AM, David Hildenbrand wrote:
>> We only have two FOLL_SPLIT_PMD users. While uprobe refuses hugetlb
>> early, make_device_exclusive_range() can end up getting called on
>> hugetlb VMAs.
>>
>> Right now, this means that with a PMD-sized hugetlb page, we can end
>> up calling split_huge_pmd(), because pmd_trans_huge() also succeeds
>> with hugetlb PMDs.
>>
>> For example, using a modified hmm-test selftest one can trigger:
>>
>> [  207.017134][T14945] ------------[ cut here ]------------
>> [  207.018614][T14945] kernel BUG at mm/page_table_check.c:87!
>> [  207.019716][T14945] Oops: invalid opcode: 0000 [#1] PREEMPT SMP KASAN NOPTI
>> [  207.021072][T14945] CPU: 3 UID: 0 PID: ...
>> [  207.023036][T14945] Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 1.16.3-2.fc40 04/01/2014
>> [  207.024834][T14945] RIP: 0010:page_table_check_clear.part.0+0x488/0x510
>> [  207.026128][T14945] Code: ...
>> [  207.029965][T14945] RSP: 0018:ffffc9000cb8f348 EFLAGS: 00010293
>> [  207.031139][T14945] RAX: 0000000000000000 RBX: 00000000ffffffff RCX: ffffffff8249a0cd
>> [  207.032649][T14945] RDX: ffff88811e883c80 RSI: ffffffff8249a357 RDI: ffff88811e883c80
>> [  207.034183][T14945] RBP: ffff888105c0a050 R08: 0000000000000005 R09: 0000000000000000
>> [  207.035688][T14945] R10: 00000000ffffffff R11: 0000000000000003 R12: 0000000000000001
>> [  207.037203][T14945] R13: 0000000000000200 R14: 0000000000000001 R15: dffffc0000000000
>> [  207.038711][T14945] FS:  00007f2783275740(0000) GS:ffff8881f4980000(0000) knlGS:0000000000000000
>> [  207.040407][T14945] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
>> [  207.041660][T14945] CR2: 00007f2782c00000 CR3: 0000000132356000 CR4: 0000000000750ef0
>> [  207.043196][T14945] PKRU: 55555554
>> [  207.043880][T14945] Call Trace:
>> [  207.044506][T14945]  <TASK>
>> [  207.045086][T14945]  ? __die+0x51/0x92
>> [  207.045864][T14945]  ? die+0x29/0x50
>> [  207.046596][T14945]  ? do_trap+0x250/0x320
>> [  207.047430][T14945]  ? do_error_trap+0xe7/0x220
>> [  207.048346][T14945]  ? page_table_check_clear.part.0+0x488/0x510
>> [  207.049535][T14945]  ? handle_invalid_op+0x34/0x40
>> [  207.050494][T14945]  ? page_table_check_clear.part.0+0x488/0x510
>> [  207.051681][T14945]  ? exc_invalid_op+0x2e/0x50
>> [  207.052589][T14945]  ? asm_exc_invalid_op+0x1a/0x20
>> [  207.053596][T14945]  ? page_table_check_clear.part.0+0x1fd/0x510
>> [  207.054790][T14945]  ? page_table_check_clear.part.0+0x487/0x510
>> [  207.055993][T14945]  ? page_table_check_clear.part.0+0x488/0x510
>> [  207.057195][T14945]  ? page_table_check_clear.part.0+0x487/0x510
>> [  207.058384][T14945]  __page_table_check_pmd_clear+0x34b/0x5a0
>> [  207.059524][T14945]  ? __pfx___page_table_check_pmd_clear+0x10/0x10
>> [  207.060775][T14945]  ? __pfx___mutex_unlock_slowpath+0x10/0x10
>> [  207.061940][T14945]  ? __pfx___lock_acquire+0x10/0x10
>> [  207.062967][T14945]  pmdp_huge_clear_flush+0x279/0x360
>> [  207.064024][T14945]  split_huge_pmd_locked+0x82b/0x3750
>> ...
>>
>> Before commit 9cb28da54643 ("mm/gup: handle hugetlb in the generic
>> follow_page_mask code"), we would have ignored the flag; instead, let's
> 
> ...and so after that commit (which doesn't touch FOLL_SPLIT_PMD, we no
> longer ignore the flag? At a first look at that commit, I don't quite
> understand the connection, can you clarify just a bit for me?

Sure! Before that commit we always went via hugetlb_follow_page_mask(), 
so we never ended up in follow_pmd_mask().

hugetlb_follow_page_mask() didn't check for the flag ("ignored it"), so 
we would not have crashed in GUP.

Thanks!
diff mbox series

Patch

diff --git a/mm/gup.c b/mm/gup.c
index 3883b307780e..61e751baf862 100644
--- a/mm/gup.c
+++ b/mm/gup.c
@@ -1283,6 +1283,9 @@  static int check_vma_flags(struct vm_area_struct *vma, unsigned long gup_flags)
 	if ((gup_flags & FOLL_LONGTERM) && vma_is_fsdax(vma))
 		return -EOPNOTSUPP;
 
+	if ((gup_flags & FOLL_SPLIT_PMD) && is_vm_hugetlb_page(vma))
+		return -EOPNOTSUPP;
+
 	if (vma_is_secretmem(vma))
 		return -EFAULT;