Re: [PATCH 2/5] mm: Clear vmf->pte after pte_unmap_same() returns
From: David Hildenbrand <hidden>
Date: 2021-09-02 07:20:16
Also in:
lkml
On 01.09.21 22:56, Peter Xu wrote:
quoted hunk ↗ jump to hunk
pte_unmap_same() will always unmap the pte pointer. After the unmap, vmf->pte will not be valid any more, we should clear it. It was safe only because no one is accessing vmf->pte after pte_unmap_same() returns, since the only caller of pte_unmap_same() (so far) is do_swap_page(), where vmf->pte will in most cases be overwritten very soon. Directly pass in vmf into pte_unmap_same() and then we can also avoid the long parameter list too, which should be a nice cleanup. Reviewed-by: Miaohe Lin <linmiaohe@huawei.com> Signed-off-by: Peter Xu <peterx@redhat.com> --- mm/memory.c | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-)diff --git a/mm/memory.c b/mm/memory.c index 25fc46e87214..204141e8a53d 100644 --- a/mm/memory.c +++ b/mm/memory.c@@ -2724,19 +2724,20 @@ EXPORT_SYMBOL_GPL(apply_to_existing_page_range); * proceeding (but do_wp_page is only called after already making such a check; * and do_anonymous_page can safely check later on). */ -static inline int pte_unmap_same(struct mm_struct *mm, pmd_t *pmd, - pte_t *page_table, pte_t orig_pte) +static inline int pte_unmap_same(struct vm_fault *vmf) { int same = 1; #if defined(CONFIG_SMP) || defined(CONFIG_PREEMPTION) if (sizeof(pte_t) > sizeof(unsigned long)) { - spinlock_t *ptl = pte_lockptr(mm, pmd); + spinlock_t *ptl = pte_lockptr(vmf->vma->vm_mm, vmf->pmd); spin_lock(ptl); - same = pte_same(*page_table, orig_pte); + same = pte_same(*vmf->pte, vmf->orig_pte); spin_unlock(ptl); } #endif - pte_unmap(page_table); + pte_unmap(vmf->pte); + /* After unmap of pte, the pointer is invalid now - clear it. */
I'd just drop the comment, it's what we do in similar code.
quoted hunk ↗ jump to hunk
+ vmf->pte = NULL; return same; }@@ -3487,7 +3488,7 @@ vm_fault_t do_swap_page(struct vm_fault *vmf) vm_fault_t ret = 0; void *shadow = NULL; - if (!pte_unmap_same(vma->vm_mm, vmf->pmd, vmf->pte, vmf->orig_pte)) + if (!pte_unmap_same(vmf)) goto out;
Funny, I prototyped something similar yesterday. I did it via same = pte_lock_same(vma->vm_mm, vmf->pmd, vmf->pte, vmf->orig_pte); pte_unmap(vmf->pte); vmf->pte = NULL; if (!same) goto out; To just move handling to the caller. But this also looks fine, whatever you prefer. Reviewed-by: David Hildenbrand <redacted> -- Thanks, David / dhildenb