linux-kernel - Re: [PATCH -next 1/7] mm: huge_memory: make __do_huge_pmd_anonymous

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]

Date:   Fri, 13 Jan 2023 14:25:37 +0000
From:   Matthew Wilcox <willy@...radead.org>
To:     Kefeng Wang <wangkefeng.wang@...wei.com>
Cc:     akpm@...ux-foundation.org, linux-mm@...ck.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH -next 1/7] mm: huge_memory: make
 __do_huge_pmd_anonymous_page() to take a folio

On Thu, Jan 12, 2023 at 04:30:00PM +0800, Kefeng Wang wrote:
> Let's __do_huge_pmd_anonymous_page() take a folio and convert related
> functions to use folios.

No, this is actively wrong!  Andrew, please drop this patch.

If we want to support folio sizes larger than PMD size (and I think we
do), we need to be able to specify precisely which page in the folio
is to be stored at this PTE.  The *interface* must remain struct page.
We can convert from page to folio within the function, but we *MUST NOT*
go the other way.

>  static vm_fault_t __do_huge_pmd_anonymous_page(struct vm_fault *vmf,
> -			struct page *page, gfp_t gfp)
> +			struct folio *folio, gfp_t gfp)
>  {
>  	struct vm_area_struct *vma = vmf->vma;
> +	struct page *page = &folio->page;

... ie this is bad and wrong.

> @@ -834,7 +835,7 @@ vm_fault_t do_huge_pmd_anonymous_page(struct vm_fault *vmf)
>  		count_vm_event(THP_FAULT_FALLBACK);
>  		return VM_FAULT_FALLBACK;
>  	}
> -	return __do_huge_pmd_anonymous_page(vmf, &folio->page, gfp);
> +	return __do_huge_pmd_anonymous_page(vmf, folio, gfp);
>  }
>  
>  static void insert_pfn_pmd(struct vm_area_struct *vma, unsigned long addr,

A reasonable person might ask "But Matthew, you allocated a folio here,
then you're converting back to a struct page to call
__do_huge_pmd_anonymous_page() so isn't this a sensible patch?"

And I would say "still no".  This is a question of interfaces, and
even though __do_huge_pmd_anonymous_page() is static and has precisely
one caller today that always allocates a folio of precisely PMD size,
I suspect it will need to be more visible in the future and the
conversion of the interface from page to folio misleads people.