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 44FDFC433FE for ; Wed, 19 Jan 2022 18:43:23 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 6AAA56B0072; Wed, 19 Jan 2022 13:43:22 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id 658E46B0073; Wed, 19 Jan 2022 13:43:22 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 56F1B6B0074; Wed, 19 Jan 2022 13:43:22 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0178.hostedemail.com [216.40.44.178]) by kanga.kvack.org (Postfix) with ESMTP id 43CBF6B0072 for ; Wed, 19 Jan 2022 13:43:22 -0500 (EST) Received: from smtpin29.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay04.hostedemail.com (Postfix) with ESMTP id 009A79518E for ; Wed, 19 Jan 2022 18:43:21 +0000 (UTC) X-FDA: 79047909444.29.CFE44D5 Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by imf07.hostedemail.com (Postfix) with ESMTP id 6E70140012 for ; Wed, 19 Jan 2022 18:43:20 +0000 (UTC) Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id C31131FB; Wed, 19 Jan 2022 10:43:19 -0800 (PST) Received: from [10.57.67.190] (unknown [10.57.67.190]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 1E4B93F73D; Wed, 19 Jan 2022 10:43:16 -0800 (PST) Message-ID: Date: Wed, 19 Jan 2022 18:43:10 +0000 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (Windows NT 10.0; rv:91.0) Gecko/20100101 Thunderbird/91.5.0 Subject: Re: [PATCH] vmap(): don't allow invalid pages Content-Language: en-GB To: Matthew Wilcox Cc: Yury Norov , Catalin Marinas , Will Deacon , Andrew Morton , Nicholas Piggin , Ding Tianhong , Anshuman Khandual , Alexey Klimov , linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, linux-mm@kvack.org References: <20220118235244.540103-1-yury.norov@gmail.com> From: Robin Murphy In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-Rspamd-Queue-Id: 6E70140012 X-Stat-Signature: 3n5cbi1n1rgkcp36jrbz959sn1owp31q Authentication-Results: imf07.hostedemail.com; dkim=none; dmarc=pass (policy=none) header.from=arm.com; spf=pass (imf07.hostedemail.com: domain of robin.murphy@arm.com designates 217.140.110.172 as permitted sender) smtp.mailfrom=robin.murphy@arm.com X-Rspamd-Server: rspam02 X-HE-Tag: 1642617800-798510 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: On 2022-01-19 16:27, Matthew Wilcox wrote: > On Wed, Jan 19, 2022 at 01:28:14PM +0000, Robin Murphy wrote: >>> + if (WARN_ON(!pfn_valid(page_to_pfn(page)))) >> >> Is it page_to_pfn() guaranteed to work without blowing up if page is invalid >> in the first place? Looking at the CONFIG_SPARSEMEM case I'm not sure that's >> true... > > Even if it does blow up, at least it's blowing up here where someone > can start to debug it, rather than blowing up on first access, where > we no longer have the invlid struct page pointer. But if that were the case then we'd blow up on the following line when mk_pte(page, prot) ends up calling it same anyway. Indeed it's arguably the best-case scenario since it would also blow up in page_address() if we hit the vmap_range loop rather than going down the vmap_small_pages_range_noflush() path. Furthermore, if you *are* lucky enough to take a fault upon accessing a bogus mapping, then surely a phys_to_page() calculation on whatever ended up in the PTE should get you back the original "pointer" anyway, shouldn't it? Sure it's a bit more work to extract the caller out of the VMA if necessary, but hey, that's debugging! Maybe vmap() failing means you then pass the offending nonsense to __free_pages() and that ruins your day anyway... The implications are infinitely worse if you've mapped something that did happen to be a valid page, but wasn't the *right* page, such that you don't fault but corrupt things or trigger a fatal system error instead. I'd echo Mark's point that if we can't trust a page pointer to be correct then we've already lost. In general the proportion of "wrong" pointers one can viably attempt to detect is so small that it's rarely ever worth trying, and the cases that are so obviously wrong tend to show up well enough in normal operation (although NULL-safety is of course a bit of a special case when it can simplify API usage). > I don't think we have a 'page_valid' function which will tell us whether > a random pointer is actually a struct page or not. Indeed, my impression is that the only legitimate way to get hold of a page pointer without assumed provenance is via pfn_to_page(), which is where pfn_valid() comes in. Thus pfn_valid(page_to_pfn()) really *should* be a tautology. I guess in this instance it would be technically feasible to implement a function which checks "is this a correctly-aligned pointer within memmap bounds", but IMO that would be a massive step in the wrong direction. We're developers; sometimes we introduce bugs when developing code, and it takes effort to debug them, but that still doesn't make it a good idea to optimise normal code paths for the expectation of writing new catastrophically-bad bugs. Plus logically if such a "page_valid()" check could be justified at all then that should rightfully lead to a churn-fest of adding it to pretty much every interface which accepts page pointers - one half of vmap() is hardly special. FWIW, If anything I reckon a DEBUG_VM option that made checks within page_to_x/x_to_page as appropriate would help Yury's issue just as much, while having the potential to be considerably more useful in general. Cheers, Robin.