linux-kernel - Re: [PATCH v4 4/4] mm/gup: clean up codes in fault_in

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: <Z/sOUfK0VoiC7d+w@MiWiFi-R3L-srv>
Date: Sun, 13 Apr 2025 09:07:29 +0800
From: Baoquan He <bhe@...hat.com>
To: David Hildenbrand <david@...hat.com>
Cc: linux-mm@...ck.org, akpm@...ux-foundation.org, osalvador@...e.de,
	yanjun.zhu@...ux.dev, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v4 4/4] mm/gup: clean up codes in fault_in_xxx() functions

On 04/11/25 at 01:41pm, David Hildenbrand wrote:
> On 11.04.25 13:15, Baoquan He wrote:
> > On 04/11/25 at 10:54am, David Hildenbrand wrote:
> > > On 10.04.25 05:57, Baoquan He wrote:
> > > > The code style in fault_in_readable() and fault_in_writable() is a
> > > > little inconsistent with fault_in_safe_writeable(). In fault_in_readable()
> > > > and fault_in_writable(), it uses 'uaddr' passed in as loop cursor. While
> > > > in fault_in_safe_writeable(), local variable 'start' is used as loop
> > > > cursor. This may mislead people when reading code or making change in
> > > > these codes.
> > > > 
> > > > Here define explicit loop cursor and use for loop to simplify codes in
> > > > these three functions. These cleanup can make them be consistent in
> > > > code style and improve readability.
> > > > 
> > > > Signed-off-by: Baoquan He <bhe@...hat.com>
> > > > ---
> > > >    mm/gup.c | 65 +++++++++++++++++++++++---------------------------------
> > > >    1 file changed, 26 insertions(+), 39 deletions(-)
> > > > 
> > > > diff --git a/mm/gup.c b/mm/gup.c
> > > > index 77a5bc622567..a76bd7e90a71 100644
> > > > --- a/mm/gup.c
> > > > +++ b/mm/gup.c
> > > > @@ -2113,28 +2113,24 @@ static long __get_user_pages_locked(struct mm_struct *mm, unsigned long start,
> > > >     */
> > > >    size_t fault_in_writeable(char __user *uaddr, size_t size)
> > > >    {
> > > > -	char __user *start = uaddr, *end;
> > > > +	const unsigned long start = (unsigned long)uaddr;
> > > > +	const unsigned long end = start + size;
> > > > +	unsigned long cur = start;
> > > 
> > > I would initialize cur in the for loop header, makes the loop easier to
> > > read.
> > 
> > Both is fine to me. It's to satisfy checkpatch.sh which complains about
> > exceeding 80 char in the line.
> 
> Did checkpatch.sh actually complain? You might be happy to learn that the
> new limit is 100. :)

That's great to know. I never noticed this and always wrap via vim's
indication.

> 
> [...]
> 
> > > /* Stop once we overflow to 0. */
> > > end = PAGE_ALIGN(end)
> > > if (start < end)
> > > 	end = 0;
> > > 
> > > for (cur = start; cur != end; cur = PAGE_ALIGN_DOWN(cur + PAGE_SIZE))
> > > 	unsafe_put_user(0, (char __user *)cur, out);
> > > 
> > > Essentially, removing the "cur" check from the loop condition. Not sure if
> > > that is better.
> > 
> > The current code is simpler. Your now saying may save the CPU execution
> > instructions a little bit. Both is fine to me.
> > 
> > I don't have strong preference, I can make v4 to address these concerns
> > if decided. Thanks for careful checking.
> 
> Whatever you prefer!

Great, will make change in v4. Thx.