Re: kernel BUG in munlock_vma_pages_range

[Vlastimil Babka]

On 12/13/2013 09:49 AM, Bob Liu wrote:
> On 12/13/2013 05:05 AM, Sasha Levin wrote:
>> On 12/12/2013 07:41 AM, Vlastimil Babka wrote:
>>> On 12/12/2013 06:03 AM, Bob Liu wrote:
>>>>
>>>> On 12/12/2013 11:16 AM, Sasha Levin wrote:
>>>>> On 12/11/2013 05:59 PM, Vlastimil Babka wrote:
>>>>>> On 12/09/2013 09:26 PM, Sasha Levin wrote:
>>>>>>> On 12/09/2013 12:12 PM, Vlastimil Babka wrote:
>>>>>>>> On 12/09/2013 06:05 PM, Sasha Levin wrote:
>>>>>>>>> On 12/09/2013 04:34 AM, Vlastimil Babka wrote:
>>>>>>>>>> Hello, I will look at it, thanks.
>>>>>>>>>> Do you have specific reproduction instructions?
>>>>>>>>>
>>>>>>>>> Not really, the fuzzer hit it once and I've been unable to trigger
>>>>>>>>> it again. Looking at
>>>>>>>>> the piece of code involved it might have had something to do with
>>>>>>>>> hugetlbfs, so I'll crank
>>>>>>>>> up testing on that part.
>>>>>>>>
>>>>>>>> Thanks. Do you have trinity log and the .config file? I'm currently
>>>>>>>> unable to even boot linux-next
>>>>>>>> with my config/setup due to a GPF.
>>>>>>>> Looking at code I wouldn't expect that it could encounter a tail
>>>>>>>> page, without first encountering a
>>>>>>>> head page and skipping the whole huge page. At least in THP case, as
>>>>>>>> TLB pages should be split when
>>>>>>>> a vma is split. As for hugetlbfs, it should be skipped for
>>>>>>>> mlock/munlock operations completely. One
>>>>>>>> of these assumptions is probably failing here...
>>>>>>>
>>>>>>> If it helps, I've added a dump_page() in case we hit a tail page
>>>>>>> there and got:
>>>>>>>
>>>>>>> [  980.172299] page:ffffea003e5e8040 count:0 mapcount:1
>>>>>>> mapping:          (null) index:0
>>>>>>> x0
>>>>>>> [  980.173412] page flags: 0x2fffff80008000(tail)
>>>>>>>
>>>>>>> I can also add anything else in there to get other debug output if
>>>>>>> you think of something else useful.
>>>>>>
>>>>>> Please try the following. Thanks in advance.
>>>>>
>>>>> [  428.499889] page:ffffea003e5c0040 count:0 mapcount:4
>>>>> mapping:          (null) index:0x0
>>>>> [  428.499889] page flags: 0x2fffff80008000(tail)
>>>>> [  428.499889] start=140117131923456 pfn=16347137
>>>>> orig_start=140117130543104 page_increm
>>>>> =1 vm_start=140117130543104 vm_end=140117134688256 vm_flags=135266419
>>>>> [  428.499889] first_page pfn=16347136
>>>>> [  428.499889] page:ffffea003e5c0000 count:204 mapcount:44
>>>>> mapping:ffff880fb5c466c1 inde
>>>>> x:0x7f6f8fe00
>>>>> [  428.499889] page flags:
>>>>> 0x2fffff80084068(uptodate|lru|active|head|swapbacked)
>>>>
>>>>    From this print, it looks like the page is still a huge page.
>>>> One situation I guess is a huge page which isn't PageMlocked and passed
>>>> to munlock_vma_page(). I'm not sure whether this will happen.
>>>
>>> Yes that's quite likely the case. It's not illegal to happen I would say.
>>>
>>>> Please take a try this patch.
>>>
>>> I've made a simpler version that does away with the ugly page_mask
>>> thing completely.
>>> Please try that as well. Thanks.
>>>
>>> Also when working on this I think I found another potential but much
>>> rare problem
>>> when munlock_vma_page races with a THP split. That would however
>>> manifest such that
>>> part of the former tail pages would stay PageMlocked. But that still
>>> needs more thought.
>>> The bug at hand should however be fixed by this patch.
>>
>> Yup, this patch seems to fix the issue previously reported.
>>
>> However, I'll piggyback another thing that popped up now that the vm
>> could run for a while which
>> also seems to be caused by the original patch. It looks like a pretty
>> straightforward deadlock, but

Sigh, put one down, patch it around... :)

> Looks like put_page() in __munlock_pagevec() need to get the
> zone->lru_lock which is already held when entering __munlock_pagevec().

I've come to the same conclusion, however:

> How about fix like this?

That unfortunately removes most of the purpose of this function which was to avoid repeated locking. 

Please try this patch.

-------8<-------
From: Vlastimil Babka <vbabka@xxxxxxx>
Date: Fri, 13 Dec 2013 10:03:25 +0100
Subject: [PATCH] Deadlock in __munlock_pagevec candidate fix

---
 mm/mlock.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/mm/mlock.c b/mm/mlock.c
index a34dfdc..c97273e 100644
--- a/mm/mlock.c
+++ b/mm/mlock.c
@@ -281,10 +281,12 @@ static void __munlock_pagevec(struct pagevec *pvec, struct zone *zone)
 {
 	int i;
 	int nr = pagevec_count(pvec);
-	int delta_munlocked = -nr;
+	int delta_munlocked;
 	struct pagevec pvec_putback;
 	int pgrescued = 0;
 
+	pagevec_init(&pvec_putback, 0);
+
 	/* Phase 1: page isolation */
 	spin_lock_irq(&zone->lru_lock);
 	for (i = 0; i < nr; i++) {
@@ -313,16 +315,22 @@ skip_munlock:
 			/*
 			 * We won't be munlocking this page in the next phase
 			 * but we still need to release the follow_page_mask()
-			 * pin.
+			 * pin. We cannot do it under lru_lock however. If it's
+			 * the last pin, __page_cache_release would deadlock.
 			 */
+			pagevec_add(&pvec_putback, pvec->pages[i]);
 			pvec->pages[i] = NULL;
-			put_page(page);
-			delta_munlocked++;
 		}
 	}
+	delta_munlocked = -nr + pagevec_count(&pvec_putback);
 	__mod_zone_page_state(zone, NR_MLOCK, delta_munlocked);
 	spin_unlock_irq(&zone->lru_lock);
 
+	/* Now we can release pins of pages that we are not munlocking */
+	for (i = 0; i < pagevec_count(&pvec_putback); i++) {
+		put_page(pvec_putback.pages[i]);
+	}
+
 	/* Phase 2: page munlock */
 	pagevec_init(&pvec_putback, 0);
 	for (i = 0; i < nr; i++) {
-- 
1.8.4


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/