linux-kernel - Re: [PATCH v1] x86/mm/pat: (un)track_pfn

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <Z_QCYzEJXTnd97Sf@gmail.com>
Date: Mon, 7 Apr 2025 18:50:43 +0200
From: Ingo Molnar <mingo@...nel.org>
To: David Hildenbrand <david@...hat.com>
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


* 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.

Thanks,

	Ingo