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 BD634C54E58 for ; Tue, 26 Mar 2024 06:31:29 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 390216B009D; Tue, 26 Mar 2024 02:31:29 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 340646B009E; Tue, 26 Mar 2024 02:31:29 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 208016B009F; Tue, 26 Mar 2024 02:31:29 -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 104266B009D for ; Tue, 26 Mar 2024 02:31:29 -0400 (EDT) Received: from smtpin19.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay07.hostedemail.com (Postfix) with ESMTP id A849D16023C for ; Tue, 26 Mar 2024 06:31:28 +0000 (UTC) X-FDA: 81938218656.19.BC4442F Received: from sin.source.kernel.org (sin.source.kernel.org [145.40.73.55]) by imf24.hostedemail.com (Postfix) with ESMTP id 5E2FA18001B for ; Tue, 26 Mar 2024 06:31:25 +0000 (UTC) Authentication-Results: imf24.hostedemail.com; dkim=pass header.d=kernel.org header.s=k20201202 header.b=FSqitmzF; dmarc=pass (policy=none) header.from=kernel.org; spf=pass (imf24.hostedemail.com: domain of rppt@kernel.org designates 145.40.73.55 as permitted sender) smtp.mailfrom=rppt@kernel.org ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1711434687; 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=M9Jk/Wmx08Ad9gvmWPfhbuJuUVsjs/96beZ/lKp9nD8=; b=eXcVv9SEUK15XQPXLObzd9jQbJTpGy8LG+URBnYXjP/lt49VrW1UF4P1x42u9orMx0pIvZ RAjn5hdo0C/HicmNVzIYiM/cV7/4nld3T6mlK59+BdxiohbnQRXl/XSjwMgDdCq6wOI8sO paQaqAYyN6Sj38195HSzDbAE+89fuSw= ARC-Authentication-Results: i=1; imf24.hostedemail.com; dkim=pass header.d=kernel.org header.s=k20201202 header.b=FSqitmzF; dmarc=pass (policy=none) header.from=kernel.org; spf=pass (imf24.hostedemail.com: domain of rppt@kernel.org designates 145.40.73.55 as permitted sender) smtp.mailfrom=rppt@kernel.org ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1711434687; a=rsa-sha256; cv=none; b=LEZB8C5m5rFpm64U+PVo+2C8FRiWtM4Ta6a6w4HbVeDkxJFCIStDOlJW6sKt6UPPeI6EiC iyG2CN/jrZUXHx7YfHQdYLJakEUBqzwp5tKFIO7ywP/VTJlMWumK7kmd5icXII1MK/hRzD RauddKqm4XsupydU8qQ9vQb1MHtcdzY= Received: from smtp.kernel.org (transwarp.subspace.kernel.org [100.75.92.58]) by sin.source.kernel.org (Postfix) with ESMTP id C7388CE1E92; Tue, 26 Mar 2024 06:31:20 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id 2AEEFC433F1; Tue, 26 Mar 2024 06:31:16 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1711434680; bh=Z0yWsONmAAgRCY638TgVLjVUTE7Gba29bNLs77UQsLQ=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=FSqitmzFslaeDwwoWIC4/MrvGLjqnpsa4F/DVJ5MT8KX1CBnP3vAz0rMTuPh4J/Ec puqyiC/IudgRECfqNV97wT/8io829HobIHVG4IBKqTHop2+WkyDa2PKDeSKoPZy0cM sZbb4fVc7Yiy/TeZ1JHD062pxa6YQsLVFZky7MNUF6hrl02QOLqEFJNN4+nxIDitza HVHiDxJh1/1A9i+R0cX3j2tXscvxJSoFLoXHCv1R/vqN5m452ko2JMznd1sVEp1n2I qZxYJsD8KIOm1xDqMXTP9WOhEld4Dd43589r8sfUh0ZZ4zWTDTl7UgLDp9fv7zX1ES chtsrSOeMA+ag== Date: Tue, 26 Mar 2024 08:30:37 +0200 From: Mike Rapoport To: David Hildenbrand Cc: linux-kernel@vger.kernel.org, linux-mm@kvack.org, Andrew Morton , Miklos Szeredi , Lorenzo Stoakes , xingwei lee , yue sun Subject: Re: [PATCH v1 3/3] mm: merge folio_is_secretmem() into folio_fast_pin_allowed() Message-ID: References: <20240325134114.257544-1-david@redhat.com> <20240325134114.257544-4-david@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20240325134114.257544-4-david@redhat.com> X-Rspamd-Queue-Id: 5E2FA18001B X-Rspam-User: X-Rspamd-Server: rspam05 X-Stat-Signature: mk6ibpisxa33cmygkfw163orwir467u6 X-HE-Tag: 1711434685-904301 X-HE-Meta: U2FsdGVkX1+Xi4P+ZnZ1TY0nWAkbUAokqtZRlDRaptOuigDgOqJnkXArhk+9VIwQTTGR75tpzGXSMLD8ARy5xRZoTgcqXOJpeZrqwxeo04J8SDVuSnfhrO+ZTPWnI9O85bnqszPqTrE69Q1z43NuDkEAa10kM8WyIRVMV+W6uIrBGHhTtsCDjG0SwSm5eWeF6zUZSSDS+qMrUTYocO2iEUbPJaduMwx23ZjCl/wTuMVhLO6Q9ew/coIDzuzR0H+7rQHH46WwrB/puUt8DjcGJpAbo/T2pdjDBuf2SiwlX+hIyyHqxVqfnizMfnrtxmVGsrd/dcnbd8LhVslIBp3lfojsjG7Y6t25V2Zc1gcqLdpjw4TEhsSs1dycHaqWIUcnNB01piFYlurRkjAn3ndg+LB2lfenlnZZpmkpzpOiJmmzwR1ASD4lQVM9OqkOZ/baDceuYjZys7JwWmXru6JELdK6kZTSCbKSpO8B7fJYGFCDGr8k51DbdfvmQwZowQk1YV5Uugh+PvMSqnUeAcbG7H6ZdBzh/8k6P5vFPSp61UQiOj3ODUSaDdNBWb+0fHrNVGDW/JCwUOkSzJlhjTsi9FCv5RKBbY4I5z99eSV58d0SdIX2e4lnASdyjgtfVfLieoI67I9SgmxkxZR6gXE36mLwJZc8sgSH90CmV3iEm0nDIQj4TdbyvgBgdkB6lJAxytq9wLKGy6WOl+ep6o2KQuCDMX1zDFYOF0cqhYYyX8Qokp7ISlRon9o5LUj4lpUrXpHw2fG93lq9yeRDZhatoZdCrxOu7Q8qU7lhkxMXbOHISEkWGyDE6ZzQOZib8Q9xFLb/uv0kbL/L4s4enQ5mjLG6EN3j6mMZfAcWlipvpdg7PeIg6xvLwAmOtDGRq1bTKKOYwMi3sh6SVJsdjBFPNV2KBCbsRY4L0WoR9e6p1vGCGFIrB1lq4ProY54GpfrOI+D2gcSXK+OOcLwhSQC dGX9jppv N1x/H4ZUPrAkZW8ywqmYnZcNO64E9IqCsyYp8NocuRjne+HD+2SRjoEmx8t5up3nec4PYqHtp6BD0DCG+Gm1WEizX39cl1GIETEQ87U858TGUOiQxgfA/43I8r4PlEGX1BjFJpS0Q+QmLEOQvNEqAfwflKunSG8EiPUtZZROWgSEic4VibjnWod1VB9Bprg9tfLjKBxcXnDobobjTMOqmUvJcSF7wiATDrfzMHbFjCYPtm94spt+mP5/Qcx06Gy6c7OITz0kAXFMlaMViu/ksl3/2LgzcOH4mPa0ky8PFMXIeS6jhk/pnt4sNGxGTifU7Z+hh 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: List-Subscribe: List-Unsubscribe: On Mon, Mar 25, 2024 at 02:41:14PM +0100, David Hildenbrand wrote: > folio_is_secretmem() is currently only used during GUP-fast, and using > it in wrong context where concurrent truncation might happen, could be > problematic. > > Nowadays, folio_fast_pin_allowed() performs similar checks during > GUP-fast and contains a lot of careful handling -- READ_ONCE( -- ), sanity > checks -- lockdep_assert_irqs_disabled() -- and helpful comments on how > this handling is safe and correct. > > So let's merge folio_is_secretmem() into folio_fast_pin_allowed(), still > avoiding checking the actual mapping only if really required. > > Signed-off-by: David Hildenbrand Reviewed-by: Mike Rapoport (IBM) A few comments below, no strong feelings about them. > --- > include/linux/secretmem.h | 21 ++------------------- > mm/gup.c | 33 +++++++++++++++++++++------------ > 2 files changed, 23 insertions(+), 31 deletions(-) > > diff --git a/include/linux/secretmem.h b/include/linux/secretmem.h > index 6996f1f53f14..e918f96881f5 100644 > --- a/include/linux/secretmem.h > +++ b/include/linux/secretmem.h > @@ -6,25 +6,8 @@ > > extern const struct address_space_operations secretmem_aops; > > -static inline bool folio_is_secretmem(struct folio *folio) > +static inline bool secretmem_mapping(struct address_space *mapping) > { > - struct address_space *mapping; > - > - /* > - * Using folio_mapping() is quite slow because of the actual call > - * instruction. > - * We know that secretmem pages are not compound and LRU so we can > - * save a couple of cycles here. > - */ > - if (folio_test_large(folio) || folio_test_lru(folio)) > - return false; > - > - mapping = (struct address_space *) > - ((unsigned long)folio->mapping & ~PAGE_MAPPING_FLAGS); > - > - if (!mapping || mapping != folio->mapping) > - return false; > - > return mapping->a_ops == &secretmem_aops; > } > > @@ -38,7 +21,7 @@ static inline bool vma_is_secretmem(struct vm_area_struct *vma) > return false; > } > > -static inline bool folio_is_secretmem(struct folio *folio) > +static inline bool secretmem_mapping(struct address_space *mapping) > { > return false; > } > diff --git a/mm/gup.c b/mm/gup.c > index e7510b6ce765..69d8bc8e4451 100644 > --- a/mm/gup.c > +++ b/mm/gup.c > @@ -2472,6 +2472,8 @@ EXPORT_SYMBOL(get_user_pages_unlocked); > * This call assumes the caller has pinned the folio, that the lowest page table > * level still points to this folio, and that interrupts have been disabled. > * > + * GUP-fast must reject all secretmem folios. > + * > * Writing to pinned file-backed dirty tracked folios is inherently problematic > * (see comment describing the writable_file_mapping_allowed() function). We > * therefore try to avoid the most egregious case of a long-term mapping doing > @@ -2484,22 +2486,32 @@ EXPORT_SYMBOL(get_user_pages_unlocked); > static bool folio_fast_pin_allowed(struct folio *folio, unsigned int flags) Now when this function checks for gup in general, maybe it's worth to rename it to, say, folio_fast_gup_allowed. > { > struct address_space *mapping; > + bool check_secretmem = false; > + bool reject_file_backed = false; > unsigned long mapping_flags; > > /* > * If we aren't pinning then no problematic write can occur. A long term > * pin is the most egregious case so this is the one we disallow. > */ > - if ((flags & (FOLL_PIN | FOLL_LONGTERM | FOLL_WRITE)) != > + if ((flags & (FOLL_PIN | FOLL_LONGTERM | FOLL_WRITE)) == > (FOLL_PIN | FOLL_LONGTERM | FOLL_WRITE)) > - return true; > + reject_file_backed = true; > + > + /* We hold a folio reference, so we can safely access folio fields. */ > > - /* The folio is pinned, so we can safely access folio fields. */ > + /* secretmem folios are only order-0 folios and never LRU folios. */ Nit: ^ always > + if (IS_ENABLED(CONFIG_SECRETMEM) && !folio_test_large(folio) && > + !folio_test_lru(folio)) > + check_secretmem = true; > + > + if (!reject_file_backed && !check_secretmem) > + return true; > > if (WARN_ON_ONCE(folio_test_slab(folio))) > return false; > > - /* hugetlb mappings do not require dirty-tracking. */ > + /* hugetlb neither requires dirty-tracking nor can be secretmem. */ > if (folio_test_hugetlb(folio)) > return true; > > @@ -2535,10 +2547,12 @@ static bool folio_fast_pin_allowed(struct folio *folio, unsigned int flags) > > /* > * At this point, we know the mapping is non-null and points to an > - * address_space object. The only remaining whitelisted file system is > - * shmem. > + * address_space object. > */ > - return shmem_mapping(mapping); > + if (check_secretmem && secretmem_mapping(mapping)) > + return false; > + /* The only remaining allowed file system is shmem. */ > + return !reject_file_backed || shmem_mapping(mapping); > } > > static void __maybe_unused undo_dev_pagemap(int *nr, int nr_start, > @@ -2624,11 +2638,6 @@ static int gup_pte_range(pmd_t pmd, pmd_t *pmdp, unsigned long addr, > if (!folio) > goto pte_unmap; > > - if (unlikely(folio_is_secretmem(folio))) { > - gup_put_folio(folio, 1, flags); > - goto pte_unmap; > - } > - > if (unlikely(pmd_val(pmd) != pmd_val(*pmdp)) || > unlikely(pte_val(pte) != pte_val(ptep_get(ptep)))) { > gup_put_folio(folio, 1, flags); > -- > 2.43.2 > -- Sincerely yours, Mike.