| 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: <083865e8-572b-41b2-9221-3cee01349fab@arm.com>
Date: Mon, 28 Apr 2025 18:29:21 +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: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.
>
> 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