[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <cc9d692a-846d-4ae4-af4b-c8de8b724df6@redhat.com>
Date: Fri, 18 Oct 2024 09:47:25 +0200
From: David Hildenbrand <david@...hat.com>
To: John Hubbard <jhubbard@...dia.com>,
Andrew Morton <akpm@...ux-foundation.org>
Cc: LKML <linux-kernel@...r.kernel.org>, linux-mm@...ck.org,
Alistair Popple <apopple@...dia.com>, Shigeru Yoshida <syoshida@...hat.com>,
Jason Gunthorpe <jgg@...dia.com>, Minchan Kim <minchan@...nel.org>,
Pasha Tatashin <pasha.tatashin@...een.com>
Subject: Re: [PATCH v2 1/2] mm/gup: stop leaking pinned pages in low memory
conditions
On 18.10.24 03:17, John Hubbard wrote:
> If a driver tries to call any of the pin_user_pages*(FOLL_LONGTERM)
> family of functions, and requests "too many" pages, then the call will
> erroneously leave pages pinned. This is visible in user space as an
> actual memory leak.
>
> Repro is trivial: just make enough pin_user_pages(FOLL_LONGTERM) calls
> to exhaust memory.
>
> The root cause of the problem is this sequence, within
> __gup_longterm_locked():
>
> __get_user_pages_locked()
> rc = check_and_migrate_movable_pages()
>
> ...which gets retried in a loop. The loop error handling is incomplete,
> clearly due to a somewhat unusual and complicated tri-state error API.
> But anyway, if -ENOMEM, or in fact, any unexpected error is returned
> from check_and_migrate_movable_pages(), then __gup_longterm_locked()
> happily returns the error, while leaving the pages pinned.
Sorry for another comment, I am taking my time to look into the code again in more detail ...
migrate_longterm_unpinnable_folios() will always unpin all pages: no matter which error it returns.
a) If it returns -EAGAIN, it unpinned all folios
b) If it returns any error it first calls unpin_folios().
So shouldn't the fix just be in check_and_migrate_movable_pages()?
diff --git a/mm/gup.c b/mm/gup.c
index a82890b46a36..81fc8314e687 100644
--- a/mm/gup.c
+++ b/mm/gup.c
@@ -2403,8 +2403,9 @@ static int migrate_longterm_unpinnable_folios(
* -EAGAIN. The caller should re-pin the entire range with FOLL_PIN and then
* call this routine again.
*
- * If an error other than -EAGAIN occurs, this indicates a migration failure.
- * The caller should give up, and propagate the error back up the call stack.
+ * If an error occurs, all folios are unpinned. If an error other than
+ * -EAGAIN occurs, this indicates a migration failure. The caller should give
+ * up, and propagate the error back up the call stack.
*
* If everything is OK and all folios in the range are allowed to be pinned,
* then this routine leaves all folios pinned and returns zero for success.
@@ -2437,8 +2438,10 @@ static long check_and_migrate_movable_pages(unsigned long nr_pages,
long i, ret;
folios = kmalloc_array(nr_pages, sizeof(*folios), GFP_KERNEL);
- if (!folios)
+ if (!folios) {
+ unpin_user_pages(pages, nr_pages);
return -ENOMEM;
+ }
for (i = 0; i < nr_pages; i++)
folios[i] = page_folio(pages[i]);
Then, check_and_migrate_movable_pages() will never return with an error and
having folios pinned.
If check_and_migrate_movable_pages() -> check_and_migrate_movable_folios()
returns "0", all folios remain pinned an no harm is done.
Consequently, I think patch #2 is not really required, because it doesn't
perform the temporary allocation that could fail with -ENOMEM.
Sorry for taking a closer look only now ...
--
Cheers,
David / dhildenb
Powered by blists - more mailing lists