linux-kernel - Re: [PATCH] mm: gup: fix the fast GUP race against THP collapse

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:   Sun, 4 Sep 2022 15:21:57 -0700
From:   John Hubbard <jhubbard@...dia.com>
To:     Peter Xu <peterx@...hat.com>, Yang Shi <shy828301@...il.com>
Cc:     david@...hat.com, kirill.shutemov@...ux.intel.com, jgg@...dia.com,
        hughd@...gle.com, akpm@...ux-foundation.org, linux-mm@...ck.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] mm: gup: fix the fast GUP race against THP collapse

On 9/2/22 08:59, Peter Xu wrote:
...
> Would you mind rewording this comment a bit?  I feel it a bit weird to
> suddenly mention about thp collapse especially its details.
> 
> Maybe some statement on the whole history of why check pte, and in what
> case pmd check is needed (where the thp collapse example can be moved to,
> imho)?
> 
> One of my attempt for reference..
> 
> 		/*
> 		 * Fast-gup relies on pte change detection to avoid
> 		 * concurrent pgtable operations.
> 		 *
> 		 * To pin the page, fast-gup needs to do below in order:
> 		 * (1) pin the page (by prefetching pte), then (2) check
> 		 * pte not changed.
> 		 *
> 		 * For the rest of pgtable operations where pgtable updates
> 		 * can be racy with fast-gup, we need to do (1) clear pte,
> 		 * then (2) check whether page is pinned.
> 		 *
> 		 * Above will work for all pte-level operations, including
> 		 * thp split.
> 		 *
> 		 * For thp collapse, it's a bit more complicated because
> 		 * with RCU pgtable free fast-gup can be walking a pgtable
> 		 * page that is being freed (so pte is still valid but pmd
> 		 * can be cleared already).  To avoid race in such
> 		 * condition, we need to also check pmd here to make sure
> 		 * pmd doesn't change (corresponds to pmdp_collapse_flush()
> 		 * in the thp collide code path).
> 		 */
> 
> If you agree with the comment change, feel free to add:
> 
> Acked-by: Peter Xu <peterx@...hat.com>
> 

This seems fine, but I'd like to additionally request that we move it
to function-level documentation. Because it expands the length of the
function, which previously fit neatly on my screen. So I think it's
time to move it up a level.

thanks,

-- 
John Hubbard
NVIDIA