| Message ID | 20210807093620.21347-5-linmiaohe@huawei.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Cleanups and fixup for gup | expand |
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++;
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...
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,
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; }
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(-)