linux-kernel - Re: [PATCH v2 3/3] mm/huge_memory: don't mark refcounted folios special in vmf_insert_folio

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <43e2b05f-c499-48c3-b8e4-a23ef5efc818@lucifer.local>
Date: Thu, 12 Jun 2025 18:08:01 +0100
From: Lorenzo Stoakes <lorenzo.stoakes@...cle.com>
To: David Hildenbrand <david@...hat.com>
Cc: linux-kernel@...r.kernel.org, linux-mm@...ck.org, nvdimm@...ts.linux.dev,
        linux-cxl@...r.kernel.org, Andrew Morton <akpm@...ux-foundation.org>,
        Alistair Popple <apopple@...dia.com>,
        "Liam R. Howlett" <Liam.Howlett@...cle.com>,
        Vlastimil Babka <vbabka@...e.cz>, Mike Rapoport <rppt@...nel.org>,
        Suren Baghdasaryan <surenb@...gle.com>, Michal Hocko <mhocko@...e.com>,
        Zi Yan <ziy@...dia.com>, Baolin Wang <baolin.wang@...ux.alibaba.com>,
        Nico Pache <npache@...hat.com>, Ryan Roberts <ryan.roberts@....com>,
        Dev Jain <dev.jain@....com>, Dan Williams <dan.j.williams@...el.com>,
        Oscar Salvador <osalvador@...e.de>
Subject: Re: [PATCH v2 3/3] mm/huge_memory: don't mark refcounted folios
 special in vmf_insert_folio_pud()

On Thu, Jun 12, 2025 at 07:00:01PM +0200, David Hildenbrand wrote:
> On 12.06.25 18:49, Lorenzo Stoakes wrote:
> > On Wed, Jun 11, 2025 at 02:06:54PM +0200, David Hildenbrand wrote:
> > > Marking PUDs that map a "normal" refcounted folios as special is
> > > against our rules documented for vm_normal_page().
> >
> > Might be worth referring to specifically which rule. I'm guessing it's the
> > general one of special == don't touch (from vm_normal_page() comment):
> >
> > /*
> >   * vm_normal_page -- This function gets the "struct page" associated with a pte.
> >   *
> >   * "Special" mappings do not wish to be associated with a "struct page" (either
> >   * it doesn't exist, or it exists but they don't want to touch it). In this
> >   * case, NULL is returned here. "Normal" mappings do have a struct page.
> >   *
> >   * ...
> >   *
> >   */
>
> Well, yes, the one vm_normal_page() is all about ... ? :)

Lol yes to be fair that is pretty obvious...

>
> >
> > But don't we already violate this E.g.:
> >
> > 		if (vma->vm_ops && vma->vm_ops->find_special_page)
> > 			return vma->vm_ops->find_special_page(vma, addr);
> > > I mean this in itself perhaps means we should update this comment to say
> 'except
> > when file-backed and there is a find_special_page() hook'.
>
> I rather hope we severely break this case such that we can remove that hack.
>
> Read as in: I couldn't care less about this XEN hack, in particular, not
> documenting it.
>
> I was already wondering about hiding it behind a XEN config so not each and
> every sane user of this function has to perform this crappy-hack check.

Yeah, I'm not a fan of generalised hooks if they can be avoided, especially ones
where you pass critical data structures like VMAs.

It means you can, in theory, make no assumptions about what the caller does and
yeah.

To do this for such a stupid edge case is ridiculous.

>
> [...]
>
> > >   	}
> > >
> > > -	entry = pud_mkhuge(pfn_t_pud(pfn, prot));
> > > -	if (pfn_t_devmap(pfn))
> > > -		entry = pud_mkdevmap(entry);
> > > -	else
> > > -		entry = pud_mkspecial(entry);
> > > +	if (fop.is_folio) {
> > > +		entry = folio_mk_pud(fop.folio, vma->vm_page_prot);
> > > +
> > > +		folio_get(fop.folio);
> > > +		folio_add_file_rmap_pud(fop.folio, &fop.folio->page, vma);
> > > +		add_mm_counter(mm, mm_counter_file(fop.folio), HPAGE_PUD_NR);
> >
> > Nit, but might be nice to abstract for PMD/PUD.
>
> Which part exactly? Likely a follow-up if it should be abstracted.

Ah on second thoughts it doesn't matter, because you're using pud variants of
everything such that it wouldn't be worth it.

Disregard this ;)

>
> >
> > > +	} else {
> > > +		entry = pud_mkhuge(pfn_t_pud(fop.pfn, prot));
> >
> > Same incredibly pedantic whitespace comment from previous patch :)
>
> ;)
>
>
> --
> Cheers,
>
> David / dhildenb
>