[PATCH] mm/gup: add NULL check for unlocked parameter in fixup_user_fault()

[PATCH] mm/gup: add NULL check for unlocked parameter in fixup_user_fault()

[Liu Dalin]

While the current callers either pass a valid pointer or explicitly
pass NULL (indicating they don't need the unlock notification), it is
safer to add a defensive NULL check before dereferencing. This prevents
a kernel crash if any caller passes NULL and handle_mm_fault() returns
VM_FAULT_COMPLETED or VM_FAULT_RETRY.

Fixes smatch warnings:
- mm/gup.c:1597 fixup_user_fault() error: we previously assumed 'unlocked' could be null (see line 1573)

Fixes: 4bbd4c776a63 ("mm: move get_user_pages()-related code to separate file")
Assisted-by: smatch:2.0 [static analysis]
Signed-off-by: Liu Dalin <liudalin@kylinsec.com.cn>
---
 mm/gup.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/mm/gup.c b/mm/gup.c
index 0692119b7904..d12475a7f7cc 100644
--- a/mm/gup.c
+++ b/mm/gup.c
@@ -1594,7 +1594,8 @@ int fixup_user_fault(struct mm_struct *mm,
 		 * could tell the callers so they do not need to unlock.
 		 */
 		mmap_read_lock(mm);
-		*unlocked = true;
+		if (unlocked)
+			*unlocked = true;
 		return 0;
 	}
 
@@ -1608,7 +1609,8 @@ int fixup_user_fault(struct mm_struct *mm,
 
 	if (ret & VM_FAULT_RETRY) {
 		mmap_read_lock(mm);
-		*unlocked = true;
+		if (unlocked)
+			*unlocked = true;
 		fault_flags |= FAULT_FLAG_TRIED;
 		goto retry;
 	}
-- 
2.43.0

Re: [PATCH] mm/gup: add NULL check for unlocked parameter in fixup_user_fault()

[Jason Gunthorpe]

On Wed, Aug 26, 2026 at 02:29:43PM +0800, Liu Dalin wrote:
> While the current callers either pass a valid pointer or explicitly
> pass NULL (indicating they don't need the unlock notification), it is
> safer to add a defensive NULL check before dereferencing. This prevents
> a kernel crash if any caller passes NULL and handle_mm_fault() returns
> VM_FAULT_COMPLETED or VM_FAULT_RETRY.
> 
> Fixes smatch warnings:
> - mm/gup.c:1597 fixup_user_fault() error: we previously assumed 'unlocked' could be null (see line 1573)

It would be better to remove the previous check:

        if (unlocked)
                fault_flags |= FAULT_FLAG_ALLOW_RETRY | FAULT_FLAG_KILLABLE;

And update the kdoc if it is now true that the unlocked parameter
cannot be null

Jason

[PATCH] mm/gup: add NULL check for unlocked parameter in fixup_user_fault()

[Liu Dalin]

While the current callers either pass a valid pointer or explicitly
pass NULL (indicating they don't need the unlock notification), it is
safer to add a defensive NULL check before dereferencing. This prevents
a kernel crash if any caller passes NULL and handle_mm_fault() returns
VM_FAULT_COMPLETED or VM_FAULT_RETRY.

Fixes smatch warnings:
- mm/gup.c:1597 fixup_user_fault() error: we previously assumed 'unlocked' could be null (see line 1573)

Fixes: 4bbd4c776a63 ("mm: move get_user_pages()-related code to separate file")
Assisted-by: smatch:2.0 [static analysis]
Signed-off-by: Liu Dalin <liudalin@kylinsec.com.cn>
---
 mm/gup.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/mm/gup.c b/mm/gup.c
index 0692119b7904..d12475a7f7cc 100644
--- a/mm/gup.c
+++ b/mm/gup.c
@@ -1594,7 +1594,8 @@ int fixup_user_fault(struct mm_struct *mm,
 		 * could tell the callers so they do not need to unlock.
 		 */
 		mmap_read_lock(mm);
-		*unlocked = true;
+		if (unlocked)
+			*unlocked = true;
 		return 0;
 	}
 
@@ -1608,7 +1609,8 @@ int fixup_user_fault(struct mm_struct *mm,
 
 	if (ret & VM_FAULT_RETRY) {
 		mmap_read_lock(mm);
-		*unlocked = true;
+		if (unlocked)
+			*unlocked = true;
 		fault_flags |= FAULT_FLAG_TRIED;
 		goto retry;
 	}
-- 
2.43.0

Re: [PATCH] mm/gup: add NULL check for unlocked parameter in fixup_user_fault()

[David Hildenbrand (Arm)]

On 8/26/26 08:31, Liu Dalin wrote:
> While the current callers either pass a valid pointer or explicitly
> pass NULL (indicating they don't need the unlock notification), it is
> safer to add a defensive NULL check before dereferencing. This prevents
> a kernel crash if any caller passes NULL and handle_mm_fault() returns
> VM_FAULT_COMPLETED or VM_FAULT_RETRY.
> 
> Fixes smatch warnings:
> - mm/gup.c:1597 fixup_user_fault() error: we previously assumed 'unlocked' could be null (see line 1573)
> 
> Fixes: 4bbd4c776a63 ("mm: move get_user_pages()-related code to separate file")

If this is not an actual bugfix, we should specify Fixes:.

> Assisted-by: smatch:2.0 [static analysis]
> Signed-off-by: Liu Dalin <liudalin@kylinsec.com.cn>

You seem to have sent this patch twice by accident. :)

> ---
>  mm/gup.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/mm/gup.c b/mm/gup.c
> index 0692119b7904..d12475a7f7cc 100644
> --- a/mm/gup.c
> +++ b/mm/gup.c
> @@ -1594,7 +1594,8 @@ int fixup_user_fault(struct mm_struct *mm,
>  		 * could tell the callers so they do not need to unlock.
>  		 */
>  		mmap_read_lock(mm);
> -		*unlocked = true;
> +		if (unlocked)
> +			*unlocked = true;
>  		return 0;
>  	}
>  
> @@ -1608,7 +1609,8 @@ int fixup_user_fault(struct mm_struct *mm,
>  
>  	if (ret & VM_FAULT_RETRY) {
>  		mmap_read_lock(mm);
> -		*unlocked = true;
> +		if (unlocked)
> +			*unlocked = true;
>  		fault_flags |= FAULT_FLAG_TRIED;
>  		goto retry;
>  	}

smatch doesn't understand that VM_FAULT_RETRY|VM_FAULT_COMPLETED implies that
FAULT_FLAG_ALLOW_RETRY must be set, which implies that the lock may be dropped,
which implies that unlocked is mandatory to be specified by the caller.

Checking for "unlocked" when we just did a mmap_read_lock() does not make any
sense, sorry.

Is there a way to teach smatch about this differently?

VM_WARN_ON_ONCE(unlocked);

could be used, but I am also not quite happy about that.

-- 
Cheers,

David