linux-kernel - Re: mlockall(MCL_CURRENT) blocking infinitely

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 linux-cve-announce 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]

Message-ID: <20191106145608.ucvuwsuyijvkxz22@macbook-pro-91.dhcp.thefacebook.com>
Date:   Wed, 6 Nov 2019 09:56:09 -0500
From:   Josef Bacik <josef@...icpanda.com>
To:     snazy@...zy.de
Cc:     Jan Kara <jack@...e.cz>, Johannes Weiner <hannes@...xchg.org>,
        Vlastimil Babka <vbabka@...e.cz>,
        Michal Hocko <mhocko@...nel.org>,
        Josef Bacik <josef@...icpanda.com>,
        "Kirill A. Shutemov" <kirill@...temov.name>,
        Randy Dunlap <rdunlap@...radead.org>,
        linux-kernel@...r.kernel.org, Linux MM <linux-mm@...ck.org>,
        Andrew Morton <akpm@...ux-foundation.org>,
        "Potyra, Stefan" <Stefan.Potyra@...ktrobit.com>
Subject: Re: mlockall(MCL_CURRENT) blocking infinitely

On Wed, Nov 06, 2019 at 02:45:43PM +0100, Robert Stupp wrote:
> On Wed, 2019-11-06 at 13:03 +0100, Jan Kara wrote:
> > On Tue 05-11-19 13:22:11, Johannes Weiner wrote:
> > > What I don't quite understand yet is why the fault path doesn't
> > > make
> > > progress eventually. We must drop the mmap_sem without changing the
> > > state in any way. How can we keep looping on the same page?
> >
> > That may be a slight suboptimality with Josef's patches. If the page
> > is marked as PageReadahead, we always drop mmap_sem if we can and
> > start
> > readahead without checking whether that makes sense or not in
> > do_async_mmap_readahead(). OTOH page_cache_async_readahead() then
> > clears
> > PageReadahead so the only way how I can see we could loop like this
> > is when
> > file->ra->ra_pages is 0. Not sure if that's what's happening through.
> > We'd
> > need to find which of the paths in filemap_fault() calls
> > maybe_unlock_mmap_for_io() to tell more.
> 
> Yes, ra_pages==0
> Attached the dmesg + smaps outputs
> 
> 

Ah ok I see what's happening, __get_user_pages() returns 0 if we get an EBUSY
from faultin_page, and then __mm_populate does nend = nstart + ret * PAGE_SIZE,
which just leaves us where we are.

We need to handle the non-blocking and the locking separately in __mm_populate
so we know what's going on.  Jan's fix for the readahead thing is definitely
valid as well, but this will keep us from looping forever in other retry cases.

diff --git a/mm/gup.c b/mm/gup.c
index 8f236a335ae9..ac625805d569 100644
--- a/mm/gup.c
+++ b/mm/gup.c
@@ -1237,6 +1237,7 @@ int __mm_populate(unsigned long start, unsigned long len, int ignore_errors)
 	unsigned long end, nstart, nend;
 	struct vm_area_struct *vma = NULL;
 	int locked = 0;
+	int nonblocking = 1;
 	long ret = 0;
 
 	end = start + len;
@@ -1268,7 +1269,7 @@ int __mm_populate(unsigned long start, unsigned long len, int ignore_errors)
 		 * double checks the vma flags, so that it won't mlock pages
 		 * if the vma was already munlocked.
 		 */
-		ret = populate_vma_page_range(vma, nstart, nend, &locked);
+		ret = populate_vma_page_range(vma, nstart, nend, &nonblocking);
 		if (ret < 0) {
 			if (ignore_errors) {
 				ret = 0;
@@ -1276,6 +1277,14 @@ int __mm_populate(unsigned long start, unsigned long len, int ignore_errors)
 			}
 			break;
 		}
+
+		/*
+		 * We dropped the mmap_sem, so we need to re-lock, and the next
+		 * loop around we won't drop because nonblocking is now 0.
+		 */
+		if (!nonblocking)
+			locked = 0;
+
 		nend = nstart + ret * PAGE_SIZE;
 		ret = 0;
 	}