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 8937CC77B7C for ; Fri, 5 May 2023 14:17:50 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id C3CD76B0075; Fri, 5 May 2023 10:17:49 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id BC597900002; Fri, 5 May 2023 10:17:49 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id A3E716B007B; Fri, 5 May 2023 10:17:49 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by kanga.kvack.org (Postfix) with ESMTP id 7FD3A6B0075 for ; Fri, 5 May 2023 10:17:49 -0400 (EDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1683296269; h=from:from: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=P1oCIf84RG7+6O7buoOZWskEuD34mndm17BNvxGm8Ys=; b=ZiBya7OLixhraS5aedsvgGgsMf/ZjqmQ/EbwLc/rXcgP9ekTtZJ5H4wHSrYvyJ7Fb4j6lE 9hBo9pAumlSUEOmGMJS+kb5SIUrjPIg2XQwii42zNwLS3waDayv2OH4uYCVQbxRHXLtVa4 IMip0mfpx3CN95lGhzhZCtnB0xQOiiA= Received: from mail-wm1-f72.google.com (mail-wm1-f72.google.com [209.85.128.72]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-470-DIGYB2V0MWa8CatmkghZ-Q-1; Fri, 05 May 2023 10:17:48 -0400 X-MC-Unique: DIGYB2V0MWa8CatmkghZ-Q-1 Received: by mail-wm1-f72.google.com with SMTP id 5b1f17b1804b1-3f32b3835e9so7389485e9.1 for ; Fri, 05 May 2023 07:17:42 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1683296262; x=1685888262; h=content-transfer-encoding:in-reply-to:subject:organization:from :content-language:references:cc:to:user-agent:mime-version:date :message-id:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=P1oCIf84RG7+6O7buoOZWskEuD34mndm17BNvxGm8Ys=; b=Fw2JeF2IY6pAXFJBXKYM4RW/n2pprrrJITr+ZsYf/OzbuuOk+yq54lfoSDbWk9b2bB rfsqVdWOJMh54gd0br/w3jE4XBM6346bzah9NAE3MJsV92hqO+rBlKlBMuSxEgQYXqB7 kGElLzKWaGYbsncpnE4IqbvuI6xQCEHXc8v2Jci1J4O4bu5H6PcWGgTcfYT5GJCV/A+K fxAXZIJ7nVwPweicWPrD+i9BhpDN+YQhx1cR+zd/3L1kirX4N/go5uvgU+snK8vmtlug bXoy6zIwr45bzFHl0QSpkG/yZRMnsiHKsH5qKwqQazrs4tP6phrE8U4GTuZDDduFbLd6 5rRg== X-Gm-Message-State: AC+VfDxI7gpQdXWSTf8UvglDNmpBPurAfGSqFy+ZmfG/CJViHkR/YEYI XKwVRWP7f7n2rPWwuIKPY11Py2SlXQPhFzhJA6YcOgEA7eKWHSTn54nkxmUJOCXV8NNKsKpZYJ9 f1Fjig7MM7wM= X-Received: by 2002:adf:e948:0:b0:2fb:87f7:3812 with SMTP id m8-20020adfe948000000b002fb87f73812mr1224189wrn.1.1683296261849; Fri, 05 May 2023 07:17:41 -0700 (PDT) X-Google-Smtp-Source: ACHHUZ5TcJqqd654Tk6HhTBKZ1iWhCqzdn+unDXZ1fRotGzitSWjcybH0sIfqEWc/QeEZR4qCpa8LQ== X-Received: by 2002:adf:e948:0:b0:2fb:87f7:3812 with SMTP id m8-20020adfe948000000b002fb87f73812mr1224169wrn.1.1683296261426; Fri, 05 May 2023 07:17:41 -0700 (PDT) Received: from ?IPV6:2003:cb:c71f:6900:2b25:fc69:599e:3986? (p200300cbc71f69002b25fc69599e3986.dip0.t-ipconnect.de. [2003:cb:c71f:6900:2b25:fc69:599e:3986]) by smtp.gmail.com with ESMTPSA id h14-20020a5d6e0e000000b0030631dcbea6sm2548971wrz.77.2023.05.05.07.17.38 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Fri, 05 May 2023 07:17:40 -0700 (PDT) Message-ID: Date: Fri, 5 May 2023 16:17:38 +0200 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.10.0 To: Lorenzo Stoakes Cc: 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 , Peter Zijlstra , 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 References: From: David Hildenbrand Organization: Red Hat Subject: Re: [PATCH v8 3/3] mm/gup: disallow FOLL_LONGTERM GUP-fast writing to file-backed mappings In-Reply-To: X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Language: en-US Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit 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: >> And there is nothing wrong about pinning an anon page that's still in the >> swapcache. The following folio_test_anon() check will allow them. >> >> The check made sense in page_mapping(), but here it's not required. > > Waaaaaaaaaait a second, you were saying before:- > > "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 are we allowing or disallowing pinning anon swap cache pages? :P If we have an exclusive anon page that's still in the swap cache, sure! :) I think there are ways that can be done, and nothing would actually break. (I even wrote selftests in the cow selftests for that to amke sure it works as expected) > > I mean slow path would allow them if they are just marked anon so I'm inclined > to allow them. Exactly my reasoning. The less checks the better (especially if ordinary GUP just allows for pinning it) :) > >> >> I do agree regarding folio_test_slab(), though. Should we WARN in case we >> would have one? >> >> if (WARN_ON_ONCE(folio_test_slab(folio))) >> return false; >> > > God help us if we have a slab page at this point, so agreed worth doing, it > would surely have to arise from some dreadful bug/memory corruption. > Or some nasty race condition that we managed to ignore with rechecking if the PTEs/PMDs changed :) >>> + if (unlikely(folio_test_slab(folio) || folio_test_swapcache(folio))) >>> + return false; >>> + >>> + /* hugetlb mappings do not require dirty-tracking. */ >>> + if (folio_test_hugetlb(folio)) >>> + return true; >>> + >>> + /* >>> + * GUP-fast disables IRQs. When IRQS are disabled, RCU grace periods >>> + * cannot proceed, which means no actions performed under RCU can >>> + * proceed either. >>> + * >>> + * inodes and thus their mappings are freed under RCU, which means the >>> + * mapping cannot be freed beneath us and thus we can safely dereference >>> + * it. >>> + */ >>> + lockdep_assert_irqs_disabled(); >>> + >>> + /* >>> + * However, there may be operations which _alter_ the mapping, so ensure >>> + * we read it once and only once. >>> + */ >>> + mapping = READ_ONCE(folio->mapping); >>> + >>> + /* >>> + * The mapping may have been truncated, in any case we cannot determine >>> + * if this mapping is safe - fall back to slow path to determine how to >>> + * proceed. >>> + */ >>> + if (!mapping) >>> + return false; >>> + >>> + /* Anonymous folios are fine, other non-file backed cases are not. */ >>> + mapping_flags = (unsigned long)mapping & PAGE_MAPPING_FLAGS; >>> + if (mapping_flags) >>> + return mapping_flags == PAGE_MAPPING_ANON; >> >> KSM pages are also (shared) anonymous folios, and that check would fail -- >> which is ok (the following unsharing checks rejects long-term pinning them), >> but a bit inconstent with your comment and folio_test_anon(). >> >> It would be more consistent (with your comment and also the folio_test_anon >> implementation) to have here: >> >> return mapping_flags & PAGE_MAPPING_ANON; >> > > I explicitly excluded KSM out of fear that could be some breakage given they're > wrprotect'd + expected to CoW though? But I guess you mean they'd get picked up > by the unshare and so it doesn't matter + we wouldn't want to exclude an > PG_anon_exclusive case? Yes, unsharing handles that in the ordinary GUP and GUP-fast case. And unsharing is neither GUP-fast nor even longterm specific (for anon pages). Reason I'm brining this up is that I think it's best if we let folio_fast_pin_allowed() just check for what's absolutely GUP-fast specific. > > I'll make the change in any case given the unshare check! > > I notice the gup_huge_pgd() doesn't do an unshare but I mean, a PGD-sized huge > page probably isn't going to be CoW'd :P I spotted exactly the same thing and wondered about that (after all I added all that unsharing logic ... so I should know). I'm sure there must be a reason I didn't add it ;) ... probably we should just add it even though it might essentially be dead code for now (at least the cow selftests would try with each and every hugetlb size and eventually reveal the problem on whatever arch ends up using that code ... ). Do you want to send a patch to add unsharing to gup_huge_pgd() as well? -- Thanks, David / dhildenb