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

[David Hildenbrand <david@redhat.com>]

On 07.04.25 18:50, Ingo Molnar wrote:
> 
> * David Hildenbrand <david@redhat.com> wrote:
> 
>> On 06.04.25 19:28, Ingo Molnar wrote:
>>>
>>> * David Hildenbrand <david@redhat.com> 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