| 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
| ||
|
Message-ID: <ZKWXGnSKcOdnaeJw@x1n>
Date: Wed, 5 Jul 2023 12:15:22 -0400
From: Peter Xu <peterx@...hat.com>
To: James Houghton <jthoughton@...gle.com>
Cc: Axel Rasmussen <axelrasmussen@...gle.com>,
Alexander Viro <viro@...iv.linux.org.uk>,
Andrew Morton <akpm@...ux-foundation.org>,
Christian Brauner <brauner@...nel.org>,
David Hildenbrand <david@...hat.com>,
Huang Ying <ying.huang@...el.com>,
Hugh Dickins <hughd@...gle.com>,
Jiaqi Yan <jiaqiyan@...gle.com>,
Jonathan Corbet <corbet@....net>,
"Liam R. Howlett" <Liam.Howlett@...cle.com>,
Miaohe Lin <linmiaohe@...wei.com>,
Mike Kravetz <mike.kravetz@...cle.com>,
"Mike Rapoport (IBM)" <rppt@...nel.org>,
Muchun Song <muchun.song@...ux.dev>,
Nadav Amit <namit@...are.com>,
Naoya Horiguchi <naoya.horiguchi@....com>,
Shuah Khan <shuah@...nel.org>,
ZhangPeng <zhangpeng362@...wei.com>, linux-doc@...r.kernel.org,
linux-kernel@...r.kernel.org, linux-fsdevel@...r.kernel.org,
linux-mm@...ck.org, linux-kselftest@...r.kernel.org
Subject: Re: [PATCH v2 1/6] mm: userfaultfd: add new UFFDIO_POISON ioctl
On Wed, Jul 05, 2023 at 09:09:19AM -0700, James Houghton wrote:
> > > diff --git a/include/linux/swapops.h b/include/linux/swapops.h
> > > index 4c932cb45e0b..8259fee32421 100644
> > > --- a/include/linux/swapops.h
> > > +++ b/include/linux/swapops.h
> > > @@ -394,7 +394,8 @@ typedef unsigned long pte_marker;
> > >
> > > #define PTE_MARKER_UFFD_WP BIT(0)
> > > #define PTE_MARKER_SWAPIN_ERROR BIT(1)
> > > -#define PTE_MARKER_MASK (BIT(2) - 1)
> > > +#define PTE_MARKER_UFFD_POISON BIT(2)
> >
> > One more tab.
> >
> > Though I remembered the last time we discussed IIRC we plan to rename
> > SWAPIN_ERROR and reuse it, could you explain why a new bit is still needed?
> >
> > I think I commented this but I'll do it again: IIUC any existing host
> > swapin errors for guest pages should be reported as MCE too, afaict,
> > happened in kvm context.
>
> I think swapin errors are treated differently than poison. Swapin
> errors get VM_FAULT_SIGBUS, and poison gets VM_FAULT_HWPOISON, so
> UFFDIO_POISON should also get VM_FAULT_HWPOISON (so that's what Axel
> has implemented). And I think that needs a separate PTE marker.
My question was, should we also make SWAPIN_ERROR return VM_FAULT_HWPOISON
always?
Just to recap from what I already commented above - if a guest page got
error in swapin due to block sector failures, it should be treated as
VM_FAULT_HWPOISON too, IMHO. IOW, I think current SWAPIN_ERROR is wrong
when in kvm context and we should fix it first.
>
> >
> > > +#define PTE_MARKER_MASK (BIT(3) - 1)
> > >
> > > static inline swp_entry_t make_pte_marker_entry(pte_marker marker)
> > > {
> > > diff --git a/include/linux/userfaultfd_k.h b/include/linux/userfaultfd_k.h
> > > index ac7b0c96d351..ac8c6854097c 100644
> > > --- a/include/linux/userfaultfd_k.h
> > > +++ b/include/linux/userfaultfd_k.h
> > > @@ -46,6 +46,7 @@ enum mfill_atomic_mode {
> > > MFILL_ATOMIC_COPY,
> > > MFILL_ATOMIC_ZEROPAGE,
> > > MFILL_ATOMIC_CONTINUE,
> > > + MFILL_ATOMIC_POISON,
> > > NR_MFILL_ATOMIC_MODES,
> > > };
> > >
> > > @@ -83,6 +84,9 @@ extern ssize_t mfill_atomic_zeropage(struct mm_struct *dst_mm,
> > > extern ssize_t mfill_atomic_continue(struct mm_struct *dst_mm, unsigned long dst_start,
> > > unsigned long len, atomic_t *mmap_changing,
> > > uffd_flags_t flags);
> > > +extern ssize_t mfill_atomic_poison(struct mm_struct *dst_mm, unsigned long start,
> > > + unsigned long len, atomic_t *mmap_changing,
> > > + uffd_flags_t flags);
> > > extern int mwriteprotect_range(struct mm_struct *dst_mm,
> > > unsigned long start, unsigned long len,
> > > bool enable_wp, atomic_t *mmap_changing);
> > > diff --git a/include/uapi/linux/userfaultfd.h b/include/uapi/linux/userfaultfd.h
> > > index 66dd4cd277bd..62151706c5a3 100644
> > > --- a/include/uapi/linux/userfaultfd.h
> > > +++ b/include/uapi/linux/userfaultfd.h
> > > @@ -39,7 +39,8 @@
> > > UFFD_FEATURE_MINOR_SHMEM | \
> > > UFFD_FEATURE_EXACT_ADDRESS | \
> > > UFFD_FEATURE_WP_HUGETLBFS_SHMEM | \
> > > - UFFD_FEATURE_WP_UNPOPULATED)
> > > + UFFD_FEATURE_WP_UNPOPULATED | \
> > > + UFFD_FEATURE_POISON)
> > > #define UFFD_API_IOCTLS \
> > > ((__u64)1 << _UFFDIO_REGISTER | \
> > > (__u64)1 << _UFFDIO_UNREGISTER | \
> > > @@ -49,12 +50,14 @@
> > > (__u64)1 << _UFFDIO_COPY | \
> > > (__u64)1 << _UFFDIO_ZEROPAGE | \
> > > (__u64)1 << _UFFDIO_WRITEPROTECT | \
> > > - (__u64)1 << _UFFDIO_CONTINUE)
> > > + (__u64)1 << _UFFDIO_CONTINUE | \
> > > + (__u64)1 << _UFFDIO_POISON)
> > > #define UFFD_API_RANGE_IOCTLS_BASIC \
> > > ((__u64)1 << _UFFDIO_WAKE | \
> > > (__u64)1 << _UFFDIO_COPY | \
> > > + (__u64)1 << _UFFDIO_WRITEPROTECT | \
> > > (__u64)1 << _UFFDIO_CONTINUE | \
> > > - (__u64)1 << _UFFDIO_WRITEPROTECT)
> > > + (__u64)1 << _UFFDIO_POISON)
> >
> > May not be a large deal, but it's still better to declare the feature &
> > ioctls after all things implemented. Maybe make these few lines
> > (UFFD_API*, and the new feature bit) as the last patch to enable the
> > feature?
>
> I agree. Another option would be to have a separate feature for
> UFFDIO_POISON for hugetlb, but I don't think we should do that. :)
Yeah let's make the features "memory-type-free" if possible. :)
Thanks,
--
Peter Xu
Powered by blists - more mailing lists