Re: [PATCH v1] x86/mm/pat: (un)track_pfn_copy() fix + improvements

[David Hildenbrand]

On 07.04.25 18:50, Ingo Molnar wrote:
> * David Hildenbrand <david@xxxxxxxxxx> wrote:
>
> > On 06.04.25 19:28, Ingo Molnar wrote:
> > > * David Hildenbrand <david@xxxxxxxxxx> wrote:
> > >
> > > > We got a late smatch warning and some additional review feedback.
> > > >
> > > > 	smatch warnings:
> > > > 	mm/memory.c:1428 copy_page_range() error: uninitialized symbol 'pfn'.
> > >
> > > > -	if (!(src_vma->vm_flags & VM_PAT))
> > > > +	if (!(src_vma->vm_flags & VM_PAT)) {
> > > > +		*pfn = 0;
> > > >    		return 0;
> > > > +	}
> > >
> > > >    static inline int track_pfn_copy(struct vm_area_struct *dst_vma,
> > > >    		struct vm_area_struct *src_vma, unsigned long *pfn)
> > > >    {
> > > > +	*pfn = 0;
> > > >    	return 0;
> > > >    }
> > >
> > > That's way too ugly. There's nothing wrong with not touching 'pfn'
> > > in the error path: in fact it's pretty standard API where output
> > > pointers may not get set on errors.
> >
> > We're not concerned about the error path, though.
>
> Sorry, indeed, not an error path, but the !VM_PAT path above - but
> still a similar argument applies IMHO.
>
> > > If Smatch has a problem with it, Smatch should be fixed, or the false
> > > positive warning should be worked around by initializing 'pfn' in the
> > > callers.
> >
> > We could adjust the documentation of track_pfn_copy, to end up with the
> > following:
> >
> > diff --git a/include/linux/pgtable.h b/include/linux/pgtable.h
> > index e2b705c149454..b50447ef1c921 100644
> > --- a/include/linux/pgtable.h
> > +++ b/include/linux/pgtable.h
> > @@ -1511,8 +1511,9 @@ static inline void track_pfn_insert(struct vm_area_struct *vma, pgprot_t *prot,
> >   /*
> >    * track_pfn_copy is called when a VM_PFNMAP VMA is about to get the page
> > - * tables copied during copy_page_range(). On success, stores the pfn to be
> > - * passed to untrack_pfn_copy().
> > + * tables copied during copy_page_range(). Will store the pfn to be
> > + * passed to untrack_pfn_copy() only if there is something to be untracked.
> > + * Callers should initialize the pfn to 0.
> >    */
> >   static inline int track_pfn_copy(struct vm_area_struct *dst_vma,
> >                  struct vm_area_struct *src_vma, unsigned long *pfn)
> > @@ -1522,7 +1523,9 @@ static inline int track_pfn_copy(struct vm_area_struct *dst_vma,
> >   /*
> >    * untrack_pfn_copy is called when a VM_PFNMAP VMA failed to copy during
> > - * copy_page_range(), but after track_pfn_copy() was already called.
> > + * copy_page_range(), but after track_pfn_copy() was already called. Can
> > + * be called even if track_pfn_copy() did not actually track anything:
> > + * handled internally.
> >    */
> >   static inline void untrack_pfn_copy(struct vm_area_struct *dst_vma,
> >                  unsigned long pfn)
> > diff --git a/mm/memory.c b/mm/memory.c
> > index 2d8c265fc7d60..1a35165622e1c 100644
> > --- a/mm/memory.c
> > +++ b/mm/memory.c
> > @@ -1361,7 +1361,7 @@ copy_page_range(struct vm_area_struct *dst_vma, struct vm_area_struct *src_vma)
> >          struct mm_struct *dst_mm = dst_vma->vm_mm;
> >          struct mm_struct *src_mm = src_vma->vm_mm;
> >          struct mmu_notifier_range range;
> > -       unsigned long next, pfn;
> > +       unsigned long next, pfn = 0;
>
> Ack.
>
> I hate it how uninitialized variables are even a thing in C, and why
> there's no compiler switch to turn it off for the kernel. (At least for
> non-struct variables. Even for structs I would zero-initialize and
> *maybe* allow a non-initialized opt-in for cases where it matters. It
> matters in very few cases in praxis. And don't get me started about the
> stupidity that is to not initialize holes in struct members ...)
>
> Over the decades we've lived through numerous nasty bugs for very
> little tangible code generation benefits.

Ok, let me resend with that. (I'll still tag it as a fix due do the 
weird UB scenario when passing uninitialized values to a non-inline 
function ...)

--
Cheers,

David / dhildenb