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: <f03f7e13-3d37-4d4a-87a6-61731744f476@redhat.com>
Date: Fri, 11 Apr 2025 10:54:09 +0200
From: David Hildenbrand <david@...hat.com>
To: Baoquan He <bhe@...hat.com>, linux-mm@...ck.org
Cc: 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 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.

>   
>   	if (unlikely(size == 0))
>   		return 0;
> +

Would not add that line to keep it like fault_in_readable() below.

>   	if (!user_write_access_begin(uaddr, size))
>   		return size;
> -	if (!PAGE_ALIGNED(uaddr)) {
> -		unsafe_put_user(0, uaddr, out);
> -		uaddr = (char __user *)PAGE_ALIGN((unsigned long)uaddr);
> -	}
> -	end = (char __user *)PAGE_ALIGN((unsigned long)start + size);
> -	if (unlikely(end < start))
> -		end = NULL;
> -	while (uaddr != end) {
> -		unsafe_put_user(0, uaddr, out);
> -		uaddr += PAGE_SIZE;
> -	}
> +
> +	/* Stop once we overflow to 0. */
> +	for (; cur && cur < end; cur = PAGE_ALIGN_DOWN(cur + PAGE_SIZE))
> +		unsafe_put_user(0, (char __user *)cur, out);
>   

Staring at fault_in_safe_writeable(), we could also do

/* 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.

In any case, if all functions later look similar and clearer it's a big win.

-- 
Cheers,

David / dhildenb