Re: [PATCH v4 05/14] guest_memfd: Use folio_has_hwpoisoned_page()

Re: [PATCH v4 05/14] guest_memfd: Use folio_has_hwpoisoned_page()

[Matthew Wilcox]

Sashiko review:
> --- Patch [5]: [PATCH v4 05/14] guest_memfd: Use folio_has_hwpoisoned_page() ---
> commit b4db4098463261fdf87a2e92b28ce2b4c9445e74
> Author: Matthew Wilcox (Oracle) <willy@infradead.org>
> guest_memfd: Use folio_has_hwpoisoned_page()
> 
> This commit changes guest_memfd to use folio_has_hwpoisoned_page() instead of
> folio_test_hwpoison() to correctly detect poisoned pages on all large folios,
> not just hugetlb folios.
> 
> > diff --git a/virt/kvm/guest_memfd.c b/virt/kvm/guest_memfd.c
> > index db57c5766ab60..94eedcc7f1a9e 100644
> > --- a/virt/kvm/guest_memfd.c
> > +++ b/virt/kvm/guest_memfd.c
> > @@ -774,7 +774,7 @@ static struct folio *__kvm_gmem_get_pfn(struct file *file,
> >  	if (IS_ERR(folio))
> >  		return folio;
> >
> > -	if (folio_test_hwpoison(folio)) {
> > +	if (folio_has_hwpoisoned_page(folio)) {
> >  		folio_unlock(folio);
> >  		folio_put(folio);
> >  		return ERR_PTR(-EHWPOISON);
> 
> This is a pre-existing issue, but while looking at hwpoison
> checks in guest_memfd.c, I noticed a missing check in
> kvm_gmem_fault_user_mapping() that might trigger a host kernel MCE panic.
> 
> If a guest_memfd folio is allocated (e.g. via fallocate) but not zeroed, it
> remains in the page cache without the uptodate flag. If a memory failure
> occurs on this page, it is marked hwpoisoned.
> 
> Later, a fault could trigger kvm_gmem_fault_user_mapping() which executes:
> 
>     folio = kvm_gmem_get_folio(inode, vmf->pgoff);
>     ...
>     if (!folio_test_uptodate(folio)) {
>         clear_highpage(folio_page(folio, 0));
>         folio_mark_uptodate(folio);
>     }
> 
> Because there is no folio_has_hwpoisoned_page() check before clearing the
> page, could the kernel-mode write in clear_highpage() to the uninitialized,
> poisoned page trigger an unrecoverable MCE panic?
> 
> Should kvm_gmem_fault_user_mapping() include the same HWPoison check as
> __kvm_gmem_get_pfn() and return VM_FAULT_HWPOISON if the page is poisoned?

I have no idea, but the purpose of this patch is to stop kvm from using
the wrong API.  This is definitely scope creep beyond my level of interest
and the aims of this patch series.  I've added the KVM people to the cc
here, but I'm not going to do anything more with this review.

Re: [PATCH v4 05/14] guest_memfd: Use folio_has_hwpoisoned_page()

[Sean Christopherson]

On Fri, Jul 24, 2026, Matthew Wilcox wrote:
> Sashiko review:
> > --- Patch [5]: [PATCH v4 05/14] guest_memfd: Use folio_has_hwpoisoned_page() ---
> > commit b4db4098463261fdf87a2e92b28ce2b4c9445e74
> > Author: Matthew Wilcox (Oracle) <willy@infradead.org>
> > guest_memfd: Use folio_has_hwpoisoned_page()
> > 
> > This commit changes guest_memfd to use folio_has_hwpoisoned_page() instead of
> > folio_test_hwpoison() to correctly detect poisoned pages on all large folios,
> > not just hugetlb folios.
> > 
> > > diff --git a/virt/kvm/guest_memfd.c b/virt/kvm/guest_memfd.c
> > > index db57c5766ab60..94eedcc7f1a9e 100644
> > > --- a/virt/kvm/guest_memfd.c
> > > +++ b/virt/kvm/guest_memfd.c
> > > @@ -774,7 +774,7 @@ static struct folio *__kvm_gmem_get_pfn(struct file *file,
> > >  	if (IS_ERR(folio))
> > >  		return folio;
> > >
> > > -	if (folio_test_hwpoison(folio)) {
> > > +	if (folio_has_hwpoisoned_page(folio)) {
> > >  		folio_unlock(folio);
> > >  		folio_put(folio);
> > >  		return ERR_PTR(-EHWPOISON);
> > 
> > This is a pre-existing issue, but while looking at hwpoison
> > checks in guest_memfd.c, I noticed a missing check in
> > kvm_gmem_fault_user_mapping() that might trigger a host kernel MCE panic.
> > 
> > If a guest_memfd folio is allocated (e.g. via fallocate) but not zeroed, it
> > remains in the page cache without the uptodate flag. If a memory failure
> > occurs on this page, it is marked hwpoisoned.
> > 
> > Later, a fault could trigger kvm_gmem_fault_user_mapping() which executes:
> > 
> >     folio = kvm_gmem_get_folio(inode, vmf->pgoff);
> >     ...
> >     if (!folio_test_uptodate(folio)) {
> >         clear_highpage(folio_page(folio, 0));
> >         folio_mark_uptodate(folio);
> >     }
> > 
> > Because there is no folio_has_hwpoisoned_page() check before clearing the
> > page, could the kernel-mode write in clear_highpage() to the uninitialized,
> > poisoned page trigger an unrecoverable MCE panic?
> > 
> > Should kvm_gmem_fault_user_mapping() include the same HWPoison check as
> > __kvm_gmem_get_pfn() and return VM_FAULT_HWPOISON if the page is poisoned?
> 
> I have no idea, but the purpose of this patch is to stop kvm from using
> the wrong API.  This is definitely scope creep beyond my level of interest
> and the aims of this patch series.  I've added the KVM people to the cc
> here, but I'm not going to do anything more with this review.

Heh, you don't want to sign up for a week or two of onion peeling?

In the future, please Cc at least kvm@ on any guest_memfd changes, both to avoid
unexpected conflicts (the code is under heavy development), and because us KVM
people are still learning how the hell do filesystems stuff, and can use all the
help and knowledge sharing we can get.

Thanks!

Re: [PATCH v4 05/14] guest_memfd: Use folio_has_hwpoisoned_page()

[Ackerley Tng]

Sean Christopherson <seanjc@google.com> writes:

> On Fri, Jul 24, 2026, Matthew Wilcox wrote:
>> Sashiko review:
>> > --- Patch [5]: [PATCH v4 05/14] guest_memfd: Use folio_has_hwpoisoned_page() ---
>> > commit b4db4098463261fdf87a2e92b28ce2b4c9445e74
>> > Author: Matthew Wilcox (Oracle) <willy@infradead.org>
>> > guest_memfd: Use folio_has_hwpoisoned_page()
>> >
>> > This commit changes guest_memfd to use folio_has_hwpoisoned_page() instead of
>> > folio_test_hwpoison() to correctly detect poisoned pages on all large folios,
>> > not just hugetlb folios.
>> >
>> > > diff --git a/virt/kvm/guest_memfd.c b/virt/kvm/guest_memfd.c
>> > > index db57c5766ab60..94eedcc7f1a9e 100644
>> > > --- a/virt/kvm/guest_memfd.c
>> > > +++ b/virt/kvm/guest_memfd.c
>> > > @@ -774,7 +774,7 @@ static struct folio *__kvm_gmem_get_pfn(struct file *file,
>> > >  	if (IS_ERR(folio))
>> > >  		return folio;
>> > >
>> > > -	if (folio_test_hwpoison(folio)) {
>> > > +	if (folio_has_hwpoisoned_page(folio)) {
>> > >  		folio_unlock(folio);
>> > >  		folio_put(folio);
>> > >  		return ERR_PTR(-EHWPOISON);
>> >
>> > This is a pre-existing issue, but while looking at hwpoison
>> > checks in guest_memfd.c, I noticed a missing check in
>> > kvm_gmem_fault_user_mapping() that might trigger a host kernel MCE panic.
>> >
>> > If a guest_memfd folio is allocated (e.g. via fallocate) but not zeroed, it
>> > remains in the page cache without the uptodate flag. If a memory failure
>> > occurs on this page, it is marked hwpoisoned.
>> >
>> > Later, a fault could trigger kvm_gmem_fault_user_mapping() which executes:
>> >
>> >     folio = kvm_gmem_get_folio(inode, vmf->pgoff);
>> >     ...
>> >     if (!folio_test_uptodate(folio)) {
>> >         clear_highpage(folio_page(folio, 0));
>> >         folio_mark_uptodate(folio);
>> >     }
>> >
>> > Because there is no folio_has_hwpoisoned_page() check before clearing the
>> > page, could the kernel-mode write in clear_highpage() to the uninitialized,
>> > poisoned page trigger an unrecoverable MCE panic?
>> >
>> > Should kvm_gmem_fault_user_mapping() include the same HWPoison check as
>> > __kvm_gmem_get_pfn() and return VM_FAULT_HWPOISON if the page is poisoned?
>>
>> I have no idea, but the purpose of this patch is to stop kvm from using
>> the wrong API.  This is definitely scope creep beyond my level of interest
>> and the aims of this patch series.  I've added the KVM people to the cc
>> here, but I'm not going to do anything more with this review.
>
> Heh, you don't want to sign up for a week or two of onion peeling?
>

Looks like Sashiko is right, we should fix this separately.

>
> [...snip...]
>

Re: [PATCH v4 05/14] guest_memfd: Use folio_has_hwpoisoned_page()

[Matthew Wilcox]

On Fri, Jul 24, 2026 at 08:16:25AM -0700, Sean Christopherson wrote:
> Heh, you don't want to sign up for a week or two of onion peeling?

Oh, you have no idea!

Here's my onion so far ...

1. I want to simplify filemap_fault()
2. That requires changing all the architecture fault handlers
3. So let's push some of that down to the MM with a new wrapper around
   handle_mm_fault()
4. This would be easier if we could hoist the creation of struct
   vm_fault from __handle_mm_fault() to handle_mm_fault()
5. Which would be possible if Jane's series to fix hugetlb use of pgoff
   had landed
6. Oh, hang on, handling hwpoison on hugetlb is actually broken in a
   few different ways (some pointed out by Sashiko, some I noticed
   myself)

I honestly thought I was just going to rearrange Jane's patch series
a bit and resubmit it, but fixing the hugetlb hwpoison handling has
consumed me for the past couple of weeks.

Re: [PATCH v4 05/14] guest_memfd: Use folio_has_hwpoisoned_page()

[Michael S. Tsirkin]

On Fri, Jul 24, 2026 at 07:04:52PM +0100, Matthew Wilcox wrote:
> On Fri, Jul 24, 2026 at 08:16:25AM -0700, Sean Christopherson wrote:
> > Heh, you don't want to sign up for a week or two of onion peeling?
> 
> Oh, you have no idea!
> 
> Here's my onion so far ...
> 
> 1. I want to simplify filemap_fault()
> 2. That requires changing all the architecture fault handlers
> 3. So let's push some of that down to the MM with a new wrapper around
>    handle_mm_fault()
> 4. This would be easier if we could hoist the creation of struct
>    vm_fault from __handle_mm_fault() to handle_mm_fault()
> 5. Which would be possible if Jane's series to fix hugetlb use of pgoff
>    had landed
> 6. Oh, hang on, handling hwpoison on hugetlb is actually broken in a
>    few different ways (some pointed out by Sashiko, some I noticed
>    myself)
> 
> I honestly thought I was just going to rearrange Jane's patch series
> a bit and resubmit it, but fixing the hugetlb hwpoison handling has
> consumed me for the past couple of weeks.


Last time I tried to fix hwpoison, i wasted untold hours on it, only
to decide that it was always racy and does not seem to be worth fixing.
At least yours isn't a race.

-- 
MST