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 25B1EC7EE22 for ; Mon, 15 May 2023 12:14:32 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 9F092900004; Mon, 15 May 2023 08:14:31 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 97970900002; Mon, 15 May 2023 08:14:31 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 81B4E900004; Mon, 15 May 2023 08:14:31 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0017.hostedemail.com [216.40.44.17]) by kanga.kvack.org (Postfix) with ESMTP id 6B220900002 for ; Mon, 15 May 2023 08:14:31 -0400 (EDT) Received: from smtpin16.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay06.hostedemail.com (Postfix) with ESMTP id 414CBAF518 for ; Mon, 15 May 2023 12:14:31 +0000 (UTC) X-FDA: 80792382342.16.BF55543 Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by imf06.hostedemail.com (Postfix) with ESMTP id 46098180010 for ; Mon, 15 May 2023 12:14:27 +0000 (UTC) Authentication-Results: imf06.hostedemail.com; dkim=none; spf=pass (imf06.hostedemail.com: domain of ryan.roberts@arm.com designates 217.140.110.172 as permitted sender) smtp.mailfrom=ryan.roberts@arm.com; dmarc=pass (policy=none) header.from=arm.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1684152868; 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:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=WaBTulVml1AZNKU13OGViapnLunTU6o/SdDGjP3WAkU=; b=rptHGMzssjVzMuO8yADzLTDIP1Ul99BkmizLwHFFGhTS+sCNVvN+9bw7ijLN/l0m8NuoPn fu5bTKF9qMxSLj96UXjmb5iReJD4NAC0U0zgndnWVIZQbdLuVc+/lD5XqbaStAUxQZ0EBO SOvihBkkHgcDY8MAvNZjiC8aNPuDAhM= ARC-Authentication-Results: i=1; imf06.hostedemail.com; dkim=none; spf=pass (imf06.hostedemail.com: domain of ryan.roberts@arm.com designates 217.140.110.172 as permitted sender) smtp.mailfrom=ryan.roberts@arm.com; dmarc=pass (policy=none) header.from=arm.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1684152868; a=rsa-sha256; cv=none; b=4ACc0F5Xwz7J8YnM/GljHFIbSodroq4e9ADTnXphIimG0gXCEQ9EGre0fLmrN8H9enTMJk DnMXQdQvQ1ALlmqnEvl52y0TD5tSCuycHKtXBpvZH9Q/l5dbKYx4FFeopPpTUXhL1TTBmd ycPRpmqI53gl1lWndA9CkiUTjthtF2c= 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 BB232FEC; Mon, 15 May 2023 05:15:11 -0700 (PDT) Received: from [10.57.73.22] (unknown [10.57.73.22]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 9FB783F663; Mon, 15 May 2023 05:14:25 -0700 (PDT) Message-ID: <47077d53-050b-5521-3dd8-dfd0f5e89269@arm.com> Date: Mon, 15 May 2023 13:14:24 +0100 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:102.0) Gecko/20100101 Thunderbird/102.11.0 Subject: Re: [RESEND PATCH v1 1/5] mm: vmalloc must set pte via arch code To: Lorenzo Stoakes Cc: Andrew Morton , "Matthew Wilcox (Oracle)" , "Kirill A. Shutemov" , SeongJae Park , "linux-kernel@vger.kernel.org" , "linux-mm@kvack.org" , "damon@lists.linux.dev" , Christoph Hellwig , Uladzislau Rezki References: <20230511132113.80196-1-ryan.roberts@arm.com> <20230511132113.80196-2-ryan.roberts@arm.com> <2d43731e-3a38-c96e-320e-6a0dc16f10e4@arm.com> <54ecd324-91ac-4fbc-8c47-46f12b2f5256@lucifer.local> From: Ryan Roberts In-Reply-To: <54ecd324-91ac-4fbc-8c47-46f12b2f5256@lucifer.local> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-Rspam-User: X-Rspamd-Server: rspam01 X-Rspamd-Queue-Id: 46098180010 X-Stat-Signature: fti53gxrp9oo3e1xnr7cwes6jdmycn7w X-HE-Tag: 1684152867-872524 X-HE-Meta: U2FsdGVkX1+Odq+ethACB/4uF8JDm3J8+eHc8BkrqOe3YkdPk0ujis+m51xYALzmup5wjyym3piVfj0IBirM6D5CmY2Ll5OD9qWiDXvQo5NoAPgy670OigZX1QtgNlBDkN2PcX0mLkhcGMT4r6kVZoHwIh2IkDcnTiAnAnBuG/mZ359h2NPkGBlIg+aK61YW83NSOullz4+1PSw5DEvdsXP2oGJXdXWNq83GuqMb0Eb19HG24c9T8jy0W6gzAAJNVQwZR52hqZVS7BKU1kFA9wGSEApo3TirVVe4iiOR05uJfAenTYpo0+iJXPqroDj7ASfMtmpaB2JtITkrJX9txGfesrHuDtS6QcU85cQ0skIrmDI/YH7FjToR7PMGwnfFiMyW+rHwyygh+/g/P0Y3vS8MS7Dwn2ZxlUPGiEiAd7BxzBBrex9m1YPF+Fp+H1Q9yWygbK4AoEdqZBaMXt9S1BOGqHSqOPlTfdx5A2IrfWC7sg9KY524MmP6H0hAcFWwUX/8ZqZBKmlbEIqB3bY5wkx9SfVUrGS/cN7fpZJBbbtULgB708NFhKuEmLvk0XhwQQ4VjLhIfrI6fteE/XTYbVfhLREOB9F+gAsJiezBSY1cwW1C+uzEMsioB9zH0Ei9yoUXfxjDk8PVHxfCKZtk2MC8HWKj8rYGx0lrBuA5q5qrX9WZD27wcQUJ20j1RCGidyK40cy23loydrTXzl3HycRd/2bbG+J0BIlfEC42bJurY54psMjlGGId2ZGh+E0jFoYM48be7ZDmUg3kNL+FoDbMkwGvFWwBHFzgidNqBAGdjDrDMF+sxMQsWIBOdqJXjnOhwq+y9soe4CFI+LknE/zuh5KJIuM5Sncn8t6qLSxsXHydjsAT8wGcARu1DAnL1ruT/uWktKR6movJ6hWCKmBZasymFmnuZ88nkEdtsru5F0SvJtAL1qPVl+9BQ3per5j1poDs/rajA+xbiGj eNcSpJDF zCwjDggNGhHxVsCd7l51QIfikBvLnGUoVGsgHJ+PAZf41vGpHzuj8Lf4+/GmGrRmvXuOux4E2JxgbZKtqopcgCIoFAFSmiDmqjpjQRKM8QR+U80ah+G+cA4HuLjgTS8Hx+TT6fXBV2cL9rWIP8/op2oqp+Xn9R8TZxuWsqJV/PgXc7f4CGIEEIbqt0JO5IVRHMxuEXUfLauHRY4M= 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 15/05/2023 12:25, Lorenzo Stoakes wrote: > On Mon, May 15, 2023 at 09:29:16AM +0100, Ryan Roberts wrote: >> Hi Lorenzo, >> >> Thanks for the review - I appreciate it! >> >> >> On 13/05/2023 14:14, Lorenzo Stoakes wrote: >>> You've not cc'd the vmalloc reviewers, including the author of 3e9a9e256b1e >>> whose patch you purport to fix. Please remember to run get_maintainers.pl >>> on all files you patch and cc them at least on relevant patches. >>> >>> Have added Christoph + Uladzislau as cc. >> >> I did run get_maintainers.pl, but it gave me 82 names. I assumed I wouldn't be >> making any friends by CCing everyone, so tried to choose what I thought was a >> sensible base. I guess I didn't quite get it right. Sorry about that. Thanks for >> noticing and adding the right people. > > Right you mean across the whole of the patch set? Different people have > different approaches as to how to cc patch sets as a whole, but it's not > optional to include maintainers and reviewers on patches, so you should at least > cc- them on individual patches. > > It's ok, it's really easy to mess this up, I have managed every variant of doing > this the wrong way myself... :) Well I look forward to tripping over all the other variants in due course. ;-) > >> >>> >>> You'll definitely want an ack from Christoph on this! >>> >>> On Thu, May 11, 2023 at 02:21:09PM +0100, Ryan Roberts wrote: >>>> It is bad practice to directly set pte entries within a pte table. >>>> Instead all modifications must go through arch-provided helpers such as >>>> set_pte_at() to give the arch code visibility and allow it to validate >>>> (and potentially modify) the operation. >>> >>> This does make sense, and I see for example in xtensa that an arch-specific >>> instruction is issued under certain circumstances so I do suspect we should >>> do this. >> >> arm64 provides another example, where barriers are required to ensure the page >> table walker sees the new pte and no fault is raised. See >> arch/arm64/include/asm/pgtable.h:set_pte() (which is called by its >> implementation of set_pte_at()). > > Ack, yeah I do think your patch is correct. > >> >>> >>> As for validation, the function never indicates an error, so only in the >>> sense that a WARN_ON() could _in theory_ trigger is it being >>> validated. This might be quite a nitty point :) as set_pte_at() has no >>> means of indicating an error. But maybe to be pedantic 'check' rather than >>> 'validate'? >> >> I'm sorry, I'm not sure what you are asking here? set_pte_at() forms part of the >> contract with he arch code and is defined never to return an error. Some >> implementations might have code enabled in debug configs to detect incorrect >> usage and emit warnings (see arm64's implementation). > > I'm saying that 'validate' implies to me that you assess whether the value is > correct and behave differently accordingly. It's something of a pedantic point, > but perhaps 'check' is better here. Ahh, you were critiqing the commit message, sorry totally missed that. I'll change 'validate' to 'check' in v2. > >> >>> >>>> >>>> Fixes: 3e9a9e256b1e ("mm: add a vmap_pfn function") >>> >>> Not sure if this is really 'fixing' anything, I mean ostensibly, but not >>> sure if the tag is relevant here, that is more so for a bug being >>> introduced, and unless an issue has arisen not sure if it's >>> appropriate. But this might be a nit, again! >> >> Well I'm happy to remove it if that's the concensus. But I do believe there is a >> real bug here. At least on arm64, the barriers are needed to prevent a race with >> the page table walker. That said, the only place in the tree I can see >> vmap_pfn() used, is in the i915 driver, which I guess has never been used on an >> arm64 platform. > > Yeah, again this might be a little too nitty! And I totally understand where > you're coming from, I do agree this is appears to be an issue and your solution > is right, it just feels less like an obvious 'bug' and more of an oversight. But > I am being pedantic, and am not overly worried if you retain it :) OK, I'm going to retain it. > >> >>> >>>> Signed-off-by: Ryan Roberts >>>> --- >>>> mm/vmalloc.c | 5 ++++- >>>> 1 file changed, 4 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/mm/vmalloc.c b/mm/vmalloc.c >>>> index 9683573f1225..d8d2fe797c55 100644 >>>> --- a/mm/vmalloc.c >>>> +++ b/mm/vmalloc.c >>>> @@ -2899,10 +2899,13 @@ struct vmap_pfn_data { >>>> static int vmap_pfn_apply(pte_t *pte, unsigned long addr, void *private) >>>> { >>>> struct vmap_pfn_data *data = private; >>>> + pte_t ptent; >>>> >>>> if (WARN_ON_ONCE(pfn_valid(data->pfns[data->idx]))) >>>> return -EINVAL; >>>> - *pte = pte_mkspecial(pfn_pte(data->pfns[data->idx++], data->prot)); >>>> + >>>> + ptent = pte_mkspecial(pfn_pte(data->pfns[data->idx++], data->prot)); >>>> + set_pte_at(&init_mm, addr, pte, ptent);> >>> While we're refactoring, it'd be nice to stash data->pfns[data->idx] into a >>> local pfn variable. >> >> OK, I'll do this for v2. > > Thanks! > >> >> Thanks, >> Ryan >> >> >>> >>>> return 0; >>>> } >>>> >>>> -- >>>> 2.25.1 >>>> >> > > Sorry to get into the weeds here a bit, overall I think this patch is fine, I > would like Christoph to take a look given it's his code however. No problem; I'm new here, so just having someone taking the time to respond with specific feedback, is a win as far as I'm concerned! Thanks, Ryan