* [PATCH v1] x86/mm/pat: (un)track_pfn_copy() fix + improvements
@ 2025-04-04 12:49 David Hildenbrand
2025-04-04 14:19 ` Lorenzo Stoakes
2025-04-06 17:28 ` Ingo Molnar
0 siblings, 2 replies; 10+ messages in thread
From: David Hildenbrand @ 2025-04-04 12:49 UTC (permalink / raw)
To: linux-kernel
Cc: linux-mm, x86, David Hildenbrand, kernel test robot,
Dan Carpenter, Andrew Morton, Lorenzo Stoakes, Dave Hansen,
Andy Lutomirski, Peter Zijlstra, Thomas Gleixner, Ingo Molnar,
Borislav Petkov, Rik van Riel, H. Peter Anvin, Linus Torvalds
We got a late smatch warning and some additional review feedback.
smatch warnings:
mm/memory.c:1428 copy_page_range() error: uninitialized symbol 'pfn'.
We actually use the pfn only when it is properly initialized; however,
we may pass an uninitialized value to a function -- although it will not
use it that likely still is UB in C.
Fix it by always initializing pfn when track_pfn_copy() returns 0 --
just as we document ("On success, stores the pfn to be passed to
untrack_pfn_copy()"). In addition, to avoid further wrong smatch
warnings, just initialize pfn = 0 in the caller as well.
While at it, clarify the doc of untrack_pfn_copy(), that internal checks
make sure if we actually have to untrack anything.
Fixes: dc84bc2aba85 ("x86/mm/pat: Fix VM_PAT handling when fork() fails in copy_page_range()")
Reported-by: kernel test robot <lkp@intel.com>
Reported-by: Dan Carpenter <error27@gmail.com>
Closes: https://lore.kernel.org/r/202503270941.IFILyNCX-lkp@intel.com/
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
Cc: Dave Hansen <dave.hansen@linux.intel.com>
Cc: Andy Lutomirski <luto@kernel.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Borislav Petkov <bp@alien8.de>
Cc: Rik van Riel <riel@surriel.com>
Cc: "H. Peter Anvin" <hpa@zytor.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Signed-off-by: David Hildenbrand <david@redhat.com>
---
arch/x86/mm/pat/memtype.c | 4 +++-
include/linux/pgtable.h | 5 ++++-
mm/memory.c | 2 +-
3 files changed, 8 insertions(+), 3 deletions(-)
diff --git a/arch/x86/mm/pat/memtype.c b/arch/x86/mm/pat/memtype.c
index 72d8cbc611583..9ad3e5b055d8a 100644
--- a/arch/x86/mm/pat/memtype.c
+++ b/arch/x86/mm/pat/memtype.c
@@ -992,8 +992,10 @@ int track_pfn_copy(struct vm_area_struct *dst_vma,
pgprot_t pgprot;
int rc;
- if (!(src_vma->vm_flags & VM_PAT))
+ if (!(src_vma->vm_flags & VM_PAT)) {
+ *pfn = 0;
return 0;
+ }
/*
* Duplicate the PAT information for the dst VMA based on the src
diff --git a/include/linux/pgtable.h b/include/linux/pgtable.h
index e2b705c149454..9457064292141 100644
--- a/include/linux/pgtable.h
+++ b/include/linux/pgtable.h
@@ -1517,12 +1517,15 @@ static inline void track_pfn_insert(struct vm_area_struct *vma, pgprot_t *prot,
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;
}
/*
* 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;
bool is_cow;
int ret;
--
2.48.1
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v1] x86/mm/pat: (un)track_pfn_copy() fix + improvements
2025-04-04 12:49 [PATCH v1] x86/mm/pat: (un)track_pfn_copy() fix + improvements David Hildenbrand
@ 2025-04-04 14:19 ` Lorenzo Stoakes
2025-04-06 17:28 ` Ingo Molnar
1 sibling, 0 replies; 10+ messages in thread
From: Lorenzo Stoakes @ 2025-04-04 14:19 UTC (permalink / raw)
To: David Hildenbrand
Cc: linux-kernel, linux-mm, x86, kernel test robot, Dan Carpenter,
Andrew Morton, Dave Hansen, Andy Lutomirski, Peter Zijlstra,
Thomas Gleixner, Ingo Molnar, Borislav Petkov, Rik van Riel,
H. Peter Anvin, Linus Torvalds
On Fri, Apr 04, 2025 at 02:49:31PM +0200, David Hildenbrand 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'.
>
> We actually use the pfn only when it is properly initialized; however,
> we may pass an uninitialized value to a function -- although it will not
> use it that likely still is UB in C.
>
> Fix it by always initializing pfn when track_pfn_copy() returns 0 --
> just as we document ("On success, stores the pfn to be passed to
> untrack_pfn_copy()"). In addition, to avoid further wrong smatch
> warnings, just initialize pfn = 0 in the caller as well.
>
> While at it, clarify the doc of untrack_pfn_copy(), that internal checks
> make sure if we actually have to untrack anything.
>
> Fixes: dc84bc2aba85 ("x86/mm/pat: Fix VM_PAT handling when fork() fails in copy_page_range()")
> Reported-by: kernel test robot <lkp@intel.com>
> Reported-by: Dan Carpenter <error27@gmail.com>
> Closes: https://lore.kernel.org/r/202503270941.IFILyNCX-lkp@intel.com/
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
> Cc: Dave Hansen <dave.hansen@linux.intel.com>
> Cc: Andy Lutomirski <luto@kernel.org>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Ingo Molnar <mingo@redhat.com>
> Cc: Borislav Petkov <bp@alien8.de>
> Cc: Rik van Riel <riel@surriel.com>
> Cc: "H. Peter Anvin" <hpa@zytor.com>
> Cc: Linus Torvalds <torvalds@linux-foundation.org>
> Signed-off-by: David Hildenbrand <david@redhat.com>
LGTM,
Reviewed-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
> ---
> arch/x86/mm/pat/memtype.c | 4 +++-
> include/linux/pgtable.h | 5 ++++-
> mm/memory.c | 2 +-
> 3 files changed, 8 insertions(+), 3 deletions(-)
>
> diff --git a/arch/x86/mm/pat/memtype.c b/arch/x86/mm/pat/memtype.c
> index 72d8cbc611583..9ad3e5b055d8a 100644
> --- a/arch/x86/mm/pat/memtype.c
> +++ b/arch/x86/mm/pat/memtype.c
> @@ -992,8 +992,10 @@ int track_pfn_copy(struct vm_area_struct *dst_vma,
> pgprot_t pgprot;
> int rc;
>
> - if (!(src_vma->vm_flags & VM_PAT))
> + if (!(src_vma->vm_flags & VM_PAT)) {
> + *pfn = 0;
> return 0;
> + }
>
> /*
> * Duplicate the PAT information for the dst VMA based on the src
> diff --git a/include/linux/pgtable.h b/include/linux/pgtable.h
> index e2b705c149454..9457064292141 100644
> --- a/include/linux/pgtable.h
> +++ b/include/linux/pgtable.h
> @@ -1517,12 +1517,15 @@ static inline void track_pfn_insert(struct vm_area_struct *vma, pgprot_t *prot,
> 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;
> }
>
> /*
> * 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;
> bool is_cow;
> int ret;
>
> --
> 2.48.1
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v1] x86/mm/pat: (un)track_pfn_copy() fix + improvements
2025-04-04 12:49 [PATCH v1] x86/mm/pat: (un)track_pfn_copy() fix + improvements David Hildenbrand
2025-04-04 14:19 ` Lorenzo Stoakes
@ 2025-04-06 17:28 ` Ingo Molnar
2025-04-07 7:23 ` David Hildenbrand
1 sibling, 1 reply; 10+ messages in thread
From: Ingo Molnar @ 2025-04-06 17:28 UTC (permalink / raw)
To: David Hildenbrand
Cc: linux-kernel, linux-mm, x86, kernel test robot, Dan Carpenter,
Andrew Morton, Lorenzo Stoakes, Dave Hansen, Andy Lutomirski,
Peter Zijlstra, Thomas Gleixner, Ingo Molnar, Borislav Petkov,
Rik van Riel, H. Peter Anvin, Linus Torvalds
* 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.
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.
Thanks,
Ingo
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v1] x86/mm/pat: (un)track_pfn_copy() fix + improvements
2025-04-06 17:28 ` Ingo Molnar
@ 2025-04-07 7:23 ` David Hildenbrand
2025-04-07 16:50 ` Ingo Molnar
0 siblings, 1 reply; 10+ messages in thread
From: David Hildenbrand @ 2025-04-07 7:23 UTC (permalink / raw)
To: Ingo Molnar
Cc: linux-kernel, linux-mm, x86, kernel test robot, Dan Carpenter,
Andrew Morton, Lorenzo Stoakes, Dave Hansen, Andy Lutomirski,
Peter Zijlstra, Thomas Gleixner, Ingo Molnar, Borislav Petkov,
Rik van Riel, H. Peter Anvin, Linus Torvalds
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.
>
> 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;
bool is_cow;
int ret;
--
Cheers,
David / dhildenb
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v1] x86/mm/pat: (un)track_pfn_copy() fix + improvements
2025-04-07 7:23 ` David Hildenbrand
@ 2025-04-07 16:50 ` Ingo Molnar
2025-04-07 18:33 ` David Hildenbrand
2025-04-07 18:33 ` Dan Carpenter
0 siblings, 2 replies; 10+ messages in thread
From: Ingo Molnar @ 2025-04-07 16:50 UTC (permalink / raw)
To: David Hildenbrand
Cc: linux-kernel, linux-mm, x86, kernel test robot, Dan Carpenter,
Andrew Morton, Lorenzo Stoakes, Dave Hansen, Andy Lutomirski,
Peter Zijlstra, Thomas Gleixner, Ingo Molnar, Borislav Petkov,
Rik van Riel, H. Peter Anvin, Linus Torvalds
* 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.
Thanks,
Ingo
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v1] x86/mm/pat: (un)track_pfn_copy() fix + improvements
2025-04-07 16:50 ` Ingo Molnar
@ 2025-04-07 18:33 ` David Hildenbrand
2025-04-07 18:33 ` Dan Carpenter
1 sibling, 0 replies; 10+ messages in thread
From: David Hildenbrand @ 2025-04-07 18:33 UTC (permalink / raw)
To: Ingo Molnar
Cc: linux-kernel, linux-mm, x86, kernel test robot, Dan Carpenter,
Andrew Morton, Lorenzo Stoakes, Dave Hansen, Andy Lutomirski,
Peter Zijlstra, Thomas Gleixner, Ingo Molnar, Borislav Petkov,
Rik van Riel, H. Peter Anvin, Linus Torvalds
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v1] x86/mm/pat: (un)track_pfn_copy() fix + improvements
2025-04-07 16:50 ` Ingo Molnar
2025-04-07 18:33 ` David Hildenbrand
@ 2025-04-07 18:33 ` Dan Carpenter
2025-04-07 18:59 ` Ingo Molnar
1 sibling, 1 reply; 10+ messages in thread
From: Dan Carpenter @ 2025-04-07 18:33 UTC (permalink / raw)
To: Ingo Molnar
Cc: David Hildenbrand, linux-kernel, linux-mm, x86,
kernel test robot, Dan Carpenter, Andrew Morton, Lorenzo Stoakes,
Dave Hansen, Andy Lutomirski, Peter Zijlstra, Thomas Gleixner,
Ingo Molnar, Borislav Petkov, Rik van Riel, H. Peter Anvin,
Linus Torvalds
On Mon, Apr 07, 2025 at 06:50:43PM +0200, Ingo Molnar wrote:
> > 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 ...)
Everyone sane uses CONFIG_INIT_STACK_ALL_ZERO these days.
regards,
dan carpenter
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v1] x86/mm/pat: (un)track_pfn_copy() fix + improvements
2025-04-07 18:33 ` Dan Carpenter
@ 2025-04-07 18:59 ` Ingo Molnar
2025-04-08 2:51 ` Nathan Chancellor
0 siblings, 1 reply; 10+ messages in thread
From: Ingo Molnar @ 2025-04-07 18:59 UTC (permalink / raw)
To: Dan Carpenter
Cc: David Hildenbrand, linux-kernel, linux-mm, x86,
kernel test robot, Dan Carpenter, Andrew Morton, Lorenzo Stoakes,
Dave Hansen, Andy Lutomirski, Peter Zijlstra, Thomas Gleixner,
Ingo Molnar, Borislav Petkov, Rik van Riel, H. Peter Anvin,
Linus Torvalds
* Dan Carpenter <dan.carpenter@linaro.org> wrote:
> > 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 ...)
>
> Everyone sane uses CONFIG_INIT_STACK_ALL_ZERO these days.
Good, although why is this compiler option named so weirdly in Clang:
CC_AUTO_VAR_INIT_ZERO_ENABLER := -enable-trivial-auto-var-init-zero-knowing-it-will-be-removed-from-clang
Hopefully it is named thusly because Clang has adopted GCC's
-ftrivial-auto-var-init=zero?
Thanks,
Ingo
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v1] x86/mm/pat: (un)track_pfn_copy() fix + improvements
2025-04-07 18:59 ` Ingo Molnar
@ 2025-04-08 2:51 ` Nathan Chancellor
2025-04-08 8:19 ` Ingo Molnar
0 siblings, 1 reply; 10+ messages in thread
From: Nathan Chancellor @ 2025-04-08 2:51 UTC (permalink / raw)
To: Ingo Molnar
Cc: Dan Carpenter, David Hildenbrand, linux-kernel, linux-mm, x86,
kernel test robot, Dan Carpenter, Andrew Morton, Lorenzo Stoakes,
Dave Hansen, Andy Lutomirski, Peter Zijlstra, Thomas Gleixner,
Ingo Molnar, Borislav Petkov, Rik van Riel, H. Peter Anvin,
Linus Torvalds
On Mon, Apr 07, 2025 at 08:59:44PM +0200, Ingo Molnar wrote:
> Good, although why is this compiler option named so weirdly in Clang:
>
> CC_AUTO_VAR_INIT_ZERO_ENABLER := -enable-trivial-auto-var-init-zero-knowing-it-will-be-removed-from-clang
>
> Hopefully it is named thusly because Clang has adopted GCC's
> -ftrivial-auto-var-init=zero?
Clang did -ftrivial-auto-var-init first, where the original author added
both pattern and zero but intended to remove zero once pattern has been
optimized enough compared to zero (if I remember and understand
correctly), so the "enabler" flag was added to try and make that clear.
Eventually, Kees leveraged both Linus's stated desire for initializing
stack variables [1] and GCC 12 landing -ftrivial-auto-var-init=zero
without a separate enable option to deprecate the "enabler" flag in
clang 16 [2] and remove it altogether in clang 18 [3].
[1]: https://lore.kernel.org/CAHk-=wgTM+cN7zyUZacGQDv3DuuoA4LORNPWgb1Y_Z1p4iedNQ@mail.gmail.com/
[2]: https://github.com/llvm/llvm-project/commit/aef03c9b3bed5cef5a1940774b80128aefcb4095
[3]: https://github.com/llvm/llvm-project/commit/00e54d04ae2802d498741097d4b83e898bc99c5b
Cheers,
Nathan
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v1] x86/mm/pat: (un)track_pfn_copy() fix + improvements
2025-04-08 2:51 ` Nathan Chancellor
@ 2025-04-08 8:19 ` Ingo Molnar
0 siblings, 0 replies; 10+ messages in thread
From: Ingo Molnar @ 2025-04-08 8:19 UTC (permalink / raw)
To: Nathan Chancellor
Cc: Dan Carpenter, David Hildenbrand, linux-kernel, linux-mm, x86,
kernel test robot, Dan Carpenter, Andrew Morton, Lorenzo Stoakes,
Dave Hansen, Andy Lutomirski, Peter Zijlstra, Thomas Gleixner,
Ingo Molnar, Borislav Petkov, Rik van Riel, H. Peter Anvin,
Linus Torvalds
* Nathan Chancellor <nathan@kernel.org> wrote:
> On Mon, Apr 07, 2025 at 08:59:44PM +0200, Ingo Molnar wrote:
> > Good, although why is this compiler option named so weirdly in Clang:
> >
> > CC_AUTO_VAR_INIT_ZERO_ENABLER := -enable-trivial-auto-var-init-zero-knowing-it-will-be-removed-from-clang
> >
> > Hopefully it is named thusly because Clang has adopted GCC's
> > -ftrivial-auto-var-init=zero?
>
> Clang did -ftrivial-auto-var-init first, where the original author added
> both pattern and zero but intended to remove zero once pattern has been
> optimized enough compared to zero (if I remember and understand
> correctly), so the "enabler" flag was added to try and make that clear.
> Eventually, Kees leveraged both Linus's stated desire for initializing
> stack variables [1] and GCC 12 landing -ftrivial-auto-var-init=zero
> without a separate enable option to deprecate the "enabler" flag in
> clang 16 [2] and remove it altogether in clang 18 [3].
>
> [1]: https://lore.kernel.org/CAHk-=wgTM+cN7zyUZacGQDv3DuuoA4LORNPWgb1Y_Z1p4iedNQ@mail.gmail.com/
> [2]: https://github.com/llvm/llvm-project/commit/aef03c9b3bed5cef5a1940774b80128aefcb4095
> [3]: https://github.com/llvm/llvm-project/commit/00e54d04ae2802d498741097d4b83e898bc99c5b
Cool, thanks for the explanation!
Thanks,
Ingo
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2025-04-08 8:19 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2025-04-04 12:49 [PATCH v1] x86/mm/pat: (un)track_pfn_copy() fix + improvements David Hildenbrand
2025-04-04 14:19 ` Lorenzo Stoakes
2025-04-06 17:28 ` Ingo Molnar
2025-04-07 7:23 ` David Hildenbrand
2025-04-07 16:50 ` Ingo Molnar
2025-04-07 18:33 ` David Hildenbrand
2025-04-07 18:33 ` Dan Carpenter
2025-04-07 18:59 ` Ingo Molnar
2025-04-08 2:51 ` Nathan Chancellor
2025-04-08 8:19 ` Ingo Molnar
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox