linux-kernel - Re: [RFC PATCH 02/16] mm/madvise: split out populate behavior check logic

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: <20250305231809.136776-1-sj@kernel.org>
Date: Wed,  5 Mar 2025 15:18:09 -0800
From: SeongJae Park <sj@...nel.org>
To: Shakeel Butt <shakeel.butt@...ux.dev>
Cc: SeongJae Park <sj@...nel.org>,
	"Liam R. Howlett" <howlett@...il.com>,
	Andrew Morton <akpm@...ux-foundation.org>,
	David Hildenbrand <david@...hat.com>,
	Lorenzo Stoakes <lorenzo.stoakes@...cle.com>,
	Vlastimil Babka <vbabka@...e.cz>,
	kernel-team@...a.com,
	linux-kernel@...r.kernel.org,
	linux-mm@...ck.org
Subject: Re: [RFC PATCH 02/16] mm/madvise: split out populate behavior check logic

On Wed, 5 Mar 2025 12:32:52 -0800 Shakeel Butt <shakeel.butt@...ux.dev> wrote:

> On Wed, Mar 05, 2025 at 10:15:57AM -0800, SeongJae Park wrote:
> > madvise_do_behavior() has a long open-coded 'behavior' check for
> > MADV_POPULATE_{READ,WRITE}.  It adds multiple layers[1] and make the
> > code arguably take longer time to read.  Like is_memory_failure(), split
> > out the check to a separate function.  This is not technically removing
> > the additional layer but discourage further extending the switch-case.
> > Also it makes madvise_do_behavior() code shorter and therefore easier to
> > read.
> > 
> > [1] https://lore.kernel.org/bd6d0bf1-c79e-46bd-a810-9791efb9ad73@lucifer.local
> > 
> > Signed-off-by: SeongJae Park <sj@...nel.org>
> > ---
> >  mm/madvise.c | 20 +++++++++++++-------
> >  1 file changed, 13 insertions(+), 7 deletions(-)
> > 
> > diff --git a/mm/madvise.c b/mm/madvise.c
> > index dbc8fec05cc6..4a91590656dc 100644
> > --- a/mm/madvise.c
> > +++ b/mm/madvise.c
> > @@ -1633,6 +1633,17 @@ static bool is_valid_madvise(unsigned long start, size_t len_in, int behavior)
> >  	return true;
> >  }
> >  
> > +static bool is_memory_populate(int behavior)
> 
> No strong opinion on this patch but if you want to keep it, the above
> name feels weird. How about either is_madvise_populate() or
> is_populate_memory()?

I wanted to make this reads consistent with other similar purpose ones like
is_memory_failure(behavior).  I have no strong opinions, either, though.
Unless someone makes a voice here, I will rename this to is_madvise_populate()
in the next version.

> 
> > +{
> > +	switch (behavior) {
> > +	case MADV_POPULATE_READ:
> > +	case MADV_POPULATE_WRITE:
> > +		return true;
> > +	default:
> > +		return false;
> > +	}
> > +}

Thanks,
SJ

[...]