[v1] 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()

Posted by Liu Dalin 1 month ago

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()

Posted by Jason Gunthorpe 1 month ago

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