| 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
| ||
|
Message-ID: <b4e3b6c8-9ef9-4785-8c6c-f427f4fdf492@arm.com>
Date: Tue, 29 Apr 2025 10:29:32 +0530
From: Dev Jain <dev.jain@....com>
To: Lance Yang <lance.yang@...ux.dev>, akpm@...ux-foundation.org
Cc: ryan.roberts@....com, david@...hat.com, willy@...radead.org,
linux-mm@...ck.org, linux-kernel@...r.kernel.org, catalin.marinas@....com,
will@...nel.org, Liam.Howlett@...cle.com, lorenzo.stoakes@...cle.com,
vbabka@...e.cz, jannh@...gle.com, anshuman.khandual@....com,
peterx@...hat.com, joey.gouly@....com, ioworker0@...il.com,
baohua@...nel.org, kevin.brodsky@....com, quic_zhenhuah@...cinc.com,
christophe.leroy@...roup.eu, yangyicong@...ilicon.com,
linux-arm-kernel@...ts.infradead.org, namit@...are.com, hughd@...gle.com,
yang@...amperecomputing.com, ziy@...dia.com
Subject: Re: [PATCH 6/7] mm: Batch around can_change_pte_writable()
On 28/04/25 6:53 pm, Lance Yang wrote:
>
>
> On 2025/4/28 20:59, Dev Jain wrote:
>>
>>
>> On 28/04/25 6:20 pm, Lance Yang wrote:
>>> Hey Dev,
>>>
>>> On 2025/4/28 20:04, Dev Jain wrote:
>>>> In preparation for patch 7, we need to properly batch around
>>>> can_change_pte_writable(). We batch around pte_needs_soft_dirty_wp() by
>>>> the corresponding fpb flag, we batch around the page-anon exclusive
>>>> check
>>>> using folio_maybe_mapped_shared(); modify_prot_start_ptes() collects
>>>> the
>>>> dirty and access bits across the batch, therefore batching across
>>>> pte_dirty(): this is correct since the dirty bit on the PTE really
>>>> is just an indication that the folio got written to, so even if
>>>> the PTE is not actually dirty (but one of the PTEs in the batch is),
>>>> the wp-fault optimization can be made.
>>>>
>>>> Signed-off-by: Dev Jain <dev.jain@....com>
>>>> ---
>>>> include/linux/mm.h | 4 ++--
>>>> mm/gup.c | 2 +-
>>>> mm/huge_memory.c | 4 ++--
>>>> mm/memory.c | 6 +++---
>>>> mm/mprotect.c | 9 ++++++---
>>>> 5 files changed, 14 insertions(+), 11 deletions(-)
>>>>
>>>> diff --git a/include/linux/mm.h b/include/linux/mm.h
>>>> index 5eb0d77c4438..ffa02e15863f 100644
>>>> --- a/include/linux/mm.h
>>>> +++ b/include/linux/mm.h
>>>> @@ -2710,8 +2710,8 @@ int get_cmdline(struct task_struct *task, char
>>>> *buffer, int buflen);
>>>> #define MM_CP_UFFD_WP_ALL (MM_CP_UFFD_WP | \
>>>> MM_CP_UFFD_WP_RESOLVE)
>>>> -bool can_change_pte_writable(struct vm_area_struct *vma, unsigned
>>>> long addr,
>>>> - pte_t pte);
>>>> +bool can_change_ptes_writable(struct vm_area_struct *vma, unsigned
>>>> long addr,
>>>> + pte_t pte, struct folio *folio, unsigned int nr);
>>>> extern long change_protection(struct mmu_gather *tlb,
>>>> struct vm_area_struct *vma, unsigned long start,
>>>> unsigned long end, unsigned long cp_flags);
>>>> diff --git a/mm/gup.c b/mm/gup.c
>>>> index 84461d384ae2..6a605fc5f2cb 100644
>>>> --- a/mm/gup.c
>>>> +++ b/mm/gup.c
>>>> @@ -614,7 +614,7 @@ static inline bool
>>>> can_follow_write_common(struct page *page,
>>>> return false;
>>>> /*
>>>> - * See can_change_pte_writable(): we broke COW and could map
>>>> the page
>>>> + * See can_change_ptes_writable(): we broke COW and could map
>>>> the page
>>>> * writable if we have an exclusive anonymous page ...
>>>> */
>>>> return page && PageAnon(page) && PageAnonExclusive(page);
>>>> diff --git a/mm/huge_memory.c b/mm/huge_memory.c
>>>> index 28c87e0e036f..e5496c0d9e7e 100644
>>>> --- a/mm/huge_memory.c
>>>> +++ b/mm/huge_memory.c
>>>> @@ -2032,12 +2032,12 @@ static inline bool
>>>> can_change_pmd_writable(struct vm_area_struct *vma,
>>>> return false;
>>>> if (!(vma->vm_flags & VM_SHARED)) {
>>>> - /* See can_change_pte_writable(). */
>>>> + /* See can_change_ptes_writable(). */
>>>> page = vm_normal_page_pmd(vma, addr, pmd);
>>>> return page && PageAnon(page) && PageAnonExclusive(page);
>>>> }
>>>> - /* See can_change_pte_writable(). */
>>>> + /* See can_change_ptes_writable(). */
>>>> return pmd_dirty(pmd);
>>>> }
>>>> diff --git a/mm/memory.c b/mm/memory.c
>>>> index b9e8443aaa86..b1fda3de8d27 100644
>>>> --- a/mm/memory.c
>>>> +++ b/mm/memory.c
>>>> @@ -750,7 +750,7 @@ static void restore_exclusive_pte(struct
>>>> vm_area_struct *vma,
>>>> pte = pte_mkuffd_wp(pte);
>>>> if ((vma->vm_flags & VM_WRITE) &&
>>>> - can_change_pte_writable(vma, address, pte)) {
>>>> + can_change_ptes_writable(vma, address, pte, NULL, 1)) {
>>>> if (folio_test_dirty(folio))
>>>> pte = pte_mkdirty(pte);
>>>> pte = pte_mkwrite(pte, vma);
>>>> @@ -5767,7 +5767,7 @@ static void numa_rebuild_large_mapping(struct
>>>> vm_fault *vmf, struct vm_area_stru
>>>> ptent = pte_modify(ptent, vma->vm_page_prot);
>>>> writable = pte_write(ptent);
>>>> if (!writable && pte_write_upgrade &&
>>>> - can_change_pte_writable(vma, addr, ptent))
>>>> + can_change_ptes_writable(vma, addr, ptent, NULL, 1))
>>>> writable = true;
>>>> }
>>>> @@ -5808,7 +5808,7 @@ static vm_fault_t do_numa_page(struct vm_fault
>>>> *vmf)
>>>> */
>>>> writable = pte_write(pte);
>>>> if (!writable && pte_write_upgrade &&
>>>> - can_change_pte_writable(vma, vmf->address, pte))
>>>> + can_change_ptes_writable(vma, vmf->address, pte, NULL, 1))
>>>> writable = true;
>>>> folio = vm_normal_folio(vma, vmf->address, pte);
>>>> diff --git a/mm/mprotect.c b/mm/mprotect.c
>>>> index 33eabc995584..362fd7e5457d 100644
>>>> --- a/mm/mprotect.c
>>>> +++ b/mm/mprotect.c
>>>> @@ -40,8 +40,8 @@
>>>> #include "internal.h"
>>>> -bool can_change_pte_writable(struct vm_area_struct *vma, unsigned
>>>> long addr,
>>>> - pte_t pte)
>>>> +bool can_change_ptes_writable(struct vm_area_struct *vma, unsigned
>>>> long addr,
>>>> + pte_t pte, struct folio *folio, unsigned int nr)
>>>> {
>>>> struct page *page;
>>>> @@ -67,6 +67,9 @@ bool can_change_pte_writable(struct vm_area_struct
>>>> *vma, unsigned long addr,
>>>> * write-fault handler similarly would map them writable
>>>> without
>>>> * any additional checks while holding the PT lock.
>>>> */
>>>> + if (unlikely(nr != 1))
>>>> + return !folio_maybe_mapped_shared(folio);
>>>> +
>>>> page = vm_normal_page(vma, addr, pte);
>>>> return page && PageAnon(page) && PageAnonExclusive(page);
>>>> }
>>>
>>> IIUC, As mentioned in the comment above, we should do the same
>>> anonymous check
>>> to large folios. And folio_maybe_mapped_shared() already handles both
>>> order-0
>>> and large folios nicely, so we could simplify the logic as follows:
>>
>> Thanks. Although we will have to call vm_normal_folio() in case of !
>> folio, since we may not have the folio already for nr == 1 case.
>
> Ah, I see. Should we still check folio_test_anon() when nr != 1?
According to the comment, "We can only special-case on exclusive
anonymous pages", I would say yes.
>
> Thanks,
> Lance
>
>>
>>>
>>> diff --git a/mm/mprotect.c b/mm/mprotect.c
>>> index 1605e89349d2..df56a30bb241 100644
>>> --- a/mm/mprotect.c
>>> +++ b/mm/mprotect.c
>>> @@ -43,8 +43,6 @@
>>> bool can_change_ptes_writable(struct vm_area_struct *vma, unsigned
>>> long addr,
>>> pte_t pte, struct folio *folio,
>>> unsigned int nr)
>>> {
>>> - struct page *page;
>>> -
>>> if (WARN_ON_ONCE(!(vma->vm_flags & VM_WRITE)))
>>> return false;
>>>
>>> @@ -67,11 +65,7 @@ bool can_change_ptes_writable(struct
>>> vm_area_struct *vma, unsigned long addr,
>>> * write-fault handler similarly would map them
>>> writable without
>>> * any additional checks while holding the PT lock.
>>> */
>>> - if (unlikely(nr != 1))
>>> - return !folio_maybe_mapped_shared(folio);
>>> -
>>> - page = vm_normal_page(vma, addr, pte);
>>> - return page && PageAnon(page) &&
>>> PageAnonExclusive(page);
>>> + return folio_test_anon(folio) && !
>>> folio_maybe_mapped_shared(folio);
>>> }
>>>
>>> VM_WARN_ON_ONCE(is_zero_pfn(pte_pfn(pte)) && pte_dirty(pte));
>>> --
>>>
>>> Thanks,
>>> Lance
>>>
>>>> @@ -222,7 +225,7 @@ static long change_pte_range(struct mmu_gather
>>>> *tlb,
>>>> */
>>>> if ((cp_flags & MM_CP_TRY_CHANGE_WRITABLE) &&
>>>> !pte_write(ptent) &&
>>>> - can_change_pte_writable(vma, addr, ptent))
>>>> + can_change_ptes_writable(vma, addr, ptent, folio, 1))
>>>> ptent = pte_mkwrite(ptent, vma);
>>>> ptep_modify_prot_commit(vma, addr, pte, oldpte, ptent);
>>>
>>>
>>
>
>
Powered by blists - more mailing lists