linux-kernel - Re: [PATCH 2/5] mm,hugetlb: Document the reason to lock the folio in the faulting 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]

Message-ID: <dac7aa4c-136d-4a01-9601-7bc0314a98a2@redhat.com>
Date: Tue, 17 Jun 2025 15:42:09 +0200
From: David Hildenbrand <david@...hat.com>
To: Oscar Salvador <osalvador@...e.de>
Cc: Andrew Morton <akpm@...ux-foundation.org>,
 Muchun Song <muchun.song@...ux.dev>, James Houghton <jthoughton@...gle.com>,
 Peter Xu <peterx@...hat.com>, Gavin Guo <gavinguo@...lia.com>,
 linux-mm@...ck.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/5] mm,hugetlb: Document the reason to lock the folio in
 the faulting path

On 17.06.25 14:50, Oscar Salvador wrote:
> On Tue, Jun 17, 2025 at 02:10:09PM +0200, Oscar Salvador wrote:
>> On Tue, Jun 17, 2025 at 02:08:16PM +0200, David Hildenbrand wrote:
>>> folio lock is a sleeping lock, PTL is a spinlock. :)
>>
>> Lol yes, overlooked that totally.
>> And I also saw the comment from mm/rmap.c about lockin order.
> 
> So, we could do something like this:
> 
>          if (folio_test_anon(old_folio)) {
>                  spin_unlock(vmf->ptl);
>                  folio_lock(old_folio);
>                  spin_lock(vmf->ptl);
>                  vmf->pte = hugetlb_walk(vma, vmf->address, huge_page_size(h));
>                  if (unlikely(!vmf->pte ||
>                      !pte_same(huge_ptep_get(mm, vmf->address, vmf->pte), pte))) {
>                          spin_unlock(vmf->ptl);
>                          folio_unlock(old_folio);
>                          goto out_take_lock;
>                  }
> 
>                  if (folio_mapcount(old_folio == 1)) {
>                          if (!PageAnonExclusive(&old_folio->page)) {
>                                  folio_move_anon_rmap(old_folio, vma);
>                                  SetPageAnonExclusive(&old_folio->page);
>                          }
>                          if (likely(!unshare))
>                                  set_huge_ptep_maybe_writable(vma, vmf->address,
>                                                               vmf->pte);
> 
>                          spin_unlock(vmf->ptl);
>                          folio_unlock(old_folio);
>                          goto out_take_lock;
>                  }
>                  VM_BUG_ON_PAGE(folio_test_anon(old_folio) &&
>                                 PageAnonExclusive(&old_folio->page), &old_folio->page);
>                  spin_unlock(vmf->ptl);
>                  folio_unlock(old_folio);
>                  spin_lock(vmf->ptl);
>                  vmf->pte = hugetlb_walk(vma, vmf->address, huge_page_size(h));
>                  if (unlikely(!vmf->pte ||
>                     !pte_same(huge_ptep_get(mm, vmf->address, vmf->pte), pte)))
>                          return 0;
>          }
> 
> Hopefully we can do some refactor here, because I quite dislike the
> unlock-lock-retake-unlock-blah cycle.

Yes. As an alternative, keep locking it in the caller and only unlock in 
the !anon case?

-- 
Cheers,

David / dhildenb