lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:   Thu, 23 Jun 2022 17:47:12 -0300
From:   Jason Gunthorpe <jgg@...dia.com>
To:     Alex Williamson <alex.williamson@...hat.com>,
        Alistair Popple <apopple@...dia.com>
Cc:     David Hildenbrand <david@...hat.com>, akpm@...ux-foundation.org,
        minchan@...nel.org, linux-mm@...ck.org,
        linux-kernel@...r.kernel.org, paulmck@...nel.org,
        jhubbard@...dia.com, joaodias@...gle.com
Subject: Re: [PATCH] mm: Re-allow pinning of zero pfns

On Thu, Jun 23, 2022 at 02:21:39PM -0600, Alex Williamson wrote:

> check_and_migrate_movable_pages() perpetually returns zero.  I believe
> this is because folio_is_pinnable() previously returned true, and now
> returns false.

Indeed, it is a bug that check_and_migrate_movable_pages() returns
0 when it didn't do anything. It should return an error code.

Hum.. Alistair, maybe you should look at this as well, I'm struggling
alot to understand how it is safe to drop the reference on the page
but hold a pointer to it on the movable_page_list - sure it was
isolated - but why does that mean it won't be concurrently unmapped
and freed?

Anyhow, it looks like the problem is the tortured logic in this
function, what do you think about this:

diff --git a/mm/gup.c b/mm/gup.c
index 5512644076246d..2ffcb3f4ff4a7b 100644
--- a/mm/gup.c
+++ b/mm/gup.c
@@ -1879,10 +1879,15 @@ static long check_and_migrate_movable_pages(unsigned long nr_pages,
 					    unsigned int gup_flags)
 {
 	unsigned long isolation_error_count = 0, i;
+	struct migration_target_control mtc = {
+		.nid = NUMA_NO_NODE,
+		.gfp_mask = GFP_USER | __GFP_NOWARN,
+	};
 	struct folio *prev_folio = NULL;
 	LIST_HEAD(movable_page_list);
 	bool drain_allow = true;
-	int ret = 0;
+	int not_migrated;
+	int ret;
 
 	for (i = 0; i < nr_pages; i++) {
 		struct folio *folio = page_folio(pages[i]);
@@ -1919,16 +1924,13 @@ static long check_and_migrate_movable_pages(unsigned long nr_pages,
 				    folio_nr_pages(folio));
 	}
 
-	if (!list_empty(&movable_page_list) || isolation_error_count)
-		goto unpin_pages;
-
 	/*
 	 * If list is empty, and no isolation errors, means that all pages are
-	 * in the correct zone.
+	 * in the correct zone, nothing to do.
 	 */
-	return nr_pages;
+	if (list_empty(&movable_page_list) && !isolation_error_count)
+		return nr_pages;
 
-unpin_pages:
 	if (gup_flags & FOLL_PIN) {
 		unpin_user_pages(pages, nr_pages);
 	} else {
@@ -1936,20 +1938,22 @@ static long check_and_migrate_movable_pages(unsigned long nr_pages,
 			put_page(pages[i]);
 	}
 
-	if (!list_empty(&movable_page_list)) {
-		struct migration_target_control mtc = {
-			.nid = NUMA_NO_NODE,
-			.gfp_mask = GFP_USER | __GFP_NOWARN,
-		};
+	if (isolation_error_count) {
+		ret = -EINVAL;
+		goto err_putback;
+	}
 
-		ret = migrate_pages(&movable_page_list, alloc_migration_target,
-				    NULL, (unsigned long)&mtc, MIGRATE_SYNC,
-				    MR_LONGTERM_PIN, NULL);
-		if (ret > 0) /* number of pages not migrated */
-			ret = -ENOMEM;
+	not_migrated = migrate_pages(&movable_page_list, alloc_migration_target,
+				     NULL, (unsigned long)&mtc, MIGRATE_SYNC,
+				     MR_LONGTERM_PIN, NULL);
+	if (not_migrated > 0) {
+		ret = -ENOMEM;
+		goto err_putback;
 	}
+	return 0;
 
-	if (ret && !list_empty(&movable_page_list))
+err_putback:
+	if (!list_empty(&movable_page_list))
 		putback_movable_pages(&movable_page_list);
 	return ret;
 }

> If I generate an errno here, QEMU reports failing on the pc.rom memory
> region at 0xc0000.  Thanks,

Ah, a ROM region that is all zero'd makes some sense why it has gone
unnoticed as a bug.

Jason

Powered by blists - more mailing lists