linux-kernel - Re: [PATCH 2/2] mm: convert do_set

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: <aBuOjrH1UpyTf8I9@casper.infradead.org>
Date: Wed, 7 May 2025 17:47:10 +0100
From: Matthew Wilcox <willy@...radead.org>
To: Baolin Wang <baolin.wang@...ux.alibaba.com>
Cc: akpm@...ux-foundation.org, david@...hat.com, hannes@...xchg.org,
	lorenzo.stoakes@...cle.com, Liam.Howlett@...cle.com,
	npache@...hat.com, ryan.roberts@....com, dev.jain@....com,
	ziy@...dia.com, vbabka@...e.cz, rppt@...nel.org, surenb@...gle.com,
	mhocko@...e.com, linux-mm@...ck.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/2] mm: convert do_set_pmd() to take a folio

On Wed, May 07, 2025 at 08:36:54PM +0800, Baolin Wang wrote:
> On 2025/5/7 20:10, Matthew Wilcox wrote:
> > Because I see nowhere in this patch that you initialise 'page'.
> 
> Please look at the following code in do_set_pmd(), and the 'page' will be
> initialized before using.
> 
>         if (thp_disabled_by_hw() || vma_thp_disabled(vma, vma->vm_flags))
>                 return ret;
> 
>         if (!thp_vma_suitable_order(vma, haddr, PMD_ORDER))
>                 return ret;
> 
>         if (folio_order(folio) != HPAGE_PMD_ORDER)
>                 return ret;
>         page = &folio->page;

Ah, fair, I missed that.

> > And that's really the important part.  You seem to be assuming that a
> > folio will never be larger than PMD size, and I'm not comfortable with
> 
> No, I have no this assumption. But do_set_pmd() is used to establish PMD
> mappings for the PMD-sized folios, and we already have PMD-sized checks to
> validate the folio size:
> 
>         if (!thp_vma_suitable_order(vma, haddr, PMD_ORDER))
>                 return ret;
> 
>         if (folio_order(folio) != HPAGE_PMD_ORDER)
>                 return ret;
> 
> > that assumption.  It's a limitation I put in place a few years ago so we
> > didn't have to find and fix all those assumptions immediately, but I
> > imagine that some day we'll want to have larger folios.
> > 
> > So unless you can derive _which_ page in the folio we want to map from
> 
> IMO, for PMD mapping of a PMD-sized folio, we do not need to know _which_
> page in the folio we want to map, because we'll always map the entire
> PMD-sized folio.

There's a difference between "Assert that the folio is PMD sized" inside
the function because we know there are still problems, and "Change the
interface so we can't specify which page inside the folio is the one
we're actually interested in".

I reiterate the NACK to this patch.