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
