linux-kernel - Re: [RFC PATCH] docs/mm: add VMA locks documentation

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] [day] [month] [year] [list]

Message-ID: <CAG48ez3hyaymBo_Y9V2Hpx8TRHbE2WyZoeLhi1n0VW9Np7iw2Q@mail.gmail.com>
Date: Wed, 6 Nov 2024 19:09:58 +0100
From: Jann Horn <jannh@...gle.com>
To: Qi Zheng <zhengqi.arch@...edance.com>
Cc: Lorenzo Stoakes <lorenzo.stoakes@...cle.com>, Jonathan Corbet <corbet@....net>, 
	Andrew Morton <akpm@...ux-foundation.org>, "Liam R . Howlett" <Liam.Howlett@...cle.com>, 
	Vlastimil Babka <vbabka@...e.cz>, Alice Ryhl <aliceryhl@...gle.com>, Boqun Feng <boqun.feng@...il.com>, 
	Matthew Wilcox <willy@...radead.org>, Mike Rapoport <rppt@...nel.org>, linux-mm@...ck.org, 
	linux-kernel@...r.kernel.org, Suren Baghdasaryan <surenb@...gle.com>, 
	"open list:DOCUMENTATION" <linux-doc@...r.kernel.org>
Subject: Re: [RFC PATCH] docs/mm: add VMA locks documentation

On Wed, Nov 6, 2024 at 4:09 AM Qi Zheng <zhengqi.arch@...edance.com> wrote:
> On 2024/11/5 05:29, Jann Horn wrote:
> > On Mon, Nov 4, 2024 at 5:42 PM Lorenzo Stoakes
>
> [...]
>
> >
> > I think it's important to know about the existence of hardware writes
> > because it means you need atomic operations when making changes to
> > page tables. Like, for example, in many cases when changing a present
> > PTE, you can't even use READ_ONCE()/WRITE_ONCE() for PTEs and need
> > atomic RMW operations instead - see for example ptep_get_and_clear(),
> > which is basically implemented in arch code as an atomic xchg so that
> > it can't miss concurrent A/D bit updates.
> >
>
> Totally agree! But I noticed before that ptep_clear() doesn't seem
> to need atomic operations because it doesn't need to care about the
> A/D bit.
>
> I once looked at the history of how the ptep_clear() was introduced.
> If you are interested, you can take a look at my local draft below.
> Maybe I missed something.
>
> ```
> mm: pgtable: make ptep_clear() non-atomic
>
>      In the generic ptep_get_and_clear() implementation, it is just a simple
>      combination of ptep_get() and pte_clear(). But for some architectures
>      (such as x86 and arm64, etc), the hardware will modify the A/D bits
> of the
>      page table entry, so the ptep_get_and_clear() needs to be overwritten
>      and implemented as an atomic operation to avoid contention, which has a
>      performance cost.
>
>      The commit d283d422c6c4 ("x86: mm: add x86_64 support for page table
>      check") adds the ptep_clear() on the x86, and makes it call
>      ptep_get_and_clear() when CONFIG_PAGE_TABLE_CHECK is enabled. The page
>      table check feature does not actually care about the A/D bits, so only
>      ptep_get() + pte_clear() should be called. But considering that the
> page
>      table check is a debug option, this should not have much of an impact.
>
>      But then the commit de8c8e52836d ("mm: page_table_check: add hooks to
>      public helpers") changed ptep_clear() to unconditionally call
>      ptep_get_and_clear(), so that the  CONFIG_PAGE_TABLE_CHECK check can be
>      put into the page table check stubs (in
> include/linux/page_table_check.h).
>      This also cause performance loss to the kernel without
>      CONFIG_PAGE_TABLE_CHECK enabled, which doesn't make sense.
>
>      To fix it, just calling ptep_get() and pte_clear() in the ptep_clear().
>
>      Signed-off-by: Qi Zheng <zhengqi.arch@...edance.com>
>
> diff --git a/include/linux/pgtable.h b/include/linux/pgtable.h
> index 117b807e3f894..2ace92293f5f5 100644
> --- a/include/linux/pgtable.h
> +++ b/include/linux/pgtable.h
> @@ -506,7 +506,10 @@ static inline void clear_young_dirty_ptes(struct
> vm_area_struct *vma,
>   static inline void ptep_clear(struct mm_struct *mm, unsigned long addr,
>                                pte_t *ptep)
>   {
> -       ptep_get_and_clear(mm, addr, ptep);
> +       pte_t pte = ptep_get(ptep);
> +
> +       pte_clear(mm, addr, ptep);
> +       page_table_check_pte_clear(mm, pte);
>   }
>
> ```

ptep_clear() is currently only used in debug code and in khugepaged
collapse paths, which are fairly expensive, so I don't think the cost
of an extra atomic RMW op should matter here; but yeah, the change
looks correct to me.