Received-SPF: pass (google.com: domain of linux-kernel+bounces-85093-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Precedence: bulk
MIME-Version: 1.0
References: <20240227232100.478238-1-pbonzini@redhat.com> <20240227232100.478238-18-pbonzini@redhat.com>
 <Zd6W-aLnovAI1FL3@google.com> <CAJD7tkapC6es9qjaOf=SmE9XYUdbh_fAperjSe9hy=_iqdB0wQ@mail.gmail.com>
 <Zd8x3w2mwyAufKvm@casper.infradead.org>
In-Reply-To: <Zd8x3w2mwyAufKvm@casper.infradead.org>
From: Paolo Bonzini <pbonzini@redhat.com>
Date: Wed, 28 Feb 2024 14:28:45 +0100
Message-ID: <CABgObfZ9LFDrtLkMaT5LVwy0Z2QMk6SqJ104+D=w7o9i0gEu+g@mail.gmail.com>
Subject: Re: [PATCH 17/21] filemap: add FGP_CREAT_ONLY
To: Matthew Wilcox <willy@infradead.org>
Cc: Yosry Ahmed <yosryahmed@google.com>, Sean Christopherson <seanjc@google.com>, 
	linux-kernel@vger.kernel.org, kvm@vger.kernel.org, michael.roth@amd.com, 
	isaku.yamahata@intel.com, thomas.lendacky@amd.com
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Wed, Feb 28, 2024 at 2:15=E2=80=AFPM 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=E2=80=AFPM Sean Christopherson <seanjc@goo=
gle.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_spa=
ce *mapping,
> > > >   * * %FGP_CREAT - If no folio is present then a new folio is alloc=
ated,
> > > >   *   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, unloc=
k 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) /* to=
p 6 bits */
> > > >
> > > >  #define FGP_WRITEBEGIN               (FGP_LOCK | FGP_WRITE | FGP_C=
REAT | 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 add=
ress_space *mapping, pgoff_t index,
> > > >               folio =3D 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
> > > >
> > > >
> > >
>