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 03C01C7EE23 for ; Mon, 15 May 2023 11:25:20 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 5E461900003; Mon, 15 May 2023 07:25:20 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 594B8900002; Mon, 15 May 2023 07:25:20 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 45BF8900003; Mon, 15 May 2023 07:25:20 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0016.hostedemail.com [216.40.44.16]) by kanga.kvack.org (Postfix) with ESMTP id 3675F900002 for ; Mon, 15 May 2023 07:25:20 -0400 (EDT) Received: from smtpin19.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay10.hostedemail.com (Postfix) with ESMTP id C64CDC1494 for ; Mon, 15 May 2023 11:25:19 +0000 (UTC) X-FDA: 80792258358.19.D3644CC Received: from mail-ed1-f48.google.com (mail-ed1-f48.google.com [209.85.208.48]) by imf24.hostedemail.com (Postfix) with ESMTP id D12CE180016 for ; Mon, 15 May 2023 11:25:17 +0000 (UTC) Authentication-Results: imf24.hostedemail.com; dkim=pass header.d=gmail.com header.s=20221208 header.b=jbbvynNq; spf=pass (imf24.hostedemail.com: domain of lstoakes@gmail.com designates 209.85.208.48 as permitted sender) smtp.mailfrom=lstoakes@gmail.com; dmarc=pass (policy=none) header.from=gmail.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1684149917; 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=qYw5o0HvEEWDxkyofYj3KGUxctwFLArsyHtXdfw7FXA=; b=CF5sfP/p78x5fpIPB+Oxt54HkF6zCNyTiis6uooNxDWgnQ5nmnJUL0duW7gR0pgbL8MS8o 8Qtd1YubPyDzLH4Fpkuaa+iab0Eln5cSKmFZBp4Ia1j0686eSa2sY8C0ifNmbI5J75i0Iq 6tJdARXUfoaSAeYEWHaYmbYPfHGDtQU= ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1684149917; a=rsa-sha256; cv=none; b=qaEqdXNjtZoyj2bT4Hocv9iMzzTR5iOmSChF8E8xy8bsbbj2AydiTVl0ENoNHg3V1fhszU nBY6NMPymlD8XgAUSf7DSPrsOEQwsmR0Mg4Lv+UGNfqRaFMWxXS6/J99jMQu1Ai+bNm40k CVNBpfrEa//qq5PjXmrlQ0urNM3HObg= ARC-Authentication-Results: i=1; imf24.hostedemail.com; dkim=pass header.d=gmail.com header.s=20221208 header.b=jbbvynNq; spf=pass (imf24.hostedemail.com: domain of lstoakes@gmail.com designates 209.85.208.48 as permitted sender) smtp.mailfrom=lstoakes@gmail.com; dmarc=pass (policy=none) header.from=gmail.com Received: by mail-ed1-f48.google.com with SMTP id 4fb4d7f45d1cf-50bcae898b2so22347654a12.0 for ; Mon, 15 May 2023 04:25:17 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20221208; t=1684149916; x=1686741916; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:from:to:cc:subject:date:message-id:reply-to; bh=qYw5o0HvEEWDxkyofYj3KGUxctwFLArsyHtXdfw7FXA=; b=jbbvynNqFDwn5QGIBqkxPLjOSvS9oQg0sxK8yIwEW1+QFpaNk5RVVAxyeKo8OlEX8y TWq0itO/EsZROeoya7JAVtTucCE9VuwFEDst6h2TgueFAsjiZ+nfPNq40EU4WTySbNhD NP4m1kFt4ktBIXCefxWIG/SDV8rBzF2S49Ah3k/0dRQVsY7xF88UiXf6z3qaQzQ9CbEu CsMgHcBG/rk9NtzxOY6zQcQ5caE8et0FgVHapn7BHz3Lif7Mh+P1mBfRjMM6eU19jfmi 3nOCUihOFR05tuKO6bhuiZjWTfJqa140N/1xcCUsUU1WYhO1qQKSIyP9zdRY4uE1otZH 0FLA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1684149916; x=1686741916; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:x-gm-message-state:from:to:cc:subject:date :message-id:reply-to; bh=qYw5o0HvEEWDxkyofYj3KGUxctwFLArsyHtXdfw7FXA=; b=B4mQqsb1dk/W7+BnG3a/XIrTA4BXzLFVLNqVurQQFL6gG4sjzLuoGykZPPZ/mofNQK unUmvl9EADBkJ5WiN00vZj6dwmr629Tvxr45ygid6f9+kUNiU18CHNOgL3q7fpZg1Zsb 6aIr4W8Js4jrlrv6G3Le+W7vP5nYWZjkHv5ZNuRzFPfVXBi5XNvI2gDR36NR/6MEvYEE 2FHZQsIzRW5WEs2gdosnBRyj4dNAx8EFcWA/0W3awTjOA7u5INvxjhe8V269Iw1Ni3Hd NixpuHZfulZVONiLvrbaZaE08BeAwVNcU0Sgp1TYICV0fneS7iEK7J9ouK9DjgCTvdK3 FJ0g== X-Gm-Message-State: AC+VfDxBVsm7Di3ZEXGZ6GD7ecBTvbRUtvAYAmkyDreX1GhUa6d2uwVV utU4TpwWmlhh51p8cFbipco= X-Google-Smtp-Source: ACHHUZ4Qid7cgUwxhiv6jrJ7EnqBsE3iqwJImnN2vzCNgOHm4KFYH0qvM0L5MJTHJ6iYr10udidLow== X-Received: by 2002:a17:906:db08:b0:94f:1c90:cb70 with SMTP id xj8-20020a170906db0800b0094f1c90cb70mr30936700ejb.66.1684149915837; Mon, 15 May 2023 04:25:15 -0700 (PDT) Received: from localhost ([31.94.21.70]) by smtp.gmail.com with ESMTPSA id h9-20020a170906584900b0094e597f0e4dsm9248259ejs.121.2023.05.15.04.25.14 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 15 May 2023 04:25:15 -0700 (PDT) Date: Mon, 15 May 2023 12:25:14 +0100 From: Lorenzo Stoakes To: Ryan Roberts 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 Subject: Re: [RESEND PATCH v1 1/5] mm: vmalloc must set pte via arch code Message-ID: <54ecd324-91ac-4fbc-8c47-46f12b2f5256@lucifer.local> References: <20230511132113.80196-1-ryan.roberts@arm.com> <20230511132113.80196-2-ryan.roberts@arm.com> <2d43731e-3a38-c96e-320e-6a0dc16f10e4@arm.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <2d43731e-3a38-c96e-320e-6a0dc16f10e4@arm.com> X-Rspamd-Queue-Id: D12CE180016 X-Stat-Signature: jnt1etspewb5eqc33en8fwaiwzhaazs8 X-Rspam-User: X-Rspamd-Server: rspam09 X-HE-Tag: 1684149917-466340 X-HE-Meta: U2FsdGVkX1/wxcn3tXsipkRgSCNKB2BbpP8gpog3VLiDhtTlfD7UMOIaMEDDGrgFfwYX1Dlz3GxmvUjMskMoxnVGJYYxqPYJZyiFRgSWcUHMGjZpum7Wbb2kdPXTMdo3zgke1RevjZeofgA74uhNEbnLeOCUQ2yS4fiFTW+oaNMyy+WOcqSK1p23s98U00crDi+ZE06EqaynJExx+X1v6IhApSnnFE54aBz6WuOQWZK4nHkkLqjc3EDiu6suNCzan0FSiwcG78S1jQ+ni2sWUGOjoAedjqJ9df5jY8HsQ5ro+sWO2raU59pYENut1qv3NHedcap3i4B4Hj1GUI8oFWtBpfqQIL52DBIM19pqSDYkn33ETlW3okJ0I2prBS8iTDi6p9NaVBDbTf5rTrmesAwny4rGJ6yMGL2V9cP0qY2G8kLHhshq8uyzf/e8/vlDrXv3PuSF7PdyX4qLIewUqKgdspQAdD3Wms7scTAbm9llmA66fGQRxxUCqKBWNfqQRDqXpeql8d7yk+RtZujwQonQUDx4Ja18lA2qUT2ckKbtj5cLjDbbycLBP0HRqJv3Msb2H+TLbDx8PP7C0M9gJ6IlknJjRhDd3sX1MZydboQ+svp4zq/UB+Xei1MeF4yv/Rpn6+6uDluXXkFOKXJBRrOHpmf7LisAuDSATlmuwP3uoCEipkolg3eHFMWeWcdsD4WeoKj+s+wGABrpyDndaFod6EU3o2vk+RRq9bV2qn6KgtOT8e+S9ZevpKr818KSb6FaYLUe2qw1rkuw84GmLBL6SFE1Dlh7WCCELAxzElKylhGe7Srkh7CmfFskfV7WDUzXS+KYxQMHP2nYaGlQFSg6w5fpcqJyGqKY+piE1VxxKCVzreNkpT6X5rUP2kf80Jx2oIe8LCT8KB5ywO29Sj3vJMnFxmMoKOtBgF37c6D/eAAqY8rAST/6CrkaHa5TuZzshR8Rh6SHn67MVTY 22KT+RUe pDxtDgmayLcrLIKrEljCckFqpRJD9sPjFTnSw4O+ii3ea2sc1XEEcgSOl/ZXLOKuEDMB67WFGBgd1ltLtwJboESkf211WTcofghRdyiCYWaCt2A0dFn5sCA1QQ8PAC29okrTCe8vyc4Png0A5WWGxcTSi76zPVQmlS90qmimIyWaDXrvpMiT3/bTfF2nU3WcaJ0kLMZq8MaskFgZwyIp84nJY3q9kqs14ohxA/upUGWpwbEaIsw7Wmgd0/G8tUDNiMz6t4WQ2gmQkPd23JqNtXwCq8xFN+dHTtKwH7R4t6yn8sfrGTomz+Ew8+hyW1e1NQ1jwGW9ixykMADm02KxrPU+2DBhMo0kanBib1Jkp46JfQ01x7fJzKwH3T9AL/nl55DZZzkD5tS/hQ1g85avWOrOYjDIliwEIypMTmB9zOcfO908wSFs8e3Ylpkj5vH0pWFEFf9KFGthF/G20aKzFEVWfYndUeMOGjdvYxz1eQ0ala44= 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 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... :) > > > > > 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. > > > > >> > >> 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 :) > > > > >> 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.