mm: Don't access uninitialized memmaps in PFN walkers

There are three places where we access uninitialized memmaps, namely:
- /proc/kpagecount
- /proc/kpageflags
- /proc/kpagecgroup

We have initialized memmaps either when the section is online or when the page was initialized to the ZONE_DEVICE. Uninitialized memmaps contain garbage and in the worst case trigger kernel BUGs, especially with CONFIG_PAGE_POISONING.

For example, not onlining a DIMM during boot and calling /proc/kpagecount with CONFIG_PAGE_POISONING:

:/# cat /proc/kpagecount > tmp.test
[   95.600592] BUG: unable to handle page fault for address: fffffffffffffffe
[   95.601238] #PF: supervisor read access in kernel mode
[   95.601675] #PF: error_code(0x0000) - not-present page
[   95.602116] PGD 114616067 P4D 114616067 PUD 114618067 PMD 0
[   95.602596] Oops: 0000 [#1] SMP NOPTI
[   95.602920] CPU: 0 PID: 469 Comm: cat Not tainted 5.4.0-rc1-next-20191004+ #11
[   95.603547] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS rel-1.12.1-0-ga5cab58e9a3f-prebuilt.qemu.4
[   95.604521] RIP: 0010:kpagecount_read+0xce/0x1e0
[   95.604917] Code: e8 09 83 e0 3f 48 0f a3 02 73 2d 4c 89 e7 48 c1 e7 06 48 03 3d ab 51 01 01 74 1d 48 8b 57 08 480
[   95.606450] RSP: 0018:ffffa14e409b7e78 EFLAGS: 00010202
[   95.606904] RAX: fffffffffffffffe RBX: 0000000000020000 RCX: 0000000000000000
[   95.607519] RDX: 0000000000000001 RSI: 00007f76b5595000 RDI: fffff35645000000
[   95.608128] RBP: 00007f76b5595000 R08: 0000000000000001 R09: 0000000000000000
[   95.608731] R10: 0000000000000000 R11: 0000000000000000 R12: 0000000000140000
[   95.609327] R13: 0000000000020000 R14: 00007f76b5595000 R15: ffffa14e409b7f08
[   95.609924] FS:  00007f76b577d580(0000) GS:ffff8f41bd400000(0000) knlGS:0000000000000000
[   95.610599] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[   95.611083] CR2: fffffffffffffffe CR3: 0000000078960000 CR4: 00000000000006f0
[   95.611686] Call Trace:
[   95.611906]  proc_reg_read+0x3c/0x60
[   95.612228]  vfs_read+0xc5/0x180
[   95.612505]  ksys_read+0x68/0xe0
[   95.612785]  do_syscall_64+0x5c/0xa0
[   95.613092]  entry_SYSCALL_64_after_hwframe+0x49/0xbe

For now, let's drop support for ZONE_DEVICE from the three pseudo files in order to fix this. To distinguish offline memory (with garbage memmap) from ZONE_DEVICE memory with properly initialized memmaps, we would have to check get_dev_pagemap() and pfn_zone_device_reserved() right now. The usage of both (especially, special casing devmem) is frowned upon and needs to be reworked. The fundamental issue we have is:

	if (pfn_to_online_page(pfn)) {
		/* memmap initialized */
	} else if (pfn_valid(pfn)) {
		/*
		 * ???
		 * a) offline memory. memmap garbage.
		 * b) devmem: memmap initialized to ZONE_DEVICE.
		 * c) devmem: reserved for driver. memmap garbage.
		 * (d) devmem: memmap currently initializing - garbage)
		 */
	}

We'll leave the pfn_zone_device_reserved() check in stable_page_flags() in place as that function is also used from memory failure. We now no longer dump information about pages that are not in use anymore - offline.

Reported-by Qian Cai <[email protected]>

Cc Dan Williams <[email protected]>

Cc Alexey Dobriyan <[email protected]>

Cc Andrew Morton <[email protected]>

Cc Stephen Rothwell <[email protected]>

Cc Toshiki Fukasawa <[email protected]>

Cc Pankaj gupta <[email protected]>

Cc Mike Rapoport <[email protected]>

Cc Anthony Yznaga <[email protected]>

Cc Michal Hocko <[email protected]>

Cc "Aneesh Kumar K.V" <[email protected]>

Cc [email protected]

Signed-off-by David Hildenbrand <[email protected]>

---
 fs/proc/page.c | 28 ++++++++++++++++------------
 1 file changed, 16 insertions(+), 12 deletions(-)

fs/proc/page.c +16 -12

index decd3fe39674..e40dbfe1168e 100644
@@ -42,10 +42,12 @@ static ssize_t kpagecount_read(struct file *file, char __user *buf,
 		return -EINVAL;
 
 	while (count > 0) {
-		if (pfn_valid(pfn))
-			ppage = pfn_to_page(pfn);
-		else
-			ppage = NULL;
+		/*
+		 * TODO: ZONE_DEVICE support requires to identify
+		 * memmaps that were actually initialized.
+		 */
+		ppage = pfn_to_online_page(pfn);
+
 		if (!ppage || PageSlab(ppage) || page_has_type(ppage))
 			pcount = 0;
 		else

@@ -218,10 +220,11 @@ static ssize_t kpageflags_read(struct file *file, char __user *buf,
 		return -EINVAL;
 
 	while (count > 0) {
-		if (pfn_valid(pfn))
-			ppage = pfn_to_page(pfn);
-		else
-			ppage = NULL;
+		/*
+		 * TODO: ZONE_DEVICE support requires to identify
+		 * memmaps that were actually initialized.
+		 */
+		ppage = pfn_to_online_page(pfn);
 
 		if (put_user(stable_page_flags(ppage), out)) {
 			ret = -EFAULT;

@@ -263,10 +266,11 @@ static ssize_t kpagecgroup_read(struct file *file, char __user *buf,
 		return -EINVAL;
 
 	while (count > 0) {
-		if (pfn_valid(pfn))
-			ppage = pfn_to_page(pfn);
-		else
-			ppage = NULL;
+		/*
+		 * TODO: ZONE_DEVICE support requires to identify
+		 * memmaps that were actually initialized.
+		 */
+		ppage = pfn_to_online_page(pfn);
 
 		if (ppage)
 			ino = page_cgroup_ino(ppage);

Signature · 1 line

2.21.0

There are three places where we access uninitialized memmaps, namely:
- /proc/kpagecount
- /proc/kpageflags
- /proc/kpagecgroup

We have initialized memmaps either when the section is online or when the page was initialized to the ZONE_DEVICE. Uninitialized memmaps contain garbage and in the worst case trigger kernel BUGs, especially with CONFIG_PAGE_POISONING.

For example, not onlining a DIMM during boot and calling /proc/kpagecount with CONFIG_PAGE_POISONING:

:/# cat /proc/kpagecount > tmp.test
[   95.600592] BUG: unable to handle page fault for address: fffffffffffffffe
[   95.601238] #PF: supervisor read access in kernel mode
[   95.601675] #PF: error_code(0x0000) - not-present page
[   95.602116] PGD 114616067 P4D 114616067 PUD 114618067 PMD 0
[   95.602596] Oops: 0000 [#1] SMP NOPTI
[   95.602920] CPU: 0 PID: 469 Comm: cat Not tainted 5.4.0-rc1-next-20191004+ #11
[   95.603547] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS rel-1.12.1-0-ga5cab58e9a3f-prebuilt.qemu.4
[   95.604521] RIP: 0010:kpagecount_read+0xce/0x1e0
[   95.604917] Code: e8 09 83 e0 3f 48 0f a3 02 73 2d 4c 89 e7 48 c1 e7 06 48 03 3d ab 51 01 01 74 1d 48 8b 57 08 480
[   95.606450] RSP: 0018:ffffa14e409b7e78 EFLAGS: 00010202
[   95.606904] RAX: fffffffffffffffe RBX: 0000000000020000 RCX: 0000000000000000
[   95.607519] RDX: 0000000000000001 RSI: 00007f76b5595000 RDI: fffff35645000000
[   95.608128] RBP: 00007f76b5595000 R08: 0000000000000001 R09: 0000000000000000
[   95.608731] R10: 0000000000000000 R11: 0000000000000000 R12: 0000000000140000
[   95.609327] R13: 0000000000020000 R14: 00007f76b5595000 R15: ffffa14e409b7f08
[   95.609924] FS:  00007f76b577d580(0000) GS:ffff8f41bd400000(0000) knlGS:0000000000000000
[   95.610599] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[   95.611083] CR2: fffffffffffffffe CR3: 0000000078960000 CR4: 00000000000006f0
[   95.611686] Call Trace:
[   95.611906]  proc_reg_read+0x3c/0x60
[   95.612228]  vfs_read+0xc5/0x180
[   95.612505]  ksys_read+0x68/0xe0
[   95.612785]  do_syscall_64+0x5c/0xa0
[   95.613092]  entry_SYSCALL_64_after_hwframe+0x49/0xbe

For now, let's drop support for ZONE_DEVICE from the three pseudo files in order to fix this. To distinguish offline memory (with garbage memmap) from ZONE_DEVICE memory with properly initialized memmaps, we would have to check get_dev_pagemap() and pfn_zone_device_reserved() right now. The usage of both (especially, special casing devmem) is frowned upon and needs to be reworked. The fundamental issue we have is:

	if (pfn_to_online_page(pfn)) {
		/* memmap initialized */
	} else if (pfn_valid(pfn)) {
		/*
		 * ???
		 * a) offline memory. memmap garbage.
		 * b) devmem: memmap initialized to ZONE_DEVICE.
		 * c) devmem: reserved for driver. memmap garbage.
		 * (d) devmem: memmap currently initializing - garbage)
		 */
	}

We'll leave the pfn_zone_device_reserved() check in stable_page_flags() in place as that function is also used from memory failure. We now no longer dump information about pages that are not in use anymore - offline.

Reported-by Qian Cai <[email protected]>

Cc Dan Williams <[email protected]>

Cc Alexey Dobriyan <[email protected]>

Cc Andrew Morton <[email protected]>

Cc Stephen Rothwell <[email protected]>

Cc Toshiki Fukasawa <[email protected]>

Cc Pankaj gupta <[email protected]>

Cc Mike Rapoport <[email protected]>

Cc Anthony Yznaga <[email protected]>

Cc Michal Hocko <[email protected]>

Cc "Aneesh Kumar K.V" <[email protected]>

Cc [email protected]

Signed-off-by David Hildenbrand <[email protected]>

---
  fs/proc/page.c | 28 ++++++++++++++++------------
  1 file changed, 16 insertions(+), 12 deletions(-)

fs/proc/page.c +11 -10

index decd3fe39674..e40dbfe1168e 100644
@@ -42,10 +42,12 @@ static ssize_t kpagecount_read(struct file *file, char __user *buf,
  		return -EINVAL;
 
  	while (count > 0) {
-		if (pfn_valid(pfn))
-			ppage = pfn_to_page(pfn);
-		else
-			ppage = NULL;
+		/*
+		 * TODO: ZONE_DEVICE support requires to identify
+		 * memmaps that were actually initialized.
+		 */
+		ppage = pfn_to_online_page(pfn);
+
  		if (!ppage || PageSlab(ppage) || page_has_type(ppage))
  			pcount = 0;
  		else

@@ -218,10 +220,11 @@ static ssize_t kpageflags_read(struct file *file, char __user *buf,
  		return -EINVAL;
 
  	while (count > 0) {
-		if (pfn_valid(pfn))
-			ppage = pfn_to_page(pfn);
-		else
-			ppage = NULL;
+		/*
+		 * TODO: ZONE_DEVICE support requires to identify
+		 * memmaps that were actually initialized.
+		 */
+		ppage = pfn_to_online_page(pfn);
 
  		if (put_user(stable_page_flags(ppage), out)) {
  			ret = -EFAULT;

@@ -263,10 +266,11 @@ static ssize_t kpagecgroup_read(struct file *file, char __user *buf,
  		return -EINVAL;
 
  	while (count > 0) {
-		if (pfn_valid(pfn))
-			ppage = pfn_to_page(pfn);

We should check for pfn_to_online_page() to not access uninitialized memmaps. Reshuffle the code so we don't have to duplicate the error message.

Cc Naoya Horiguchi <[email protected]>

Cc Andrew Morton <[email protected]>

Cc Michal Hocko <[email protected]>

Signed-off-by David Hildenbrand <[email protected]>

---
 mm/memory-failure.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

mm/memory-failure.c +1 -1

index 7ef849da8278..e866e6e5660b 100644
@@ -1253,17 +1253,19 @@ int memory_failure(unsigned long pfn, int flags)
 	if (!sysctl_memory_failure_recovery)
 		panic("Memory failure on page %lx", pfn);
 
-	if (!pfn_valid(pfn)) {
+	p = pfn_to_online_page(pfn);

Quoted 20 lines · David Hildenbrand

We should check for pfn_to_online_page() to not access uninitialized memmaps. Reshuffle the code so we don't have to duplicate the error message.

Cc Naoya Horiguchi <[email protected]>

Cc Andrew Morton <[email protected]>

Cc Michal Hocko <[email protected]>

Signed-off-by David Hildenbrand <[email protected]>

---
 mm/memory-failure.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

mm/memory-failure.c +0 -0

index 7ef849da8278..e866e6e5660b 100644
@@ -1253,17 +1253,19 @@ int memory_failure(unsigned long pfn, int flags)
 	if (!sysctl_memory_failure_recovery)
 		panic("Memory failure on page %lx", pfn);
 

mm/memory-failure.c int memory_failure(unsigned long pfn, int flags) · line 1266

Quoted 12 lines

mm/memory-failure.c +8 -1

-	if (!pfn_valid(pfn)) {
+	p = pfn_to_online_page(pfn);
+	if (!p) {
+		if (pfn_valid(pfn)) {
+			pgmap = get_dev_pagemap(pfn, NULL);
+			if (pgmap)
+				return memory_failure_dev_pagemap(pfn, flags,
+								  pgmap);
+		}
 		pr_err("Memory failure: %#lx: memory outside kernel control\n",
 			pfn);
 		return -ENXIO;

Don't we need that earlier at hwpoison_inject level?

Quoted 21 lines · Michal Hocko

Quoted 20 lines · David Hildenbrand

We should check for pfn_to_online_page() to not access uninitialized memmaps. Reshuffle the code so we don't have to duplicate the error message.

Cc Naoya Horiguchi <[email protected]>

Cc Andrew Morton <[email protected]>

Cc Michal Hocko <[email protected]>

Signed-off-by David Hildenbrand <[email protected]>

---
 mm/memory-failure.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

mm/memory-failure.c +0 -0

index 7ef849da8278..e866e6e5660b 100644
@@ -1253,17 +1253,19 @@ int memory_failure(unsigned long pfn, int flags)
 	if (!sysctl_memory_failure_recovery)
 		panic("Memory failure on page %lx", pfn);
 

Quoted 15 lines

mm/memory-failure.c int memory_failure(unsigned long pfn, int flags) · line 1266

Quoted 12 lines

mm/memory-failure.c +8 -1

-	if (!pfn_valid(pfn)) {
+	p = pfn_to_online_page(pfn);
+	if (!p) {
+		if (pfn_valid(pfn)) {
+			pgmap = get_dev_pagemap(pfn, NULL);
+			if (pgmap)
+				return memory_failure_dev_pagemap(pfn, flags,
+								  pgmap);
+		}
 		pr_err("Memory failure: %#lx: memory outside kernel control\n",
 			pfn);
 		return -ENXIO;

Don't we need that earlier at hwpoison_inject level?

Theoretically yes, this is another instance. But pfn_to_online_page(pfn) alone would not be sufficient as discussed. We would, again, have to special-case ZONE_DEVICE via things like get_dev_pagemap() ...

But mm/hwpoison-inject.c:hwpoison_inject() is a pure debug feature either way:

/*
* Note that the below poison/unpoison interfaces do not involve
* hardware status change, hence do not require hardware support.
* They are mainly for testing hwpoison in software level. */

So it's not that bad compared to memory_failure() called from real HW or drivers/base/memory.c:soft_offline_page_store()/hard_offline_page_store()

Quoted 50 lines · David Hildenbrand

Quoted 21 lines · Michal Hocko

Quoted 20 lines · David Hildenbrand

We should check for pfn_to_online_page() to not access uninitialized memmaps. Reshuffle the code so we don't have to duplicate the error message.

Cc Naoya Horiguchi <[email protected]>

Cc Andrew Morton <[email protected]>

Cc Michal Hocko <[email protected]>

Signed-off-by David Hildenbrand <[email protected]>

---
 mm/memory-failure.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

mm/memory-failure.c +0 -0

index 7ef849da8278..e866e6e5660b 100644
@@ -1253,17 +1253,19 @@ int memory_failure(unsigned long pfn, int flags)
 	if (!sysctl_memory_failure_recovery)
 		panic("Memory failure on page %lx", pfn);
 

Quoted 15 lines

mm/memory-failure.c int memory_failure(unsigned long pfn, int flags) · line 1266

Quoted 12 lines

mm/memory-failure.c +8 -1

-	if (!pfn_valid(pfn)) {
+	p = pfn_to_online_page(pfn);
+	if (!p) {
+		if (pfn_valid(pfn)) {
+			pgmap = get_dev_pagemap(pfn, NULL);
+			if (pgmap)
+				return memory_failure_dev_pagemap(pfn, flags,
+								  pgmap);
+		}
 		pr_err("Memory failure: %#lx: memory outside kernel control\n",
 			pfn);
 		return -ENXIO;

Don't we need that earlier at hwpoison_inject level?

Theoretically yes, this is another instance. But pfn_to_online_page(pfn) alone would not be sufficient as discussed. We would, again, have to special-case ZONE_DEVICE via things like get_dev_pagemap() ...

But mm/hwpoison-inject.c:hwpoison_inject() is a pure debug feature either way:

/*
* Note that the below poison/unpoison interfaces do not involve
* hardware status change, hence do not require hardware support.
* They are mainly for testing hwpoison in software level. */

Quoted 2 lines

So it's not that bad compared to memory_failure() called from real HW or drivers/base/memory.c:soft_offline_page_store()/hard_offline_page_store()

Yes, this is just a toy. And yes we need to handle zone device pages here because a) people likely want to test MCE behavior even on these pages and b) HW can really trigger MCEs there as well. I was just pointing that the patch is likely incomplete.

Quoted 51 lines · Michal Hocko

Quoted 50 lines · David Hildenbrand

Quoted 21 lines · Michal Hocko

Quoted 20 lines · David Hildenbrand

We should check for pfn_to_online_page() to not access uninitialized memmaps. Reshuffle the code so we don't have to duplicate the error message.

Cc Naoya Horiguchi <[email protected]>

Cc Andrew Morton <[email protected]>

Cc Michal Hocko <[email protected]>

Signed-off-by David Hildenbrand <[email protected]>

---
 mm/memory-failure.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

mm/memory-failure.c +0 -0

index 7ef849da8278..e866e6e5660b 100644
@@ -1253,17 +1253,19 @@ int memory_failure(unsigned long pfn, int flags)
 	if (!sysctl_memory_failure_recovery)
 		panic("Memory failure on page %lx", pfn);
 

Quoted 15 lines

mm/memory-failure.c int memory_failure(unsigned long pfn, int flags) · line 1266

Quoted 12 lines

mm/memory-failure.c +8 -1

-	if (!pfn_valid(pfn)) {
+	p = pfn_to_online_page(pfn);
+	if (!p) {
+		if (pfn_valid(pfn)) {
+			pgmap = get_dev_pagemap(pfn, NULL);
+			if (pgmap)
+				return memory_failure_dev_pagemap(pfn, flags,
+								  pgmap);
+		}
 		pr_err("Memory failure: %#lx: memory outside kernel control\n",
 			pfn);
 		return -ENXIO;

Don't we need that earlier at hwpoison_inject level?

Theoretically yes, this is another instance. But pfn_to_online_page(pfn) alone would not be sufficient as discussed. We would, again, have to special-case ZONE_DEVICE via things like get_dev_pagemap() ...

But mm/hwpoison-inject.c:hwpoison_inject() is a pure debug feature either way:

/*
* Note that the below poison/unpoison interfaces do not involve
* hardware status change, hence do not require hardware support.
* They are mainly for testing hwpoison in software level. */

Quoted 8 lines

Quoted 2 lines

So it's not that bad compared to memory_failure() called from real HW or drivers/base/memory.c:soft_offline_page_store()/hard_offline_page_store()

Yes, this is just a toy. And yes we need to handle zone device pages here because a) people likely want to test MCE behavior even on these pages and b) HW can really trigger MCEs there as well. I was just pointing that the patch is likely incomplete.

I rather think this deserves a separate patch as it is a separate interface :)

I do wonder why hwpoison_inject() has to perform so much extra work compared to other memory_failure() users. This smells like legacy leftovers to me, but I might be wrong. The interface is fairly old, though. Does anybody know why we need this magic? I can spot quite some duplicate checks/things getting performed.

Naiive me would just make the interface perform the same as hard_offline_page_store(). But most probably I am not getting the real purpose of both different interfaces.

HWPOISON_INJECT is only selected for DEBUG_KERNEL, so I would have guessed that fixing this is not urgent.

BTW: mm/memory-failure.c:soft_offline_page() also looks wrong and needs fixing to make sure we access initialized memmaps.

Quoted 61 lines · David Hildenbrand

Quoted 59 lines · Michal Hocko

Quoted 44 lines · David Hildenbrand

Quoted 21 lines · Michal Hocko

Quoted 20 lines · David Hildenbrand

We should check for pfn_to_online_page() to not access uninitialized memmaps. Reshuffle the code so we don't have to duplicate the error message.

Cc Naoya Horiguchi <[email protected]>

Cc Andrew Morton <[email protected]>

Cc Michal Hocko <[email protected]>

Signed-off-by David Hildenbrand <[email protected]>

---
 mm/memory-failure.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

mm/memory-failure.c +0 -0

index 7ef849da8278..e866e6e5660b 100644
@@ -1253,17 +1253,19 @@ int memory_failure(unsigned long pfn, int flags)
 	if (!sysctl_memory_failure_recovery)
 		panic("Memory failure on page %lx", pfn);
 

Quoted 15 lines

mm/memory-failure.c int memory_failure(unsigned long pfn, int flags) · line 1266

Quoted 12 lines

mm/memory-failure.c +8 -1

-	if (!pfn_valid(pfn)) {
+	p = pfn_to_online_page(pfn);
+	if (!p) {
+		if (pfn_valid(pfn)) {
+			pgmap = get_dev_pagemap(pfn, NULL);
+			if (pgmap)
+				return memory_failure_dev_pagemap(pfn, flags,
+								  pgmap);
+		}
 		pr_err("Memory failure: %#lx: memory outside kernel control\n",
 			pfn);
 		return -ENXIO;

Don't we need that earlier at hwpoison_inject level?

Theoretically yes, this is another instance. But pfn_to_online_page(pfn) alone would not be sufficient as discussed. We would, again, have to special-case ZONE_DEVICE via things like get_dev_pagemap() ...

But mm/hwpoison-inject.c:hwpoison_inject() is a pure debug feature either way:

Quoted 8 lines

/*
* Note that the below poison/unpoison interfaces do not involve
* hardware status change, hence do not require hardware support.
* They are mainly for testing hwpoison in software level. */

So it's not that bad compared to memory_failure() called from real HW or drivers/base/memory.c:soft_offline_page_store()/hard_offline_page_store()

Yes, this is just a toy. And yes we need to handle zone device pages here because a) people likely want to test MCE behavior even on these pages and b) HW can really trigger MCEs there as well. I was just pointing that the patch is likely incomplete.

Quoted 8 lines

I rather think this deserves a separate patch as it is a separate interface :)

I do wonder why hwpoison_inject() has to perform so much extra work compared to other memory_failure() users. This smells like legacy leftovers to me, but I might be wrong. The interface is fairly old, though. Does anybody know why we need this magic? I can spot quite some duplicate checks/things getting performed.

Quoted 4 lines

Naiive me would just make the interface perform the same as hard_offline_page_store(). But most probably I am not getting the real purpose of both different interfaces.

Quoted 7 lines

HWPOISON_INJECT is only selected for DEBUG_KERNEL, so I would have guessed that fixing this is not urgent.

BTW: mm/memory-failure.c:soft_offline_page() also looks wrong and needs fixing to make sure we access initialized memmaps.

To be more precise, soft_offline_page_store() needs a pfn_to_online_page() check. Will send a patch.

Quoted 65 lines · David Hildenbrand

Quoted 64 lines · David Hildenbrand

Quoted 51 lines · Michal Hocko

Quoted 50 lines · David Hildenbrand

Quoted 21 lines · Michal Hocko

Quoted 20 lines · David Hildenbrand

We should check for pfn_to_online_page() to not access uninitialized memmaps. Reshuffle the code so we don't have to duplicate the error message.

Cc Naoya Horiguchi <[email protected]>

Cc Andrew Morton <[email protected]>

Cc Michal Hocko <[email protected]>

Signed-off-by David Hildenbrand <[email protected]>

---
  mm/memory-failure.c | 14 ++++++++------
  1 file changed, 8 insertions(+), 6 deletions(-)

mm/memory-failure.c +0 -0

index 7ef849da8278..e866e6e5660b 100644
@@ -1253,17 +1253,19 @@ int memory_failure(unsigned long pfn, int flags)
  	if (!sysctl_memory_failure_recovery)
  		panic("Memory failure on page %lx", pfn);
 

Quoted 15 lines

mm/memory-failure.c int memory_failure(unsigned long pfn, int flags) · line 1266

Quoted 12 lines

mm/memory-failure.c +8 -1

-	if (!pfn_valid(pfn)) {
+	p = pfn_to_online_page(pfn);
+	if (!p) {
+		if (pfn_valid(pfn)) {
+			pgmap = get_dev_pagemap(pfn, NULL);
+			if (pgmap)
+				return memory_failure_dev_pagemap(pfn, flags,
+								  pgmap);
+		}
  		pr_err("Memory failure: %#lx: memory outside kernel control\n",
  			pfn);
  		return -ENXIO;

Don't we need that earlier at hwpoison_inject level?

Theoretically yes, this is another instance. But pfn_to_online_page(pfn) alone would not be sufficient as discussed. We would, again, have to special-case ZONE_DEVICE via things like get_dev_pagemap() ...

But mm/hwpoison-inject.c:hwpoison_inject() is a pure debug feature either way:

/*
* Note that the below poison/unpoison interfaces do not involve
* hardware status change, hence do not require hardware support.
* They are mainly for testing hwpoison in software level. */

Quoted 8 lines

Quoted 2 lines

So it's not that bad compared to memory_failure() called from real HW or drivers/base/memory.c:soft_offline_page_store()/hard_offline_page_store()

Yes, this is just a toy. And yes we need to handle zone device pages here because a) people likely want to test MCE behavior even on these pages and b) HW can really trigger MCEs there as well. I was just pointing that the patch is likely incomplete.

I rather think this deserves a separate patch as it is a separate interface :)

Quoted 5 lines

Quoted 5 lines

I do wonder why hwpoison_inject() has to perform so much extra work compared to other memory_failure() users. This smells like legacy leftovers to me, but I might be wrong. The interface is fairly old, though. Does anybody know why we need this magic? I can spot quite some duplicate checks/things getting performed.

It concerns me too, this *is* an old legacy code. I guess it was left as-is because no one complained about it. That's not good, so I'll do some cleanup.

Quoted 5 lines

Quoted 5 lines

Naiive me would just make the interface perform the same as hard_offline_page_store(). But most probably I am not getting the real purpose of both different interfaces.

Maybe for historical reason, we have these slightly different interfaces:

- corrupt-pfn
- purely for debugging purpose
- paired with unpoison-pfn
- used by in-kernel tool tools/vm/page-types.c
- hard_offline_page
- paired with soft_offline_page
- used by other userspace tools like mcelog

But these don't explain why implemented differently, so I think that both should be written in the same manner.

Quoted 10 lines

Quoted 7 lines

HWPOISON_INJECT is only selected for DEBUG_KERNEL, so I would have guessed that fixing this is not urgent.

BTW: mm/memory-failure.c:soft_offline_page() also looks wrong and needs fixing to make sure we access initialized memmaps.

To be more precise, soft_offline_page_store() needs a pfn_to_online_page() check. Will send a patch.

Thanks for finding this.

- Naoya Horiguchi

Quoted 66 lines · Naoya Horiguchi

Quoted 65 lines · David Hildenbrand

Quoted 64 lines · David Hildenbrand

Quoted 51 lines · Michal Hocko

Quoted 50 lines · David Hildenbrand

Quoted 21 lines · Michal Hocko

Quoted 20 lines · David Hildenbrand

We should check for pfn_to_online_page() to not access uninitialized memmaps. Reshuffle the code so we don't have to duplicate the error message.

Cc Naoya Horiguchi <[email protected]>

Cc Andrew Morton <[email protected]>

Cc Michal Hocko <[email protected]>

Signed-off-by David Hildenbrand <[email protected]>

---
  mm/memory-failure.c | 14 ++++++++------
  1 file changed, 8 insertions(+), 6 deletions(-)

mm/memory-failure.c +0 -0

index 7ef849da8278..e866e6e5660b 100644
@@ -1253,17 +1253,19 @@ int memory_failure(unsigned long pfn, int flags)
  	if (!sysctl_memory_failure_recovery)
  		panic("Memory failure on page %lx", pfn);
 

Quoted 15 lines

mm/memory-failure.c int memory_failure(unsigned long pfn, int flags) · line 1266

Quoted 12 lines

mm/memory-failure.c +8 -1

-	if (!pfn_valid(pfn)) {
+	p = pfn_to_online_page(pfn);
+	if (!p) {
+		if (pfn_valid(pfn)) {
+			pgmap = get_dev_pagemap(pfn, NULL);
+			if (pgmap)
+				return memory_failure_dev_pagemap(pfn, flags,
+								  pgmap);
+		}
  		pr_err("Memory failure: %#lx: memory outside kernel control\n",
  			pfn);
  		return -ENXIO;

Don't we need that earlier at hwpoison_inject level?

Theoretically yes, this is another instance. But pfn_to_online_page(pfn) alone would not be sufficient as discussed. We would, again, have to special-case ZONE_DEVICE via things like get_dev_pagemap() ...

But mm/hwpoison-inject.c:hwpoison_inject() is a pure debug feature either way:

/*
* Note that the below poison/unpoison interfaces do not involve
* hardware status change, hence do not require hardware support.
* They are mainly for testing hwpoison in software level. */

Quoted 8 lines

Quoted 2 lines

So it's not that bad compared to memory_failure() called from real HW or drivers/base/memory.c:soft_offline_page_store()/hard_offline_page_store()

Yes, this is just a toy. And yes we need to handle zone device pages here because a) people likely want to test MCE behavior even on these pages and b) HW can really trigger MCEs there as well. I was just pointing that the patch is likely incomplete.

I rather think this deserves a separate patch as it is a separate interface :)

Quoted 8 lines

Quoted 5 lines

Quoted 5 lines

I do wonder why hwpoison_inject() has to perform so much extra work compared to other memory_failure() users. This smells like legacy leftovers to me, but I might be wrong. The interface is fairly old, though. Does anybody know why we need this magic? I can spot quite some duplicate checks/things getting performed.

It concerns me too, this *is* an old legacy code. I guess it was left as-is because no one complained about it. That's not good, so I'll do some cleanup.

Most of that stuff was introduced in

commit 31d3d3484f9bd263925ecaa341500ac2df3a5d9b

Author: Wu Fengguang <[email protected]>
Date:   Wed Dec 16 12:19:59 2009 +0100

HWPOISON: limit hwpoison injector to known page types

__memory_failure()'s workflow is

set PG_hwpoison
//...
unset PG_hwpoison if didn't pass hwpoison filter

That could kill unrelated process if it happens to page fault on the page with the (temporary) PG_hwpoison. The race should be big enough to appear in stress tests.

Fix it by grabbing the page and checking filter at inject time. This also avoids the very noisy "Injecting memory failure..." messages.

Now, we still have the same "issue" in memory_failure() today:

	if (TestSetPageHWPoison(p)) {
		pr_err("Memory failure: %#lx: already hardware poisoned\n",
			pfn);
		return 0;
	}
[...]
	if (hwpoison_filter(p)) {
		if (TestClearPageHWPoison(p))
			num_poisoned_pages_dec();
		unlock_page(p);
		put_hwpoison_page(p);
		return 0;
	}

However, I don't understand why we need that special handling only for this debug interface and not the other users.

I'd vote for ripping out that legacy crap (so the interface works correctly with ZONE_DEVICE) and instead (if really required) rework memory_failure() to not produce such side effects.

I do agree. The two should be really using the same code. My understanding was that MADV_HWPOISON was there to test the actual MCE behavior (and the man page seems to agree with that).

Oscar is working on a rewrite. Not sure he has considered this as well.

We should check for pfn_to_online_page() to not access uninitialized memmaps. Reshuffle the code so we don't have to duplicate the error message.

Cc Naoya Horiguchi <[email protected]>

Cc Andrew Morton <[email protected]>

Cc Michal Hocko <[email protected]>

Signed-off-by David Hildenbrand <[email protected]>

---
 mm/memory-failure.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

mm/memory-failure.c +8 -1

index 7ef849da8278..e866e6e5660b 100644
@@ -1253,17 +1253,19 @@ int memory_failure(unsigned long pfn, int flags)
 	if (!sysctl_memory_failure_recovery)
 		panic("Memory failure on page %lx", pfn);
 
-	if (!pfn_valid(pfn)) {
+	p = pfn_to_online_page(pfn);
+	if (!p) {
+		if (pfn_valid(pfn)) {
+			pgmap = get_dev_pagemap(pfn, NULL);
+			if (pgmap)
+				return memory_failure_dev_pagemap(pfn, flags,
+								  pgmap);
+		}

Quoted 34 lines · David Hildenbrand

We should check for pfn_to_online_page() to not access uninitialized memmaps. Reshuffle the code so we don't have to duplicate the error message.

Cc Naoya Horiguchi <[email protected]>

Cc Andrew Morton <[email protected]>

Cc Michal Hocko <[email protected]>

Signed-off-by David Hildenbrand <[email protected]>

---
 mm/memory-failure.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

mm/memory-failure.c +8 -1

index 7ef849da8278..e866e6e5660b 100644
@@ -1253,17 +1253,19 @@ int memory_failure(unsigned long pfn, int flags)
 	if (!sysctl_memory_failure_recovery)
 		panic("Memory failure on page %lx", pfn);
 
-	if (!pfn_valid(pfn)) {
+	p = pfn_to_online_page(pfn);
+	if (!p) {
+		if (pfn_valid(pfn)) {
+			pgmap = get_dev_pagemap(pfn, NULL);
+			if (pgmap)
+				return memory_failure_dev_pagemap(pfn, flags,
+								  pgmap);
+		}
 		pr_err("Memory failure: %#lx: memory outside kernel control\n",
 			pfn);
 		return -ENXIO;
 	}
 

mm/memory-failure.c int memory_failure(unsigned long pfn, int flags) · line 1266

Quoted 5 lines

mm/memory-failure.c +0 -5

-	pgmap = get_dev_pagemap(pfn, NULL);
-	if (pgmap)
-		return memory_failure_dev_pagemap(pfn, flags, pgmap);
-
-	p = pfn_to_page(pfn);

This change seems to assume that memory_failure_dev_pagemap() is never called for online pages. Is it an intended behavior?
Or the concept "online pages" is not applicable to zone device pages?

Quoted 35 lines · Naoya Horiguchi

Quoted 34 lines · David Hildenbrand

We should check for pfn_to_online_page() to not access uninitialized memmaps. Reshuffle the code so we don't have to duplicate the error message.

Cc Naoya Horiguchi <[email protected]>

Cc Andrew Morton <[email protected]>

Cc Michal Hocko <[email protected]>

Signed-off-by David Hildenbrand <[email protected]>

---
 mm/memory-failure.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

mm/memory-failure.c +8 -1

index 7ef849da8278..e866e6e5660b 100644
@@ -1253,17 +1253,19 @@ int memory_failure(unsigned long pfn, int flags)
 	if (!sysctl_memory_failure_recovery)
 		panic("Memory failure on page %lx", pfn);
 
-	if (!pfn_valid(pfn)) {
+	p = pfn_to_online_page(pfn);
+	if (!p) {
+		if (pfn_valid(pfn)) {
+			pgmap = get_dev_pagemap(pfn, NULL);
+			if (pgmap)
+				return memory_failure_dev_pagemap(pfn, flags,
+								  pgmap);
+		}
 		pr_err("Memory failure: %#lx: memory outside kernel control\n",
 			pfn);
 		return -ENXIO;
 	}
 

Quoted 9 lines

mm/memory-failure.c int memory_failure(unsigned long pfn, int flags) · line 1266

Quoted 5 lines

mm/memory-failure.c +0 -5

-	pgmap = get_dev_pagemap(pfn, NULL);
-	if (pgmap)
-		return memory_failure_dev_pagemap(pfn, flags, pgmap);
-
-	p = pfn_to_page(pfn);

This change seems to assume that memory_failure_dev_pagemap() is never called for online pages. Is it an intended behavior?
Or the concept "online pages" is not applicable to zone device pages?

Yes, that's the real culprit. ZONE_DEVICE/devmem pages are never online (SECTION_IS_ONLINE). The terminology "online" only applies to pages that were given to the buddy. And as we support sup-section hotadd for devmem, we cannot easily make use of the section flag it. I already proposed somewhere to convert SECTION_IS_ONLINE to a subsection bitmap and call it something like pfn_active().

pfn_online() would then be "pfn_active() && zone != ZONE_DEVICE". And we could use pfn_active() everywhere to test for initialized memmaps (well, besides some special cases like device reserved memory that does not span full sub-sections). Until now, nobody volunteered and I have other things to do.

Quoted 9 lines

mm/memory-failure.c int memory_failure(unsigned long pfn, int flags) · line 1266

Quoted 5 lines

mm/memory-failure.c +0 -5

-	pgmap = get_dev_pagemap(pfn, NULL);
-	if (pgmap)
-		return memory_failure_dev_pagemap(pfn, flags, pgmap);
-
-	p = pfn_to_page(pfn);

This change seems to assume that memory_failure_dev_pagemap() is never called for online pages. Is it an intended behavior?
Or the concept "online pages" is not applicable to zone device pages?

Yes, that's the real culprit. ZONE_DEVICE/devmem pages are never online (SECTION_IS_ONLINE). The terminology "online" only applies to pages that were given to the buddy. And as we support sup-section hotadd for devmem, we cannot easily make use of the section flag it. I already proposed somewhere to convert SECTION_IS_ONLINE to a subsection bitmap and call it something like pfn_active().

pfn_online() would then be "pfn_active() && zone != ZONE_DEVICE". And we could use pfn_active() everywhere to test for initialized memmaps (well, besides some special cases like device reserved memory that does not span full sub-sections). Until now, nobody volunteered and I have other things to do.

Quoted 17 lines · On Thu, 10 Oct 2019 09:17:42 +0200 David Hildenbrand <[email protected]> wrote:

Quoted 9 lines

mm/memory-failure.c int memory_failure(unsigned long pfn, int flags) · line 1266

Quoted 5 lines

mm/memory-failure.c +0 -5

-	pgmap = get_dev_pagemap(pfn, NULL);
-	if (pgmap)
-		return memory_failure_dev_pagemap(pfn, flags, pgmap);
-
-	p = pfn_to_page(pfn);

This change seems to assume that memory_failure_dev_pagemap() is never called for online pages. Is it an intended behavior?
Or the concept "online pages" is not applicable to zone device pages?

Yes, that's the real culprit. ZONE_DEVICE/devmem pages are never online (SECTION_IS_ONLINE). The terminology "online" only applies to pages that were given to the buddy. And as we support sup-section hotadd for devmem, we cannot easily make use of the section flag it. I already proposed somewhere to convert SECTION_IS_ONLINE to a subsection bitmap and call it something like pfn_active().

Quoted 5 lines

pfn_online() would then be "pfn_active() && zone != ZONE_DEVICE". And we could use pfn_active() everywhere to test for initialized memmaps (well, besides some special cases like device reserved memory that does not span full sub-sections). Until now, nobody volunteered and I have other things to do.

Is it worth a code comment or two to make this clearer?

We should check for pfn_to_online_page() to not access uninitialized memmaps. Reshuffle the code so we don't have to duplicate the error message.

Cc Naoya Horiguchi <[email protected]>

Cc Andrew Morton <[email protected]>

Cc Michal Hocko <[email protected]>

Signed-off-by David Hildenbrand <[email protected]>

---
  mm/memory-failure.c | 14 ++++++++------
  1 file changed, 8 insertions(+), 6 deletions(-)

mm/memory-failure.c +8 -1

index 7ef849da8278..e866e6e5660b 100644
@@ -1253,17 +1253,19 @@ int memory_failure(unsigned long pfn, int flags)
  	if (!sysctl_memory_failure_recovery)
  		panic("Memory failure on page %lx", pfn);
 
-	if (!pfn_valid(pfn)) {
+	p = pfn_to_online_page(pfn);
+	if (!p) {
+		if (pfn_valid(pfn)) {
+			pgmap = get_dev_pagemap(pfn, NULL);
+			if (pgmap)
+				return memory_failure_dev_pagemap(pfn, flags,
+								  pgmap);
+		}
  		pr_err("Memory failure: %#lx: memory outside kernel control\n",
  			pfn);
  		return -ENXIO;