linux-kernel - Re: [PATCH v4 06/11] mm: thp: check pmd migration entry in common path

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]

Date:   Fri, 24 Mar 2017 12:09:38 -0500
From:   Zi Yan <zi.yan@...t.com>
To:     "Kirill A. Shutemov" <kirill@...temov.name>
CC:     Zi Yan <zi.yan@...rutgers.edu>, linux-kernel@...r.kernel.org,
        linux-mm@...ck.org, kirill.shutemov@...ux.intel.com,
        akpm@...ux-foundation.org, minchan@...nel.org, vbabka@...e.cz,
        mgorman@...hsingularity.net, mhocko@...nel.org,
        n-horiguchi@...jp.nec.com, khandual@...ux.vnet.ibm.com,
        dnellans@...dia.com
Subject: Re: [PATCH v4 06/11] mm: thp: check pmd migration entry in common
 path



Kirill A. Shutemov wrote:
> On Fri, Mar 24, 2017 at 11:09:25AM -0500, Zi Yan wrote:
>> Kirill A. Shutemov wrote:
>>> On Mon, Mar 13, 2017 at 11:45:02AM -0400, Zi Yan wrote:
>>> Again. That's doesn't look right..
>> It will be changed:
>>
>>  	ptl = pmd_lock(mm, pmd);
>> +retry_locked:
>> +	if (unlikely(!pmd_present(*pmd))) {
>> +		if (likely(!(flags & FOLL_MIGRATION))) {
>> +			spin_unlock(ptl);
>> +			return no_page_table(vma, flags);
>> +		}
>> +		pmd_migration_entry_wait(mm, pmd);
>> +		goto retry_locked;
> 
> Nope. pmd_migration_entry_wait() unlocks the ptl.

Right. This chunk is wrong. pmd_migrtion_entry_wait() actually locks
pmd, then unlocks it and waits on the page if it is suitable.

An simple fix could be:

+retry_locked:
 	ptl = pmd_lock(mm, pmd);
+	if (unlikely(!pmd_present(*pmd))) {
+	        spin_unlock(ptl);
+		if (likely(!(flags & FOLL_MIGRATION)))
+			return no_page_table(vma, flags);
+		pmd_migration_entry_wait(mm, pmd);
+		goto retry_locked;
+       }

Or is it better to change pmd_migration_entry_wait() to
void pmd_migration_entry_wait(struct mm_struct *mm, pmd_t *pmd,
spinlock_t *ptl)? So that if ptl is NULL, then it takes the pmd lock and
unlocks it; if ptl is specified, it only unlocks it. This can avoid the
redundant unlock and lock in the code above, when
pmd_migration_entry_wait() is called.

Thanks.

--
Best Regards,
Yan Zi