linux-kernel

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-next>] [day] [month] [year] [list]

Message-ID: <g5vxcrmdjjsvrjeo5k6uzwypctv2mvbteoommwwpv6sfjpbcyd@lackqksyahfs>
Date: Wed, 2 Apr 2025 12:37:24 +0100
From: Lorenzo Stoakes <lorenzo.stoakes@...cle.com>
To: David Hildenbrand <david@...hat.com>
Cc: kernel test robot <lkp@...el.com>, oe-kbuild@...ts.linux.dev,
        Dan Carpenter <error27@...il.com>, linux-mm@...ck.org,
        linux-kernel@...r.kernel.org,
        "Liam R. Howlett" <Liam.Howlett@...cle.com>, x86@...nel.org

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@...ifer.local>
Reply-To:
In-Reply-To: <b21bcd61-faf0-4ad8-b644-99794794594f@...hat.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@...el.com
> > > CC: oe-kbuild-all@...ts.linux.dev
> > > In-Reply-To: <20250325191951.471185-1-david@...hat.com>
> > > References: <20250325191951.471185-1-david@...hat.com>
> > > TO: David Hildenbrand <david@...hat.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@...el.com>
> > > | Reported-by: Dan Carpenter <error27@...il.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