[071/212] mm: gup: fix potential pgmap refcnt leak in __gup_device_huge()

Commit Message

Andrew Morton Sept. 2, 2021, 9:53 p.m. UTC

From: Miaohe Lin <[e-mail]>
Subject: mm: gup: fix potential pgmap refcnt leak in __gup_device_huge()

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().

[[e-mail]: simplify, cleanup]
[[e-mail]: fix return value]
Link: https://lkml.kernel.org/r/[e-mail]
Signed-off-by: Miaohe Lin <[e-mail]>
Fixes: 3faa52c03f44 ("mm/gup: track FOLL_PIN pages")
Reviewed-by: John Hubbard <[e-mail]>
Reviewed-by: Claudio Imbrenda <[e-mail]>
Cc: Jan Kara <[e-mail]>
Cc: Kirill A. Shutemov <[e-mail]>
Signed-off-by: Andrew Morton <[e-mail]>
---

 mm/gup.c |   12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

Comments

Linus Torvalds Sept. 3, 2021, 4:35 p.m. UTC | #1

On Thu, Sep 2, 2021 at 2:53 PM Andrew Morton <[e-mail]> wrote:
>
> +       int ret = 1;
..
> +                       ret = 0;
> +                       break;
..
> +                       ret = 0;
> +                       break;
..
> +       return ret;

Hmm. I think "ret" is unnecessary, and this could just have been

        return addr == end;

at the end to check that we did it all.

No?

                 Linus

John Hubbard Sept. 3, 2021, 5:55 p.m. UTC | #2

On 9/3/21 9:35 AM, Linus Torvalds wrote:
> On Thu, Sep 2, 2021 at 2:53 PM Andrew Morton <[e-mail]> wrote:
>>
>> +       int ret = 1;
> ..
>> +                       ret = 0;
>> +                       break;
> ..
>> +                       ret = 0;
>> +                       break;
> ..
>> +       return ret;
> 
> Hmm. I think "ret" is unnecessary, and this could just have been
> 
>          return addr == end;
> 
> at the end to check that we did it all.
> 
> No?
> 

Yes, definitely.

So, to be extra clear even though this is tiny and trivial: this incremental
change, on top of the current patch, looks good to me:

diff --git a/mm/gup.c b/mm/gup.c
index 7a406d79bd2e..74142c621556 100644
--- a/mm/gup.c
+++ b/mm/gup.c
@@ -2241,7 +2241,6 @@ static int __gup_device_huge(unsigned long pfn, unsigned long addr,
  {
  	int nr_start = *nr;
  	struct dev_pagemap *pgmap = NULL;
-	int ret = 1;

  	do {
  		struct page *page = pfn_to_page(pfn);
@@ -2249,14 +2248,12 @@ static int __gup_device_huge(unsigned long pfn, unsigned long addr,
  		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)++;
@@ -2264,7 +2261,7 @@ static int __gup_device_huge(unsigned long pfn, unsigned long addr,
  	} while (addr += PAGE_SIZE, addr != end);

  	put_dev_pagemap(pgmap);
-	return ret;
+	return addr == end;
  }

  static int __gup_device_huge_pmd(pmd_t orig, pmd_t *pmdp, unsigned long addr,



thanks,

Linus Torvalds Sept. 3, 2021, 7:01 p.m. UTC | #3

On Fri, Sep 3, 2021 at 10:55 AM John Hubbard <[e-mail]> wrote:
>
> On 9/3/21 9:35 AM, Linus Torvalds wrote:
> >
> > Hmm. I think "ret" is unnecessary, and this could just have been
> >
> >          return addr == end;
> >
> > at the end to check that we did it all.
>
> Yes, definitely.
>
> So, to be extra clear even though this is tiny and trivial: this incremental
> change, on top of the current patch, looks good to me:

Ack.  I did the merge of Andrew's series as-is, to avoid even more
disruption (I already skipped two sub-series in there), but wouldn't
mind this simplification.

That said, it's not like it's a big deal, just me reacting to that
patch being larger than strictly needed. It's not like the extra "ret"
thing is confusing or wrong per se.

So I'll leave it to others if they want to pursue this.

                Linus

Patch

--- a/mm/gup.c~mm-gup-fix-potential-pgmap-refcnt-leak-in-__gup_device_huge
+++ 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,21 +2248,22 @@  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;
+			ret = 0;
+			break;
 		}
 		SetPageReferenced(page);
 		pages[*nr] = page;
 		if (unlikely(!try_grab_page(page, flags))) {
 			undo_dev_pagemap(nr, nr_start, flags, pages);
-			return 0;
+			ret = 0;
+			break;
 		}
 		(*nr)++;
 		pfn++;
 	} while (addr += PAGE_SIZE, addr != end);
 
-	if (pgmap)
-		put_dev_pagemap(pgmap);
-	return 1;
+	put_dev_pagemap(pgmap);
+	return ret;
 }
 
 static int __gup_device_huge_pmd(pmd_t orig, pmd_t *pmdp, unsigned long addr,