linux-kernel - Re: [PATCH v4 4/5] mm: make vma cache SLAB_TYPESAFE_BY

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: <CAJuCfpFO3Hj+7f10e0Pnvf0U7-dHeYgvjK+4AFD8V=kmG4JA=w@mail.gmail.com>
Date: Tue, 19 Nov 2024 22:37:37 -0800
From: Suren Baghdasaryan <surenb@...gle.com>
To: Matthew Wilcox <willy@...radead.org>
Cc: akpm@...ux-foundation.org, liam.howlett@...cle.com, 
	lorenzo.stoakes@...cle.com, mhocko@...e.com, vbabka@...e.cz, 
	hannes@...xchg.org, mjguzik@...il.com, oliver.sang@...el.com, 
	mgorman@...hsingularity.net, david@...hat.com, peterx@...hat.com, 
	oleg@...hat.com, dave@...olabs.net, paulmck@...nel.org, brauner@...nel.org, 
	dhowells@...hat.com, hdanton@...a.com, hughd@...gle.com, minchan@...gle.com, 
	jannh@...gle.com, shakeel.butt@...ux.dev, souravpanda@...gle.com, 
	pasha.tatashin@...een.com, corbet@....net, linux-doc@...r.kernel.org, 
	linux-mm@...ck.org, linux-kernel@...r.kernel.org, kernel-team@...roid.com
Subject: Re: [PATCH v4 4/5] mm: make vma cache SLAB_TYPESAFE_BY_RCU

On Tue, Nov 19, 2024 at 8:36 PM Matthew Wilcox <willy@...radead.org> wrote:
>
> On Tue, Nov 19, 2024 at 04:08:25PM -0800, Suren Baghdasaryan wrote:
> > +static inline void vma_clear(struct vm_area_struct *vma)
> > +{
> > +     /* Preserve vma->vm_lock */
> > +     memset(vma, 0, VMA_BEFORE_LOCK);
> > +     memset(VMA_LOCK_END(vma), 0, VMA_AFTER_LOCK);
> > +}
>
> This isn't how you're supposed to handle constructors.  You've fixed
> the immediate problem rather than writing the code in the intended style.

Yeah, I don't like this myself but the only alternative I can think of
is to set the struct members individually.

>
> > +static void vm_area_ctor(void *data)
> > +{
> > +     vma_lock_init(data);
> > +}
>
> After the ctor has run, the object should be in the same state as
> it is after it's freed.  If you want to memset the entire thing
> then you can do it in the ctor.  But there should be no need to
> do it in vma_init().

IIUC, your suggestion is to memset() the vma and initialize vm_lock
inside the ctor. Then when it's time to free the vma, we reset all
members except vm_lock before freeing the vma. As you mention later,
members like anon_vma_chain, which are already clear, also won't need
to be reset at this point. Am I understanding your proposal correctly?

BTW, if so, then vma_copy() will have to also copy vma members individually.

>
> And there's lots of things you can move from vma_init() to the ctor.
> For example, at free time, anon_vma_chain should be an empty list.
> So if you init it in the ctor, you can avoid doing it in vma_init().

True.

> I'd suggest that vma_numab_state_free() should be the place which
> sets vma->numab_state to NULL and we can delete vma_numab_state_init()
> entirely.

Sounds good to me.

Please confirm if I correctly got your idea and I'll update this patch.
Thanks for the feedback!

>