linux-kernel - Re: [PATCH v1 2/4] mm: smaller folio_pte

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: <8f08ae90-8062-433d-9798-19a23de35167@lucifer.local>
Date: Fri, 27 Jun 2025 19:39:21 +0100
From: Lorenzo Stoakes <lorenzo.stoakes@...cle.com>
To: David Hildenbrand <david@...hat.com>
Cc: linux-kernel@...r.kernel.org, linux-mm@...ck.org,
        Andrew Morton <akpm@...ux-foundation.org>,
        "Liam R. Howlett" <Liam.Howlett@...cle.com>,
        Vlastimil Babka <vbabka@...e.cz>, Jann Horn <jannh@...gle.com>,
        Mike Rapoport <rppt@...nel.org>,
        Suren Baghdasaryan <surenb@...gle.com>, Michal Hocko <mhocko@...e.com>,
        Zi Yan <ziy@...dia.com>, Matthew Brost <matthew.brost@...el.com>,
        Joshua Hahn <joshua.hahnjy@...il.com>, Rakie Kim <rakie.kim@...com>,
        Byungchul Park <byungchul@...com>, Gregory Price <gourry@...rry.net>,
        Ying Huang <ying.huang@...ux.alibaba.com>,
        Alistair Popple <apopple@...dia.com>, Pedro Falcato <pfalcato@...e.de>,
        Rik van Riel <riel@...riel.com>, Harry Yoo <harry.yoo@...cle.com>
Subject: Re: [PATCH v1 2/4] mm: smaller folio_pte_batch() improvements

On Fri, Jun 27, 2025 at 07:02:16PM +0200, David Hildenbrand wrote:
> On 27.06.25 18:51, Lorenzo Stoakes wrote:
> > On Fri, Jun 27, 2025 at 01:55:08PM +0200, David Hildenbrand wrote:
> > > Let's clean up a bit:
> > >
> > > (1) No need for start_ptep vs. ptep anymore, we can simply use ptep
> > >
> > > (2) Let's switch to "unsigned int" for everything
> > >
> > > (3) We can simplify the code by leaving the pte unchanged after the
> > >      pte_same() check.
> > >
> > > (4) Clarify that we should never exceed a single VMA; it indicates a
> > >      problem in the caller.
> > >
> > > No functional change intended.
> > >
> > > Signed-off-by: David Hildenbrand <david@...hat.com>

Given below, LGTM:

Reviewed-by: Lorenzo Stoakes <lorenzo.stoakes@...cle.com>

> > > -static inline int folio_pte_batch(struct folio *folio, unsigned long addr,
> > > -		pte_t *start_ptep, pte_t pte, int max_nr, fpb_t flags,
> > > +static inline unsigned int folio_pte_batch(struct folio *folio, unsigned long addr,
> >
> > Do we need to worry about propagating this type change?
> >
> > mremap_folio_pte_batch() and zap_present_ptes() return the value as an int for example.
> >
> > I mean I doubt we're going to be seeing an overflow here :) but maybe worth
> > propagating this everywhere.
>
> Yeah, I'm planning on cleaning some of that stuff separately. As you say,
> this shouldn't cause harm.
>
> Really, it could only cause harm if someone would be doing
>
> "- folio_pte_batch()" and then assigning it to a "long".

Right yeah.

>
> >
> > > +		pte_t *ptep, pte_t pte, unsigned int max_nr, fpb_t flags,
> > >   		bool *any_writable, bool *any_young, bool *any_dirty)
> > >   {
> > > -	pte_t expected_pte, *ptep;
> > > -	bool writable, young, dirty;
> > > -	int nr, cur_nr;
> > > +	unsigned int nr, cur_nr;
> > > +	pte_t expected_pte;
> > >
> > >   	if (any_writable)
> > >   		*any_writable = false;
> > > @@ -267,29 +267,22 @@ static inline int folio_pte_batch(struct folio *folio, unsigned long addr,
> > >   	max_nr = min_t(unsigned long, max_nr,
> > >   		       folio_pfn(folio) + folio_nr_pages(folio) - pte_pfn(pte));
> > >
> > > -	nr = pte_batch_hint(start_ptep, pte);
> > > +	nr = pte_batch_hint(ptep, pte);
> > >   	expected_pte = __pte_batch_clear_ignored(pte_advance_pfn(pte, nr), flags);
> > > -	ptep = start_ptep + nr;
> > > +	ptep = ptep + nr;
> > >
> > >   	while (nr < max_nr) {
> > >   		pte = ptep_get(ptep);
> > > -		if (any_writable)
> > > -			writable = !!pte_write(pte);
> > > -		if (any_young)
> > > -			young = !!pte_young(pte);
> > > -		if (any_dirty)
> > > -			dirty = !!pte_dirty(pte);
> > > -		pte = __pte_batch_clear_ignored(pte, flags);
> > >
> > > -		if (!pte_same(pte, expected_pte))
> > > +		if (!pte_same(__pte_batch_clear_ignored(pte, flags), expected_pte))
> >
> > Doing this here will change the output of any_writable, any_young:
> >
> > static inline pte_t __pte_batch_clear_ignored(pte_t pte, fpb_t flags)
> > {
> > 	...
> > 	return pte_wrprotect(pte_mkold(pte));
> > }
> >
> > So we probably need to get these values earlier?
>
>
> Note that __pte_batch_clear_ignored() leaves the pte unchanged, and only
> returns the modified pte. (like pte_mkold() and friends).
>
> So what we read through ptep_get() is still what we have after the
> pte_same() check.

Yeah you're right, sorry, these are just chaging the value that is returned for
comparison against expected_pte.

Then LGTM!