Received-SPF: pass (google.com: domain of linux-kernel+bounces-203445-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Date: Wed, 5 Jun 2024 17:20:35 -0700
From: Dan Williams <dan.j.williams@intel.com>
To: Pasha Tatashin <pasha.tatashin@soleen.com>, Peter Xu <peterx@redhat.com>
CC: <linux-kernel@vger.kernel.org>, <linux-mm@kvack.org>, Andrew Morton
	<akpm@linux-foundation.org>, Dan Williams <dan.j.williams@intel.com>
Subject: Re: [PATCH] mm/page_table_check: Fix crash on ZONE_DEVICE
Message-ID: <666100d39dee4_2d412294b3@dwillia2-mobl3.amr.corp.intel.com.notmuch>
References: <20240605212146.994486-1-peterx@redhat.com>
 <CA+CK2bCCeamiaoTFybTd5nW39ixVPDM2gV2rMmK+2PfFAyE+9w@mail.gmail.com>
Content-Type: text/plain; charset="utf-8"
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <CA+CK2bCCeamiaoTFybTd5nW39ixVPDM2gV2rMmK+2PfFAyE+9w@mail.gmail.com>
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?OEZveDRGb0JSKzRNUlM2b0FrK216WmxjNTVBcmo4WEpQN2ZiTENBVlBWa0JT?=
 =?utf-8?B?SDBxWmJ1L2hZdkxsM3VJeDIzMUhHSjh4TFROdWJOU2J6REpBNkNxTDZjRHFC?=
 =?utf-8?B?cHN3SGdaN24yWUpWR0djUkIyTGVLWUI0NjNGbGtEbUZOR1F0OTYyZGk1S0Zw?=
 =?utf-8?B?RzdEMG9Dd25aL2lVMnMzc2JKK0hUaXpGNlQyUmdRT0VyL2RlWVhhMGM3NU0y?=
 =?utf-8?B?YjdjRnZIaFgyZ3p2SFMzM211Y3hlK1BTL2IvTlowOTNDQmQ2d3ZxNjVHd2JQ?=
 =?utf-8?B?N2tMVXdVVCtiT1JOOHlNY2FJQzlVdVRCVXJZLzhGZE9NdCtQa25uRlRRbUZO?=
 =?utf-8?B?U3JBOGpBWklPSmwybTk4dnN5UmM5S0ovVEJsaGx5YW5oekdMekZYYjhQS0ky?=
 =?utf-8?B?d0xXZGNTM0xmbVh2bkEyeTlSMUN2SDJQT1lSK1JXdVJ6cCtPVkRhNy9tM20x?=
 =?utf-8?B?c0YzYm5NWDROb0ljc21sbUJJb3QycE9GbUpKTkVVYTVMZmI4QUFDZkJsbkJN?=
 =?utf-8?B?QnJDeHl5dnFVOVZwaWx0VFhVMGc3SFZEZ3ZDQng3TkpBS0JLTENTVmhvRWhE?=
 =?utf-8?B?dTVmZjJ1NnVuSFdBRzI5MFZGQlhTQnF1Z2lGWEVKaXNEYnBaUEhyK1NZZGdy?=
 =?utf-8?B?emMrMVV0akFod3dtdkl0K0FlMDRwMllYWXdGYlJFdWcwanplOWwzanNXZFNi?=
 =?utf-8?B?NzNtQnlFSk9oTkhIWWdCZHloZ0ZhK0VkV2JXTU1oSCtxeXVTMUIzR2NWZXNN?=
 =?utf-8?B?cjFhRVpJYy9QY05GK0ZnUXl2SGIvb1ZxbFR3WVo2Rjh4V0U2Skg3MUhTSG45?=
 =?utf-8?B?M3dERlBudHVQTkhHZTgzQ2NQbGE3a2xEL3QvSGtyMXBLRkVnZUFSUnI5WGMr?=
 =?utf-8?B?dFdCM3c1ZU9rL3RWQ0tzR1doT0l1THFJMXNBemJaTE82cWE1MW42QUxtUy9I?=
 =?utf-8?B?MDg0b1JzT3pzVEFydEdSQ2IxVUZQeTRGeWJXQVpKMlA0eHVJSDFoMUhuRHY0?=
 =?utf-8?B?ZnhMeFdWSnBzT01kdFF6QmRhZGQwQjh2QzdRTWkvYW9aZ3lNVE9OTUlGTFlt?=
 =?utf-8?B?dU9zZm0yNVlEdk9LU2ZKancyaEVrRmZCem5YRWFSVkRxSGRqVFJ1UGRVMjA5?=
 =?utf-8?B?SlkxenpkT1REajhTeXlPVjhpZ1lFS2xCZ0lIb1NzWWI5QkdZRTNOSDFRSmxr?=
 =?utf-8?B?elZhSkVoQzlOOEZvbTdhcE1mTkZtb3RsQWdwRHpYY0wwYUI4OEpENWlVYXdt?=
 =?utf-8?B?NWZta2NqdUlzcHNEOGl2VWxUSzhqM29SdFBqOGhreVR6cjJkVHdYZElzd1Rr?=
 =?utf-8?B?OUVhSHFpWWdkQXZaWGh3Vk1hQ1FSbXE4MS80SkZpbkpvdm9HUEx0NnZGQy9v?=
 =?utf-8?B?Mm5mYytLRjd2L1laV3lVT1Rlc2t0bnF3WHJWdEZPRzk3TTdXYTZwYUw3Q3Zi?=
 =?utf-8?B?ZWllZ1JIS05sZ0dlcGtFaGY0UG5mdmlZanRMSUFMbm80bU51Q21Qb0JsWFRa?=
 =?utf-8?B?a1VxNEd2ckxkaHh1UW5NUjlrTGx0MGF6by91S09pZ2duczMrZ01qc0NMbnRm?=
 =?utf-8?B?MnROdEYrMGY4U1UzMmt5OHRqOFNUZ3FoMDBkS3F0WTI5cGZGc0JhT002YjdK?=
 =?utf-8?B?SjRhZ3R4K0p5MFEvbW0zUDlvamh3S3NkdGtEY2JLa1cwaUhVZ3hLTHgrSU0r?=
 =?utf-8?B?UEp0dWlhd2ZRL3gzVDZ0VVp2UFFubkNmYVB3NGorT3IweU9jS0VTUjQzVUc1?=
 =?utf-8?B?R1M1K25MZ3RwT2NPTzZkb083UFN3LzVBZVhFbHNTYm1NRkVKNmtyT0owdE41?=
 =?utf-8?B?UStjQUJJVjdRUmd2Mi9DR1RUMkcvYmMyNFF1TUsrSGdHNmZ4VjhEeGJUbFhZ?=
 =?utf-8?B?Q1loUXZBK0RzK3c0c0FrWE1ycTBDVnpMK3YvM3I4WHBKb2l5NHVpY3dCS1k1?=
 =?utf-8?B?eDdBd3NvZ2toQmlFdUdSYVh0T0ZPbVc3dHNWeXVBQ0NuYW9IVlVhWHZvK3hZ?=
 =?utf-8?B?RHh5QXJDZGV1KzVnTHBzRUg5SklacjR5emhUZjJ0RWpJNlRhaHdLNXlCSTZq?=
 =?utf-8?B?RzJNMUVaOHg3Wk05cU9sT1hDNnFQSytPL1o5WFUxdWtFTkRkVHRZY0dIZzdF?=
 =?utf-8?B?Rk1SbU1iWU12VUJiVEtuYzZsR1h3b1RqdTZPVWxNWmkxQ3BVVVVseWNhSkJQ?=
 =?utf-8?B?V3c9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: f273ebe1-28d4-439f-2744-08dc85be7d9f
X-MS-Exchange-CrossTenant-AuthSource: PH8PR11MB8107.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 06 Jun 2024 00:20:37.8445
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: yWTyAjwzumih9Mjc0LqEMhHFpMrqLTQmczslN6C2ds8Q90IkQpLrNNCkbe/7hd+mokk1bdqOGkAZNStxqnc4xJT5H+tFTCgYMahu+QEh2rQ=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: PH0PR11MB4871
X-OriginatorOrg: intel.com

Pasha Tatashin wrote:
> On Wed, Jun 5, 2024 at 5:21 PM Peter Xu <peterx@redhat.com> wrote:
> >
> > Not all pages may apply to pgtable check.  One example is ZONE_DEVICE
> > pages: they map PFNs directly, and they don't allocate page_ext at all even
> > if there's struct page around.  One may reference devm_memremap_pages().
> >
> > When both ZONE_DEVICE and page-table-check enabled, then try to map some
> > dax memories, one can trigger kernel bug constantly now when the kernel was
> > trying to inject some pfn maps on the dax device:
> >
> >  kernel BUG at mm/page_table_check.c:55!
> >
> > While it's pretty legal to use set_pxx_at() for ZONE_DEVICE pages for page
> > fault resolutions, skip all the checks if page_ext doesn't even exist in
> > pgtable checker, which applies to ZONE_DEVICE but maybe more.
> 
> Thank you for reporting this bug. A few comments below:
> 
> >
> > Cc: Dan Williams <dan.j.williams@intel.com>
> > Cc: Pasha Tatashin <pasha.tatashin@soleen.com>
> > Signed-off-by: Peter Xu <peterx@redhat.com>
> > ---
> >  mm/page_table_check.c | 11 ++++++++++-
> >  1 file changed, 10 insertions(+), 1 deletion(-)
> >
> > diff --git a/mm/page_table_check.c b/mm/page_table_check.c
> > index 4169576bed72..509c6ef8de40 100644
> > --- a/mm/page_table_check.c
> > +++ b/mm/page_table_check.c
> > @@ -73,6 +73,9 @@ static void page_table_check_clear(unsigned long pfn, unsigned long pgcnt)
> >         page = pfn_to_page(pfn);
> >         page_ext = page_ext_get(page);
> >
> > +       if (!page_ext)
> > +               return;
> 
> I would replace the above with the following, here and in other places:
> 
> if (!page_ext) {
>   WARN_ONCE(!is_zone_device_page(page),
>                           "page_ext is missing for a non-device page\n");
>   return;
> }

Hmm, but this function is silent for the !pfn_valid(@pfn) case, and the
old cold has BUG_ON(!page_ext). So we know the caller is not being
careful about @pfn, and existing code is likely avoiding the BUG_ON().

The justification for the WARN_ONCE(), or maybe VM_WARN_ONCE(), would
be if there is a high likelihood that ongoing kernel changes introduce
more pfn_valid() but not page_ext covered pages? Is that a realistic
scenario?