linux-kernel - Re: [RFC PATCH 1/5] mm: madvise: refactor madvise

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: <aC8ZY_B7RKc9RMzw@kernel.org>
Date: Thu, 22 May 2025 15:32:35 +0300
From: Mike Rapoport <rppt@...nel.org>
To: David Hildenbrand <david@...hat.com>
Cc: Lorenzo Stoakes <lorenzo.stoakes@...cle.com>,
	Andrew Morton <akpm@...ux-foundation.org>,
	"Liam R . Howlett" <Liam.Howlett@...cle.com>,
	Vlastimil Babka <vbabka@...e.cz>, Jann Horn <jannh@...gle.com>,
	Arnd Bergmann <arnd@...db.de>,
	Christian Brauner <brauner@...nel.org>, linux-mm@...ck.org,
	linux-arch@...r.kernel.org, linux-kernel@...r.kernel.org,
	SeongJae Park <sj@...nel.org>, Usama Arif <usamaarif642@...il.com>
Subject: Re: [RFC PATCH 1/5] mm: madvise: refactor madvise_populate()

On Tue, May 20, 2025 at 12:42:33PM +0200, David Hildenbrand wrote:
> On 20.05.25 12:36, Lorenzo Stoakes wrote:
> > On Tue, May 20, 2025 at 12:30:24PM +0200, David Hildenbrand wrote:
> > > On 19.05.25 22:52, Lorenzo Stoakes wrote:
> > > > Use a for-loop rather than a while with the update of the start argument at
> > > > the end of the while-loop.
> > > > 
> > > > This is in preparation for a subsequent commit which modifies this
> > > > function, we therefore separate the refactoring from the actual change
> > > > cleanly by separating the two.
> > > > 
> > > > Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@...cle.com>
> > > > ---
> > > >    mm/madvise.c | 39 ++++++++++++++++++++-------------------
> > > >    1 file changed, 20 insertions(+), 19 deletions(-)
> > > > 
> > > > diff --git a/mm/madvise.c b/mm/madvise.c
> > > > index 8433ac9b27e0..63cc69daa4c7 100644
> > > > --- a/mm/madvise.c
> > > > +++ b/mm/madvise.c
> > > > @@ -967,32 +967,33 @@ static long madvise_populate(struct mm_struct *mm, unsigned long start,
> > > >    	int locked = 1;
> > > >    	long pages;
> > > > -	while (start < end) {
> > > > +	for (; start < end; start += pages * PAGE_SIZE) {
> > > >    		/* Populate (prefault) page tables readable/writable. */
> > > >    		pages = faultin_page_range(mm, start, end, write, &locked);
> > > >    		if (!locked) {
> > > >    			mmap_read_lock(mm);
> > > >    			locked = 1;
> > > >    		}
> > > > -		if (pages < 0) {
> > > > -			switch (pages) {
> > > > -			case -EINTR:
> > > > -				return -EINTR;
> > > > -			case -EINVAL: /* Incompatible mappings / permissions. */
> > > > -				return -EINVAL;
> > > > -			case -EHWPOISON:
> > > > -				return -EHWPOISON;
> > > > -			case -EFAULT: /* VM_FAULT_SIGBUS or VM_FAULT_SIGSEGV */
> > > > -				return -EFAULT;
> > > > -			default:
> > > > -				pr_warn_once("%s: unhandled return value: %ld\n",
> > > > -					     __func__, pages);
> > > > -				fallthrough;
> > > > -			case -ENOMEM: /* No VMA or out of memory. */
> > > > -				return -ENOMEM;
> > > > -			}
> > > > +
> > > > +		if (pages >= 0)
> > > > +			continue;
> > > > +
> > > > +		switch (pages) {
> > > > +		case -EINTR:
> > > > +			return -EINTR;
> > > > +		case -EINVAL: /* Incompatible mappings / permissions. */
> > > > +			return -EINVAL;
> > > > +		case -EHWPOISON:
> > > > +			return -EHWPOISON;
> > > > +		case -EFAULT: /* VM_FAULT_SIGBUS or VM_FAULT_SIGSEGV */
> > > > +			return -EFAULT;
> > > > +		default:
> > > > +			pr_warn_once("%s: unhandled return value: %ld\n",
> > > > +				     __func__, pages);
> > > > +			fallthrough;
> > > > +		case -ENOMEM: /* No VMA or out of memory. */
> > > > +			return -ENOMEM;
> > > 
> > > Can we limit it to what the patch description says? "Use a for-loop rather
> > > than a while", or will that be a problem for the follow-up patch?
> > 
> > Well, kind of the point is that we can remove a level of indentation also, which
> > then makes life easier in subsequent patch.
> > 
> > Happy to change description or break into two (but that seems a bit over the top
> > maybe? :>)
> 
> Probably just mention it, otherwise it looks a bit like unrelated churn :)

And for refactoring patches it's always useful to mention "no functional
change" ;-)

Acked-by: Mike Rapoport (Microsoft) <rppt@...nel.org>
 
> Acked-by: David Hildenbrand <david@...hat.com>
> 
> -- 
> Cheers,
> 
> David / dhildenb
> 
> 

-- 
Sincerely yours,
Mike.