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

[Nhật Anh Nguyễn Duy]

Hi David,

Thanks for taking the time to review.

My motivation came from running Smatch over fixup_user_fault(), which flagged
the dereference as a potential NULL pointer issue. Because the relationship
between FAULT_FLAG_ALLOW_RETRY and the unlocked pointer is subtle when
first reading through the path, I initially misread it as an unguarded
dereference in v1.

When Andrew pointed out the invariant, I thought documenting it might help
future readers or newer contributors who hit the same static analysis warning.

If adding a comment isn't the right approach here, would you prefer leaving
the implicit contract as-is, or is an explicit runtime check (such as
VM_WARN_ON_ONCE) something more fitting?

Thanks,
Anh

On Mon, Oct 5, 2026 at 5:29 PM David Hildenbrand (Arm) <david@xxxxxxxxxx> wrote:
>
> 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