[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <12196206-16b0-4913-b087-1f59ff808603@redhat.com>
Date: Mon, 7 Apr 2025 20:33:53 +0200
From: David Hildenbrand <david@...hat.com>
To: Ingo Molnar <mingo@...nel.org>
Cc: linux-kernel@...r.kernel.org, linux-mm@...ck.org, x86@...nel.org,
kernel test robot <lkp@...el.com>, Dan Carpenter <error27@...il.com>,
Andrew Morton <akpm@...ux-foundation.org>,
Lorenzo Stoakes <lorenzo.stoakes@...cle.com>,
Dave Hansen <dave.hansen@...ux.intel.com>, Andy Lutomirski
<luto@...nel.org>, Peter Zijlstra <peterz@...radead.org>,
Thomas Gleixner <tglx@...utronix.de>, Ingo Molnar <mingo@...hat.com>,
Borislav Petkov <bp@...en8.de>, Rik van Riel <riel@...riel.com>,
"H. Peter Anvin" <hpa@...or.com>,
Linus Torvalds <torvalds@...ux-foundation.org>
Subject: Re: [PATCH v1] x86/mm/pat: (un)track_pfn_copy() fix + improvements
On 07.04.25 18:50, Ingo Molnar wrote:
>
> * David Hildenbrand <david@...hat.com> wrote:
>
>> On 06.04.25 19:28, Ingo Molnar wrote:
>>>
>>> * David Hildenbrand <david@...hat.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
Powered by blists - more mailing lists