Re: [PATCH v2 4/5] mm: make vma cache SLAB_TYPESAFE_BY_RCU

[Jann Horn]

On Wed, Nov 13, 2024 at 9:59 PM Matthew Wilcox <willy@xxxxxxxxxxxxx> wrote:
> On Wed, Nov 13, 2024 at 05:44:00PM +0100, Jann Horn wrote:
> > Something like NULL or (void*)1 is fine with me but please don't do
> > pointer-to-itself - we shouldn't unnecessarily store a pointer to an
> > object of one type in a pointer field of an incompatible type, that
> > increases the risk of creating type confusion issues (both in the
> > memory corruption sense and in the Spectre sense). I know MM already
> > has several places where similar stuff can happen (in particular
> > page->mapping), but here it seems like unnecessary risk to me.
>
> Hm?  I don't think page->mapping can ever point at page.  As far as I
> know, we have four cases, discriminated by the bottom two bits:
>
> 0 - NULL or address_space
> 1 - anon_vma
> 2 - movable_ops
> 3 - ksm_stable_node

Oh, I didn't even know about cases 2 and 3.

Ah, yes, I didn't mean it can point at itself, I meant the pattern of
having a pointer declared as "points to type A" ("struct address_space
*mapping") while it actually points at other types sometimes.

> In fact, we're almost done eliminating page->mapping.  Just a few
> filesystems and device drivers left to go.

Ah, you mean by replacing it with folio->mapping as in
https://lore.kernel.org/all/20241025190822.1319162-4-willy@xxxxxxxxxxxxx/
?

> Would it be halpful if we did:
>
> -       struct address_space *mapping;
> +       union {
> +               struct address_space *mapping;
> +               unsigned long raw_mapping;
> +       };
>
> and had non-filesystems use raw_mapping and do the masking?

While I think that would look a tiny bit tidier, I don't think it
would make a significant difference for page->mapping or
folio->mapping. In the context of the VMA field, the question is about
whether we make it possible for the same memory location to contain
values of different types, which I think increases the risk of
programmer mistakes or speculative confusions; while in the context of
the page->mapping field, the same memory location can contain values
of different types either way. So while aesthetically I think having a
union for the mapping field would look a little nicer, I don't
actually see much benefit in making such a change.