[PATCH v5 0/5] mm: Fix MF_DELAYED handling on memory failure

[PATCH v5 0/5] mm: Fix MF_DELAYED handling on memory failure

[Lisa Wang]

Here's a fifth revision to fix MF_DELAYED handling on memory failure.

This patch series addresses an issue in the memory failure handling path
where MF_DELAYED is incorrectly treated as an error. This issue was
discovered while testing memory failure handling for guest_memfd.

The proposed solution involves -
1. Clarifying the definition of MF_DELAYED to mean that memory failure
   handling is only partially completed, and that the metadata for the
   memory that failed (as in struct page/folio) is still referenced.
2. Updating shmem’s handling to align with the clarified definition.
3. Updating how the result of .error_remove_folio() is interpreted.

Changes from v4:
+ Add "fallthrough;" in the memory-failure test, as suggested by David
+ Add Fixes tags in the commit message, as suggested by Miaohe
+ Rebase to the branch "mm-stable"
+ Split the KVM selftests to the future series

Thanks for review from Ackerley, Andrew, David and Miaohe!

+ RFC v4: https://lore.kernel.org/all/20260602-memory-failure-mf-delayed-fix-v4-0-a5bc7db5a9b2@google.com/

Signed-off-by: Lisa Wang <wyihan@google.com>
---
Lisa Wang (5):
      mm: memory_failure: Clarify the MF_DELAYED definition
      mm: memory_failure: Allow truncate_error_folio to return MF_DELAYED
      mm: shmem: Update shmem handler to the MF_DELAYED definition
      mm: memory_failure: Generalize extra_pins handling to all MF_DELAYED cases
      mm: selftests: Add shmem into memory failure test

 mm/memory-failure.c                         |  29 ++++---
 mm/shmem.c                                  |   2 +-
 tools/testing/selftests/mm/memory-failure.c | 118 +++++++++++++++++++++++++++-
 3 files changed, 130 insertions(+), 19 deletions(-)
---
base-commit: 08745c62350126bc31b09548137be87e2866f628
change-id: 20260527-memory-failure-mf-delayed-fix-7d5a8f4a8a8b

Best regards,
-- 
Lisa Wang <wyihan@google.com>

[PATCH v5 1/5] mm: memory_failure: Clarify the MF_DELAYED definition

[Lisa Wang]

This patch clarifies the definition of MF_DELAYED to represent cases
where a folio's removal is initiated but not immediately completed
(e.g., due to remaining metadata references).

Acked-by: David Hildenbrand (Arm) <david@kernel.org>
Acked-by: Miaohe Lin <linmiaohe@huawei.com>
Reviewed-by: Ackerley Tng <ackerleytng@google.com>
Signed-off-by: Lisa Wang <wyihan@google.com>
---
 mm/memory-failure.c | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

diff --git a/mm/memory-failure.c b/mm/memory-failure.c
index aaf14608b30e..e698d0e9711d 100644
--- a/mm/memory-failure.c
+++ b/mm/memory-failure.c
@@ -847,24 +847,25 @@ static int kill_accessing_process(struct task_struct *p, unsigned long pfn,
 }
 
 /*
- * MF_IGNORED - The m-f() handler marks the page as PG_hwpoisoned'ed.
+ * MF_IGNORED - The m-f() handler marks the page as PG_hwpoison'ed.
  * But it could not do more to isolate the page from being accessed again,
  * nor does it kill the process. This is extremely rare and one of the
  * potential causes is that the page state has been changed due to
  * underlying race condition. This is the most severe outcomes.
  *
- * MF_FAILED - The m-f() handler marks the page as PG_hwpoisoned'ed.
+ * MF_FAILED - The m-f() handler marks the page as PG_hwpoison'ed.
  * It should have killed the process, but it can't isolate the page,
  * due to conditions such as extra pin, unmap failure, etc. Accessing
  * the page again may trigger another MCE and the process will be killed
  * by the m-f() handler immediately.
  *
- * MF_DELAYED - The m-f() handler marks the page as PG_hwpoisoned'ed.
- * The page is unmapped, and is removed from the LRU or file mapping.
- * An attempt to access the page again will trigger page fault and the
- * PF handler will kill the process.
+ * MF_DELAYED - The m-f() handler marks the page as PG_hwpoison'ed.
+ * It means the page was unmapped and partially isolated (e.g. removed from
+ * file mapping or the LRU) but full cleanup is deferred (e.g. the metadata
+ * for the memory, as in struct page/folio, is still referenced). Any
+ * further access to the page will result in the process being killed.
  *
- * MF_RECOVERED - The m-f() handler marks the page as PG_hwpoisoned'ed.
+ * MF_RECOVERED - The m-f() handler marks the page as PG_hwpoison'ed.
  * The page has been completely isolated, that is, unmapped, taken out of
  * the buddy system, or hole-punched out of the file mapping.
  */

-- 
2.55.0.766.g2966f0265a-goog

[PATCH v5 2/5] mm: memory_failure: Allow truncate_error_folio to return MF_DELAYED

[Lisa Wang]

The .error_remove_folio a_ops is used by different filesystems to handle
folio truncation upon discovery of a memory failure in the memory
associated with the given folio.

Currently, MF_DELAYED is treated as an error, causing "Failed to punch
page" to be written to the console. MF_DELAYED is then relayed to the
caller of truncate_error_folio() as MF_FAILED. This further causes
memory_failure() to return -EBUSY, which then always causes a SIGBUS.

This is also implies that regardless of whether the thread's memory
corruption kill policy is PR_MCE_KILL_EARLY or PR_MCE_KILL_LATE, a
memory failure with MF_DELAYED will always cause a SIGBUS.

Update truncate_error_folio() to return MF_DELAYED to the caller if the
.error_remove_folio() callback reports MF_DELAYED.

Acked-by: David Hildenbrand (Arm) <david@kernel.org>
Acked-by: Miaohe Lin <linmiaohe@huawei.com>
Reviewed-by: Ackerley Tng <ackerleytng@google.com>
Fixes: 6a46079cf57a ("HWPOISON: The high level memory error handler in the VM v7")
Fixes: a7800aa80ea4 ("KVM: Add KVM_CREATE_GUEST_MEMFD ioctl() for guest-specific backing memory")
Signed-off-by: Lisa Wang <wyihan@google.com>
---
 mm/memory-failure.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/mm/memory-failure.c b/mm/memory-failure.c
index e698d0e9711d..37ea23489b75 100644
--- a/mm/memory-failure.c
+++ b/mm/memory-failure.c
@@ -939,10 +939,12 @@ static int truncate_error_folio(struct folio *folio, unsigned long pfn,
 	if (mapping->a_ops->error_remove_folio) {
 		int err = mapping->a_ops->error_remove_folio(mapping, folio);
 
-		if (err != 0)
+		if (err == MF_DELAYED)
+			ret = err;
+		else if (err != 0)
 			pr_info("%#lx: Failed to punch page: %d\n", pfn, err);
 		else if (!filemap_release_folio(folio, GFP_NOIO))
-			pr_info("%#lx: failed to release buffers\n", pfn);
+			pr_info("%#lx: Failed to release buffers\n", pfn);
 		else
 			ret = MF_RECOVERED;
 	} else {

-- 
2.55.0.766.g2966f0265a-goog

Re: [PATCH v5 2/5] mm: memory_failure: Allow truncate_error_folio to return MF_DELAYED

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] AB-BA deadlock between `folio_lock` and `mapping->invalidate_lock` involving the memory failure handler and guest_memfd file truncation.
--

--- Patch [2]: [PATCH v5 2/5] mm: memory_failure: Allow truncate_error_folio to return MF_DELAYED ---
commit 1e7b00ec8e2bc22a20c74d21aa034f3f7f88aba8
Author: Lisa Wang <wyihan@google.com>
Subject: mm: memory_failure: Allow truncate_error_folio to return MF_DELAYED

This commit modifies truncate_error_folio() to check if the
error_remove_folio callback returns MF_DELAYED. If it does, the
function now propagates MF_DELAYED back to the caller instead of
treating it as an error.

> diff --git a/mm/memory-failure.c b/mm/memory-failure.c
> --- a/mm/memory-failure.c
> +++ b/mm/memory-failure.c
> @@ -939,10 +939,12 @@ static int truncate_error_folio(struct folio *folio, unsigned long pfn,
>  	if (mapping->a_ops->error_remove_folio) {
>  		int err = mapping->a_ops->error_remove_folio(mapping, folio);

[Severity: High]
This is a pre-existing issue, but can this callback trigger an AB-BA deadlock
between the folio lock and the mapping invalidate_lock?

The memory failure path acquires the folio lock before reaching this point:

memory_failure()
  folio_lock(folio);
  ...
  me_pagecache_clean()
    truncate_error_folio()

When the callback is kvm_gmem_error_folio(), it then acquires the
invalidate_lock for read:

virt/kvm/guest_memfd.c:kvm_gmem_error_folio() {
    ...
    filemap_invalidate_lock_shared(mapping);
    ...
}

However, the file truncation path acquires these locks in the reverse order.
It acquires the invalidate_lock for write before taking the folio lock:

virt/kvm/guest_memfd.c:kvm_gmem_punch_hole() {
    ...
    filemap_invalidate_lock(inode->i_mapping);
    kvm_gmem_invalidate_start(inode, start, end);
    truncate_inode_pages_range(inode->i_mapping, offset, ...);
}

Since truncate_inode_pages_range() acquires the folio lock, could a
concurrent fallocate hole punch and a memory failure on the same guest_memfd
file lead to a system-wide deadlock?

> -		if (err != 0)
> +		if (err == MF_DELAYED)
> +			ret = err;
> +		else if (err != 0)
>  			pr_info("%#lx: Failed to punch page: %d\n", pfn, err);
[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260823-memory-failure-mf-delayed-fix-v5-0-d97a5703e45a@google.com?part=2

[PATCH v5 3/5] mm: shmem: Update shmem handler to the MF_DELAYED definition

[Lisa Wang]

To align with the definition of MF_DELAYED, update
shmem_error_remove_folio() to return MF_DELAYED.

shmem handles memory failures but defers the actual file truncation. The
function's return value should therefore be MF_DELAYED to accurately
reflect the state.

Currently, this logical error does not cause a bug, because:

- For shmem folios, folio->private is not set.
- As a result, filemap_release_folio() is a no-op and returns true.
- This, in turn, causes truncate_error_folio() to incorrectly return
  MF_RECOVERED.
- The caller then treats MF_RECOVERED as a success condition, masking the
  issue.

The previous patch relays MF_DELAYED to the caller of
truncate_error_folio() before any logging, so returning MF_DELAYED from
shmem_error_remove_folio() will retain the original behavior of not
adding any logs.

The return value of truncate_error_folio() is consumed in action_result(),
which treats MF_DELAYED the same way as MF_RECOVERED, hence action_result()
also returns the same thing after this change.

Acked-by: David Hildenbrand (Arm) <david@kernel.org>
Acked-by: Miaohe Lin <linmiaohe@huawei.com>
Reviewed-by: Ackerley Tng <ackerleytng@google.com>
Signed-off-by: Lisa Wang <wyihan@google.com>
---
 mm/shmem.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/mm/shmem.c b/mm/shmem.c
index 774f4b18ff5c..d6f94786e709 100644
--- a/mm/shmem.c
+++ b/mm/shmem.c
@@ -5136,7 +5136,7 @@ static void __init shmem_destroy_inodecache(void)
 static int shmem_error_remove_folio(struct address_space *mapping,
 				   struct folio *folio)
 {
-	return 0;
+	return MF_DELAYED;
 }
 
 static const struct address_space_operations shmem_aops = {

-- 
2.55.0.766.g2966f0265a-goog

[PATCH v5 4/5] mm: memory_failure: Generalize extra_pins handling to all MF_DELAYED cases

[Lisa Wang]

Generalize extra_pins handling to all MF_DELAYED cases not only
shmem_mapping.

If MF_DELAYED is returned, the filemap continues to hold refcounts on the
folio. Hence, take that into account when checking for extra refcounts.

As clarified in an earlier patch, a return value of MF_DELAYED implies that
the page still has elevated refcounts. Hence, set extra_pins to true if the
return value is MF_DELAYED. This is aligned with the implementation in
me_swapcache_dirty(), where, if a folio is still in the swap cache, ret is
set to MF_DELAYED and extra_pins is set to true.

Acked-by: David Hildenbrand (Arm) <david@kernel.org>
Acked-by: Miaohe Lin <linmiaohe@huawei.com>
Reviewed-by: Ackerley Tng <ackerleytng@google.com>
Signed-off-by: Lisa Wang <wyihan@google.com>
---
 mm/memory-failure.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/mm/memory-failure.c b/mm/memory-failure.c
index 37ea23489b75..c7a00c7be368 100644
--- a/mm/memory-failure.c
+++ b/mm/memory-failure.c
@@ -1039,18 +1039,14 @@ static int me_pagecache_clean(struct page_state *ps, struct page *p)
 		goto out;
 	}
 
-	/*
-	 * The shmem page is kept in page cache instead of truncating
-	 * so is expected to have an extra refcount after error-handling.
-	 */
-	extra_pins = shmem_mapping(mapping);
-
 	/*
 	 * Truncation is a bit tricky. Enable it per file system for now.
 	 *
 	 * Open: to take i_rwsem or not for this? Right now we don't.
 	 */
 	ret = truncate_error_folio(folio, page_to_pfn(p), mapping);
+
+	extra_pins = ret == MF_DELAYED;
 	if (has_extra_refcount(ps, p, extra_pins))
 		ret = MF_FAILED;
 

-- 
2.55.0.766.g2966f0265a-goog

[PATCH v5 5/5] mm: selftests: Add shmem into memory failure test

[Lisa Wang]

Add a shmem memory failure selftest to test the shmem memory failure is
correct after modifying shmem return value.

Specifically, test the expected behavior under various scenarios
combining page dirtiness (dirty vs clean) and failure types (hard vs
soft):
+ Dirty + Hard: Trigger a SIGBUS on injection, and trigger another
  SIGBUS when reading the page again.
+ Dirty + Soft: No SIGBUS is triggered, and the original value can be
  read successfully.
+ Clean + Hard: No SIGBUS is triggered on injection, but trigger a
  SIGBUS when trying to read the page again.
+ Clean + Soft: No SIGBUS is triggered, and the page can be read
  successfully.

Acked-by: Miaohe Lin <linmiaohe@huawei.com>
Signed-off-by: Lisa Wang <wyihan@google.com>
---
 tools/testing/selftests/mm/memory-failure.c | 118 +++++++++++++++++++++++++++-
 1 file changed, 115 insertions(+), 3 deletions(-)

diff --git a/tools/testing/selftests/mm/memory-failure.c b/tools/testing/selftests/mm/memory-failure.c
index 1a5a32e22cce..5b26f0c44664 100644
--- a/tools/testing/selftests/mm/memory-failure.c
+++ b/tools/testing/selftests/mm/memory-failure.c
@@ -20,6 +20,10 @@
 
 #include "vm_util.h"
 
+#ifndef fallthrough
+# define fallthrough __attribute__((__fallthrough__))
+#endif
+
 enum inject_type {
 	MADV_HARD,
 	MADV_SOFT,
@@ -29,9 +33,14 @@ enum result_type {
 	MADV_HARD_ANON,
 	MADV_HARD_CLEAN_PAGECACHE,
 	MADV_HARD_DIRTY_PAGECACHE,
+	MADV_HARD_CLEAN_SHMEM,
+	MADV_HARD_DIRTY_SHMEM,
 	MADV_SOFT_ANON,
 	MADV_SOFT_CLEAN_PAGECACHE,
 	MADV_SOFT_DIRTY_PAGECACHE,
+	MADV_SOFT_CLEAN_SHMEM,
+	MADV_SOFT_DIRTY_SHMEM,
+	READ_ERROR,
 };
 
 static jmp_buf signal_jmp_buf;
@@ -157,17 +166,22 @@ static void check(struct __test_metadata *_metadata, FIXTURE_DATA(memory_failure
 	case MADV_HARD_CLEAN_PAGECACHE:
 	case MADV_SOFT_CLEAN_PAGECACHE:
 	case MADV_SOFT_DIRTY_PAGECACHE:
-		/* It is not expected to receive a SIGBUS signal. */
-		ASSERT_EQ(setjmp, 0);
-
+	case MADV_SOFT_DIRTY_SHMEM:
 		/* The page content should remain unchanged. */
 		ASSERT_TRUE(check_memory(vaddr, self->page_size));
+		fallthrough;
+	case MADV_HARD_CLEAN_SHMEM:
+	case MADV_SOFT_CLEAN_SHMEM:
+		/* It is not expected to receive a SIGBUS signal. */
+		ASSERT_EQ(setjmp, 0);
 
 		/* The backing pfn of addr should have changed. */
 		ASSERT_NE(pagemap_get_pfn(self->pagemap_fd, vaddr), self->pfn);
 		break;
 	case MADV_HARD_ANON:
 	case MADV_HARD_DIRTY_PAGECACHE:
+	case MADV_HARD_DIRTY_SHMEM:
+	case READ_ERROR:
 		/* The SIGBUS signal should have been received. */
 		ASSERT_EQ(setjmp, 1);
 
@@ -263,6 +277,20 @@ static int prepare_file(const char *fname, unsigned long size)
 	return fd;
 }
 
+static int prepare_shmem(const char *fname, unsigned long size)
+{
+	int fd;
+
+	fd = memfd_create(fname, 0);
+	if (fd < 0)
+		return -1;
+	if (ftruncate(fd, size) < 0) {
+		close(fd);
+		return -1;
+	}
+	return fd;
+}
+
 /* Borrowed from mm/gup_longterm.c. */
 static int get_fs_type(int fd)
 {
@@ -355,4 +383,88 @@ TEST_F(memory_failure, dirty_pagecache)
 	ASSERT_EQ(close(fd), 0);
 }
 
+TEST_F(memory_failure, dirty_shmem)
+{
+	int fd;
+	char *addr;
+	int ret;
+
+	fd = prepare_shmem("shmem-file", self->page_size);
+	if (fd < 0)
+		SKIP(return, "failed to open test shmem-file.\n");
+
+	addr = mmap(0, self->page_size, PROT_READ | PROT_WRITE,
+		    MAP_SHARED, fd, 0);
+	if (addr == MAP_FAILED) {
+		close(fd);
+		SKIP(return, "mmap failed, not enough memory.\n");
+	}
+	memset(addr, 0xce, self->page_size);
+
+	prepare(_metadata, self, addr);
+
+	ret = sigsetjmp(signal_jmp_buf, 1);
+	if (!ret && !self->injection_attempted) {
+		self->injection_attempted = true;
+		ASSERT_EQ(variant->inject(self, addr), 0);
+	}
+
+	if (variant->type == MADV_HARD) {
+		check(_metadata, self, addr, MADV_HARD_DIRTY_SHMEM, ret);
+		ret = sigsetjmp(signal_jmp_buf, 1);
+		if (ret == 0)
+			FORCE_READ(*addr);
+		check(_metadata, self, addr, READ_ERROR, ret);
+	} else {
+		check(_metadata, self, addr, MADV_SOFT_DIRTY_SHMEM, ret);
+	}
+
+	ASSERT_EQ(munmap(addr, self->page_size), 0);
+
+	ASSERT_EQ(close(fd), 0);
+}
+
+TEST_F(memory_failure, clean_shmem)
+{
+	int fd;
+	char *addr;
+	int ret;
+
+	fd = prepare_shmem("shmem-file", self->page_size);
+	if (fd < 0)
+		SKIP(return, "failed to open test shmem-file.\n");
+
+	addr = mmap(0, self->page_size, PROT_READ | PROT_WRITE,
+		    MAP_SHARED, fd, 0);
+	if (addr == MAP_FAILED) {
+		close(fd);
+		SKIP(return, "mmap failed, not enough memory.\n");
+	}
+	FORCE_READ(*addr);
+
+	prepare(_metadata, self, addr);
+
+	ret = sigsetjmp(signal_jmp_buf, 1);
+	if (!ret && !self->injection_attempted) {
+		self->injection_attempted = true;
+		ASSERT_EQ(variant->inject(self, addr), 0);
+	}
+
+	if (variant->type == MADV_HARD) {
+		check(_metadata, self, addr, MADV_HARD_CLEAN_SHMEM, ret);
+		ret = sigsetjmp(signal_jmp_buf, 1);
+		if (ret == 0)
+			FORCE_READ(*addr);
+		check(_metadata, self, addr, READ_ERROR, ret);
+	} else {
+		/* Test the address accessability without check_memory(). */
+		FORCE_READ(*addr);
+		check(_metadata, self, addr, MADV_SOFT_CLEAN_SHMEM, ret);
+	}
+
+	ASSERT_EQ(munmap(addr, self->page_size), 0);
+
+	ASSERT_EQ(close(fd), 0);
+}
+
 TEST_HARNESS_MAIN

-- 
2.55.0.766.g2966f0265a-goog

Re: [PATCH v5 5/5] mm: selftests: Add shmem into memory failure test

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] Infinite loop in memory-failure test on unexpected SIGBUS
--

--- Patch [5]: [PATCH v5 5/5] mm: selftests: Add shmem into memory failure test ---
commit b9a214a17a9d91396ae673c0a673788dd9d73afe
Author: Lisa Wang <wyihan@google.com>

mm: selftests: Add shmem into memory failure test

Add a shmem memory failure selftest to test the shmem memory failure is
correct after modifying shmem return value.

> diff --git a/tools/testing/selftests/mm/memory-failure.c b/tools/testing/selftests/mm/memory-failure.c
> index 1a5a32e22cce2..5b26f0c446644 100644
> --- a/tools/testing/selftests/mm/memory-failure.c
> +++ b/tools/testing/selftests/mm/memory-failure.c
[ ... ]
> @@ -157,17 +166,22 @@ static void check(struct __test_metadata *_metadata, FIXTURE_DATA(memory_failure
>  	case MADV_HARD_CLEAN_PAGECACHE:
>  	case MADV_SOFT_CLEAN_PAGECACHE:
>  	case MADV_SOFT_DIRTY_PAGECACHE:
> -		/* It is not expected to receive a SIGBUS signal. */
> -		ASSERT_EQ(setjmp, 0);
> -
> +	case MADV_SOFT_DIRTY_SHMEM:
>  		/* The page content should remain unchanged. */
>  		ASSERT_TRUE(check_memory(vaddr, self->page_size));

[Severity: Medium]
Will this reordering cause an infinite loop if an unexpected SIGBUS occurs?

Previously, the code checked ASSERT_EQ(setjmp, 0) before calling
check_memory(), allowing the test to fail gracefully.

With this change, check_memory() is called first. If an unexpected SIGBUS
was already received (setjmp == 1), check_memory() will access the address
again, triggering another SIGBUS. This causes the signal handler to siglongjmp
back to the sigsetjmp() site in the caller, which will then re-enter check()
and trigger the same sequence repeatedly.

> +		fallthrough;
> +	case MADV_HARD_CLEAN_SHMEM:
> +	case MADV_SOFT_CLEAN_SHMEM:
> +		/* It is not expected to receive a SIGBUS signal. */
> +		ASSERT_EQ(setjmp, 0);
>  
>  		/* The backing pfn of addr should have changed. */
>  		ASSERT_NE(pagemap_get_pfn(self->pagemap_fd, vaddr), self->pfn);
[ ... ]
> +TEST_F(memory_failure, dirty_shmem)
> +{
[ ... ]
> +	ret = sigsetjmp(signal_jmp_buf, 1);
> +	if (!ret && !self->injection_attempted) {
> +		self->injection_attempted = true;
> +		ASSERT_EQ(variant->inject(self, addr), 0);
> +	}
> +
> +	if (variant->type == MADV_HARD) {
> +		check(_metadata, self, addr, MADV_HARD_DIRTY_SHMEM, ret);
> +		ret = sigsetjmp(signal_jmp_buf, 1);
> +		if (ret == 0)
> +			FORCE_READ(*addr);
> +		check(_metadata, self, addr, READ_ERROR, ret);
> +	} else {
> +		check(_metadata, self, addr, MADV_SOFT_DIRTY_SHMEM, ret);

[Severity: Medium]
If the MADV_SOFT injection unexpectedly triggers a SIGBUS, will this else
block cause an infinite loop?

If a SIGBUS is caught during injection, sigsetjmp() returns 1, meaning
ret == 1. The else block is executed unconditionally, calling check() which now
accesses the memory before asserting setjmp == 0. This will trigger another
SIGBUS and siglongjmp back to the sigsetjmp() above, creating an infinite loop.

> +	}
> +
> +	ASSERT_EQ(munmap(addr, self->page_size), 0);
> +
> +	ASSERT_EQ(close(fd), 0);
> +}
> +
> +TEST_F(memory_failure, clean_shmem)
> +{
[ ... ]
> +	ret = sigsetjmp(signal_jmp_buf, 1);
> +	if (!ret && !self->injection_attempted) {
> +		self->injection_attempted = true;
> +		ASSERT_EQ(variant->inject(self, addr), 0);
> +	}
> +
> +	if (variant->type == MADV_HARD) {
> +		check(_metadata, self, addr, MADV_HARD_CLEAN_SHMEM, ret);
> +		ret = sigsetjmp(signal_jmp_buf, 1);
> +		if (ret == 0)
> +			FORCE_READ(*addr);
> +		check(_metadata, self, addr, READ_ERROR, ret);
> +	} else {
> +		/* Test the address accessability without check_memory(). */
> +		FORCE_READ(*addr);

[Severity: Medium]
Similar to the issue in dirty_shmem, if an unexpected SIGBUS occurs during
injection, ret will be 1, but this else block will unconditionally execute
FORCE_READ(*addr).

Since the address may be poisoned, FORCE_READ() will trigger another SIGBUS,
jumping back to the sigsetjmp() and creating an infinite loop. Should this
block check if ret == 0 before attempting to read the address?

> +		check(_metadata, self, addr, MADV_SOFT_CLEAN_SHMEM, ret);
> +	}
> +
> +	ASSERT_EQ(munmap(addr, self->page_size), 0);
> +
> +	ASSERT_EQ(close(fd), 0);
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260823-memory-failure-mf-delayed-fix-v5-0-d97a5703e45a@google.com?part=5

Re: [PATCH v5 5/5] mm: selftests: Add shmem into memory failure test

[David Hildenbrand (Arm)]

On 8/23/26 21:40, Lisa Wang wrote:
> Add a shmem memory failure selftest to test the shmem memory failure is
> correct after modifying shmem return value.
> 
> Specifically, test the expected behavior under various scenarios
> combining page dirtiness (dirty vs clean) and failure types (hard vs
> soft):
> + Dirty + Hard: Trigger a SIGBUS on injection, and trigger another
>   SIGBUS when reading the page again.
> + Dirty + Soft: No SIGBUS is triggered, and the original value can be
>   read successfully.
> + Clean + Hard: No SIGBUS is triggered on injection, but trigger a
>   SIGBUS when trying to read the page again.
> + Clean + Soft: No SIGBUS is triggered, and the page can be read
>   successfully.
> 
> Acked-by: Miaohe Lin <linmiaohe@huawei.com>
> Signed-off-by: Lisa Wang <wyihan@google.com>
> ---
>  tools/testing/selftests/mm/memory-failure.c | 118 +++++++++++++++++++++++++++-
>  1 file changed, 115 insertions(+), 3 deletions(-)
> 
> diff --git a/tools/testing/selftests/mm/memory-failure.c b/tools/testing/selftests/mm/memory-failure.c
> index 1a5a32e22cce..5b26f0c44664 100644
> --- a/tools/testing/selftests/mm/memory-failure.c
> +++ b/tools/testing/selftests/mm/memory-failure.c
> @@ -20,6 +20,10 @@
>  
>  #include "vm_util.h"
>  
> +#ifndef fallthrough
> +# define fallthrough __attribute__((__fallthrough__))
> +#endif

This doesn't belong into this test.

rseq/rseq.c selftest already uses it.

I think tools/testing/include/ already provides it.

Maybe

	#include <linux/compiler.h>

does the trick?



-- 
Cheers,

David

Re: [PATCH v5 5/5] mm: selftests: Add shmem into memory failure test

[Lisa Wang]

On Mon, Aug 24, 2026 at 03:00:47PM +0200, David Hildenbrand (Arm) wrote:
> > +++ b/tools/testing/selftests/mm/memory-failure.c
> > @@ -20,6 +20,10 @@
> >  
> >  #include "vm_util.h"
> >  
> > +#ifndef fallthrough
> > +# define fallthrough __attribute__((__fallthrough__))
> > +#endif
> 
> This doesn't belong into this test.
> 
> rseq/rseq.c selftest already uses it.
> 
> I think tools/testing/include/ already provides it.
> 
> Maybe
> 
> 	#include <linux/compiler.h>
> 
> does the trick?

Hi David,
Thank you for replying.
Would it be better to change it to /* FALLTHROUGH */
like the rest of the mm selftests?

If we prefer to include <linux/compiler.h>, we can
certainly do that, but we would also need to update
the Makefile and fix a redefinition in pkey-helpers.h.

--- a/tools/testing/selftests/mm/Makefile
+++ b/tools/testing/selftests/mm/Makefile
@@ -37,7 +37,7 @@ endif
 # LDLIBS.
 MAKEFLAGS += --no-builtin-rules

-CFLAGS = -Wall -O2 -I $(top_srcdir) $(EXTRA_CFLAGS) $(KHDR_INCLUDES) $(TOOLS_INCLUDES)
+CFLAGS = -Wall -O2 -I $(top_srcdir) $(EXTRA_CFLAGS) $(KHDR_INCLUDES) $(TOOLS_INCLUDES) -I$(top_srcdir)/tools/include
 CFLAGS += -Wunreachable-code
 LDLIBS = -lrt -lpthread -lm

--- a/tools/testing/selftests/mm/pkey-helpers.h
+++ b/tools/testing/selftests/mm/pkey-helpers.h
@@ -22,7 +22,7 @@
 typedef __u8   u8;
 typedef __u16  u16;
 typedef __u32  u32;
-typedef __u64  u64;
+// typedef __u64       u64;


Lisa

Re: [PATCH v5 5/5] mm: selftests: Add shmem into memory failure test

[Ackerley Tng]

Lisa Wang <wyihan@google.com> writes:

> On Mon, Aug 24, 2026 at 03:00:47PM +0200, David Hildenbrand (Arm) wrote:
>> > +++ b/tools/testing/selftests/mm/memory-failure.c
>> > @@ -20,6 +20,10 @@
>> >
>> >  #include "vm_util.h"
>> >
>> > +#ifndef fallthrough
>> > +# define fallthrough __attribute__((__fallthrough__))
>> > +#endif
>>
>> This doesn't belong into this test.
>>
>> rseq/rseq.c selftest already uses it.
>>
>> I think tools/testing/include/ already provides it.
>>
>> Maybe
>>
>> 	#include <linux/compiler.h>
>>
>> does the trick?
>
> Hi David,
> Thank you for replying.
> Would it be better to change it to /* FALLTHROUGH */
> like the rest of the mm selftests?
>
> If we prefer to include <linux/compiler.h>, we can
> certainly do that, but we would also need to update
> the Makefile and fix a redefinition in pkey-helpers.h.
>
> --- a/tools/testing/selftests/mm/Makefile
> +++ b/tools/testing/selftests/mm/Makefile
> @@ -37,7 +37,7 @@ endif
>  # LDLIBS.
>  MAKEFLAGS += --no-builtin-rules
>
> -CFLAGS = -Wall -O2 -I $(top_srcdir) $(EXTRA_CFLAGS) $(KHDR_INCLUDES) $(TOOLS_INCLUDES)
> +CFLAGS = -Wall -O2 -I $(top_srcdir) $(EXTRA_CFLAGS) $(KHDR_INCLUDES) $(TOOLS_INCLUDES) -I$(top_srcdir)/tools/include
>  CFLAGS += -Wunreachable-code
>  LDLIBS = -lrt -lpthread -lm
>
> --- a/tools/testing/selftests/mm/pkey-helpers.h
> +++ b/tools/testing/selftests/mm/pkey-helpers.h
> @@ -22,7 +22,7 @@
>  typedef __u8   u8;
>  typedef __u16  u16;
>  typedef __u32  u32;
> -typedef __u64  u64;
> +// typedef __u64       u64;

Removing this u64 typedef seems to be unrelated, the layers of includes
probably has some weirdness that needs to be ironed out.

If the weirdness is not too complex to iron out to be in time for 7.4, I
think we can iron that out, if not perhaps using /* FALLTHROUGH */ is
fine, to be aligned with the other mm selftests.

>
>
> Lisa