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 D8851C77B76 for ; Mon, 17 Apr 2023 15:14:39 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 543FF8E0002; Mon, 17 Apr 2023 11:14:39 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 4F4C38E0001; Mon, 17 Apr 2023 11:14:39 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 395FD8E0002; Mon, 17 Apr 2023 11:14:39 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0011.hostedemail.com [216.40.44.11]) by kanga.kvack.org (Postfix) with ESMTP id 2C5B58E0001 for ; Mon, 17 Apr 2023 11:14:39 -0400 (EDT) Received: from smtpin11.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay03.hostedemail.com (Postfix) with ESMTP id 001A3A0617 for ; Mon, 17 Apr 2023 15:14:38 +0000 (UTC) X-FDA: 80691229878.11.41753F3 Received: from mail-wm1-f49.google.com (mail-wm1-f49.google.com [209.85.128.49]) by imf04.hostedemail.com (Postfix) with ESMTP id 099B94001C for ; Mon, 17 Apr 2023 15:14:36 +0000 (UTC) Authentication-Results: imf04.hostedemail.com; dkim=pass header.d=gmail.com header.s=20221208 header.b=J6mNTdjJ; spf=pass (imf04.hostedemail.com: domain of lstoakes@gmail.com designates 209.85.128.49 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=1681744477; 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=e047zcn20dpW+BVxg6WrLYEA74ZTRhvYR9ivWfzVffI=; b=2/XOEASwurGJfNMYKtDxtq+38wn3vjfL8Z+40olT002EJW8jn6Y0rncBS4NrpmubmqF4bF U+YfyHwoBYtbMlurlHCgO8dBcwJWCaBvqab2BbbrfEuJ7yHyTbHlYnxRinv5trYt99gmos 2/lk2b8w5MJSi4k17S1rUGfqoJMPLDY= ARC-Authentication-Results: i=1; imf04.hostedemail.com; dkim=pass header.d=gmail.com header.s=20221208 header.b=J6mNTdjJ; spf=pass (imf04.hostedemail.com: domain of lstoakes@gmail.com designates 209.85.128.49 as permitted sender) smtp.mailfrom=lstoakes@gmail.com; dmarc=pass (policy=none) header.from=gmail.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1681744477; a=rsa-sha256; cv=none; b=ii34CLH64wLAzx9fq77CwrVUg8AgRLMyZ4Zw8hiZPoqu5pGuQlmgQcoF+cSSlG0f9+s+/r yX1DUBialNXDzCLyvgDSTPGoShVUTeYD+Scotb7VduCVgdU1gfmd66MXvLDw4rgpZWaLj3 v6ebKWy/+0chlkduUn5XGZ/4TY5nLxw= Received: by mail-wm1-f49.google.com with SMTP id 5b1f17b1804b1-3f09b9ac51dso44003735e9.0 for ; Mon, 17 Apr 2023 08:14:36 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20221208; t=1681744475; x=1684336475; 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=e047zcn20dpW+BVxg6WrLYEA74ZTRhvYR9ivWfzVffI=; b=J6mNTdjJXRwthJ2zCT9NEY6Z9hezMuH8x4BK/1R4BCVgk7Ci/HPN+jrcwQsKcCuTkI I1UPqwJz5ABCTk7X5opTEZxdgNLtiV+gU4ZZA1u010mlWUCYGH/bChtn7BAwLYDZ3JHk RTI8uWQsSY0HbGSVWTcygeXA3TlQzhMwGYmt3V1Id6ozIaB9PWoHyCMpAq+wrBMf57z1 SGYdFjrENk4shHH5MXl7JnDBNcBGd0m9BV/6PlyFsgA6iVj6APNfSGWyjUQ/JZLa59nc mSzqI9Ijxbahm/QxlPpW5/WVK0DYMNibbaiJENbR3l7PJJ/9luB7VdSEH8WgJVZjnIyE roWw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1681744475; x=1684336475; 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=e047zcn20dpW+BVxg6WrLYEA74ZTRhvYR9ivWfzVffI=; b=WDr8tQoF2x50cxcS9H9Wt3mVkedutSt1MJdWH6oecIj5Vk3CO1UO8dkgC0do2sADMg LG1uG8CNIUZI7AO0i6Y7U5JFwxbeHKlSYFJOZKFznukNXBdgmjiLTwRt+kqkiOtkPu0o Pi0TlQ9m8GdBY138pzeN+7UDI2uvNEy4IzK+NohyqcqeLCKakYe4PRfAM6j+UstXQ0yg AUpK+AEDNnPUU8nsmkTer6Yb3F0u2GvYXC/24K29xZGf5UO7tqrw4k32/V1mvSUQKhUN j1mTsq6JXhNIFh/Spj0TGxd1xidkVPgAArjvqCZG8oceM9g1F6zG0WgvKBnj9Va9jYor u4Eg== X-Gm-Message-State: AAQBX9cri3uBv2Nz9QfuGsr/+2T1Bd1OKCxMonaJ98HJVJjREtfS5kS7 hH2aBeTOpd+s0RBY7yZJmPE= X-Google-Smtp-Source: AKy350YbK9Uk+W2I0O+WB4raZRtNy4HfgrcXDXV+XwhnLO6MAXE8TxKBQdwImQWHd3Rphf8UasdaGw== X-Received: by 2002:adf:cf01:0:b0:2fa:abcd:59a2 with SMTP id o1-20020adfcf01000000b002faabcd59a2mr2273209wrj.30.1681744475252; Mon, 17 Apr 2023 08:14:35 -0700 (PDT) Received: from localhost (host86-156-84-164.range86-156.btcentralplus.com. [86.156.84.164]) by smtp.gmail.com with ESMTPSA id i4-20020a5d55c4000000b002f74578f494sm8341442wrw.41.2023.04.17.08.14.34 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 17 Apr 2023 08:14:34 -0700 (PDT) Date: Mon, 17 Apr 2023 16:14:33 +0100 From: Lorenzo Stoakes To: "Eric W. Biederman" Cc: Jason Gunthorpe , linux-mm@kvack.org, linux-kernel@vger.kernel.org, Andrew Morton , Matthew Wilcox , David Hildenbrand , linux-arm-kernel@lists.infradead.org, kvm@vger.kernel.org, linux-s390@vger.kernel.org, linux-fsdevel@vger.kernel.org, linux-perf-users@vger.kernel.org, linux-security-module@vger.kernel.org, Catalin Marinas , Will Deacon , Christian Borntraeger , Janosch Frank , Claudio Imbrenda , Heiko Carstens , Vasily Gorbik , Alexander Gordeev , Sven Schnelle , Kees Cook , Alexander Viro , Christian Brauner , Peter Zijlstra , Ingo Molnar , Arnaldo Carvalho de Melo , Mark Rutland , Alexander Shishkin , Jiri Olsa , Namhyung Kim , Ian Rogers , Adrian Hunter , Kentaro Takeda , Tetsuo Handa , Paul Moore , James Morris , "Serge E . Hallyn" , Paolo Bonzini Subject: Re: [PATCH 3/7] mm/gup: remove vmas parameter from get_user_pages_remote() Message-ID: References: <5a4cf1ebf1c6cdfabbf2f5209facb0180dd20006.1681508038.git.lstoakes@gmail.com> <9be77e7e-4531-4e1c-9e0d-4edbb5ad3bd5@lucifer.local> <241f0c22-f3d6-436e-a0d8-be04e281ed2f@lucifer.local> <87cz427diu.fsf@email.froward.int.ebiederm.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <87cz427diu.fsf@email.froward.int.ebiederm.org> X-Rspamd-Queue-Id: 099B94001C X-Stat-Signature: exuti1zoxcqtymq58qsds6y8y4pbw8q5 X-Rspam-User: X-Rspamd-Server: rspam08 X-HE-Tag: 1681744476-641036 X-HE-Meta: U2FsdGVkX1/ESEPVczhgm+oxxBhirxCfPj2bBqO3lPrtLcS1+aScMig46RGjjHMe5n81ZwKmcN62N9NRuhtJwCSm7XZ2rEQ/R/xogxotdR97w4smeuumR1a3kwiPYJ4AYSC7QdmAfrWB/05OGAe+n7+HDK+tJ8h2HMiA6kP2EDCtRzL7lxYs5cWpgOIHGMClVpwhb/+vWcCCoUOZkSxlXhDXyqNmZ/DGQjDjVmhgpgkqlX2JVszpkZgIRMI4KBE2bI8hH6knLKQT31xApL0s4mjUbMHdTr/IVZJyD9YTTfIBjUtdNGElfBZUFhp9TF6wVjuxC1a10dtJPWlcB2nwfRHKPOI2P0ucIU6FwnJ85W+/XTGHc080VVYBbH7na//jkmm63w5b7W16reDfr7lBzTZ3UcvNek2sDCB5C9cdaM0/HRRwhFs+HCJoYMxvU12Za+4C3Vfr72UuTvL9d3MDyuAeU4LuSseT7oouxTPF+Hrz6ECdb3epMq7mrrolkCVERJg/YYIcHHRe0fIiPcqwjAsD/M8UTkQuspRyiP6syGY6uQvYYanHowUM4yVuAUUY/CjHFziyAmIP5ki7b745b5BLvzr+CCpJ4KYu+9by8mrmjeQNTa4iEVfnk1QwoC1jvqvmuLHAPWGfk86PvBcqJ9r4uHrFvNnvGMTwHLPAmBa3UcepgG7J8mFFCgW8Zr3bXBjdxUWfU53XY2Q/vuAH++nHbK7M6ZcovG3HEcWuil1hZuKAb9UB7yjpzjwxXteb9SnQsB7RnQ4qvsaHaSNzvPaF+URxpjG5WL0dgFzo+0EEmVxFfBinaV3/jOQ/etymppskgbcQD6FGOhMb4SSLe7rnWzdGfQcLZtSXbg37JoC8zvLomfYk8VC8cPCcmHLEhdBVuHmXrNjeTBMBT85Et9dYpzEUC7Cz6fdjnOUEFBCe8HKIReZyhQvav0nOLvpgG4C2Sc3ZywERtsx2PlA pvhE8O9B 0ynuM0MHZbJ5y2EzmBc9p1L0CR4bxKEU8ZJRc46spZ40WYqmu4Tj9tUyBJ95M5ufbZkOwUxDKwaQFsisbxh2CgGsS3ix7l9R20HEQxCnLWncw3/sfx07UbqjXmGKx0nl/AORaEb8pZX+X2MstJ11e8YzFbpBJAU41qOz1lGijZJ5e5x/O9CRv0QHALrMFZxfhcU1FDFxl9LfZ5CzZi+Lg78IbrPJIQyQC8XcW4T0KA5Oi+r+X8SwFJAQfoBdYuQNRSN24UDU+1xRQfYr+8QT4Q3Pnshr+gJccNAlASHFB22hLS5pmVZaRhkjKwcs5WgUzhJRu+AOILw6W/XPO+T6x38mZuLNDeJxFWPFF8IVFrz6hyRxNmqNLigPl5R1ceYQuUTSZkjSHZKB7eNOHD8r5wwR1lJxJ38OHIPMLkrzQSIeImGqw7TXSzAg5Gy52NEMyqDKRMupn4+5XzEqL85T49HCuZoI5VcVqwHZkllPhTRBa8AQ= 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, Apr 17, 2023 at 10:07:53AM -0500, Eric W. Biederman wrote: > Lorenzo Stoakes writes: > > > On Mon, Apr 17, 2023 at 10:16:28AM -0300, Jason Gunthorpe wrote: > >> On Mon, Apr 17, 2023 at 02:13:39PM +0100, Lorenzo Stoakes wrote: > >> > On Mon, Apr 17, 2023 at 10:09:36AM -0300, Jason Gunthorpe wrote: > >> > > On Sat, Apr 15, 2023 at 12:27:31AM +0100, Lorenzo Stoakes wrote: > >> > > > The only instances of get_user_pages_remote() invocations which used the > >> > > > vmas parameter were for a single page which can instead simply look up the > >> > > > VMA directly. In particular:- > >> > > > > >> > > > - __update_ref_ctr() looked up the VMA but did nothing with it so we simply > >> > > > remove it. > >> > > > > >> > > > - __access_remote_vm() was already using vma_lookup() when the original > >> > > > lookup failed so by doing the lookup directly this also de-duplicates the > >> > > > code. > >> > > > > >> > > > This forms part of a broader set of patches intended to eliminate the vmas > >> > > > parameter altogether. > >> > > > > >> > > > Signed-off-by: Lorenzo Stoakes > >> > > > --- > >> > > > arch/arm64/kernel/mte.c | 5 +++-- > >> > > > arch/s390/kvm/interrupt.c | 2 +- > >> > > > fs/exec.c | 2 +- > >> > > > include/linux/mm.h | 2 +- > >> > > > kernel/events/uprobes.c | 10 +++++----- > >> > > > mm/gup.c | 12 ++++-------- > >> > > > mm/memory.c | 9 +++++---- > >> > > > mm/rmap.c | 2 +- > >> > > > security/tomoyo/domain.c | 2 +- > >> > > > virt/kvm/async_pf.c | 3 +-- > >> > > > 10 files changed, 23 insertions(+), 26 deletions(-) > >> > > > > >> > > > diff --git a/arch/arm64/kernel/mte.c b/arch/arm64/kernel/mte.c > >> > > > index f5bcb0dc6267..74d8d4007dec 100644 > >> > > > --- a/arch/arm64/kernel/mte.c > >> > > > +++ b/arch/arm64/kernel/mte.c > >> > > > @@ -437,8 +437,9 @@ static int __access_remote_tags(struct mm_struct *mm, unsigned long addr, > >> > > > struct page *page = NULL; > >> > > > > >> > > > ret = get_user_pages_remote(mm, addr, 1, gup_flags, &page, > >> > > > - &vma, NULL); > >> > > > - if (ret <= 0) > >> > > > + NULL); > >> > > > + vma = vma_lookup(mm, addr); > >> > > > + if (ret <= 0 || !vma) > >> > > > break; > >> > > > >> > > Given the slightly tricky error handling, it would make sense to turn > >> > > this pattern into a helper function: > >> > > > >> > > page = get_single_user_page_locked(mm, addr, gup_flags, &vma); > >> > > if (IS_ERR(page)) > >> > > [..] > >> > > > >> > > static inline struct page *get_single_user_page_locked(struct mm_struct *mm, > >> > > unsigned long addr, int gup_flags, struct vm_area_struct **vma) > >> > > { > >> > > struct page *page; > >> > > int ret; > >> > > > >> > > ret = get_user_pages_remote(*mm, addr, 1, gup_flags, &page, NULL, NULL); > >> > > if (ret < 0) > >> > > return ERR_PTR(ret); > >> > > if (WARN_ON(ret == 0)) > >> > > return ERR_PTR(-EINVAL); > >> > > *vma = vma_lookup(mm, addr); > >> > > if (WARN_ON(!*vma) { > >> > > put_user_page(page); > >> > > return ERR_PTR(-EINVAL); > >> > > } > >> > > return page; > >> > > } > >> > > > >> > > It could be its own patch so this change was just a mechanical removal > >> > > of NULL > >> > > > >> > > Jason > >> > > > >> > > >> > Agreed, I think this would work better as a follow up patch however so as > >> > not to distract too much from the core change. > >> > >> I don't think you should open code sketchy error handling in several > >> places and then clean it up later. Just do it right from the start. > >> > > > > Intent was to do smallest change possible (though through review that grew > > of course), but I see your point, in this instance this is fiddly stuff and > > probably better to abstract it to enforce correct handling. > > > > I'll respin + add something like this. > > Could you include in your description why looking up the vma after > getting the page does not introduce a race? > > I am probably silly and just looking at this quickly but it does not > seem immediately obvious why the vma and the page should match. > > I would not be surprised if you hold the appropriate mutex over the > entire operation but it just isn't apparent from the diff. > > I am concerned because it is an easy mistake to refactor something into > two steps and then discover you have introduced a race. > > Eric > The mmap_lock is held so VMAs cannot be altered and no such race can occur. get_user_pages_remote() requires that the user calls it under the lock so it is implicitly assured that this cannot happen. I'll update the description to make this clear on the next spin! (side-note: here is another irritating issue with GUP, we don't suffix with _locked() consistently)