[PATCH] mm: Update mask post pxd_clear_bad()

[PATCH] mm: Update mask post pxd_clear_bad()

[Dev Jain]

Since pxd_clear_bad() is an operation changing the state of the page tables,
we should call arch_sync_kernel_mappings() post this.

Fixes: e80d3909be42 ("mm: track page table modifications in __apply_to_page_range()")
Cc: <stable@vger.kernel.org>
Signed-off-by: Dev Jain <dev.jain@arm.com>
---
 mm/memory.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/mm/memory.c b/mm/memory.c
index 78c7ee62795e..9a4a8c710be0 100644
--- a/mm/memory.c
+++ b/mm/memory.c
@@ -2987,6 +2987,7 @@ static int apply_to_pmd_range(struct mm_struct *mm, pud_t *pud,
 			if (!create)
 				continue;
 			pmd_clear_bad(pmd);
+			*mask = PGTBL_PMD_MODIFIED;
 		}
 		err = apply_to_pte_range(mm, pmd, addr, next,
 					 fn, data, create, mask);
@@ -3023,6 +3024,7 @@ static int apply_to_pud_range(struct mm_struct *mm, p4d_t *p4d,
 			if (!create)
 				continue;
 			pud_clear_bad(pud);
+			*mask = PGTBL_PUD_MODIFIED;
 		}
 		err = apply_to_pmd_range(mm, pud, addr, next,
 					 fn, data, create, mask);
@@ -3059,6 +3061,7 @@ static int apply_to_p4d_range(struct mm_struct *mm, pgd_t *pgd,
 			if (!create)
 				continue;
 			p4d_clear_bad(p4d);
+			*mask = PGTBL_P4D_MODIFIED;
 		}
 		err = apply_to_pud_range(mm, p4d, addr, next,
 					 fn, data, create, mask);
@@ -3095,6 +3098,7 @@ static int __apply_to_page_range(struct mm_struct *mm, unsigned long addr,
 			if (!create)
 				continue;
 			pgd_clear_bad(pgd);
+			mask = PGTBL_PGD_MODIFIED;
 		}
 		err = apply_to_p4d_range(mm, pgd, addr, next,
 					 fn, data, create, &mask);
-- 
2.30.2

Re: [PATCH] mm: Update mask post pxd_clear_bad()

[Dev Jain]

On 13/03/25 11:44 pm, Dev Jain wrote:
> Since pxd_clear_bad() is an operation changing the state of the page tables,
> we should call arch_sync_kernel_mappings() post this.
> 
> Fixes: e80d3909be42 ("mm: track page table modifications in __apply_to_page_range()")
> Cc: <stable@vger.kernel.org>
> Signed-off-by: Dev Jain <dev.jain@arm.com>
> ---
>   mm/memory.c | 4 ++++
>   1 file changed, 4 insertions(+)
> 
> diff --git a/mm/memory.c b/mm/memory.c
> index 78c7ee62795e..9a4a8c710be0 100644
> --- a/mm/memory.c
> +++ b/mm/memory.c
> @@ -2987,6 +2987,7 @@ static int apply_to_pmd_range(struct mm_struct *mm, pud_t *pud,
>   			if (!create)
>   				continue;
>   			pmd_clear_bad(pmd);
> +			*mask = PGTBL_PMD_MODIFIED;

Oh well, I guess these should have been *mask |= PGTBL_PMD_MODIFIED.


>   		}
>   		err = apply_to_pte_range(mm, pmd, addr, next,
>   					 fn, data, create, mask);
> @@ -3023,6 +3024,7 @@ static int apply_to_pud_range(struct mm_struct *mm, p4d_t *p4d,
>   			if (!create)
>   				continue;
>   			pud_clear_bad(pud);
> +			*mask = PGTBL_PUD_MODIFIED;
>   		}
>   		err = apply_to_pmd_range(mm, pud, addr, next,
>   					 fn, data, create, mask);
> @@ -3059,6 +3061,7 @@ static int apply_to_p4d_range(struct mm_struct *mm, pgd_t *pgd,
>   			if (!create)
>   				continue;
>   			p4d_clear_bad(p4d);
> +			*mask = PGTBL_P4D_MODIFIED;
>   		}
>   		err = apply_to_pud_range(mm, p4d, addr, next,
>   					 fn, data, create, mask);
> @@ -3095,6 +3098,7 @@ static int __apply_to_page_range(struct mm_struct *mm, unsigned long addr,
>   			if (!create)
>   				continue;
>   			pgd_clear_bad(pgd);
> +			mask = PGTBL_PGD_MODIFIED;
>   		}
>   		err = apply_to_p4d_range(mm, pgd, addr, next,
>   					 fn, data, create, &mask);

Re: [PATCH] mm: Update mask post pxd_clear_bad()

[Matthew Wilcox]

On Thu, Mar 13, 2025 at 11:44:14PM +0530, Dev Jain wrote:
> Since pxd_clear_bad() is an operation changing the state of the page tables,
> we should call arch_sync_kernel_mappings() post this.

Could you explain why?  What effect does not calling
arch_sync_kernel_mappings() have in this case?

Re: [PATCH] mm: Update mask post pxd_clear_bad()

[Dev Jain]

On 14/03/25 2:14 am, Matthew Wilcox wrote:
> On Thu, Mar 13, 2025 at 11:44:14PM +0530, Dev Jain wrote:
>> Since pxd_clear_bad() is an operation changing the state of the page tables,
>> we should call arch_sync_kernel_mappings() post this.
> 
> Could you explain why?  What effect does not calling
> arch_sync_kernel_mappings() have in this case?

Apologies, I again forgot to explain the userspace effect.
I just found this by code inspection, using the logic the fixes commit 
uses: we should sync when we change the pxd.

The question I have been pondering on is, what is the use of the 
pxd_bad() macros, when do we actually hit a bad state, and why don't we 
just trigger a BUG when we hit pxd_bad()?

Re: [PATCH] mm: Update mask post pxd_clear_bad()

[Yeo Reum Yun]

Hi Dev,

> > Since pxd_clear_bad() is an operation changing the state of the page tables,
> > we should call arch_sync_kernel_mappings() post this.
> >
> > Fixes: e80d3909be42 ("mm: track page table modifications in __apply_to_page_range()")
> > Cc: <stable@vger.kernel.org>
> > Signed-off-by: Dev Jain <dev.jain@arm.com>
> > ---
> >   mm/memory.c | 4 ++++
> >   1 file changed, 4 insertions(+)
> >
> > diff --git a/mm/memory.c b/mm/memory.c
> > index 78c7ee62795e..9a4a8c710be0 100644
> > --- a/mm/memory.c
> > +++ b/mm/memory.c
> > @@ -2987,6 +2987,7 @@ static int apply_to_pmd_range(struct mm_struct *mm, pud_t *pud,
> >                       if (!create)
> >                               continue;
> >                       pmd_clear_bad(pmd);
> > +                     *mask = PGTBL_PMD_MODIFIED;
>
> Oh well, I guess these should have been *mask |= PGTBL_PMD_MODIFIED.
>
>
> >               }
> >               err = apply_to_pte_range(mm, pmd, addr, next,
> >                                        fn, data, create, mask);
> > @@ -3023,6 +3024,7 @@ static int apply_to_pud_range(struct mm_struct *mm, p4d_t *p4d,
> >                       if (!create)
> >                               continue;
> >                       pud_clear_bad(pud);
> > +                     *mask = PGTBL_PUD_MODIFIED;
> >               }
> >               err = apply_to_pmd_range(mm, pud, addr, next,
> >                                        fn, data, create, mask);
> > @@ -3059,6 +3061,7 @@ static int apply_to_p4d_range(struct mm_struct *mm, pgd_t *pgd,
> >                       if (!create)
> >                               continue;
> >                       p4d_clear_bad(p4d);
> > +                     *mask = PGTBL_P4D_MODIFIED;
> >               }
> >               err = apply_to_pud_range(mm, p4d, addr, next,
> >                                        fn, data, create, mask);
> > @@ -3095,6 +3098,7 @@ static int __apply_to_page_range(struct mm_struct *mm, unsigned long addr,
> >                       if (!create)
> >                               continue;
> >                       pgd_clear_bad(pgd);
> +                     mask = PGTBL_PGD_MODIFIED;
> >               }
> >               err = apply_to_p4d_range(mm, pgd, addr, next,
> >                                        fn, data, create, &mask);

I don't think this wouldn't need.
the pXd_clear_bad() is only called at creation of each level of page table,
and when it clear, the following, apply_to_pXd_range() function would be set
the make properly via pXd_alloc() and apply_to_pte_range().

Thanks.

Re: [PATCH] mm: Update mask post pxd_clear_bad()

[Dev Jain]

On 14/03/25 7:57 pm, Yeo Reum Yun wrote:
> Hi Dev,
> 
>>> Since pxd_clear_bad() is an operation changing the state of the page tables,
>>> we should call arch_sync_kernel_mappings() post this.
>>>
>>> Fixes: e80d3909be42 ("mm: track page table modifications in __apply_to_page_range()")
>>> Cc: <stable@vger.kernel.org>
>>> Signed-off-by: Dev Jain <dev.jain@arm.com>
>>> ---
>>>    mm/memory.c | 4 ++++
>>>    1 file changed, 4 insertions(+)
>>>
>>> diff --git a/mm/memory.c b/mm/memory.c
>>> index 78c7ee62795e..9a4a8c710be0 100644
>>> --- a/mm/memory.c
>>> +++ b/mm/memory.c
>>> @@ -2987,6 +2987,7 @@ static int apply_to_pmd_range(struct mm_struct *mm, pud_t *pud,
>>>                        if (!create)
>>>                                continue;
>>>                        pmd_clear_bad(pmd);
>>> +                     *mask = PGTBL_PMD_MODIFIED;
>>
>> Oh well, I guess these should have been *mask |= PGTBL_PMD_MODIFIED.
>>
>>
>>>                }
>>>                err = apply_to_pte_range(mm, pmd, addr, next,
>>>                                         fn, data, create, mask);
>>> @@ -3023,6 +3024,7 @@ static int apply_to_pud_range(struct mm_struct *mm, p4d_t *p4d,
>>>                        if (!create)
>>>                                continue;
>>>                        pud_clear_bad(pud);
>>> +                     *mask = PGTBL_PUD_MODIFIED;
>>>                }
>>>                err = apply_to_pmd_range(mm, pud, addr, next,
>>>                                         fn, data, create, mask);
>>> @@ -3059,6 +3061,7 @@ static int apply_to_p4d_range(struct mm_struct *mm, pgd_t *pgd,
>>>                        if (!create)
>>>                                continue;
>>>                        p4d_clear_bad(p4d);
>>> +                     *mask = PGTBL_P4D_MODIFIED;
>>>                }
>>>                err = apply_to_pud_range(mm, p4d, addr, next,
>>>                                         fn, data, create, mask);
>>> @@ -3095,6 +3098,7 @@ static int __apply_to_page_range(struct mm_struct *mm, unsigned long addr,
>>>                        if (!create)
>>>                                continue;
>>>                        pgd_clear_bad(pgd);
>> +                     mask = PGTBL_PGD_MODIFIED;
>>>                }
>>>                err = apply_to_p4d_range(mm, pgd, addr, next,
>>>                                         fn, data, create, &mask);
> 
> I don't think this wouldn't need.
> the pXd_clear_bad() is only called at creation of each level of page table,
> and when it clear, the following, apply_to_pXd_range() function would be set
> the make properly via pXd_alloc() and apply_to_pte_range().

Makes sense. But pxd_clear_bad() gets called in case of !pxd_none(), so 
while creating, why would the page containing the page table not be 
none? I believe it should be cleared already?

> 
> Thanks.
> 
>