(no subject)

[Lorenzo Stoakes]

Bcc:
Subject: Re: [PATCH v3] x86/mm/pat: Fix VM_PAT handling when fork() fails in
 copy_page_range()
Message-ID: <0f94adaf-37a4-4d38-b952-01c2dc474a2c@lucifer.local>
Reply-To:
In-Reply-To: <b21bcd61-faf0-4ad8-b644-99794794594f@redhat.com>

Actually let me +cc a few more so this isn't lost further :P

On Wed, Apr 02, 2025 at 01:32:52PM +0200, David Hildenbrand wrote:
> On 02.04.25 13:19, Lorenzo Stoakes wrote:
> > On Thu, Mar 27, 2025 at 09:59:02AM +0800, kernel test robot wrote:
> > > BCC: lkp@intel.com
> > > CC: oe-kbuild-all@lists.linux.dev
> > > In-Reply-To: <20250325191951.471185-1-david@redhat.com>
> > > References: <20250325191951.471185-1-david@redhat.com>
> > > TO: David Hildenbrand <david@redhat.com>
> > >
> > > Hi David,
> > >
> > > kernel test robot noticed the following build warnings:
> > >
> > > [auto build test WARNING on 38fec10eb60d687e30c8c6b5420d86e8149f7557]
> > >
> > > url:    https://github.com/intel-lab-lkp/linux/commits/David-Hildenbrand/x86-mm-pat-Fix-VM_PAT-handling-when-fork-fails-in-copy_page_range/20250326-032200
> > > base:   38fec10eb60d687e30c8c6b5420d86e8149f7557
> > > patch link:    https://lore.kernel.org/r/20250325191951.471185-1-david%40redhat.com
> > > patch subject: [PATCH v3] x86/mm/pat: Fix VM_PAT handling when fork() fails in copy_page_range()
> > > :::::: branch date: 31 hours ago
> > > :::::: commit date: 31 hours ago
> > > config: hexagon-randconfig-r073-20250327 (https://download.01.org/0day-ci/archive/20250327/202503270941.IFILyNCX-lkp@intel.com/config)
> > > compiler: clang version 21.0.0git (https://github.com/llvm/llvm-project c2692afc0a92cd5da140dfcdfff7818a5b8ce997)
> > >
> > > If you fix the issue in a separate patch/commit (i.e. not just a new version of
> > > the same patch/commit), kindly add following tags
> > > | 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/
> > >
> > > smatch warnings:
> > > mm/memory.c:1428 copy_page_range() error: uninitialized symbol 'pfn'.
>
> Huh,
>
> how did the original report not make it into my inbox ? :/

Yeah it's odd... maybe broken script?

>
> Thanks for replying Lorenzo!

NP!

>
> >
> > I have a feeling this is because if ndef __HAVE_PFNMAP_TRACKING you just
> > don't touch pfn at all, but also I see in the new track_pfn_copy() there
> > are code paths where pfn doesn't get set, but you still pass the
> > uninitialised pfn to untrack_pfn_copy()...
>
> If track_pfn_copy() returns 0 and VM_PAT applies, the pfn is set. Otherwise
> (returns an error), we immediately return from copy_page_range().
>
> So once we reach untrack_pfn_copy() ... the PFN was set.
>
> In case of !__HAVE_PFNMAP_TRACKING the pfn is not set and not used.
>
> >
> > I mean it could also be in the case of !(src_vma->vm_flags & VM_PAT) (but &
> > VM_PFNMAP), where we return 0 but still pass pfn to untrack_pfn_copy()...
>
> I assume that's what it is complaining about, and it doesn't figure out that
> the parameter is unused.
>
> So likely it's best to just initialize pfn to 0.
>
> >
> > This is all super icky, we probably want to actually have track_pfn_copy()
> > indicate whether we want to later untrack, not only if there's an error.
>
> Sounds overly-complicated. But having a pfn != 0 might work.
>
> > > Will comment accordingly on patch, but I mean I don't like the idea of
> us
> > just initialising the pfn here, because... what to?... :)
>

Sure, I mean for all of above let's have the debate on the main patch I guess so
it's in one place...

> Stared at that code for too long (and I reached a point where the PAT stuff
> absolutely annoys me).

But, also lol. Can. Relate.

>
> Thanks!
>
> --
> Cheers,
>
> David / dhildenb
>

Cheers, Lorenzo

Re: [PATCH v3] x86/mm/pat: Fix VM_PAT handling when fork() fails in copy_page_range()

[Dan Carpenter]

Sorry, I've been having trouble with my email recently...  I replied
earlier but my email got eaten on the way out.

What happened here is that the zero day bot emails go to me first and
then I review them or forward them depending on if they're a real
issue or not.

Here it's a false postive because it's set and used if the
(src_vma->vm_flags & VM_PFNMAP) flag is set.  Smatch doesn't parse
this correctly.  I've been meaning to fix this in Smatch for a
while.

But these days more and more people are using lei and b4 to get their
email so they see the unreviewed Smatch warnings.

regards,
dan carpenter

On Wed, Apr 02, 2025 at 12:37:24PM +0100, Lorenzo Stoakes wrote:
> Bcc:
> Subject: Re: [PATCH v3] x86/mm/pat: Fix VM_PAT handling when fork() fails in
>  copy_page_range()
> Message-ID: <0f94adaf-37a4-4d38-b952-01c2dc474a2c@lucifer.local>
> Reply-To:
> In-Reply-To: <b21bcd61-faf0-4ad8-b644-99794794594f@redhat.com>
> 
> Actually let me +cc a few more so this isn't lost further :P
> 
> On Wed, Apr 02, 2025 at 01:32:52PM +0200, David Hildenbrand wrote:
> > On 02.04.25 13:19, Lorenzo Stoakes wrote:
> > > On Thu, Mar 27, 2025 at 09:59:02AM +0800, kernel test robot wrote:
> > > > BCC: lkp@intel.com
> > > > CC: oe-kbuild-all@lists.linux.dev
> > > > In-Reply-To: <20250325191951.471185-1-david@redhat.com>
> > > > References: <20250325191951.471185-1-david@redhat.com>
> > > > TO: David Hildenbrand <david@redhat.com>
> > > >
> > > > Hi David,
> > > >
> > > > kernel test robot noticed the following build warnings:
> > > >
> > > > [auto build test WARNING on 38fec10eb60d687e30c8c6b5420d86e8149f7557]
> > > >
> > > > url:    https://github.com/intel-lab-lkp/linux/commits/David-Hildenbrand/x86-mm-pat-Fix-VM_PAT-handling-when-fork-fails-in-copy_page_range/20250326-032200
> > > > base:   38fec10eb60d687e30c8c6b5420d86e8149f7557
> > > > patch link:    https://lore.kernel.org/r/20250325191951.471185-1-david%40redhat.com
> > > > patch subject: [PATCH v3] x86/mm/pat: Fix VM_PAT handling when fork() fails in copy_page_range()
> > > > :::::: branch date: 31 hours ago
> > > > :::::: commit date: 31 hours ago
> > > > config: hexagon-randconfig-r073-20250327 (https://download.01.org/0day-ci/archive/20250327/202503270941.IFILyNCX-lkp@intel.com/config)
> > > > compiler: clang version 21.0.0git (https://github.com/llvm/llvm-project c2692afc0a92cd5da140dfcdfff7818a5b8ce997)
> > > >
> > > > If you fix the issue in a separate patch/commit (i.e. not just a new version of
> > > > the same patch/commit), kindly add following tags
> > > > | 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/
> > > >
> > > > smatch warnings:
> > > > mm/memory.c:1428 copy_page_range() error: uninitialized symbol 'pfn'.
> >
> > Huh,
> >
> > how did the original report not make it into my inbox ? :/
> 
> Yeah it's odd... maybe broken script?
> 
> >
> > Thanks for replying Lorenzo!
> 
> NP!
> 
> >
> > >
> > > I have a feeling this is because if ndef __HAVE_PFNMAP_TRACKING you just
> > > don't touch pfn at all, but also I see in the new track_pfn_copy() there
> > > are code paths where pfn doesn't get set, but you still pass the
> > > uninitialised pfn to untrack_pfn_copy()...
> >
> > If track_pfn_copy() returns 0 and VM_PAT applies, the pfn is set. Otherwise
> > (returns an error), we immediately return from copy_page_range().
> >
> > So once we reach untrack_pfn_copy() ... the PFN was set.
> >
> > In case of !__HAVE_PFNMAP_TRACKING the pfn is not set and not used.
> >
> > >
> > > I mean it could also be in the case of !(src_vma->vm_flags & VM_PAT) (but &
> > > VM_PFNMAP), where we return 0 but still pass pfn to untrack_pfn_copy()...
> >
> > I assume that's what it is complaining about, and it doesn't figure out that
> > the parameter is unused.
> >
> > So likely it's best to just initialize pfn to 0.
> >
> > >
> > > This is all super icky, we probably want to actually have track_pfn_copy()
> > > indicate whether we want to later untrack, not only if there's an error.
> >
> > Sounds overly-complicated. But having a pfn != 0 might work.
> >
> > > > Will comment accordingly on patch, but I mean I don't like the idea of
> > us
> > > just initialising the pfn here, because... what to?... :)
> >
> 
> Sure, I mean for all of above let's have the debate on the main patch I guess so
> it's in one place...
> 
> > Stared at that code for too long (and I reached a point where the PAT stuff
> > absolutely annoys me).
> 
> But, also lol. Can. Relate.
> 
> >
> > Thanks!
> >
> > --
> > Cheers,
> >
> > David / dhildenb
> >
> 
> Cheers, Lorenzo

Re: [PATCH v3] x86/mm/pat: Fix VM_PAT handling when fork() fails in copy_page_range()

[David Hildenbrand]

On 03.04.25 17:14, Dan Carpenter wrote:
> Sorry, I've been having trouble with my email recently...  I replied
> earlier but my email got eaten on the way out.
> 
> What happened here is that the zero day bot emails go to me first and
> then I review them or forward them depending on if they're a real
> issue or not.
> 
> Here it's a false postive because it's set and used if the
> (src_vma->vm_flags & VM_PFNMAP) flag is set.  Smatch doesn't parse
> this correctly.  I've been meaning to fix this in Smatch for a
> while.

There is a slight complication (on top of the VM_PFNMAP checks):

If "src_vma->vm_flags & VM_PAT" we
* set pfn
* set dst_vma->vm_flags |= VM_PFNMAP

Then, we only consume the pfn if "dst_vma->vm_flags & VM_PFNMAP"

While we won't be using the uninitialized pfn (good), we'd still pass an 
uninitialized pfn, which IIRC is UB; likely nothing happens on GCC 
clang, but we better handle it.

So that should better be changed; I'll send a fix.

Thanks!

-- 
Cheers,

David / dhildenb

Re: [PATCH v3] x86/mm/pat: Fix VM_PAT handling when fork() fails in copy_page_range()

[Lorenzo Stoakes]

On Thu, Apr 03, 2025 at 10:59:12PM +0200, David Hildenbrand wrote:
> On 03.04.25 17:14, Dan Carpenter wrote:
> > Sorry, I've been having trouble with my email recently...  I replied
> > earlier but my email got eaten on the way out.
> >
> > What happened here is that the zero day bot emails go to me first and
> > then I review them or forward them depending on if they're a real
> > issue or not.
> >
> > Here it's a false postive because it's set and used if the
> > (src_vma->vm_flags & VM_PFNMAP) flag is set.  Smatch doesn't parse
> > this correctly.  I've been meaning to fix this in Smatch for a
> > while.
>
> There is a slight complication (on top of the VM_PFNMAP checks):
>
> If "src_vma->vm_flags & VM_PAT" we
> * set pfn
> * set dst_vma->vm_flags |= VM_PFNMAP
>
> Then, we only consume the pfn if "dst_vma->vm_flags & VM_PFNMAP"
>
> While we won't be using the uninitialized pfn (good), we'd still pass an
> uninitialized pfn, which IIRC is UB; likely nothing happens on GCC clang,
> but we better handle it.
>
> So that should better be changed; I'll send a fix.

Maybe just worth setting pfn = 0 _as well_ in the caller, belts + braces maybe?

I mean the patch is already upstream at dc84bc2aba85. So I guess these fixes are
intended for rc generally?

>
> Thanks!
>
> --
> Cheers,
>
> David / dhildenb
>

Re: [PATCH v3] x86/mm/pat: Fix VM_PAT handling when fork() fails in copy_page_range()

[David Hildenbrand]

On 04.04.25 13:52, Lorenzo Stoakes wrote:
> On Thu, Apr 03, 2025 at 10:59:12PM +0200, David Hildenbrand wrote:
>> On 03.04.25 17:14, Dan Carpenter wrote:
>>> Sorry, I've been having trouble with my email recently...  I replied
>>> earlier but my email got eaten on the way out.
>>>
>>> What happened here is that the zero day bot emails go to me first and
>>> then I review them or forward them depending on if they're a real
>>> issue or not.
>>>
>>> Here it's a false postive because it's set and used if the
>>> (src_vma->vm_flags & VM_PFNMAP) flag is set.  Smatch doesn't parse
>>> this correctly.  I've been meaning to fix this in Smatch for a
>>> while.
>>
>> There is a slight complication (on top of the VM_PFNMAP checks):
>>
>> If "src_vma->vm_flags & VM_PAT" we
>> * set pfn
>> * set dst_vma->vm_flags |= VM_PFNMAP
>>
>> Then, we only consume the pfn if "dst_vma->vm_flags & VM_PFNMAP"
>>
>> While we won't be using the uninitialized pfn (good), we'd still pass an
>> uninitialized pfn, which IIRC is UB; likely nothing happens on GCC clang,
>> but we better handle it.
>>
>> So that should better be changed; I'll send a fix.
> 
> Maybe just worth setting pfn = 0 _as well_ in the caller, belts + braces maybe?

I'm planning on doing the following, just didn't get to testing it:


 From d340fac886c4a15d39d8e963aa8c647b19589413 Mon Sep 17 00:00:00 2001
From: David Hildenbrand <david@redhat.com>
Date: Tue, 29 Oct 2024 22:03:31 +0100
Subject: [PATCH] x86/mm/pat: (un)track_pfn_copy() fix + improvements

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

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: Andy Lutomirski <luto@kernel.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Rik van Riel <riel@surriel.com>
Cc: "H. Peter Anvin" <hpa@zytor.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Andrew Morton <akpm@linux-foundation.org>
Signed-off-by: David Hildenbrand <david@redhat.com>
---
  arch/x86/mm/pat/memtype.c | 4 +++-
  include/linux/pgtable.h   | 5 ++++-
  2 files changed, 7 insertions(+), 2 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)
-- 
2.48.1


> 
> I mean the patch is already upstream at dc84bc2aba85. So I guess these fixes are
> intended for rc generally?

Yes.

-- 
Cheers,

David / dhildenb

Re: [PATCH v3] x86/mm/pat: Fix VM_PAT handling when fork() fails in copy_page_range()

[David Hildenbrand]

On 04.04.25 14:20, David Hildenbrand wrote:
> On 04.04.25 13:52, Lorenzo Stoakes wrote:
>> On Thu, Apr 03, 2025 at 10:59:12PM +0200, David Hildenbrand wrote:
>>> On 03.04.25 17:14, Dan Carpenter wrote:
>>>> Sorry, I've been having trouble with my email recently...  I replied
>>>> earlier but my email got eaten on the way out.
>>>>
>>>> What happened here is that the zero day bot emails go to me first and
>>>> then I review them or forward them depending on if they're a real
>>>> issue or not.
>>>>
>>>> Here it's a false postive because it's set and used if the
>>>> (src_vma->vm_flags & VM_PFNMAP) flag is set.  Smatch doesn't parse
>>>> this correctly.  I've been meaning to fix this in Smatch for a
>>>> while.
>>>
>>> There is a slight complication (on top of the VM_PFNMAP checks):
>>>
>>> If "src_vma->vm_flags & VM_PAT" we
>>> * set pfn
>>> * set dst_vma->vm_flags |= VM_PFNMAP
>>>
>>> Then, we only consume the pfn if "dst_vma->vm_flags & VM_PFNMAP"
>>>
>>> While we won't be using the uninitialized pfn (good), we'd still pass an
>>> uninitialized pfn, which IIRC is UB; likely nothing happens on GCC clang,
>>> but we better handle it.
>>>
>>> So that should better be changed; I'll send a fix.
>>
>> Maybe just worth setting pfn = 0 _as well_ in the caller, belts + braces maybe?
> 
> I'm planning on doing the following, just didn't get to testing it:
> 

Ah, now I get your comment. Yeah, we could just set pfn=0 in the caller 
as well to make smatch completely happy I guess.

-- 
Cheers,

David / dhildenb

Re: [PATCH v3] x86/mm/pat: Fix VM_PAT handling when fork() fails in copy_page_range()

[Ingo Molnar]

* David Hildenbrand <david@redhat.com> wrote:

> On 04.04.25 14:20, David Hildenbrand wrote:
> > On 04.04.25 13:52, Lorenzo Stoakes wrote:
> > > On Thu, Apr 03, 2025 at 10:59:12PM +0200, David Hildenbrand wrote:
> > > > On 03.04.25 17:14, Dan Carpenter wrote:
> > > > > Sorry, I've been having trouble with my email recently...  I replied
> > > > > earlier but my email got eaten on the way out.
> > > > > 
> > > > > What happened here is that the zero day bot emails go to me first and
> > > > > then I review them or forward them depending on if they're a real
> > > > > issue or not.
> > > > > 
> > > > > Here it's a false postive because it's set and used if the
> > > > > (src_vma->vm_flags & VM_PFNMAP) flag is set.  Smatch doesn't parse
> > > > > this correctly.  I've been meaning to fix this in Smatch for a
> > > > > while.
> > > > 
> > > > There is a slight complication (on top of the VM_PFNMAP checks):
> > > > 
> > > > If "src_vma->vm_flags & VM_PAT" we
> > > > * set pfn
> > > > * set dst_vma->vm_flags |= VM_PFNMAP
> > > > 
> > > > Then, we only consume the pfn if "dst_vma->vm_flags & VM_PFNMAP"
> > > > 
> > > > While we won't be using the uninitialized pfn (good), we'd still pass an
> > > > uninitialized pfn, which IIRC is UB; likely nothing happens on GCC clang,
> > > > but we better handle it.
> > > > 
> > > > So that should better be changed; I'll send a fix.
> > > 
> > > Maybe just worth setting pfn = 0 _as well_ in the caller, belts + braces maybe?
> > 
> > I'm planning on doing the following, just didn't get to testing it:
> > 
> 
> Ah, now I get your comment. Yeah, we could just set pfn=0 in the caller as
> well to make smatch completely happy I guess.

Yeah, that's far cleaner than these rather ugly code constructs in the 
error paths. It's a pretty standard API where output pointers may not 
get touched on errors - if Smatch has a problem with it, fix Smatch or 
the callers.

Thanks,

	Ingo

Re: [PATCH v3] x86/mm/pat: Fix VM_PAT handling when fork() fails in copy_page_range()

[Dan Carpenter]

On Thu, Apr 03, 2025 at 10:59:12PM +0200, David Hildenbrand wrote:
> On 03.04.25 17:14, Dan Carpenter wrote:
> > Sorry, I've been having trouble with my email recently...  I replied
> > earlier but my email got eaten on the way out.
> > 
> > What happened here is that the zero day bot emails go to me first and
> > then I review them or forward them depending on if they're a real
> > issue or not.
> > 
> > Here it's a false postive because it's set and used if the
> > (src_vma->vm_flags & VM_PFNMAP) flag is set.  Smatch doesn't parse
> > this correctly.  I've been meaning to fix this in Smatch for a
> > while.
> 
> There is a slight complication (on top of the VM_PFNMAP checks):
> 
> If "src_vma->vm_flags & VM_PAT" we
> * set pfn
> * set dst_vma->vm_flags |= VM_PFNMAP
> 
> Then, we only consume the pfn if "dst_vma->vm_flags & VM_PFNMAP"
> 
> While we won't be using the uninitialized pfn (good), we'd still pass an
> uninitialized pfn, which IIRC is UB; likely nothing happens on GCC clang,
> but we better handle it.

Passing an uninitialized variable is UB, but if the function is inlined
then it's fine.

regards,
dan carpenter