From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by smtp.lore.kernel.org (Postfix) with ESMTP id 9A8C0C36010 for ; Mon, 7 Apr 2025 16:50:54 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id BACAD6B0008; Mon, 7 Apr 2025 12:50:53 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id B5DF06B000A; Mon, 7 Apr 2025 12:50:53 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id A4C9D6B000C; Mon, 7 Apr 2025 12:50:53 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0010.hostedemail.com [216.40.44.10]) by kanga.kvack.org (Postfix) with ESMTP id 8D1306B0008 for ; Mon, 7 Apr 2025 12:50:53 -0400 (EDT) Received: from smtpin10.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay03.hostedemail.com (Postfix) with ESMTP id 8BFEBAF23E for ; Mon, 7 Apr 2025 16:50:53 +0000 (UTC) X-FDA: 83307837186.10.02EB109 Received: from dfw.source.kernel.org (dfw.source.kernel.org [139.178.84.217]) by imf23.hostedemail.com (Postfix) with ESMTP id CBBDF14000A for ; Mon, 7 Apr 2025 16:50:51 +0000 (UTC) Authentication-Results: imf23.hostedemail.com; dkim=pass header.d=kernel.org header.s=k20201202 header.b=Qfgq5ZRH; spf=pass (imf23.hostedemail.com: domain of mingo@kernel.org designates 139.178.84.217 as permitted sender) smtp.mailfrom=mingo@kernel.org; dmarc=pass (policy=quarantine) header.from=kernel.org ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1744044652; h=from:from:sender:reply-to:subject:subject:date:date: message-id:message-id:to:to:cc:cc:mime-version:mime-version: content-type:content-type:content-transfer-encoding: in-reply-to:in-reply-to:references:references:dkim-signature; bh=+wAS4BCH/txJ320eipPylXnCFTBQI94aq3hp/Gf7eSM=; b=19VCyR4yd2rTmixO3zgWkB1GZ9JpBYNLj1/7bzUyae5NAMkwd4ovMk4ev716CKbGQXdJ55 /dIbP5FcLExdOq1WA6PtB6cPbmIW50Ky92QHo7VUG5p+i6K8ct1QShE4/NM4NshL/BI1Es Pmj7jk4Ks70yHVYtFwG7Jq7JhOwk7Dg= ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1744044652; a=rsa-sha256; cv=none; b=YumgJr5v0k9QY7FubkXWBlotRmxfyELxlvcfgC9CsX9kIphkJ9y2gsU5jE/h342ZznCCe7 EA/gr4nHkJKUD2EODSai08gt04KcjM2Cm93GgbhT6c+JOYnM9uyLvbg5QkcqFHbRPb+hQC 6eZa7mr6vGaaUmJyICcaT66GsJOk+JA= ARC-Authentication-Results: i=1; imf23.hostedemail.com; dkim=pass header.d=kernel.org header.s=k20201202 header.b=Qfgq5ZRH; spf=pass (imf23.hostedemail.com: domain of mingo@kernel.org designates 139.178.84.217 as permitted sender) smtp.mailfrom=mingo@kernel.org; dmarc=pass (policy=quarantine) header.from=kernel.org Received: from smtp.kernel.org (transwarp.subspace.kernel.org [100.75.92.58]) by dfw.source.kernel.org (Postfix) with ESMTP id B207F5C54C5; Mon, 7 Apr 2025 16:48:33 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id 97598C4CEE7; Mon, 7 Apr 2025 16:50:46 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1744044650; bh=upra86Slk5LmnBFaEkpdCTezFQg4f71K0J7SoROuknw=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=Qfgq5ZRHaW/bjR5KwsTKcQlk1IyPleTw08ENsTFbqSXFBSVPtu/4KPTmksc+kXcjS wyGjWGmpXvj9WkjGqaIM6cbXLIMN0QSnB+wnBrfbL0SeDRjQ2PHXwWE8UGFVjzD/2G knmPSZ6mhVT603/pTTzBkoeI1fE1QRLKLnZYuK4LY1wlnZZShwDlOPqTb3NcCaShDR ytxPTRB1DFFoVSSou9PhNHBd8Z0KLXXbdgIKoxBgPC7q9cCPjWZAapvfJhuQEYRSGK 8kdqhpvcZz1dSkZ4Qlxb850nt1cDA+vKr/R4uvYYhoFRQOoaKUqn7i5TZKsoh2euZa HzVuUsxVR3Cvw== Date: Mon, 7 Apr 2025 18:50:43 +0200 From: Ingo Molnar To: David Hildenbrand Cc: linux-kernel@vger.kernel.org, linux-mm@kvack.org, x86@kernel.org, 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 Subject: Re: [PATCH v1] x86/mm/pat: (un)track_pfn_copy() fix + improvements Message-ID: References: <20250404124931.2255618-1-david@redhat.com> <630caa8e-2ee2-4895-9e4e-8bf2fa079100@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <630caa8e-2ee2-4895-9e4e-8bf2fa079100@redhat.com> X-Stat-Signature: nx7jas31xt1zwh8ftxwmsojt9bxatpuw X-Rspam-User: X-Rspamd-Queue-Id: CBBDF14000A X-Rspamd-Server: rspam08 X-HE-Tag: 1744044651-843179 X-HE-Meta: U2FsdGVkX18aV84pixfOF7Xysx3I2uBgmEmREPvaAzr2pKLERCaMzxkNb4xEvE6VBwfaWau81gA9aUFCt5ZF+zHbHWrHQ8o0ZfgF6F5rxUM7jRKje2eDlRmZJ74JKtpEuOwkasBeq4VJbiKjhLqHqGz3qyKg9HMEDuXu7Bo7HuleAFMl5i5ArDusCzNc7Fnwo7kmPEOTcrrRfYxB0fPAg8kzgzUPyZxTjNavLqZyWPwl0UNZQ+wN6OhucVeyJxDeFOhlYDrGrp+iv2V1Kl55tZPh+NqoXGHBPOrWUQzCR9uzCyuDtgm2pUIZytnjfmKKhVziAC3ddYKcEIfj7zXsuehIQL7IlBj14W8bbMtTh9t6S//A2+y+j2Qx2jDt4yjOHBPTGH8cemAGmZOBAClN/4gBUl6GEEpo5777laA+3DHE4NtC5kBUWIXBj+tD8+OMXBfH5hw/xwWdmd5HREn8qFkSiirwdL1CIFcZDg+3EjGJKMX5uEqiNu8WwheQqZ/3CZhyvcehkDGmwEcp14Xb72s/5xc01/DAOPG1rxvDAl0vkDFgs6MgEWaPXFrHyzpt5H4QkpejqRY28CcqOLFrJaK6PleQkjCHhwBFqbVhIfVByZXn7MMgMSHBuAeABrHGIPVjZ4qzGe0A9/+nfnDwKkrn7cvddN4ouK4erWMVyDUzTvRHH+YlB0L4crGfyr6tk8C5IW/MuQKu/b/kLRsTQ/piy2lZp2dGkJEATBvvrdoARCUAdbyS0pCAuUPTjnvkJVqF5BYKzZmCSgSMjxiZtGOskYqYirthuMDoxAb1Q5KkatlX75cvcXbeg0nA/i3tRrnXEGRifKvDfboUasFtibzP07mJInxyNfm8OOOdgbbPVwCHyTwHZ1y8Ypz82L8RU8iD40YbWIMUjB+9ebtoAaLk2gVCFAvqgUSq5YnlpvDxWTSpWCE4Gv+Ud7Um/C+FGoKUQs2VAsXlKZzKEqn BM6/r/62 KZ6qO7M/yTkOE8shBtM1VWVDcu6/KH9rA9Vq/3vXZMITIuRJTGhchdwRDn/RpKJqCuaTJvsLgBY4VT5N4QUfcREWDORd3eUrEar490qJ6FztN5fTdtIjGfEj+drzGjBcQW6kcEaBmfdS/Ok7f/35lIS9jVjhKsCeuDMh6ihacrOQCq+YyXP0YfhLpnorW5OnpsW83vN5fyFkgJ95SSwC4PumKGPt5BJB7BiMr0uebAtZ9oyLwRhsYCSI5PsCI+yZSN822YNIp7wHSAXTBWh3t2dkV7oFnH3iEwoHN X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: List-Subscribe: List-Unsubscribe: * David Hildenbrand wrote: > On 06.04.25 19:28, Ingo Molnar wrote: > > > > * 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'. > > > > > - 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