Message ID | 20240227232100.478238-18-pbonzini@redhat.com |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel+bounces-84201-ouuuleilei=gmail.com@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a05:7300:a81b:b0:108:e6aa:91d0 with SMTP id bq27csp3038911dyb; Tue, 27 Feb 2024 16:03:19 -0800 (PST) X-Forwarded-Encrypted: i=3; AJvYcCVNnZKbIAYjNz5f+cTyuuCO6NiFmUY+2xAt1oLyENQlVJe/9ILSouSE76elE5SoIi+D0O5s2Gh/9jeS/5+tXkG/wu1gMw== X-Google-Smtp-Source: AGHT+IGSCC+AlnRFqY/Gd74MRE+OXOl64AKVDR8P+1q8PaCHf1fDLtSZIF1dozwiUSgP2vs6O2mg X-Received: by 2002:a05:6a20:9d91:b0:1a1:c1d:9032 with SMTP id mu17-20020a056a209d9100b001a10c1d9032mr4050032pzb.48.1709078599506; Tue, 27 Feb 2024 16:03:19 -0800 (PST) ARC-Seal: i=2; a=rsa-sha256; t=1709078599; cv=pass; d=google.com; s=arc-20160816; b=xWysTfkAWHtTCP18o65wyRPeb74ckzCq0bYKE+l+q1IrhaTA2P2peUOYpGQ6PnkzMc Sp2E6FIoV9dUoSZzkx4zF97wE8dt8qm4ti+KPYhiLB+ozz0rtI9L1QpTZ1+g4aXOqc1u Dexp3wZvDmv9i1Y0NzEcQgYBXWvflkvLiRPgpbduD73MNubyGqrF/kWi4K15dywGFm8I 5F+PeQ8Rsw0J1P9jHVay6zEb3deX0gOSWeroW07Prdyc5zY48D7XE44dO6pT+ebjBK4i Era4KCRIBvGCPxvAmtKZT3i0LeNqcowrH3b2HtNZEq7K6W2/twgDf/lMuzEhqq57uYP4 eTUA== ARC-Message-Signature: i=2; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=content-transfer-encoding:mime-version:list-unsubscribe :list-subscribe:list-id:precedence:references:in-reply-to:message-id :date:subject:cc:to:from:dkim-signature; bh=BG4ZL+4+5uPAuYv+ozkJ6gsYKFZe75rDBIaL5MNApAs=; fh=miuXyzfw5j8qsOURKCmYrSiXirUlKyDkR37pPJ+5+/s=; b=v31V2REKUr62zfUHElFPIqydrZT7FIqMVPCh7vZicW/jyvi3ZaPqCBIpET3qkQMgZg q3Ngbf6w9283f3WGt72YniYVojXueXRxAriX54wOqzQcL4vYXD8qzukwxt47U5rQmXxN 3Ky4J/weECOCZ1rQGYyYMIVL+47712cHkE1RHqL0JSoG5mabd9ZpgV1ClF6RKtsIPDNj oLx6dAgTGCybcZlggazN5Bu9vZVMOpPt4c5/Cq8c4cfFwz5q/ZRd2gjIfp2jT35bVMpT G5+AGwzCxm3T+Py/BSxD1S9Gxly2jETVseqgbHZEa39JSx6b7uS/Gay0arneDrdYny79 NiYQ==; dara=google.com ARC-Authentication-Results: i=2; mx.google.com; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=S+inPmar; arc=pass (i=1 spf=pass spfdomain=redhat.com dkim=pass dkdomain=redhat.com dmarc=pass fromdomain=redhat.com); spf=pass (google.com: domain of linux-kernel+bounces-84201-ouuuleilei=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) smtp.mailfrom="linux-kernel+bounces-84201-ouuuleilei=gmail.com@vger.kernel.org"; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=redhat.com Received: from sy.mirrors.kernel.org (sy.mirrors.kernel.org. [2604:1380:40f1:3f00::1]) by mx.google.com with ESMTPS id y21-20020a63e255000000b005dc8f60cdf9si6219205pgj.302.2024.02.27.16.03.19 for <ouuuleilei@gmail.com> (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 27 Feb 2024 16:03:19 -0800 (PST) Received-SPF: pass (google.com: domain of linux-kernel+bounces-84201-ouuuleilei=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1; Authentication-Results: mx.google.com; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=S+inPmar; arc=pass (i=1 spf=pass spfdomain=redhat.com dkim=pass dkdomain=redhat.com dmarc=pass fromdomain=redhat.com); spf=pass (google.com: domain of linux-kernel+bounces-84201-ouuuleilei=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) smtp.mailfrom="linux-kernel+bounces-84201-ouuuleilei=gmail.com@vger.kernel.org"; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=redhat.com Received: from smtp.subspace.kernel.org (wormhole.subspace.kernel.org [52.25.139.140]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by sy.mirrors.kernel.org (Postfix) with ESMTPS id 2E2F6B30FDC for <ouuuleilei@gmail.com>; Tue, 27 Feb 2024 23:28:18 +0000 (UTC) Received: from localhost.localdomain (localhost.localdomain [127.0.0.1]) by smtp.subspace.kernel.org (Postfix) with ESMTP id 9C59055E4F; Tue, 27 Feb 2024 23:21:18 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; dkim=pass (1024-bit key) header.d=redhat.com header.i=@redhat.com header.b="S+inPmar" Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id C90E557867 for <linux-kernel@vger.kernel.org>; Tue, 27 Feb 2024 23:21:08 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=170.10.133.124 ARC-Seal: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1709076071; cv=none; b=e+PHs/ahm53CVkd2rmF0j3z+Ohry8bLXI2U5BGf6rJfJ6EQde1kQof3oZt6mnigjGAFKcpeEXdSzfBHU1+qKAj7/ih4F8plggTPvzmmaZHtbWqQTTbbRZln36Oz3fLRG7NP78z9SL/476pOyCbLSagJOMeXO2QJCVWhe1DQ28Uo= ARC-Message-Signature: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1709076071; c=relaxed/simple; bh=KY7Vtiin2PBxkQCqTTa5Z5E0FoYbvkUJVSGQWBSa3os=; h=From:To:Cc:Subject:Date:Message-Id:In-Reply-To:References: MIME-Version:Content-Type; b=hyRTGYxRQpZXjLNHv/SuL4YUJVTA3VGUJAX1oxowqDdRWpxkekEn9aeWXOi3xbVAxJZAfVmUzf+LbEscQT6aqRwfR8bv+kE5txQGBXkQa25T7r7abO2jbr1cL6ShkR8sAwfrMbl/SgyFrScGAg7mNmetdbR1xEQF295KGaLTGvM= ARC-Authentication-Results: i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=redhat.com; spf=pass smtp.mailfrom=redhat.com; dkim=pass (1024-bit key) header.d=redhat.com header.i=@redhat.com header.b=S+inPmar; arc=none smtp.client-ip=170.10.133.124 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=redhat.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=redhat.com DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1709076067; 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=BG4ZL+4+5uPAuYv+ozkJ6gsYKFZe75rDBIaL5MNApAs=; b=S+inPmar82UGzsy/pzFbf2sT8rDtf7brfY0qcCjrsumUAqDkYa3+zZZ3nqxM2HTBMmwRgu l4jfoVb1hLd/jHvTRz1cmtGm/yGzhqBVdVEJ/kSzCyse/aXzkW1VDdgi9e6BWAzgs6p5qI j3pw3+drMoJamioyaDvwJHl7z5cWvmU= Received: from mimecast-mx02.redhat.com (mx-ext.redhat.com [66.187.233.73]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-90-mamPH8I0Pzi2AWxQuplGCw-1; Tue, 27 Feb 2024 18:21:05 -0500 X-MC-Unique: mamPH8I0Pzi2AWxQuplGCw-1 Received: from smtp.corp.redhat.com (int-mx08.intmail.prod.int.rdu2.redhat.com [10.11.54.8]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id CD66D1E441C3; Tue, 27 Feb 2024 23:21:04 +0000 (UTC) Received: from virtlab511.virt.lab.eng.bos.redhat.com (virtlab511.virt.lab.eng.bos.redhat.com [10.19.152.198]) by smtp.corp.redhat.com (Postfix) with ESMTP id A1441C0348F; Tue, 27 Feb 2024 23:21:04 +0000 (UTC) From: Paolo Bonzini <pbonzini@redhat.com> To: linux-kernel@vger.kernel.org, kvm@vger.kernel.org Cc: seanjc@google.com, michael.roth@amd.com, isaku.yamahata@intel.com, thomas.lendacky@amd.com Subject: [PATCH 17/21] filemap: add FGP_CREAT_ONLY Date: Tue, 27 Feb 2024 18:20:56 -0500 Message-Id: <20240227232100.478238-18-pbonzini@redhat.com> In-Reply-To: <20240227232100.478238-1-pbonzini@redhat.com> References: <20240227232100.478238-1-pbonzini@redhat.com> Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: <linux-kernel.vger.kernel.org> List-Subscribe: <mailto:linux-kernel+subscribe@vger.kernel.org> List-Unsubscribe: <mailto:linux-kernel+unsubscribe@vger.kernel.org> MIME-Version: 1.0 Content-Type: text/plain Content-Transfer-Encoding: 8bit X-Scanned-By: MIMEDefang 3.4.1 on 10.11.54.8 X-getmail-retrieved-from-mailbox: INBOX X-GMAIL-THRID: 1792098801709714174 X-GMAIL-MSGID: 1792098801709714174 |
Series |
TDX/SNP part 1 of n, for 6.9
|
|
Commit Message
Paolo Bonzini
Feb. 27, 2024, 11:20 p.m. UTC
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
include/linux/pagemap.h | 2 ++
mm/filemap.c | 4 ++++
2 files changed, 6 insertions(+)
Comments
On Tue, Feb 27, 2024, Paolo Bonzini wrote: This needs a changelog, and also needs to be Cc'd to someone(s) that can give it a thumbs up. > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> > --- > include/linux/pagemap.h | 2 ++ > mm/filemap.c | 4 ++++ > 2 files changed, 6 insertions(+) > > diff --git a/include/linux/pagemap.h b/include/linux/pagemap.h > index 2df35e65557d..e8ac0b32f84d 100644 > --- a/include/linux/pagemap.h > +++ b/include/linux/pagemap.h > @@ -586,6 +586,7 @@ pgoff_t page_cache_prev_miss(struct address_space *mapping, > * * %FGP_CREAT - If no folio is present then a new folio is allocated, > * added to the page cache and the VM's LRU list. The folio is > * returned locked. > + * * %FGP_CREAT_ONLY - Fail if a folio is not present > * * %FGP_FOR_MMAP - The caller wants to do its own locking dance if the > * folio is already in cache. If the folio was allocated, unlock it > * before returning so the caller can do the same dance. > @@ -606,6 +607,7 @@ typedef unsigned int __bitwise fgf_t; > #define FGP_NOWAIT ((__force fgf_t)0x00000020) > #define FGP_FOR_MMAP ((__force fgf_t)0x00000040) > #define FGP_STABLE ((__force fgf_t)0x00000080) > +#define FGP_CREAT_ONLY ((__force fgf_t)0x00000100) > #define FGF_GET_ORDER(fgf) (((__force unsigned)fgf) >> 26) /* top 6 bits */ > > #define FGP_WRITEBEGIN (FGP_LOCK | FGP_WRITE | FGP_CREAT | FGP_STABLE) > diff --git a/mm/filemap.c b/mm/filemap.c > index 750e779c23db..d5107bd0cd09 100644 > --- a/mm/filemap.c > +++ b/mm/filemap.c > @@ -1854,6 +1854,10 @@ struct folio *__filemap_get_folio(struct address_space *mapping, pgoff_t index, > folio = NULL; > if (!folio) > goto no_page; > + if (fgp_flags & FGP_CREAT_ONLY) { > + folio_put(folio); > + return ERR_PTR(-EEXIST); > + } > > if (fgp_flags & FGP_LOCK) { > if (fgp_flags & FGP_NOWAIT) { > -- > 2.39.0 > >
On Tue, Feb 27, 2024 at 6:15 PM Sean Christopherson <seanjc@google.com> wrote: > > On Tue, Feb 27, 2024, Paolo Bonzini wrote: > > This needs a changelog, and also needs to be Cc'd to someone(s) that can give it > a thumbs up. +Matthew Wilcox > > > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> > > --- > > include/linux/pagemap.h | 2 ++ > > mm/filemap.c | 4 ++++ > > 2 files changed, 6 insertions(+) > > > > diff --git a/include/linux/pagemap.h b/include/linux/pagemap.h > > index 2df35e65557d..e8ac0b32f84d 100644 > > --- a/include/linux/pagemap.h > > +++ b/include/linux/pagemap.h > > @@ -586,6 +586,7 @@ pgoff_t page_cache_prev_miss(struct address_space *mapping, > > * * %FGP_CREAT - If no folio is present then a new folio is allocated, > > * added to the page cache and the VM's LRU list. The folio is > > * returned locked. > > + * * %FGP_CREAT_ONLY - Fail if a folio is not present > > * * %FGP_FOR_MMAP - The caller wants to do its own locking dance if the > > * folio is already in cache. If the folio was allocated, unlock it > > * before returning so the caller can do the same dance. > > @@ -606,6 +607,7 @@ typedef unsigned int __bitwise fgf_t; > > #define FGP_NOWAIT ((__force fgf_t)0x00000020) > > #define FGP_FOR_MMAP ((__force fgf_t)0x00000040) > > #define FGP_STABLE ((__force fgf_t)0x00000080) > > +#define FGP_CREAT_ONLY ((__force fgf_t)0x00000100) > > #define FGF_GET_ORDER(fgf) (((__force unsigned)fgf) >> 26) /* top 6 bits */ > > > > #define FGP_WRITEBEGIN (FGP_LOCK | FGP_WRITE | FGP_CREAT | FGP_STABLE) > > diff --git a/mm/filemap.c b/mm/filemap.c > > index 750e779c23db..d5107bd0cd09 100644 > > --- a/mm/filemap.c > > +++ b/mm/filemap.c > > @@ -1854,6 +1854,10 @@ struct folio *__filemap_get_folio(struct address_space *mapping, pgoff_t index, > > folio = NULL; > > if (!folio) > > goto no_page; > > + if (fgp_flags & FGP_CREAT_ONLY) { > > + folio_put(folio); > > + return ERR_PTR(-EEXIST); > > + } > > > > if (fgp_flags & FGP_LOCK) { > > if (fgp_flags & FGP_NOWAIT) { > > -- > > 2.39.0 > > > > >
On Tue, Feb 27, 2024 at 06:17:34PM -0800, Yosry Ahmed wrote: > On Tue, Feb 27, 2024 at 6:15 PM Sean Christopherson <seanjc@google.com> wrote: > > > > On Tue, Feb 27, 2024, Paolo Bonzini wrote: > > > > This needs a changelog, and also needs to be Cc'd to someone(s) that can give it > > a thumbs up. > > +Matthew Wilcox If only there were an entry in MAINTAINERS for filemap.c ... This looks bogus to me, and if it's not bogus, it's incomplete. But it's hard to judge without a commit message that describes what it's supposed to mean. > > > > > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> > > > --- > > > include/linux/pagemap.h | 2 ++ > > > mm/filemap.c | 4 ++++ > > > 2 files changed, 6 insertions(+) > > > > > > diff --git a/include/linux/pagemap.h b/include/linux/pagemap.h > > > index 2df35e65557d..e8ac0b32f84d 100644 > > > --- a/include/linux/pagemap.h > > > +++ b/include/linux/pagemap.h > > > @@ -586,6 +586,7 @@ pgoff_t page_cache_prev_miss(struct address_space *mapping, > > > * * %FGP_CREAT - If no folio is present then a new folio is allocated, > > > * added to the page cache and the VM's LRU list. The folio is > > > * returned locked. > > > + * * %FGP_CREAT_ONLY - Fail if a folio is not present > > > * * %FGP_FOR_MMAP - The caller wants to do its own locking dance if the > > > * folio is already in cache. If the folio was allocated, unlock it > > > * before returning so the caller can do the same dance. > > > @@ -606,6 +607,7 @@ typedef unsigned int __bitwise fgf_t; > > > #define FGP_NOWAIT ((__force fgf_t)0x00000020) > > > #define FGP_FOR_MMAP ((__force fgf_t)0x00000040) > > > #define FGP_STABLE ((__force fgf_t)0x00000080) > > > +#define FGP_CREAT_ONLY ((__force fgf_t)0x00000100) > > > #define FGF_GET_ORDER(fgf) (((__force unsigned)fgf) >> 26) /* top 6 bits */ > > > > > > #define FGP_WRITEBEGIN (FGP_LOCK | FGP_WRITE | FGP_CREAT | FGP_STABLE) > > > diff --git a/mm/filemap.c b/mm/filemap.c > > > index 750e779c23db..d5107bd0cd09 100644 > > > --- a/mm/filemap.c > > > +++ b/mm/filemap.c > > > @@ -1854,6 +1854,10 @@ struct folio *__filemap_get_folio(struct address_space *mapping, pgoff_t index, > > > folio = NULL; > > > if (!folio) > > > goto no_page; > > > + if (fgp_flags & FGP_CREAT_ONLY) { > > > + folio_put(folio); > > > + return ERR_PTR(-EEXIST); > > > + } > > > > > > if (fgp_flags & FGP_LOCK) { > > > if (fgp_flags & FGP_NOWAIT) { > > > -- > > > 2.39.0 > > > > > > > >
On Wed, Feb 28, 2024 at 2:15 PM Matthew Wilcox <willy@infradead.org> wrote: > > On Tue, Feb 27, 2024 at 06:17:34PM -0800, Yosry Ahmed wrote: > > On Tue, Feb 27, 2024 at 6:15 PM Sean Christopherson <seanjc@google.com> wrote: > > > > > > On Tue, Feb 27, 2024, Paolo Bonzini wrote: > > > > > > This needs a changelog, and also needs to be Cc'd to someone(s) that can give it > > > a thumbs up. > > > > +Matthew Wilcox > > If only there were an entry in MAINTAINERS for filemap.c ... Not CCing you (or mm in general) was intentional because I first wanted a review of the KVM APIs; of course I wouldn't have committed it without an Acked-by. But yeah, not writing the changelog yet was pure laziness. Since you're here: KVM would like to add a ioctl to encrypt and install a page into guest_memfd, in preparation for launching an encrypted guest. For this API we want to rule out the possibility of overwriting a page that is already in the guest_memfd's filemap, therefore this API would pass FGP_CREAT_ONLY|FGP_CREAT into__filemap_get_folio. Do you think this is bogus... > This looks bogus to me, and if it's not bogus, it's incomplete. .. or if not, what incompleteness can you spot? Thanks, Paolo > But it's hard to judge without a commit message that describes what it's > supposed to mean. > > > > > > > > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> > > > > --- > > > > include/linux/pagemap.h | 2 ++ > > > > mm/filemap.c | 4 ++++ > > > > 2 files changed, 6 insertions(+) > > > > > > > > diff --git a/include/linux/pagemap.h b/include/linux/pagemap.h > > > > index 2df35e65557d..e8ac0b32f84d 100644 > > > > --- a/include/linux/pagemap.h > > > > +++ b/include/linux/pagemap.h > > > > @@ -586,6 +586,7 @@ pgoff_t page_cache_prev_miss(struct address_space *mapping, > > > > * * %FGP_CREAT - If no folio is present then a new folio is allocated, > > > > * added to the page cache and the VM's LRU list. The folio is > > > > * returned locked. > > > > + * * %FGP_CREAT_ONLY - Fail if a folio is not present > > > > * * %FGP_FOR_MMAP - The caller wants to do its own locking dance if the > > > > * folio is already in cache. If the folio was allocated, unlock it > > > > * before returning so the caller can do the same dance. > > > > @@ -606,6 +607,7 @@ typedef unsigned int __bitwise fgf_t; > > > > #define FGP_NOWAIT ((__force fgf_t)0x00000020) > > > > #define FGP_FOR_MMAP ((__force fgf_t)0x00000040) > > > > #define FGP_STABLE ((__force fgf_t)0x00000080) > > > > +#define FGP_CREAT_ONLY ((__force fgf_t)0x00000100) > > > > #define FGF_GET_ORDER(fgf) (((__force unsigned)fgf) >> 26) /* top 6 bits */ > > > > > > > > #define FGP_WRITEBEGIN (FGP_LOCK | FGP_WRITE | FGP_CREAT | FGP_STABLE) > > > > diff --git a/mm/filemap.c b/mm/filemap.c > > > > index 750e779c23db..d5107bd0cd09 100644 > > > > --- a/mm/filemap.c > > > > +++ b/mm/filemap.c > > > > @@ -1854,6 +1854,10 @@ struct folio *__filemap_get_folio(struct address_space *mapping, pgoff_t index, > > > > folio = NULL; > > > > if (!folio) > > > > goto no_page; > > > > + if (fgp_flags & FGP_CREAT_ONLY) { > > > > + folio_put(folio); > > > > + return ERR_PTR(-EEXIST); > > > > + } > > > > > > > > if (fgp_flags & FGP_LOCK) { > > > > if (fgp_flags & FGP_NOWAIT) { > > > > -- > > > > 2.39.0 > > > > > > > > > > > >
On Wed, Feb 28, 2024 at 02:28:45PM +0100, Paolo Bonzini wrote: > Since you're here: KVM would like to add a ioctl to encrypt and > install a page into guest_memfd, in preparation for launching an > encrypted guest. For this API we want to rule out the possibility of > overwriting a page that is already in the guest_memfd's filemap, > therefore this API would pass FGP_CREAT_ONLY|FGP_CREAT > into__filemap_get_folio. Do you think this is bogus... Would it work to start out by either asserting the memfd is empty of pages, or by evicting any existing pages? Both those seem nicer than starting, realising you've got some unencrypted memory and aborting. > > This looks bogus to me, and if it's not bogus, it's incomplete. > > ... or if not, what incompleteness can you spot? The part where we race another caller passing FGP_CREAT_ONLY and one gets an EEXIST back from filemap_add_folio(). Maybe that's not something that can happen in your use case, but it's at least semantics that need documenting.
On Wed, Feb 28, 2024 at 8:24 PM Matthew Wilcox <willy@infradead.org> wrote: > > On Wed, Feb 28, 2024 at 02:28:45PM +0100, Paolo Bonzini wrote: > > Since you're here: KVM would like to add a ioctl to encrypt and > > install a page into guest_memfd, in preparation for launching an > > encrypted guest. For this API we want to rule out the possibility of > > overwriting a page that is already in the guest_memfd's filemap, > > therefore this API would pass FGP_CREAT_ONLY|FGP_CREAT > > into__filemap_get_folio. Do you think this is bogus... > > Would it work to start out by either asserting the memfd is empty of > pages, or by evicting any existing pages? Both those seem nicer than > starting, realising you've got some unencrypted memory and aborting. Unfortunately it would be quite ugly to force userspace to do all the initialization in one go. For example, there are different kinds of pages that probably would be initialized at different points (e.g. before vs. after vCPUs are created, because the initial vCPU state is also encrypted). The thing that I want to protect against is userspace trying to initialize the same encrypted page twice. > > > This looks bogus to me, and if it's not bogus, it's incomplete. > > > > ... or if not, what incompleteness can you spot? > > The part where we race another caller passing FGP_CREAT_ONLY and one gets > an EEXIST back from filemap_add_folio(). Maybe that's not something > that can happen in your use case, but it's at least semantics that > need documenting. From the point of view of filemap_add_folio(), one of the racers wins and one fails. It doesn't matter to filemap.c if the missing synchronization is in the kernel or in userspace. In the case of KVM, the ioctl will return the number of pages before it found an existing page, or -EEXIST if that number is zero (similar to what nonblocking read does with EAGAIN). I'll improve the documentation and changelog and make sure to Cc you on the next version. Thanks again! Paolo
On Wed, Feb 28, 2024 at 02:28:45PM +0100, Paolo Bonzini wrote: > On Wed, Feb 28, 2024 at 2:15 PM Matthew Wilcox <willy@infradead.org> wrote: > > > > On Tue, Feb 27, 2024 at 06:17:34PM -0800, Yosry Ahmed wrote: > > > On Tue, Feb 27, 2024 at 6:15 PM Sean Christopherson <seanjc@google.com> wrote: > > > > > > > > On Tue, Feb 27, 2024, Paolo Bonzini wrote: > > > > > > > > This needs a changelog, and also needs to be Cc'd to someone(s) that can give it > > > > a thumbs up. > > > > > > +Matthew Wilcox > > > > If only there were an entry in MAINTAINERS for filemap.c ... > > Not CCing you (or mm in general) was intentional because I first > wanted a review of the KVM APIs; of course I wouldn't have committed > it without an Acked-by. But yeah, not writing the changelog yet was > pure laziness. > > Since you're here: KVM would like to add a ioctl to encrypt and > install a page into guest_memfd, in preparation for launching an > encrypted guest. For this API we want to rule out the possibility of > overwriting a page that is already in the guest_memfd's filemap, > therefore this API would pass FGP_CREAT_ONLY|FGP_CREAT > into__filemap_get_folio. Do you think this is bogus... > > > This looks bogus to me, and if it's not bogus, it's incomplete. > > ... or if not, what incompleteness can you spot? > > Thanks, > > Paolo > > > But it's hard to judge without a commit message that describes what it's > > supposed to mean. > > > > > > > > > > > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> > > > > > --- > > > > > include/linux/pagemap.h | 2 ++ > > > > > mm/filemap.c | 4 ++++ > > > > > 2 files changed, 6 insertions(+) > > > > > > > > > > diff --git a/include/linux/pagemap.h b/include/linux/pagemap.h > > > > > index 2df35e65557d..e8ac0b32f84d 100644 > > > > > --- a/include/linux/pagemap.h > > > > > +++ b/include/linux/pagemap.h > > > > > @@ -586,6 +586,7 @@ pgoff_t page_cache_prev_miss(struct address_space *mapping, > > > > > * * %FGP_CREAT - If no folio is present then a new folio is allocated, > > > > > * added to the page cache and the VM's LRU list. The folio is > > > > > * returned locked. > > > > > + * * %FGP_CREAT_ONLY - Fail if a folio is not present ^ So should be: Fail if a folio is present. Thanks, Yilun > > > > > * * %FGP_FOR_MMAP - The caller wants to do its own locking dance if the > > > > > * folio is already in cache. If the folio was allocated, unlock it > > > > > * before returning so the caller can do the same dance. > > > > > @@ -606,6 +607,7 @@ typedef unsigned int __bitwise fgf_t; > > > > > #define FGP_NOWAIT ((__force fgf_t)0x00000020) > > > > > #define FGP_FOR_MMAP ((__force fgf_t)0x00000040) > > > > > #define FGP_STABLE ((__force fgf_t)0x00000080) > > > > > +#define FGP_CREAT_ONLY ((__force fgf_t)0x00000100) > > > > > #define FGF_GET_ORDER(fgf) (((__force unsigned)fgf) >> 26) /* top 6 bits */ > > > > > > > > > > #define FGP_WRITEBEGIN (FGP_LOCK | FGP_WRITE | FGP_CREAT | FGP_STABLE) > > > > > diff --git a/mm/filemap.c b/mm/filemap.c > > > > > index 750e779c23db..d5107bd0cd09 100644 > > > > > --- a/mm/filemap.c > > > > > +++ b/mm/filemap.c > > > > > @@ -1854,6 +1854,10 @@ struct folio *__filemap_get_folio(struct address_space *mapping, pgoff_t index, > > > > > folio = NULL; > > > > > if (!folio) > > > > > goto no_page; > > > > > + if (fgp_flags & FGP_CREAT_ONLY) { > > > > > + folio_put(folio); > > > > > + return ERR_PTR(-EEXIST); > > > > > + } > > > > > > > > > > if (fgp_flags & FGP_LOCK) { > > > > > if (fgp_flags & FGP_NOWAIT) { > > > > > -- > > > > > 2.39.0 > > > > > > > > > > > > > > > > > >
diff --git a/include/linux/pagemap.h b/include/linux/pagemap.h index 2df35e65557d..e8ac0b32f84d 100644 --- a/include/linux/pagemap.h +++ b/include/linux/pagemap.h @@ -586,6 +586,7 @@ pgoff_t page_cache_prev_miss(struct address_space *mapping, * * %FGP_CREAT - If no folio is present then a new folio is allocated, * added to the page cache and the VM's LRU list. The folio is * returned locked. + * * %FGP_CREAT_ONLY - Fail if a folio is not present * * %FGP_FOR_MMAP - The caller wants to do its own locking dance if the * folio is already in cache. If the folio was allocated, unlock it * before returning so the caller can do the same dance. @@ -606,6 +607,7 @@ typedef unsigned int __bitwise fgf_t; #define FGP_NOWAIT ((__force fgf_t)0x00000020) #define FGP_FOR_MMAP ((__force fgf_t)0x00000040) #define FGP_STABLE ((__force fgf_t)0x00000080) +#define FGP_CREAT_ONLY ((__force fgf_t)0x00000100) #define FGF_GET_ORDER(fgf) (((__force unsigned)fgf) >> 26) /* top 6 bits */ #define FGP_WRITEBEGIN (FGP_LOCK | FGP_WRITE | FGP_CREAT | FGP_STABLE) diff --git a/mm/filemap.c b/mm/filemap.c index 750e779c23db..d5107bd0cd09 100644 --- a/mm/filemap.c +++ b/mm/filemap.c @@ -1854,6 +1854,10 @@ struct folio *__filemap_get_folio(struct address_space *mapping, pgoff_t index, folio = NULL; if (!folio) goto no_page; + if (fgp_flags & FGP_CREAT_ONLY) { + folio_put(folio); + return ERR_PTR(-EEXIST); + } if (fgp_flags & FGP_LOCK) { if (fgp_flags & FGP_NOWAIT) {