mm: Recheck page table entry with page table lock held
Subsystems (2)
PATCH 1/1 mm: Recheck page table entry with page table lock held
We clear the pte temporarily during read/modify/write update of the pte. If we take a page fault while the pte is cleared, the application can get SIGBUS. One such case is with remap_pfn_range without a backing vm_ops->fault callback. do_fault will return SIGBUS in that case.
Fix this by taking page table lock and rechecking for pte_none.
--- mm/memory.c | 31 +++++++++++++++++++++++++++---- 1 file changed, 27 insertions(+), 4 deletions(-)
mm/memory.c +27 -4
@@ -3745,10 +3745,33 @@ static vm_fault_t do_fault(struct vm_fault *vmf)struct vm_area_struct *vma = vmf->vma;vm_fault_t ret; -/* The VMA was not fully populated on mmap() or missing VM_DONTEXPAND */ -if (!vma->vm_ops->fault) - ret =VM_FAULT_SIGBUS ; -else if (!(vmf->flags &FAULT_FLAG_WRITE )) +/* +* The VMA was not fully populated on mmap() or missing VM_DONTEXPAND +*/ +if (!vma->vm_ops->fault) { + +/* +* pmd entries won't be marked none during a R/M/W cycle. +*/ +if (unlikely (pmd_none (*vmf->pmd))) + ret =VM_FAULT_SIGBUS ; +else { + vmf->ptl =pte_lockptr (vmf->vma->vm_mm, vmf->pmd); +/* +* Make sure this is not a temporary clearing of pte +* by holding ptl and checking again. A R/M/W update +* of pte involves: take ptl, clearing the pte so that +* we don't have concurrent modification by hardware +* followed by an update. +*/ +spin_lock (vmf->ptl); +if (unlikely (pte_none (*vmf->pte))) + ret =VM_FAULT_SIGBUS ; +else + ret =VM_FAULT_NOPAGE ; +spin_unlock (vmf->ptl); + } + }else if (!(vmf->flags &FAULT_FLAG_WRITE )) ret =do_read_fault (vmf);else if (!(vma->vm_flags &VM_SHARED )) ret =do_cow_fault (vmf);
Signature · 1 line
2.17.1
Quoted 4 lines · Aneesh Kumar K.V
We clear the pte temporarily during read/modify/write update of the pte. If we take a page fault while the pte is cleared, the application can get SIGBUS. One such case is with
remap_pfn_rangewithout a backingvm_ops->faultcallback.do_faultwill return SIGBUS in that case.
It would be nice to show the path that clears pte temporarily.
Quoted 33 lines
Fix this by taking page table lock and rechecking for
pte_none.Signed-off-by Aneesh Kumar K.V <[email protected]>--- mm/memory.c | 31 +++++++++++++++++++++++++++---- 1 file changed, 27 insertions(+), 4 deletions(-)mm/memory.c +14
-4@@ -3745,10 +3745,33 @@ static vm_fault_t do_fault(struct vm_fault *vmf)struct vm_area_struct *vma = vmf->vma;vm_fault_t ret; -/* The VMA was not fully populated on mmap() or missing VM_DONTEXPAND */ -if (!vma->vm_ops->fault) - ret =VM_FAULT_SIGBUS ; -else if (!(vmf->flags &FAULT_FLAG_WRITE )) +/* +* The VMA was not fully populated on mmap() or missing VM_DONTEXPAND +*/ +if (!vma->vm_ops->fault) { + +/* +* pmd entries won't be marked none during a R/M/W cycle. +*/ +if (unlikely (pmd_none (*vmf->pmd))) + ret =VM_FAULT_SIGBUS ; +else { + vmf->ptl =pte_lockptr (vmf->vma->vm_mm, vmf->pmd); +/* +* Make sure this is not a temporary clearing of pte
mm/memory.c static vm_fault_t do_fault(struct vm_fault *vmf) · line 3771
Quoted 10 lines
mm/memory.c +10
-0+* by holding ptl and checking again. A R/M/W update +* of pte involves: take ptl, clearing the pte so that +* we don't have concurrent modification by hardware +* followed by an update. +*/ +spin_lock (vmf->ptl); +if (unlikely (pte_none (*vmf->pte))) + ret =VM_FAULT_SIGBUS ; +else + ret =VM_FAULT_NOPAGE ;
We return 0 if we did nothing in fault path.
Signature · 1 line
Kirill A. Shutemov
Quoted 9 lines · Kirill A. Shutemov
Quoted 4 lines · Aneesh Kumar K.V
We clear the pte temporarily during read/modify/write update of the pte. If we take a page fault while the pte is cleared, the application can get SIGBUS. One such case is with
remap_pfn_rangewithout a backingvm_ops->faultcallback.do_faultwill return SIGBUS in that case.It would be nice to show the path that clears pte temporarily.
Quoted 1 line
Fix this by taking page table lock and rechecking for
pte_none.
we do that in the ptep_modify_prot_start/ptep_modify_prot_commit. Also in hugetlb_change_protection. The hugetlb case many not be relevant because that cannot be backed by a vma without vma->vm_ops.
What will hit this will be mprotect of a remap_pfn_range address?
Quoted 36 lines
Quoted 36 lines
Signed-off-by Aneesh Kumar K.V <[email protected]>--- mm/memory.c | 31 +++++++++++++++++++++++++++---- 1 file changed, 27 insertions(+), 4 deletions(-)mm/memory.c +18
-4@@ -3745,10 +3745,33 @@ static vm_fault_t do_fault(struct vm_fault *vmf)struct vm_area_struct *vma = vmf->vma;vm_fault_t ret; -/* The VMA was not fully populated on mmap() or missing VM_DONTEXPAND */ -if (!vma->vm_ops->fault) - ret =VM_FAULT_SIGBUS ; -else if (!(vmf->flags &FAULT_FLAG_WRITE )) +/* +* The VMA was not fully populated on mmap() or missing VM_DONTEXPAND +*/ +if (!vma->vm_ops->fault) { + +/* +* pmd entries won't be marked none during a R/M/W cycle. +*/ +if (unlikely (pmd_none (*vmf->pmd))) + ret =VM_FAULT_SIGBUS ; +else { + vmf->ptl =pte_lockptr (vmf->vma->vm_mm, vmf->pmd); +/* +* Make sure this is not a temporary clearing of pte +* by holding ptl and checking again. A R/M/W update +* of pte involves: take ptl, clearing the pte so that +* we don't have concurrent modification by hardware +* followed by an update.
Quoted 9 lines
mm/memory.c static vm_fault_t do_fault(struct vm_fault *vmf) · line 3771
Quoted 6 lines
mm/memory.c +6
-0+*/ +spin_lock (vmf->ptl); +if (unlikely (pte_none (*vmf->pte))) + ret =VM_FAULT_SIGBUS ; +else + ret =VM_FAULT_NOPAGE ;We return 0 if we did nothing in fault path.
I didn't get that. If we find the pte not none, we return so that we retry the access. Are you suggesting VM_FAULT_NOPAGE is not the right return for that?
-aneesh
Quoted 9 lines · Aneesh Kumar K.V
Quoted 8 lines · Kirill A. Shutemov
Quoted 4 lines · Aneesh Kumar K.V
We clear the pte temporarily during read/modify/write update of the pte. If we take a page fault while the pte is cleared, the application can get SIGBUS. One such case is with
remap_pfn_rangewithout a backingvm_ops->faultcallback.do_faultwill return SIGBUS in that case.It would be nice to show the path that clears pte temporarily.
Quoted 8 lines
Quoted 1 line
Quoted 1 line
Fix this by taking page table lock and rechecking for
pte_none.we do that in the ptep_modify_prot_start/ptep_modify_prot_commit. Also in
hugetlb_change_protection. The hugetlb case many not be relevant because that cannot be backed by a vma withoutvma->vm_ops.What will hit this will be mprotect of a
remap_pfn_rangeaddress?
Sounds right. Please update commit message.
Quoted 46 lines
Quoted 36 lines
Quoted 36 lines
Signed-off-by Aneesh Kumar K.V <[email protected]>--- mm/memory.c | 31 +++++++++++++++++++++++++++---- 1 file changed, 27 insertions(+), 4 deletions(-)mm/memory.c +18
-4@@ -3745,10 +3745,33 @@ static vm_fault_t do_fault(struct vm_fault *vmf)struct vm_area_struct *vma = vmf->vma;vm_fault_t ret; -/* The VMA was not fully populated on mmap() or missing VM_DONTEXPAND */ -if (!vma->vm_ops->fault) - ret =VM_FAULT_SIGBUS ; -else if (!(vmf->flags &FAULT_FLAG_WRITE )) +/* +* The VMA was not fully populated on mmap() or missing VM_DONTEXPAND +*/ +if (!vma->vm_ops->fault) { + +/* +* pmd entries won't be marked none during a R/M/W cycle. +*/ +if (unlikely (pmd_none (*vmf->pmd))) + ret =VM_FAULT_SIGBUS ; +else { + vmf->ptl =pte_lockptr (vmf->vma->vm_mm, vmf->pmd); +/* +* Make sure this is not a temporary clearing of pte +* by holding ptl and checking again. A R/M/W update +* of pte involves: take ptl, clearing the pte so that +* we don't have concurrent modification by hardware +* followed by an update. Quoted 9 lines
mm/memory.c static vm_fault_t do_fault(struct vm_fault *vmf) · line 3771
Quoted 6 lines
mm/memory.c +6
-0+*/ +spin_lock (vmf->ptl); +if (unlikely (pte_none (*vmf->pte))) + ret =VM_FAULT_SIGBUS ; +else + ret =VM_FAULT_NOPAGE ;We return 0 if we did nothing in fault path.
Quoted 3 lines
I didn't get that. If we find the pte not none, we return so that we retry the access. Are you suggesting
VM_FAULT_NOPAGEis not the right return for that?
We usually use VM_FAULT_NOPAGE to indicate that ->fault() installed the pte and we don't need to do anything. We don't touch pte in this page fault.
It doesn't make difference in this particular case, nobody cares upper by stack. Just a nitpick.
Signature · 1 line
Kirill A. Shutemov