linux-kernel - Re: [Question] Mlocked count will not be decreased

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: <b41f2c9a-7e74-529f-2ec1-3d9ae369dcb5@suse.cz>
Date:   Wed, 24 May 2017 15:16:03 +0200
From:   Vlastimil Babka <vbabka@...e.cz>
To:     Xishi Qiu <qiuxishi@...wei.com>
Cc:     Yisheng Xie <xieyisheng1@...wei.com>,
        Kefeng Wang <wangkefeng.wang@...wei.com>, linux-mm@...ck.org,
        linux-kernel@...r.kernel.org, zhongjiang <zhongjiang@...wei.com>
Subject: Re: [Question] Mlocked count will not be decreased

On 05/24/2017 02:10 PM, Xishi Qiu wrote:
> On 2017/5/24 19:52, Vlastimil Babka wrote:
> 
>> On 05/24/2017 01:38 PM, Xishi Qiu wrote:
>>>>
>>>> Race condition with what? Who else would isolate our pages?
>>>>
>>>
>>> Hi Vlastimil,
>>>
>>> I find the root cause, if the page was not cached on the current cpu,
>>> lru_add_drain() will not push it to LRU. So we should handle fail
>>> case in mlock_vma_page().
>>
>> Yeah that would explain it.
>>
>>> follow_page_pte()
>>> 		...
>>> 		if (page->mapping && trylock_page(page)) {
>>> 			lru_add_drain();  /* push cached pages to LRU */
>>> 			/*
>>> 			 * Because we lock page here, and migration is
>>> 			 * blocked by the pte's page reference, and we
>>> 			 * know the page is still mapped, we don't even
>>> 			 * need to check for file-cache page truncation.
>>> 			 */
>>> 			mlock_vma_page(page);
>>> 			unlock_page(page);
>>> 		}
>>> 		...
>>>
>>> I think we should add yisheng's patch, also we should add the following change.
>>> I think it is better than use lru_add_drain_all().
>>
>> I agree about yisheng's fix (but v2 didn't address my comments). I don't
>> think we should add the hunk below, as that deviates from the rest of
>> the design.
> 
> Hi Vlastimil,
> 
> The rest of the design is that mlock should always success here, right?

The rest of the design allows a temporary disconnect between mlocked
flag and being placed on unevictable lru.

> If we don't handle the fail case, the page will be in anon/file lru list
> later when call __pagevec_lru_add(), but NR_MLOCK increased,
> this is wrong, right?

It's not wrong, the page cannot get evicted even if on wrong lru, so
effectively it's already mlocked. We would be underaccounting NR_MLOCK.

> Thanks,
> Xishi Qiu
> 
>>
>> Thanks,
>> Vlastimil
>>
>>> diff --git a/mm/mlock.c b/mm/mlock.c
>>> index 3d3ee6c..ca2aeb9 100644
>>> --- a/mm/mlock.c
>>> +++ b/mm/mlock.c
>>> @@ -88,6 +88,11 @@ void mlock_vma_page(struct page *page)
>>>  		count_vm_event(UNEVICTABLE_PGMLOCKED);
>>>  		if (!isolate_lru_page(page))
>>>  			putback_lru_page(page);
>>> +		else {
>>> +			ClearPageMlocked(page);
>>> +			mod_zone_page_state(page_zone(page), NR_MLOCK,
>>> +					-hpage_nr_pages(page));
>>> +		}
>>>  	}
>>>  }
>>>
>>> Thanks,
>>> Xishi Qiu
>>>
>>
>>
>> .
>>
> 
> 
> 
> --
> To unsubscribe, send a message with 'unsubscribe linux-mm' in
> the body to majordomo@...ck.org.  For more info on Linux MM,
> see: http://www.linux-mm.org/ .
> Don't email: <a href=mailto:"dont@...ck.org"> email@...ck.org </a>
>