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 CC2B1C77B7E for ; Tue, 2 May 2023 18:17:17 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 27D6B6B0074; Tue, 2 May 2023 14:17:17 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 22E3A6B0078; Tue, 2 May 2023 14:17:17 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 0F6196B007B; Tue, 2 May 2023 14:17:17 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from mail-wr1-f45.google.com (mail-wr1-f45.google.com [209.85.221.45]) by kanga.kvack.org (Postfix) with ESMTP id B3A106B0074 for ; Tue, 2 May 2023 14:17:16 -0400 (EDT) Received: by mail-wr1-f45.google.com with SMTP id ffacd0b85a97d-2fddb442d47so3900039f8f.2 for ; Tue, 02 May 2023 11:17:16 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20221208; t=1683051436; x=1685643436; 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=R3bHT20tkh2F79+SFx041a+gGb7WqEwkVCZSdSx9/lw=; b=ABZvkaFx4jKAf56HV0DtLrke19279svzYn5R3maOZMPNdxk0ObOArzxg1z10J93D2a yLqJcK/Q43aIz0gAQN6GUv32LLd4ihSJH2F3lDhmH6FsxDa2erMpPikt/SxAVgopgXh/ kePR/OzazJRcy7NYIKiVq0ZpgVDTP1xoUDJqWPO64eI6859mQ7bOQbyBJ1ugVRvqeNck ybXXGPgAtjCcK3M5B0rdV9P+ADK+khmo7zHMhxkuQ2Fp77MhaAhxZbRtET12rtM07E52 ixbhgq27zTpHgcZGNlvbBdyVkC5sSxn0J2bqVU/ZNhFeOPyhwZpIeArFjZC3/h2p331x RF/w== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1683051436; x=1685643436; 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=R3bHT20tkh2F79+SFx041a+gGb7WqEwkVCZSdSx9/lw=; b=YsoQX9tsJVPODVy5I9K37euIRuONjZIaCcglDd+PFWQLOb/NuagT/fRUAQcjg6Bv4w 3WtysvSfec5HVOJ9DkhLR7N9u+b4gy4eJVTe8khGFzi37U1T7ZFuD1USJnLqMcZTqIzR a1HXEDL0EUkujkkh15P7j/1H4coBtQwePuzlQxNfZ4GhdDubaznzC5kN8I313dW0htpP j2MJJfW+wzHYm+Xh7ljU+E99/ZFyp4znnsYSGZPstfaSK3xr4FZ8xtEJ5C5bLncESDFo qXtttYkzGVy9uEfWNT9IX8XghAapmcorOZwHityLqgyzV5t2wye5ckEX8XEJKfGGGR3i DePQ== X-Gm-Message-State: AC+VfDw7+oq7Dhc5L3KRMKyn3J8nSRn+IdDdSB6qo4Ve7hbw90AFQJdo 9sgNu5YuqWTntgHCkKFR1iM= X-Google-Smtp-Source: ACHHUZ5PfpjRwsbkRTjC3oEfUSuMXhuAYSi7RouJe4rf6mD72BqoN0/2utYW1uSDsC/8YUwsJgFW7w== X-Received: by 2002:adf:f147:0:b0:2fd:98a8:e800 with SMTP id y7-20020adff147000000b002fd98a8e800mr13943385wro.7.1683051435620; Tue, 02 May 2023 11:17:15 -0700 (PDT) Received: from localhost (host86-156-84-164.range86-156.btcentralplus.com. [86.156.84.164]) by smtp.gmail.com with ESMTPSA id h3-20020a5d5043000000b002c70ce264bfsm31561902wrt.76.2023.05.02.11.17.14 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 02 May 2023 11:17:14 -0700 (PDT) Date: Tue, 2 May 2023 19:17:14 +0100 From: Lorenzo Stoakes To: David Hildenbrand Cc: Peter Zijlstra , linux-mm@kvack.org, linux-kernel@vger.kernel.org, Andrew Morton , Jason Gunthorpe , Jens Axboe , Matthew Wilcox , Dennis Dalessandro , Leon Romanovsky , Christian Benvenuti , Nelson Escobar , Bernard Metzler , Ingo Molnar , Arnaldo Carvalho de Melo , Mark Rutland , Alexander Shishkin , Jiri Olsa , Namhyung Kim , Ian Rogers , Adrian Hunter , Bjorn Topel , Magnus Karlsson , Maciej Fijalkowski , Jonathan Lemon , "David S . Miller" , Eric Dumazet , Jakub Kicinski , Paolo Abeni , Christian Brauner , Richard Cochran , Alexei Starovoitov , Daniel Borkmann , Jesper Dangaard Brouer , John Fastabend , linux-fsdevel@vger.kernel.org, linux-perf-users@vger.kernel.org, netdev@vger.kernel.org, bpf@vger.kernel.org, Oleg Nesterov , Jason Gunthorpe , John Hubbard , Jan Kara , "Kirill A . Shutemov" , Pavel Begunkov , Mika Penttila , Dave Chinner , Theodore Ts'o , Peter Xu , Matthew Rosato , "Paul E . McKenney" , Christian Borntraeger , Mike Rapoport Subject: Re: [PATCH v7 3/3] mm/gup: disallow FOLL_LONGTERM GUP-fast writing to file-backed mappings Message-ID: <3a8c672d-4e6c-4705-9d6c-509d3733eb05@lucifer.local> References: <1691115d-dba4-636b-d736-6a20359a67c3@redhat.com> <20230502172231.GH1597538@hirez.programming.kicks-ass.net> <406fd43a-a051-5fbe-6f66-a43f5e7e7573@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <406fd43a-a051-5fbe-6f66-a43f5e7e7573@redhat.com> 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 Tue, May 02, 2023 at 07:34:06PM +0200, David Hildenbrand wrote: > On 02.05.23 19:22, Peter Zijlstra wrote: > > On Tue, May 02, 2023 at 07:13:49PM +0200, David Hildenbrand wrote: > > > [...] > > > > > > > +{ > > > > + struct address_space *mapping; > > > > + > > > > + /* > > > > + * GUP-fast disables IRQs - this prevents IPIs from causing page tables > > > > + * to disappear from under us, as well as preventing RCU grace periods > > > > + * from making progress (i.e. implying rcu_read_lock()). > > > > + * > > > > + * This means we can rely on the folio remaining stable for all > > > > + * architectures, both those that set CONFIG_MMU_GATHER_RCU_TABLE_FREE > > > > + * and those that do not. > > > > + * > > > > + * We get the added benefit that given inodes, and thus address_space, > > > > + * objects are RCU freed, we can rely on the mapping remaining stable > > > > + * here with no risk of a truncation or similar race. > > > > + */ > > > > + lockdep_assert_irqs_disabled(); > > > > + > > > > + /* > > > > + * If no mapping can be found, this implies an anonymous or otherwise > > > > + * non-file backed folio so in this instance we permit the pin. > > > > + * > > > > + * shmem and hugetlb mappings do not require dirty-tracking so we > > > > + * explicitly whitelist these. > > > > + * > > > > + * Other non dirty-tracked folios will be picked up on the slow path. > > > > + */ > > > > + mapping = folio_mapping(folio); > > > > + return !mapping || shmem_mapping(mapping) || folio_test_hugetlb(folio); > > > > > > "Folios in the swap cache return the swap mapping" -- you might disallow > > > pinning anonymous pages that are in the swap cache. > > > > > > I recall that there are corner cases where we can end up with an anon page > > > that's mapped writable but still in the swap cache ... so you'd fallback to > > > the GUP slow path (acceptable for these corner cases, I guess), however > > > especially the comment is a bit misleading then. > > > > > > So I'd suggest not dropping the folio_test_anon() check, or open-coding it > > > ... which will make this piece of code most certainly easier to get when > > > staring at folio_mapping(). Or to spell it out in the comment (usually I > > > prefer code over comments). > > > > So how stable is folio->mapping at this point? Can two subsequent reads > > get different values? (eg. an actual mapping and NULL) > > > > If so, folio_mapping() itself seems to be missing a READ_ONCE() to avoid > > the compiler from emitting the load multiple times. > > I can only talk about anon pages in this specific call order here (check > first, then test if the PTE changed in the meantime): we don't care if we > get two different values. If we get a different value the second time, > surely we (temporarily) pinned an anon page that is no longer mapped (freed > in the meantime). But in that case (even if we read garbage folio->mapping > and made the wrong call here), we'll detect afterwards that the PTE changed, > and unpin what we (temporarily) pinned. As folio_test_anon() only checks two > bits in folio->mapping it's fine, because we won't dereference garbage > folio->mapping. > > With folio_mapping() on !anon and READ_ONCE() ... good question. Kirill said > it would be fairly stable, but I suspect that it could change (especially if > we call it before validating if the PTE changed as I described further > below). > > Now, if we read folio->mapping after checking if the page we pinned is still > mapped (PTE unchanged), at least the page we pinned cannot be reused in the > meantime. I suspect that we can still read "NULL" on the second read. But > whatever we dereference from the first read should still be valid, even if > the second read would have returned NULL ("rcu freeing"). > On a specific point - if mapping turns out to be NULL after we confirm stable PTE, I'd be inclined to reject and let the slow path take care of it, would you agree that that's the correct approach? I guess you could take that to mean that the page is no longer mapped so is safe, but it feels like refusing it would be the safe course. > -- > Thanks, > > David / dhildenb >