linux-kernel - Re: [PATCH v5 1/2] KVM: guest_memfd: add generic population via write

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <CAGtprH8PJa-hV6DsfUKevZQs=73CUf8ow9i2-sdxbj555De9Aw@mail.gmail.com>
Date: Mon, 15 Sep 2025 18:08:39 -0700
From: Vishal Annapurve <vannapurve@...gle.com>
To: kalyazin@...zon.com
Cc: James Houghton <jthoughton@...gle.com>, "Kalyazin, Nikita" <kalyazin@...zon.co.uk>, 
	"pbonzini@...hat.com" <pbonzini@...hat.com>, "shuah@...nel.org" <shuah@...nel.org>, 
	"kvm@...r.kernel.org" <kvm@...r.kernel.org>, 
	"linux-kselftest@...r.kernel.org" <linux-kselftest@...r.kernel.org>, 
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>, "michael.day@....com" <michael.day@....com>, 
	"david@...hat.com" <david@...hat.com>, "Roy, Patrick" <roypat@...zon.co.uk>, 
	"Thomson, Jack" <jackabt@...zon.co.uk>, "Manwaring, Derek" <derekmn@...zon.com>, 
	"Cali, Marco" <xmarcalx@...zon.co.uk>
Subject: Re: [PATCH v5 1/2] KVM: guest_memfd: add generic population via write

On Mon, Sep 15, 2025 at 4:01 AM Nikita Kalyazin <kalyazin@...zon.com> wrote:
>
> On 13/09/2025 01:32, Vishal Annapurve wrote:
> > On Fri, Sep 12, 2025 at 3:35 PM James Houghton <jthoughton@...gle.com> wrote:
> >>
> >>>>> +
> >>>>> +       if (folio_test_uptodate(folio)) {
> >>>>> +               folio_unlock(folio);
> >>>>> +               folio_put(folio);
> >>>>> +               return -ENOSPC;
> >>>>
> >>>> Does it actually matter for the folio not to be uptodate? It seems
> >>>> unnecessarily restrictive not to be able to overwrite data if we're
> >>>> saying that this is only usable for unencrypted memory anyway.
> >>>
> >>> In the context of direct map removal [1] it does actually because when
> >>> we mark a folio as prepared, we remove it from the direct map making it
> >>> inaccessible to the way write() performs the copy.  It does not matter
> >>> if direct map removal isn't enabled though.  Do you think it should be
> >>> conditional?
> >>
> >> Oh, good point. It's simpler (both to implement and to describe) to
> >> disallow a second write() call in all cases (no matter if the direct
> >> map for the page has been removed or if the contents have been
> >> encrypted), so I'm all for leaving it unconditional like you have now.
> >> Thanks!
> >
> > Are we deviating from the way read/write semantics work for the other
> > filesystems? I don't think other filesystems carry this restriction of
> > one-time-write only. Do we strictly need the differing semantics?
>
> Yes, I believe we are deviating from other "regular" filesystems, but I
> don't think what we propose is too uncommon as in "special" filesystems
> such as sysfs subsequent calls to attributes like "remove" will likely
> fail as well (not due to up-to-date flag though).
>
> > Maybe it would be simpler to not overload uptodate flag and just not
> > allow read/write if folio is not mapped in the direct map for non-conf
> > VMs (assuming there could be other ways to deduce that information).
>
> The only such interface I'm aware of is kernel_page_present() so the
> check may look like:
>
>         for (i = 0; i < folio_nr_pages(folio); i++) {
>                 struct page *page = folio_page(folio, i);
>                 if (!kernel_page_present(page)) {
>                         folio_unlock(folio);
>                         folio_put(folio);
>                         return -ENOSPC;
>                 }
>         }
>
> However, kernel_page_present() is not currently exported to modules.

I think it should be exposed if there is no cleaner way to deduce if a
folio is mapped in the direct map. That being said, we should probably
cleanly separate the series to add write population support and the
series for removal from direct map [1] and figure out the order in
which they need to be merged upstream.  I would still vote for not
overloading folio_test_uptodate() in either series.

Ackerley and Fuad are planning to post a series just for supporting
in-place conversion for 4K pages which is going to introduce a maple
tree for storing private/shared-ness of ranges. We could possibly
augment the support to track directmap removal there. RFC version [2]
is a good reference for now.

[1] https://lore.kernel.org/kvm/20250912091708.17502-1-roypat@amazon.co.uk/
[2] https://lore.kernel.org/kvm/d3832fd95a03aad562705872cbda5b3d248ca321.1747264138.git.ackerleytng@google.com/#t