Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20230706225037.1164380-1-axelrasmussen@google.com>
 <20230706225037.1164380-5-axelrasmussen@google.com> <ZKgVISe0vkRKVZuG@x1n>
In-Reply-To: <ZKgVISe0vkRKVZuG@x1n>
From:   Axel Rasmussen <axelrasmussen@google.com>
Date:   Fri, 7 Jul 2023 12:56:04 -0700
Message-ID: <CAJHvVci8ZEdH2Nqft3bp6_PpFgqL2rqJDyPh_pVPx8oZAWfyqA@mail.gmail.com>
Subject: Re: [PATCH v3 4/8] mm: userfaultfd: add new UFFDIO_POISON ioctl
To:     Peter Xu <peterx@redhat.com>
Cc:     Alexander Viro <viro@zeniv.linux.org.uk>,
        Andrew Morton <akpm@linux-foundation.org>,
        Brian Geffon <bgeffon@google.com>,
        Christian Brauner <brauner@kernel.org>,
        David Hildenbrand <david@redhat.com>,
        Gaosheng Cui <cuigaosheng1@huawei.com>,
        Huang Ying <ying.huang@intel.com>,
        Hugh Dickins <hughd@google.com>,
        James Houghton <jthoughton@google.com>,
        "Jan Alexander Steffens (heftig)" <heftig@archlinux.org>,
        Jiaqi Yan <jiaqiyan@google.com>,
        Jonathan Corbet <corbet@lwn.net>,
        Kefeng Wang <wangkefeng.wang@huawei.com>,
        "Liam R. Howlett" <Liam.Howlett@oracle.com>,
        Miaohe Lin <linmiaohe@huawei.com>,
        Mike Kravetz <mike.kravetz@oracle.com>,
        "Mike Rapoport (IBM)" <rppt@kernel.org>,
        Muchun Song <muchun.song@linux.dev>,
        Nadav Amit <namit@vmware.com>,
        Naoya Horiguchi <naoya.horiguchi@nec.com>,
        Ryan Roberts <ryan.roberts@arm.com>,
        Shuah Khan <shuah@kernel.org>,
        Suleiman Souhlal <suleiman@google.com>,
        Suren Baghdasaryan <surenb@google.com>,
        "T.J. Alumbaugh" <talumbau@google.com>,
        Yu Zhao <yuzhao@google.com>,
        ZhangPeng <zhangpeng362@huawei.com>, linux-doc@vger.kernel.org,
        linux-kernel@vger.kernel.org, linux-fsdevel@vger.kernel.org,
        linux-mm@kvack.org, linux-kselftest@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Fri, Jul 7, 2023 at 6:37=E2=80=AFAM Peter Xu <peterx@redhat.com> wrote:
>
> On Thu, Jul 06, 2023 at 03:50:32PM -0700, Axel Rasmussen wrote:
> > The basic idea here is to "simulate" memory poisoning for VMs. A VM
> > running on some host might encounter a memory error, after which some
> > page(s) are poisoned (i.e., future accesses SIGBUS). They expect that
> > once poisoned, pages can never become "un-poisoned". So, when we live
> > migrate the VM, we need to preserve the poisoned status of these pages.
> >
> > When live migrating, we try to get the guest running on its new host as
> > quickly as possible. So, we start it running before all memory has been
> > copied, and before we're certain which pages should be poisoned or not.
> >
> > So the basic way to use this new feature is:
> >
> > - On the new host, the guest's memory is registered with userfaultfd, i=
n
> >   either MISSING or MINOR mode (doesn't really matter for this purpose)=
.
> > - On any first access, we get a userfaultfd event. At this point we can
> >   communicate with the old host to find out if the page was poisoned.
> > - If so, we can respond with a UFFDIO_POISON - this places a swap marke=
r
> >   so any future accesses will SIGBUS. Because the pte is now "present",
> >   future accesses won't generate more userfaultfd events, they'll just
> >   SIGBUS directly.
> >
> > UFFDIO_POISON does not handle unmapping previously-present PTEs. This
> > isn't needed, because during live migration we want to intercept
> > all accesses with userfaultfd (not just writes, so WP mode isn't useful
> > for this). So whether minor or missing mode is being used (or both), th=
e
> > PTE won't be present in any case, so handling that case isn't needed.
> >
> > Similarly, UFFDIO_POISON won't replace existing PTE markers. This might
> > be okay to do, but it seems to be safer to just refuse to overwrite any
> > existing entry (like a UFFD_WP PTE marker).
> >
> > Signed-off-by: Axel Rasmussen <axelrasmussen@google.com>
>
> I agree the current behavior is not as clear, especially after hwpoison
> introduced.
>
> uffdio-copy is special right now that it can overwrite a marker, so a bug=
gy
> userapp can also overwrite a poisoned entry, but it also means the userap=
p
> is broken already, so may not really matter much.
>
> While zeropage wasn't doing that. I think that was just overlooked - i
> assume it has the same reasoning as uffdio-copy otherwise.. and no one ju=
st
> used zeropage over a wp marker yet, or just got it work-arounded by
> unprotect+zeropage.
>
> Not yet sure whether it'll make sense to unify this a bit, but making the
> new poison api to be strict look fine.  If you have any thoughts after
> reading feel free to keep the discussion going, I can ack this one I thin=
k
> (besides my rename request in 1st patch):

Agreed, it would be nice to unify things. In my v2 I had anon/shmem
and hugetlbfs behaving differently in this respect, for the same
reason - it was just overlooked / cargo culted from existing code. If
nothing else I think a single ioctl should be consistent across memory
types! Heh.

But I also think you're right and it's not exactly intentional that
copy / zeropage / etc are different in this respect. Some unification
would be nice, although I'm not 100% sure what that looks like
concretely.

My rule of thumb is, in cases where we can't imagine a real use case,
it's better to be too strict rather than too loose. And in the future,
it's less disruptive to loosen restrictions rather than tighten them
(potentially breaking something which used to work).

I'll leave untangling this to some future series, though.

>
> Acked-by: Peter Xu <peterx@redhat.com>
>
> --
> Peter Xu
>