From: Pu Lehui <pulehui@huawei.com>
When executing move_ptes, the new_pte must be NULL, otherwise it will be
overwritten by the old_pte, and cause the abnormal new_pte to be leaked.
In order to make this problem to be more explicit, let's add
WARN_ON_ONCE when new_pte is not NULL.
Suggested-by: Oleg Nesterov <oleg@redhat.com>
Signed-off-by: Pu Lehui <pulehui@huawei.com>
---
mm/mremap.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/mm/mremap.c b/mm/mremap.c
index 83e359754961..4e2491f8c2ce 100644
--- a/mm/mremap.c
+++ b/mm/mremap.c
@@ -237,6 +237,8 @@ static int move_ptes(struct pagetable_move_control *pmc,
for (; old_addr < old_end; old_pte++, old_addr += PAGE_SIZE,
new_pte++, new_addr += PAGE_SIZE) {
+ WARN_ON_ONCE(!pte_none(*new_pte));
+
if (pte_none(ptep_get(old_pte)))
continue;
--
2.34.1
On Thu, May 29, 2025 at 03:56:48PM +0000, Pu Lehui wrote:
> From: Pu Lehui <pulehui@huawei.com>
>
> When executing move_ptes, the new_pte must be NULL, otherwise it will be
> overwritten by the old_pte, and cause the abnormal new_pte to be leaked.
> In order to make this problem to be more explicit, let's add
> WARN_ON_ONCE when new_pte is not NULL.
>
> Suggested-by: Oleg Nesterov <oleg@redhat.com>
> Signed-off-by: Pu Lehui <pulehui@huawei.com>
Reviewed-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
(both this and the amended version :)
> ---
> mm/mremap.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/mm/mremap.c b/mm/mremap.c
> index 83e359754961..4e2491f8c2ce 100644
> --- a/mm/mremap.c
> +++ b/mm/mremap.c
> @@ -237,6 +237,8 @@ static int move_ptes(struct pagetable_move_control *pmc,
>
> for (; old_addr < old_end; old_pte++, old_addr += PAGE_SIZE,
> new_pte++, new_addr += PAGE_SIZE) {
> + WARN_ON_ONCE(!pte_none(*new_pte));
> +
I mean, we really really should not ever be seeing a mapped PTE here, so I think
a WARN_ON_ONCE() is fine.
We unmap anything ahead of time, and only I think this uprobe breakpoint
installation would ever cause this to be the case.
We can make this a VM_WARN_ON_ONCE() too I suppose, just in case there's
something we're not thinking of, but I'd say at some point we'd want to change
it to a WARN_ON_ONCE().
> if (pte_none(ptep_get(old_pte)))
> continue;
>
> --
> 2.34.1
>
On 05/30, Lorenzo Stoakes wrote:
>
> > --- a/mm/mremap.c
> > +++ b/mm/mremap.c
> > @@ -237,6 +237,8 @@ static int move_ptes(struct pagetable_move_control *pmc,
> >
> > for (; old_addr < old_end; old_pte++, old_addr += PAGE_SIZE,
> > new_pte++, new_addr += PAGE_SIZE) {
> > + WARN_ON_ONCE(!pte_none(*new_pte));
> > +
>
> I mean, we really really should not ever be seeing a mapped PTE here, so I think
> a WARN_ON_ONCE() is fine.
>
> We unmap anything ahead of time, and only I think this uprobe breakpoint
> installation would ever cause this to be the case.
>
> We can make this a VM_WARN_ON_ONCE() too I suppose, just in case there's
> something we're not thinking of, but I'd say at some point we'd want to change
> it to a WARN_ON_ONCE().
Note also that move_normal_pmd/move_normal_pud use WARN_ON_ONCE(!xxx_none(...)),
not VM_WARN_ON_ONCE().
Oleg.
On Thu, 29 May 2025 15:56:48 +0000 Pu Lehui <pulehui@huaweicloud.com> wrote:
> From: Pu Lehui <pulehui@huawei.com>
>
> When executing move_ptes, the new_pte must be NULL, otherwise it will be
> overwritten by the old_pte, and cause the abnormal new_pte to be leaked.
> In order to make this problem to be more explicit, let's add
> WARN_ON_ONCE when new_pte is not NULL.
>
> ...
>
> --- a/mm/mremap.c
> +++ b/mm/mremap.c
> @@ -237,6 +237,8 @@ static int move_ptes(struct pagetable_move_control *pmc,
>
> for (; old_addr < old_end; old_pte++, old_addr += PAGE_SIZE,
> new_pte++, new_addr += PAGE_SIZE) {
> + WARN_ON_ONCE(!pte_none(*new_pte));
> +
> if (pte_none(ptep_get(old_pte)))
> continue;
>
We now have no expectation that this will trigger, yes? It's a sanity
check that patch [1/4] is working? Perhaps VM_WARN_ON_ONCE() would be
more appropriate. And maybe even a comment:
/* temporary, remove this one day */
On 2025/5/30 3:19, Andrew Morton wrote:
> On Thu, 29 May 2025 15:56:48 +0000 Pu Lehui <pulehui@huaweicloud.com> wrote:
>
>> From: Pu Lehui <pulehui@huawei.com>
>>
>> When executing move_ptes, the new_pte must be NULL, otherwise it will be
>> overwritten by the old_pte, and cause the abnormal new_pte to be leaked.
>> In order to make this problem to be more explicit, let's add
>> WARN_ON_ONCE when new_pte is not NULL.
>>
>> ...
>>
>> --- a/mm/mremap.c
>> +++ b/mm/mremap.c
>> @@ -237,6 +237,8 @@ static int move_ptes(struct pagetable_move_control *pmc,
>>
>> for (; old_addr < old_end; old_pte++, old_addr += PAGE_SIZE,
>> new_pte++, new_addr += PAGE_SIZE) {
>> + WARN_ON_ONCE(!pte_none(*new_pte));
>> +
>> if (pte_none(ptep_get(old_pte)))
>> continue;
>>
>
> We now have no expectation that this will trigger, yes? It's a sanity
Hi Andrew,
This can sanitize abnormal new_pte. It is expected that uprobe would not
come in later, but others, uncertain🤔? So it will be a good alert. And
after patch 1 it will not trigger WARNING.
> check that patch [1/4] is working? Perhaps VM_WARN_ON_ONCE() would be
Agree, should I respin one more?
> more appropriate. And maybe even a comment:
>
> /* temporary, remove this one day */
>
On Fri, 30 May 2025 09:24:54 +0800 Pu Lehui <pulehui@huaweicloud.com> wrote:
> > check that patch [1/4] is working? Perhaps VM_WARN_ON_ONCE() would be
>
> Agree, should I respin one more?
That's OK, I added this:
--- a/mm/mremap.c~mm-expose-abnormal-new_pte-during-move_ptes-fix
+++ a/mm/mremap.c
@@ -237,7 +237,7 @@ static int move_ptes(struct pagetable_mo
for (; old_addr < old_end; old_pte++, old_addr += PAGE_SIZE,
new_pte++, new_addr += PAGE_SIZE) {
- WARN_ON_ONCE(!pte_none(*new_pte));
+ VM_WARN_ON_ONCE(!pte_none(*new_pte));
if (pte_none(ptep_get(old_pte)))
continue;
_
© 2016 - 2026 Red Hat, Inc.