linux-kernel - Re: [PATCH 11/12] mm: pgtable: introduce generic __tlb_remove

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: <6666f651-2a59-40fb-b984-60669d4fdaa2@bytedance.com>
Date: Tue, 17 Dec 2024 11:42:02 +0800
From: Qi Zheng <zhengqi.arch@...edance.com>
To: Peter Zijlstra <peterz@...radead.org>,
 Vishal Moola <vishal.moola@...il.com>
Cc: tglx@...utronix.de, david@...hat.com, jannh@...gle.com, hughd@...gle.com,
 yuzhao@...gle.com, willy@...radead.org, muchun.song@...ux.dev,
 vbabka@...nel.org, lorenzo.stoakes@...cle.com, akpm@...ux-foundation.org,
 rientjes@...gle.com, linux-mm@...ck.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 11/12] mm: pgtable: introduce generic __tlb_remove_table()



On 2024/12/17 02:12, Peter Zijlstra wrote:
> On Mon, Dec 16, 2024 at 08:52:06PM +0800, Qi Zheng wrote:
>>
>>
>> On 2024/12/16 20:00, Peter Zijlstra wrote:
>>> On Sat, Dec 14, 2024 at 05:02:57PM +0800, Qi Zheng wrote:
>>
>> [...]
>>
>>>> +#ifndef __HAVE_ARCH_TLB_REMOVE_TABLE
>>>> +static inline void __tlb_remove_table(void *_table)
>>>> +{
>>>> +	struct ptdesc *ptdesc = (struct ptdesc *)_table;
>>>> +
>>>> +	pagetable_dtor(ptdesc);
>>>> +	pagetable_free(ptdesc);
>>>> +}
>>>> +#endif
>>>
>>>
>>> Spot the fail...
>>>
>>> That said, all this ptdesc stuff is another giant trainwreck. Let me
>>> clean that up for you.
>>
>> It looks like you want to revert what was done in this patch series:
>>
>> https://lore.kernel.org/all/20230807230513.102486-1-vishal.moola@gmail.com/
>>
>> But why? It seems that splitting ptdesc from struct page is a good
>> thing?
> 
> Because we're explicitly allocating pages for the page-tables, and also,
> code like:
> 
> tlb_remove_page_ptdesc((tlb), (page_ptdesc(pte)));
> 
> static inline void tlb_remove_page_ptdesc(struct mmu_gather *tlb, struct ptdesc *pt)
> {
> 	tlb_remove_page(tlb, ptdesc_page(pt));
> }
> 
> Just makes me upset.

Aha, this strikes me as odd too.

Also +CC Vishal Moola, the author of ptdesc, who may be able to provide
more background information. If Vishal has no objection, I will try to
remove tlb_remove_ptdesc() and tlb_remove_page_ptdesc() as Peter suggested.

> 
> Just bloody write tlb_remove_page() and call it a day.
> 
> All that nonsense is just obfuscation at this point.

In addition, will remove the duplicates of __tlb_remove_table,
asm-generic/pgalloc.h pte_free(), pmd_free(), __pud_free() and
__p4d_free() as follows:

diff --git a/include/asm-generic/pgalloc.h b/include/asm-generic/pgalloc.h
index 3673e9c29504e..370f5b579ff88 100644
--- a/include/asm-generic/pgalloc.h
+++ b/include/asm-generic/pgalloc.h
@@ -107,10 +107,7 @@ static inline pgtable_t pte_alloc_one_noprof(struct 
mm_struct *mm)
   */
  static inline void pte_free(struct mm_struct *mm, struct page *pte_page)
  {
-       struct ptdesc *ptdesc = page_ptdesc(pte_page);
-
-       pagetable_dtor(ptdesc);
-       pagetable_free(ptdesc);
+       pagetable_dtor_free(pte_page);
  }


@@ -150,11 +147,7 @@ static inline pmd_t *pmd_alloc_one_noprof(struct 
mm_struct *mm, unsigned long ad
  #ifndef __HAVE_ARCH_PMD_FREE
  static inline void pmd_free(struct mm_struct *mm, pmd_t *pmd)
  {
-       struct ptdesc *ptdesc = virt_to_ptdesc(pmd);
-
-       BUG_ON((unsigned long)pmd & (PAGE_SIZE-1));
-       pagetable_dtor(ptdesc);
-       pagetable_free(ptdesc);
+       pagetable_dtor_free(virt_to_page(pmd));
  }
  #endif

@@ -199,11 +192,7 @@ static inline pud_t *pud_alloc_one_noprof(struct 
mm_struct *mm, unsigned long ad

  static inline void __pud_free(struct mm_struct *mm, pud_t *pud)
  {
-       struct ptdesc *ptdesc = virt_to_ptdesc(pud);
-
-       BUG_ON((unsigned long)pud & (PAGE_SIZE-1));
-       pagetable_dtor(ptdesc);
-       pagetable_free(ptdesc);
+       pagetable_dtor_free(virt_to_page(pud));
  }

  #ifndef __HAVE_ARCH_PUD_FREE
@@ -254,11 +243,7 @@ static inline p4d_t *p4d_alloc_one_noprof(struct 
mm_struct *mm, unsigned long ad

  static inline void __p4d_free(struct mm_struct *mm, p4d_t *p4d)
  {
-       struct ptdesc *ptdesc = virt_to_ptdesc(p4d);
-
-       BUG_ON((unsigned long)p4d & (PAGE_SIZE-1));
-       pagetable_dtor(ptdesc);
-       pagetable_free(ptdesc);
+       pagetable_dtor_free(virt_to_page(p4d));
  }

  #ifndef __HAVE_ARCH_P4D_FREE
diff --git a/include/asm-generic/tlb.h b/include/asm-generic/tlb.h
index 939a813023d7e..b34d014c23ef0 100644
--- a/include/asm-generic/tlb.h
+++ b/include/asm-generic/tlb.h
@@ -211,10 +211,7 @@ struct mmu_table_batch {
  #ifndef __HAVE_ARCH_TLB_REMOVE_TABLE
  static inline void __tlb_remove_table(void *_table)
  {
-       struct ptdesc *ptdesc = (struct ptdesc *)_table;
-
-       pagetable_dtor(ptdesc);
-       pagetable_free(ptdesc);
+       pagetable_dtor_free(_table);
  }
  #endif

diff --git a/include/linux/mm.h b/include/linux/mm.h
index 497035a78849b..11829860ec05e 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -3064,6 +3064,14 @@ static inline void pagetable_dtor(struct ptdesc 
*ptdesc)
         lruvec_stat_sub_folio(folio, NR_PAGETABLE);
  }

+static inline void pagetable_dtor_free(void *table)
+{
+       struct ptdesc *ptdesc = page_ptdesc((struct page *)table);
+
+        pagetable_dtor(ptdesc);
+        pagetable_dtor(ptdesc);
+}
+
  static inline bool pagetable_pte_ctor(struct ptdesc *ptdesc)
  {
         struct folio *folio = ptdesc_folio(ptdesc);

Thanks!