| Message ID | 20250313181414.78512-1-dev.jain@arm.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | mm: Update mask post pxd_clear_bad() | expand |
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);
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?
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()?
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.
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);
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(+)