linux-kernel - Re: [RFC PATCH 1/3] mm, hugetlb: Clean up locking in hugetlb_fault and hugetlb

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: <1602a87b-b1bc-4b53-abe7-dce8adddbe46@redhat.com>
Date: Tue, 3 Jun 2025 19:19:13 +0200
From: David Hildenbrand <david@...hat.com>
To: Peter Xu <peterx@...hat.com>
Cc: Oscar Salvador <osalvador@...e.de>,
 Andrew Morton <akpm@...ux-foundation.org>,
 Muchun Song <muchun.song@...ux.dev>, James Houghton <jthoughton@...gle.com>,
 Gavin Guo <gavinguo@...lia.com>, linux-mm@...ck.org,
 linux-kernel@...r.kernel.org
Subject: Re: [RFC PATCH 1/3] mm, hugetlb: Clean up locking in hugetlb_fault
 and hugetlb_wp

>> As stated elsewhere, the mapcount check + folio_move_anon_rmap need the
>> folio lock.
> 
> Could you elaborate what would go wrong if we do folio_move_anon_rmap()
> without folio lock here?  Just to make sure we're on the same page: we
> already have pgtable lock held, and we decided to reuse an anonymous
> hugetlb page.

For now we have

VM_BUG_ON_FOLIO(!folio_test_locked(folio), folio);

right at the beginning of folio_move_anon_rmap().

That dates back to

commit c44b674323f4a2480dbeb65d4b487fa5f06f49e0
Author: Rik van Riel <riel@...hat.com>
Date:   Fri Mar 5 13:42:09 2010 -0800

     rmap: move exclusively owned pages to own anon_vma in do_wp_page()
     
     When the parent process breaks the COW on a page, both the original which
     is mapped at child and the new page which is mapped parent end up in that
     same anon_vma.  Generally this won't be a problem, but for some workloads
     it could preserve the O(N) rmap scanning complexity.
     
     A simple fix is to ensure that, when a page which is mapped child gets
     reused in do_wp_page, because we already are the exclusive owner, the page
     gets moved to our own exclusive child's anon_vma.


My recollection is that the folio lock protects folio->mapping. So relevant rmap walks
that hold the folio lock can assume that folio->mapping and
thereby folio_anon_vma() cannot change.

folio_lock_anon_vma_read() documents something regarding the folio lock protecting the
anon_vma.

I can only speculate that locking the folio is cheaper than locking the relevant anon_vma, and
that rmap code depends on that.


I'll note that in the introducing commit we didn't use the WRITE_ONCE, though. That was added in

commit 16f5e707d6f6f7644ff07e583b8f18c3dcc5499f
Author: Alex Shi <alexs@...nel.org>
Date:   Tue Dec 15 12:33:42 2020 -0800

     mm/rmap: stop store reordering issue on page->mapping

But I don't think that the folio lock was a replacement to that WRITE_ONCE.

-- 
Cheers,

David / dhildenb