Re: [bug report] mm/pgtable: allow pte_offset_map[_lock]() to fail

[Hugh Dickins <hughd@google.com>]

On Tue, 11 Jul 2023, Dan Carpenter wrote:

> Hello Hugh Dickins,
> 
> The patch 0d940a9b270b: "mm/pgtable: allow pte_offset_map[_lock]() to
> fail" from Jun 8, 2023, leads to the following Smatch static checker
> warning:
> 
> 	mm/userfaultfd.c:321 mfill_atomic_pte_poison()
> 	error: uninitialized symbol 'ptl'.

In mm-unstable or linux-next: not a problem in 6.5-rc.
Thanks, Dan: yes.

A little unfair to blame my patch for code that went in later, but it's
a good reminder that I do need to keep checking for new usages of
pte_offset_map[_lock](), and this is the only one so far.
Great that Smatch is helping me with that.

Fix to the guilty patch follows.

Hugh

> 
> mm/userfaultfd.c
>     292 static int mfill_atomic_pte_poison(pmd_t *dst_pmd,
>     293                                    struct vm_area_struct *dst_vma,
>     294                                    unsigned long dst_addr,
>     295                                    uffd_flags_t flags)
>     296 {
>     297         int ret;
>     298         struct mm_struct *dst_mm = dst_vma->vm_mm;
>     299         pte_t _dst_pte, *dst_pte;
>     300         spinlock_t *ptl;
>     301 
>     302         _dst_pte = make_pte_marker(PTE_MARKER_POISONED);
>     303         dst_pte = pte_offset_map_lock(dst_mm, dst_pmd, dst_addr, &ptl);
>                                                                          ^^^^
> The __pte_offset_map_lock() function does not initialize ptl if it
> returns NULL.
> 
>     304 
>     305         if (mfill_file_over_size(dst_vma, dst_addr)) {
>     306                 ret = -EFAULT;
>     307                 goto out_unlock;
>     308         }
>     309 
>     310         ret = -EEXIST;
>     311         /* Refuse to overwrite any PTE, even a PTE marker (e.g. UFFD WP). */
>     312         if (!pte_none(*dst_pte))
>     313                 goto out_unlock;
>     314 
>     315         set_pte_at(dst_mm, dst_addr, dst_pte, _dst_pte);
>     316 
>     317         /* No need to invalidate - it was non-present before */
>     318         update_mmu_cache(dst_vma, dst_addr, dst_pte);
>     319         ret = 0;
>     320 out_unlock:
> --> 321         pte_unmap_unlock(dst_pte, ptl);
>     322         return ret;
>     323 }
> 
> regards,
> dan carpenter