linux-kernel - Re: [PATCH] mm: Wire up tail page poisoning over ->mappings

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: <ZOLL7f+Ihc93lyo0@casper.infradead.org>
Date:   Mon, 21 Aug 2023 03:29:01 +0100
From:   Matthew Wilcox <willy@...radead.org>
To:     Peter Xu <peterx@...hat.com>
Cc:     linux-mm@...ck.org, linux-kernel@...r.kernel.org,
        Yang Shi <shy828301@...il.com>,
        Mike Kravetz <mike.kravetz@...cle.com>,
        David Hildenbrand <david@...hat.com>,
        "Kirill A . Shutemov" <kirill@...temov.name>,
        Hugh Dickins <hughd@...gle.com>,
        Andrew Morton <akpm@...ux-foundation.org>
Subject: Re: [PATCH] mm: Wire up tail page poisoning over ->mappings

On Sun, Aug 20, 2023 at 09:13:55PM -0400, Peter Xu wrote:
> > https://lore.kernel.org/linux-mm/ZNp7yUgUrIpILnXu@casper.infradead.org/
> 
> https://lore.kernel.org/linux-mm/ZNqFv0AwkfDKExiw@x1n/#t
> 
> Firstly, I've answered and you didn't follow that up.

I didn't see it.  I get a lot of email ...

> > > More importantly, I think this is over-parametrisation.  If you start to
> > > use extra fields in struct folio, just change the code in page_alloc.c
> > > directly.
> 
> Change the hard-coded "2"s in different functions?  Can you kindly explain
> why can't we just have a macro to help?

Because it's unnecessary.  You're putting in way too much effort here
for something that might happen once.

> Setting tail mapping for tail 1/2 is even wrong, which part of this patch
> fixes:
> 
> @@ -428,7 +428,8 @@ static inline void prep_compound_tail(struct page *head, int tail_idx)
>  {
>         struct page *p = head + tail_idx;
> 
> -       p->mapping = TAIL_MAPPING;
> +       if (tail_idx > TAIL_MAPPING_REUSED_MAX)
> +               p->mapping = TAIL_MAPPING;
>         set_compound_head(p, head);
>         set_page_private(p, 0);
>  }

I didn't see this.  This is wrong.  tail->mapping is only reused for
large rmappable pages.  It's not reused for other compound pages.

If you really insist on cleaning this up, the special casing of tail pages
should move out of page_alloc entirely.  folio_undo_large_rmappable()
should restore TAIL_MAPPING for all tail pages that were modified by
folio_prep_large_rmappable().

The other thing we should do is verify that the additional large-rmap
fields have the correct values in folio_undo_large_rmappable().

But let's look back to why TAIL_MAPPING was introduced.  Commit
1c290f642101e poisoned tail->mapping to catch users which forgot to
call compound_head().  So we can actually remove TAIL_MAPPING entirely
if we get rid of page->mapping.

You probably think that's an unattainable goal; there are something like
340 occurrences of the string 'page->mapping' in the kernel right now
(and there are probably instances of struct page named something other
than 'page'), but a lot of those are actually in comments, which would
be my fault for not fixing them during folio conversions.

However, I have a small patch series which enables 'allnoconfig' to
build after renaming page->mapping to page->_mapping.  Aside from fs/
there are *very* few places which look at page->mapping [1].  I'll post
that patch series tomorrow.

I think with some serious work, we can land "remove page->mapping"
(which would include removing TAIL_MAPPING) by the end of the year.
And that work gets us closer to the goal of shrinking struct page.

[1] device-dax, intel_th, mthca, cortina, fb_defio