Re: [PATCH v1] mm: drop NULL return check of pte_offset_map_lock()

[Vlastimil Babka]

On 05/18/2017 08:42 AM, Naoya Horiguchi wrote:
> pte_offset_map_lock() finds and takes ptl, and returns pte.
> But some callers return without unlocking the ptl when pte == NULL,
> which seems weird.
> 
> Git history said that !pte check in change_pte_range() was introduced in
> commit 1ad9f620c3a2 ("mm: numa: recheck for transhuge pages under lock
> during protection changes") and still remains after commit 175ad4f1e7a2
> ("mm: mprotect: use pmd_trans_unstable instead of taking the pmd_lock")
> which partially reverts 1ad9f620c3a2. So I think that it's just dead code.
> 
> Many other caller of pte_offset_map_lock() never check NULL return, so
> let's do likewise.
> 
> Signed-off-by: Naoya Horiguchi <n-horiguchi@xxxxxxxxxxxxx>

Acked-by: Vlastimil Babka <vbabka@xxxxxxx>

> ---
>  mm/memory.c   | 2 --
>  mm/mprotect.c | 2 --
>  2 files changed, 4 deletions(-)
> 
> diff --git v4.11-rc6-mmotm-2017-04-13-14-50/mm/memory.c v4.11-rc6-mmotm-2017-04-13-14-50_patched/mm/memory.c
> index 8ae6700..c17fad1d 100644
> --- v4.11-rc6-mmotm-2017-04-13-14-50/mm/memory.c
> +++ v4.11-rc6-mmotm-2017-04-13-14-50_patched/mm/memory.c
> @@ -4040,8 +4040,6 @@ static int __follow_pte_pmd(struct mm_struct *mm, unsigned long address,
>  		goto out;
>  
>  	ptep = pte_offset_map_lock(mm, pmd, address, ptlp);
> -	if (!ptep)
> -		goto out;
>  	if (!pte_present(*ptep))
>  		goto unlock;
>  	*ptepp = ptep;
> diff --git v4.11-rc6-mmotm-2017-04-13-14-50/mm/mprotect.c v4.11-rc6-mmotm-2017-04-13-14-50_patched/mm/mprotect.c
> index 8fd010f..d60a1ee 100644
> --- v4.11-rc6-mmotm-2017-04-13-14-50/mm/mprotect.c
> +++ v4.11-rc6-mmotm-2017-04-13-14-50_patched/mm/mprotect.c
> @@ -58,8 +58,6 @@ static unsigned long change_pte_range(struct vm_area_struct *vma, pmd_t *pmd,
>  	 * reading.
>  	 */
>  	pte = pte_offset_map_lock(vma->vm_mm, pmd, addr, &ptl);
> -	if (!pte)
> -		return 0;
>  
>  	/* Get target node for single threaded private VMAs */
>  	if (prot_numa && !(vma->vm_flags & VM_SHARED) &&
>