linux-kernel - Re: [PATCH RFC 3/3] mm: remove folio_test_anon(folio)==false path in __folio_add_anon

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: <c50287f2-1c83-438b-ba4a-c08ef91b5ba9@redhat.com>
Date: Fri, 14 Jun 2024 13:10:01 +0200
From: David Hildenbrand <david@...hat.com>
To: Barry Song <21cnbao@...il.com>
Cc: akpm@...ux-foundation.org, linux-mm@...ck.org, chrisl@...nel.org,
 linux-kernel@...r.kernel.org, mhocko@...e.com, ryan.roberts@....com,
 baolin.wang@...ux.alibaba.com, yosryahmed@...gle.com, shy828301@...il.com,
 surenb@...gle.com, v-songbaohua@...o.com, willy@...radead.org,
 ying.huang@...el.com, yuzhao@...gle.com
Subject: Re: [PATCH RFC 3/3] mm: remove folio_test_anon(folio)==false path in
 __folio_add_anon_rmap()

>> I don't think that is required? We are only working with anon folios. Or
>> were you able to trigger this? (which would be weird)
> 
> I didn't trigger this. but I am not sure if kfifo is always anon based on
> the code context.
> 
> for page,  it is 100% anon(otherwise "goto out"), but I am not quite
> sure about kpage
> by the code context.
> 
> static int try_to_merge_one_page(struct vm_area_struct *vma,
>                                   struct page *page, struct page *kpage)
> {
>          pte_t orig_pte = __pte(0);
>          int err = -EFAULT;
> 
>          if (page == kpage)                      /* ksm page forked */
>                  return 0;
> 
>          if (!PageAnon(page))
>                  goto out;
>          ....
> }
> 
> Then I saw this
> 
> static int replace_page(struct vm_area_struct *vma, struct page *page,
>                          struct page *kpage, pte_t orig_pte)
> {
>          ...
>          VM_BUG_ON_PAGE(PageAnonExclusive(page), page);
>          VM_BUG_ON_FOLIO(folio_test_anon(kfolio) && PageAnonExclusive(kpage),
>                          kfolio);
> }
> 
> If kfolio is always anon, we should have used
> VM_BUG_ON_FOLIO(PageAnonExclusive(kpage), folio)
> just like
> VM_BUG_ON_PAGE(PageAnonExclusive(page), page);
> without "folio_test_anon(kfolio)".
> 
> So I lost my way.

try_to_merge_one_page() is either called with a KSM page (anon) from 
try_to_merge_with_ksm_page() or with the shared zeropage (!anon and must 
never become anon) from cmp_and_merge_page().

So in replace_page(), we either have an ksm/anon page or the shared 
zeropage.

We never updated the documentation of replace_page() to spell out that 
"kpage" can also be the shared zeropage.

Note how replace_page() calls folio_add_anon_rmap_pte() not for the 
shared zeropage.

If we would have to craft a new anon page things would be going terribly 
wrong.

So not, this (!anon -> anon) must not happen and if it were to happen, 
it would be a real bug and your check in  folio_add_anon_rmap_pte() 
would catch it.

-- 
Cheers,

David / dhildenb