Re: [PATCH v2] mm/gup: document unlocked invariant in fixup_user_fault

[David Hildenbrand (Arm)]

On 10/5/26 11:37, Nguyen Duy Nhat Anh wrote:
> Static analysis tools flag potential NULL pointer
> dereferences of 'unlocked' in fixup_user_fault() when handling
> VM_FAULT_COMPLETED or VM_FAULT_RETRY.
> 
> These warnings are false positives. 'unlocked' is only dereferenced when
> handle_mm_fault() returns VM_FAULT_COMPLETED or VM_FAULT_RETRY. Both of
> these return codes require FAULT_FLAG_ALLOW_RETRY to be set in
> fault_flags, which fixup_user_fault() only sets if 'unlocked' is non-NULL
> upon entry. Therefore, if 'unlocked' is NULL, the control flow branches
> that dereference 'unlocked' are unreachable.
> 
> However, this part of the code is subtle and can trip up
> contributors or automated tools. Document this invariant with a comment
> above 'if (unlocked)' where fault_flags is constructed, explaining why
> omitting FAULT_FLAG_ALLOW_RETRY guarantees 'unlocked' will not be
> dereferenced later in the fault recovery loop.
> 
> Suggested-by: Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx>
> Signed-off-by: Nguyen Duy Nhat Anh <neganhat@xxxxxxxxx>
> ---
> v1: https://lore.kernel.org/linux-mm/20261003194846.205918-1-neganhat@xxxxxxxxx/
> 
> v2 changes:
> - Instead of adding runtime NULL checks at dereference sites,
>   document the FAULT_FLAG_ALLOW_RETRY invariant above if (unlocked).
> ---
>  mm/gup.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/mm/gup.c b/mm/gup.c
> index eb898ea1ee22..58fa75441da1 100644
> --- a/mm/gup.c
> +++ b/mm/gup.c
> @@ -1570,6 +1570,12 @@ int fixup_user_fault(struct mm_struct *mm,
>  
>  	address = untagged_addr_remote(mm, address);
>  
> +	/*
> +	 * If the caller passes 'unlocked' as NULL, FAULT_FLAG_ALLOW_RETRY is omitted.
> +	 * This guarantees handle_mm_fault() will never drop the lock or
> +	 * return VM_FAULT_COMPLETED / VM_FAULT_RETRY, making subsequent
> +	 * dereferences of 'unlocked' unreachable when NULL.
> +	 */
>  	if (unlocked)
>  		fault_flags |= FAULT_FLAG_ALLOW_RETRY | FAULT_FLAG_KILLABLE;
>  

For which audience is the comment targeted? For people that know what they are
doing? No.

For tools that report such problems? No.

So I don't think this (4 lines of comment) is the right way to improve this.

-- 
Cheers,

David