[PATCH 0/5] Cleanups and fixup for gup

[PATCH 0/5] Cleanups and fixup for gup

[Miaohe Lin]

Hi all,
This series contains cleanups to remove unneeded variable, useless
BUG_ON and use helper to improve readability. Also we fix a potential
pgmap refcnt leak. More details can be found in the respective
changelogs. Thanks!

Miaohe Lin (5):
  mm: gup: remove set but unused local variable major
  mm: gup: remove unneed local variable orig_refs
  mm: gup: remove useless BUG_ON in __get_user_pages()
  mm: gup: fix potential pgmap refcnt leak in __gup_device_huge()
  mm: gup: use helper PAGE_ALIGNED in populate_vma_page_range()

 mm/gup.c | 16 ++++++----------
 1 file changed, 6 insertions(+), 10 deletions(-)

-- 
2.23.0

[PATCH 1/5] mm: gup: remove set but unused local variable major

[Miaohe Lin]

Since commit a2beb5f1efed ("mm: clean up the last pieces of page fault
accountings"), the local variable major is unused. Remove it.

Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>
---
 mm/gup.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/mm/gup.c b/mm/gup.c
index 77150624f77a..430495cb1b91 100644
--- a/mm/gup.c
+++ b/mm/gup.c
@@ -1276,7 +1276,7 @@ int fixup_user_fault(struct mm_struct *mm,
 		     bool *unlocked)
 {
 	struct vm_area_struct *vma;
-	vm_fault_t ret, major = 0;
+	vm_fault_t ret;
 
 	address = untagged_addr(address);
 
@@ -1296,7 +1296,6 @@ int fixup_user_fault(struct mm_struct *mm,
 		return -EINTR;
 
 	ret = handle_mm_fault(vma, address, fault_flags, NULL);
-	major |= ret & VM_FAULT_MAJOR;
 	if (ret & VM_FAULT_ERROR) {
 		int err = vm_fault_to_errno(ret, 0);
 
-- 
2.23.0

[PATCH 2/5] mm: gup: remove unneed local variable orig_refs

[Miaohe Lin]

Remove unneed local variable orig_refs since refs is unchanged now.

Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>
---
 mm/gup.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/mm/gup.c b/mm/gup.c
index 430495cb1b91..d6056dcab02c 100644
--- a/mm/gup.c
+++ b/mm/gup.c
@@ -117,8 +117,6 @@ __maybe_unused struct page *try_grab_compound_head(struct page *page,
 	if (flags & FOLL_GET)
 		return try_get_compound_head(page, refs);
 	else if (flags & FOLL_PIN) {
-		int orig_refs = refs;
-
 		/*
 		 * Can't do FOLL_LONGTERM + FOLL_PIN gup fast path if not in a
 		 * right zone, so fail and let the caller fall back to the slow
@@ -150,7 +148,7 @@ __maybe_unused struct page *try_grab_compound_head(struct page *page,
 			page_ref_add(page, refs * (GUP_PIN_COUNTING_BIAS - 1));
 
 		mod_node_page_state(page_pgdat(page), NR_FOLL_PIN_ACQUIRED,
-				    orig_refs);
+				    refs);
 
 		return page;
 	}
-- 
2.23.0

[PATCH 3/5] mm: gup: remove useless BUG_ON in __get_user_pages()

[Miaohe Lin]

Indeed, this BUG_ON couldn't catch anything useful. We are sure ret == 0
here because we would already bail out if ret != 0 and ret is untouched
till here.

Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>
---
 mm/gup.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/mm/gup.c b/mm/gup.c
index d6056dcab02c..d7e4507de6b1 100644
--- a/mm/gup.c
+++ b/mm/gup.c
@@ -1149,7 +1149,6 @@ static long __get_user_pages(struct mm_struct *mm,
 					 * We must stop here.
 					 */
 					BUG_ON(gup_flags & FOLL_NOWAIT);
-					BUG_ON(ret != 0);
 					goto out;
 				}
 				continue;
-- 
2.23.0

[PATCH 4/5] mm: gup: fix potential pgmap refcnt leak in __gup_device_huge()

[Miaohe Lin]

When failed to try_grab_page, put_dev_pagemap() is missed. So pgmap
refcnt will leak in this case. Also we remove the check for pgmap
against NULL as it's also checked inside the put_dev_pagemap().

Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>
Fixes: 3faa52c03f44 ("mm/gup: track FOLL_PIN pages")
---
 mm/gup.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/mm/gup.c b/mm/gup.c
index d7e4507de6b1..8c89e614d4aa 100644
--- a/mm/gup.c
+++ b/mm/gup.c
@@ -2253,14 +2253,14 @@ static int __gup_device_huge(unsigned long pfn, unsigned long addr,
 		pages[*nr] = page;
 		if (unlikely(!try_grab_page(page, flags))) {
 			undo_dev_pagemap(nr, nr_start, flags, pages);
+			put_dev_pagemap(pgmap);
 			return 0;
 		}
 		(*nr)++;
 		pfn++;
 	} while (addr += PAGE_SIZE, addr != end);
 
-	if (pgmap)
-		put_dev_pagemap(pgmap);
+	put_dev_pagemap(pgmap);
 	return 1;
 }
 
-- 
2.23.0

[PATCH 5/5] mm: gup: use helper PAGE_ALIGNED in populate_vma_page_range()

[Miaohe Lin]

Use helper PAGE_ALIGNED to check if address is aligned to PAGE_SIZE.
Minor readability improvement.

Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>
---
 mm/gup.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/mm/gup.c b/mm/gup.c
index 8c89e614d4aa..802a3deb50cd 100644
--- a/mm/gup.c
+++ b/mm/gup.c
@@ -1471,8 +1471,8 @@ long populate_vma_page_range(struct vm_area_struct *vma,
 	unsigned long nr_pages = (end - start) / PAGE_SIZE;
 	int gup_flags;
 
-	VM_BUG_ON(start & ~PAGE_MASK);
-	VM_BUG_ON(end   & ~PAGE_MASK);
+	VM_BUG_ON(!PAGE_ALIGNED(start));
+	VM_BUG_ON(!PAGE_ALIGNED(end));
 	VM_BUG_ON_VMA(start < vma->vm_start, vma);
 	VM_BUG_ON_VMA(end   > vma->vm_end, vma);
 	mmap_assert_locked(mm);
-- 
2.23.0

Re: [PATCH 4/5] mm: gup: fix potential pgmap refcnt leak in __gup_device_huge()

[Andrew Morton]

On Sat, 7 Aug 2021 17:36:19 +0800 Miaohe Lin <linmiaohe@huawei.com> wrote:

> When failed to try_grab_page, put_dev_pagemap() is missed. So pgmap
> refcnt will leak in this case. Also we remove the check for pgmap
> against NULL as it's also checked inside the put_dev_pagemap().
> 
> ...
>
> --- a/mm/gup.c
> +++ b/mm/gup.c
> @@ -2253,14 +2253,14 @@ static int __gup_device_huge(unsigned long pfn, unsigned long addr,
>  		pages[*nr] = page;
>  		if (unlikely(!try_grab_page(page, flags))) {
>  			undo_dev_pagemap(nr, nr_start, flags, pages);
> +			put_dev_pagemap(pgmap);
>  			return 0;
>  		}
>  		(*nr)++;
>  		pfn++;
>  	} while (addr += PAGE_SIZE, addr != end);
>  
> -	if (pgmap)
> -		put_dev_pagemap(pgmap);
> +	put_dev_pagemap(pgmap);
>  	return 1;
>  }

We can simplify further, and remove the troublesome multiple return points?

--- a/mm/gup.c~mm-gup-fix-potential-pgmap-refcnt-leak-in-__gup_device_huge-fix
+++ a/mm/gup.c
@@ -2247,14 +2247,13 @@ static int __gup_device_huge(unsigned lo
 		pgmap = get_dev_pagemap(pfn, pgmap);
 		if (unlikely(!pgmap)) {
 			undo_dev_pagemap(nr, nr_start, flags, pages);
-			return 0;
+			break;
 		}
 		SetPageReferenced(page);
 		pages[*nr] = page;
 		if (unlikely(!try_grab_page(page, flags))) {
 			undo_dev_pagemap(nr, nr_start, flags, pages);
-			put_dev_pagemap(pgmap);
-			return 0;
+			break;
 		}
 		(*nr)++;
 		pfn++;
_

Re: [PATCH 4/5] mm: gup: fix potential pgmap refcnt leak in __gup_device_huge()

[Andrew Morton]

On Sat, 7 Aug 2021 11:41:12 -0700 Andrew Morton <akpm@linux-foundation.org> wrote:

> We can simplify further, and remove the troublesome multiple return points?
> 

oops.

--- a/mm/gup.c~mm-gup-fix-potential-pgmap-refcnt-leak-in-__gup_device_huge-fix-fix
+++ a/mm/gup.c
@@ -2240,6 +2240,7 @@ static int __gup_device_huge(unsigned lo
 {
 	int nr_start = *nr;
 	struct dev_pagemap *pgmap = NULL;
+	int ret = 1;
 
 	do {
 		struct page *page = pfn_to_page(pfn);
@@ -2247,12 +2248,14 @@ static int __gup_device_huge(unsigned lo
 		pgmap = get_dev_pagemap(pfn, pgmap);
 		if (unlikely(!pgmap)) {
 			undo_dev_pagemap(nr, nr_start, flags, pages);
+			ret = 0;
 			break;
 		}
 		SetPageReferenced(page);
 		pages[*nr] = page;
 		if (unlikely(!try_grab_page(page, flags))) {
 			undo_dev_pagemap(nr, nr_start, flags, pages);
+			ret = 0;
 			break;
 		}
 		(*nr)++;
@@ -2260,7 +2263,7 @@ static int __gup_device_huge(unsigned lo
 	} while (addr += PAGE_SIZE, addr != end);
 
 	put_dev_pagemap(pgmap);
-	return 1;
+	return ret;
 }
 
 static int __gup_device_huge_pmd(pmd_t orig, pmd_t *pmdp, unsigned long addr,

Not sure if it's worth bothering, really...

Re: [PATCH 0/5] Cleanups and fixup for gup

[John Hubbard]

On 8/7/21 2:36 AM, Miaohe Lin wrote:
> Hi all,
> This series contains cleanups to remove unneeded variable, useless
> BUG_ON and use helper to improve readability. Also we fix a potential
> pgmap refcnt leak. More details can be found in the respective
> changelogs. Thanks!
> 
> Miaohe Lin (5):
>    mm: gup: remove set but unused local variable major
>    mm: gup: remove unneed local variable orig_refs
>    mm: gup: remove useless BUG_ON in __get_user_pages()
>    mm: gup: fix potential pgmap refcnt leak in __gup_device_huge()
>    mm: gup: use helper PAGE_ALIGNED in populate_vma_page_range()
> 
>   mm/gup.c | 16 ++++++----------
>   1 file changed, 6 insertions(+), 10 deletions(-)
> 

For the series:

Reviewed-by: John Hubbard <jhubbard@nvidia.com>


thanks,
-- 
John Hubbard
NVIDIA

Re: [PATCH 4/5] mm: gup: fix potential pgmap refcnt leak in __gup_device_huge()

[John Hubbard]

On 8/7/21 11:45 AM, Andrew Morton wrote:
> On Sat, 7 Aug 2021 11:41:12 -0700 Andrew Morton <akpm@linux-foundation.org> wrote:
> 
>> We can simplify further, and remove the troublesome multiple return points?
>>
> 
> oops.

I sent a reviewed by to the "+" fixup email, but just realized that that did not
hit the main
  mailing list. So:



For the end result of these stacked fixes to this file:



Reviewed-by: John Hubbard <jhubbard@nvidia.com>



thanks,

-- 

John Hubbard

NVIDIA
> 
> --- a/mm/gup.c~mm-gup-fix-potential-pgmap-refcnt-leak-in-__gup_device_huge-fix-fix
> +++ a/mm/gup.c
> @@ -2240,6 +2240,7 @@ static int __gup_device_huge(unsigned lo
>   {
>   	int nr_start = *nr;
>   	struct dev_pagemap *pgmap = NULL;
> +	int ret = 1;
>   
>   	do {
>   		struct page *page = pfn_to_page(pfn);
> @@ -2247,12 +2248,14 @@ static int __gup_device_huge(unsigned lo
>   		pgmap = get_dev_pagemap(pfn, pgmap);
>   		if (unlikely(!pgmap)) {
>   			undo_dev_pagemap(nr, nr_start, flags, pages);
> +			ret = 0;
>   			break;
>   		}
>   		SetPageReferenced(page);
>   		pages[*nr] = page;
>   		if (unlikely(!try_grab_page(page, flags))) {
>   			undo_dev_pagemap(nr, nr_start, flags, pages);
> +			ret = 0;
>   			break;
>   		}
>   		(*nr)++;
> @@ -2260,7 +2263,7 @@ static int __gup_device_huge(unsigned lo
>   	} while (addr += PAGE_SIZE, addr != end);
>   
>   	put_dev_pagemap(pgmap);
> -	return 1;
> +	return ret;
>   }
>   
>   static int __gup_device_huge_pmd(pmd_t orig, pmd_t *pmdp, unsigned long addr,
> 
> Not sure if it's worth bothering, really...
> 

Re: [PATCH 1/5] mm: gup: remove set but unused local variable major

[David Hildenbrand]

On 07.08.21 11:36, Miaohe Lin wrote:
> Since commit a2beb5f1efed ("mm: clean up the last pieces of page fault
> accountings"), the local variable major is unused. Remove it.
> 
> Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>
> ---
>   mm/gup.c | 3 +--
>   1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/mm/gup.c b/mm/gup.c
> index 77150624f77a..430495cb1b91 100644
> --- a/mm/gup.c
> +++ b/mm/gup.c
> @@ -1276,7 +1276,7 @@ int fixup_user_fault(struct mm_struct *mm,
>   		     bool *unlocked)
>   {
>   	struct vm_area_struct *vma;
> -	vm_fault_t ret, major = 0;
> +	vm_fault_t ret;
>   
>   	address = untagged_addr(address);
>   
> @@ -1296,7 +1296,6 @@ int fixup_user_fault(struct mm_struct *mm,
>   		return -EINTR;
>   
>   	ret = handle_mm_fault(vma, address, fault_flags, NULL);
> -	major |= ret & VM_FAULT_MAJOR;
>   	if (ret & VM_FAULT_ERROR) {
>   		int err = vm_fault_to_errno(ret, 0);
>   
> 

Reviewed-by: David Hildenbrand <david@redhat.com>

-- 
Thanks,

David / dhildenb

Re: [PATCH 2/5] mm: gup: remove unneed local variable orig_refs

[David Hildenbrand]

On 07.08.21 11:36, Miaohe Lin wrote:
> Remove unneed local variable orig_refs since refs is unchanged now.
> 
> Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>
> ---
>   mm/gup.c | 4 +---
>   1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/mm/gup.c b/mm/gup.c
> index 430495cb1b91..d6056dcab02c 100644
> --- a/mm/gup.c
> +++ b/mm/gup.c
> @@ -117,8 +117,6 @@ __maybe_unused struct page *try_grab_compound_head(struct page *page,
>   	if (flags & FOLL_GET)
>   		return try_get_compound_head(page, refs);
>   	else if (flags & FOLL_PIN) {
> -		int orig_refs = refs;
> -
>   		/*
>   		 * Can't do FOLL_LONGTERM + FOLL_PIN gup fast path if not in a
>   		 * right zone, so fail and let the caller fall back to the slow
> @@ -150,7 +148,7 @@ __maybe_unused struct page *try_grab_compound_head(struct page *page,
>   			page_ref_add(page, refs * (GUP_PIN_COUNTING_BIAS - 1));
>   
>   		mod_node_page_state(page_pgdat(page), NR_FOLL_PIN_ACQUIRED,
> -				    orig_refs);
> +				    refs);
>   
>   		return page;
>   	}
> 

Reviewed-by: David Hildenbrand <david@redhat.com>

-- 
Thanks,

David / dhildenb

Re: [PATCH 3/5] mm: gup: remove useless BUG_ON in __get_user_pages()

[David Hildenbrand]

On 07.08.21 11:36, Miaohe Lin wrote:
> Indeed, this BUG_ON couldn't catch anything useful. We are sure ret == 0
> here because we would already bail out if ret != 0 and ret is untouched
> till here.
> 
> Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>
> ---
>   mm/gup.c | 1 -
>   1 file changed, 1 deletion(-)
> 
> diff --git a/mm/gup.c b/mm/gup.c
> index d6056dcab02c..d7e4507de6b1 100644
> --- a/mm/gup.c
> +++ b/mm/gup.c
> @@ -1149,7 +1149,6 @@ static long __get_user_pages(struct mm_struct *mm,
>   					 * We must stop here.
>   					 */
>   					BUG_ON(gup_flags & FOLL_NOWAIT);
> -					BUG_ON(ret != 0);
>   					goto out;
>   				}
>   				continue;
> 

Reviewed-by: David Hildenbrand <david@redhat.com>

-- 
Thanks,

David / dhildenb

Re: [PATCH 5/5] mm: gup: use helper PAGE_ALIGNED in populate_vma_page_range()

[David Hildenbrand]

On 07.08.21 11:36, Miaohe Lin wrote:
> Use helper PAGE_ALIGNED to check if address is aligned to PAGE_SIZE.
> Minor readability improvement.
> 
> Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>
> ---
>   mm/gup.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/mm/gup.c b/mm/gup.c
> index 8c89e614d4aa..802a3deb50cd 100644
> --- a/mm/gup.c
> +++ b/mm/gup.c
> @@ -1471,8 +1471,8 @@ long populate_vma_page_range(struct vm_area_struct *vma,
>   	unsigned long nr_pages = (end - start) / PAGE_SIZE;
>   	int gup_flags;
>   
> -	VM_BUG_ON(start & ~PAGE_MASK);
> -	VM_BUG_ON(end   & ~PAGE_MASK);
> +	VM_BUG_ON(!PAGE_ALIGNED(start));
> +	VM_BUG_ON(!PAGE_ALIGNED(end));
>   	VM_BUG_ON_VMA(start < vma->vm_start, vma);
>   	VM_BUG_ON_VMA(end   > vma->vm_end, vma);
>   	mmap_assert_locked(mm);
> 

Making it look more like faultin_vma_page_range(), nice :)

Reviewed-by: David Hildenbrand <david@redhat.com>

-- 
Thanks,

David / dhildenb

Re: [PATCH 0/5] Cleanups and fixup for gup

[Claudio Imbrenda]

On Sat, 7 Aug 2021 17:36:15 +0800
Miaohe Lin <linmiaohe@huawei.com> wrote:

> Hi all,
> This series contains cleanups to remove unneeded variable, useless
> BUG_ON and use helper to improve readability. Also we fix a potential
> pgmap refcnt leak. More details can be found in the respective
> changelogs. Thanks!
> 
> Miaohe Lin (5):
>   mm: gup: remove set but unused local variable major
>   mm: gup: remove unneed local variable orig_refs
>   mm: gup: remove useless BUG_ON in __get_user_pages()
>   mm: gup: fix potential pgmap refcnt leak in __gup_device_huge()
>   mm: gup: use helper PAGE_ALIGNED in populate_vma_page_range()
> 
>  mm/gup.c | 16 ++++++----------
>  1 file changed, 6 insertions(+), 10 deletions(-)
> 

Whole series (including the fixup for the return value):

Reviewed-by: Claudio Imbrenda <imbrenda@linux.ibm.com>