[PATCH 0/2] fix kernel panic due to use uninitialized struct pages

[PATCH 0/2] fix kernel panic due to use uninitialized struct pages

[Toshiki Fukasawa]

A kernel panic was observed during reading /proc/kpageflags for
first few pfns allocated by pmem namespace:

BUG: unable to handle page fault for address: fffffffffffffffe
[  114.495280] #PF: supervisor read access in kernel mode
[  114.495738] #PF: error_code(0x0000) - not-present page
[  114.496203] PGD 17120e067 P4D 17120e067 PUD 171210067 PMD 0
[  114.496713] Oops: 0000 [#1] SMP PTI
[  114.497037] CPU: 9 PID: 1202 Comm: page-types Not tainted 5.3.0-rc1 #1
[  114.497621] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS rel-1.11.0-0-g63451fca13-prebuilt.qemu-project.org 04/01/2014
[  114.498706] RIP: 0010:stable_page_flags+0x27/0x3f0
[  114.499142] Code: 82 66 90 66 66 66 66 90 48 85 ff 0f 84 d1 03 00 00 41 54 55 48 89 fd 53 48 8b 57 08 48 8b 1f 48 8d 42 ff 83 e2 01 48 0f 44 c7 <48> 8b 00 f6 c4 02 0f 84 57 03 00 00 45 31 e4 48 8b 55 08 48 89 ef
[  114.500788] RSP: 0018:ffffa5e601a0fe60 EFLAGS: 00010202
[  114.501373] RAX: fffffffffffffffe RBX: ffffffffffffffff RCX: 0000000000000000
[  114.502009] RDX: 0000000000000001 RSI: 00007ffca13a7310 RDI: ffffd07489000000
[  114.502637] RBP: ffffd07489000000 R08: 0000000000000001 R09: 0000000000000000
[  114.503270] R10: 0000000000000000 R11: 0000000000000000 R12: 0000000000240000
[  114.503896] R13: 0000000000080000 R14: 00007ffca13a7310 R15: ffffa5e601a0ff08
[  114.504530] FS:  00007f0266c7f540(0000) GS:ffff962dbbac0000(0000) knlGS:0000000000000000
[  114.505245] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[  114.505754] CR2: fffffffffffffffe CR3: 000000023a204000 CR4: 00000000000006e0
[  114.506401] Call Trace:
[  114.506660]  kpageflags_read+0xb1/0x130
[  114.507051]  proc_reg_read+0x39/0x60
[  114.507387]  vfs_read+0x8a/0x140
[  114.507686]  ksys_pread64+0x61/0xa0
[  114.508021]  do_syscall_64+0x5f/0x1a0
[  114.508372]  entry_SYSCALL_64_after_hwframe+0x44/0xa9
[  114.508844] RIP: 0033:0x7f0266ba426b

Earlier approach to fix this was discussed here:
https://marc.info/?l=linux-mm&m=152964770000672&w=2

This patchset is another approach to fix it and also provide
a fix for potential future bugs discovered in the process.

Toshiki Fukasawa (2):
  /proc/kpageflags: prevent an integer overflow in stable_page_flags()
  /proc/kpageflags: do not use uninitialized struct pages

 fs/proc/page.c           | 40 +++++++++++++++++++++-------------------
 include/linux/memremap.h |  6 ++++++
 kernel/memremap.c        | 20 ++++++++++++++++++++
 3 files changed, 47 insertions(+), 19 deletions(-)

-- 
1.8.3.1

[PATCH 1/2] /proc/kpageflags: prevent an integer overflow in stable_page_flags()

[Toshiki Fukasawa]

stable_page_flags() returns kpageflags info in u64, but it uses
"1 << KPF_*" internally which is considered as int. This type mismatch
causes no visible problem now, but it will if you set bit 32 or more as
done in a subsequent patch. So use BIT_ULL in order to avoid future
overflow issues.

Signed-off-by: Toshiki Fukasawa <t-fukasawa@vx.jp.nec.com>
---
 fs/proc/page.c | 37 ++++++++++++++++++-------------------
 1 file changed, 18 insertions(+), 19 deletions(-)

diff --git a/fs/proc/page.c b/fs/proc/page.c
index 544d1ee..69064ad 100644
--- a/fs/proc/page.c
+++ b/fs/proc/page.c
@@ -95,7 +95,7 @@ u64 stable_page_flags(struct page *page)
 	 * it differentiates a memory hole from a page with no flags
 	 */
 	if (!page)
-		return 1 << KPF_NOPAGE;
+		return BIT_ULL(KPF_NOPAGE);
 
 	k = page->flags;
 	u = 0;
@@ -107,22 +107,22 @@ u64 stable_page_flags(struct page *page)
 	 * simple test in page_mapped() is not enough.
 	 */
 	if (!PageSlab(page) && page_mapped(page))
-		u |= 1 << KPF_MMAP;
+		u |= BIT_ULL(KPF_MMAP);
 	if (PageAnon(page))
-		u |= 1 << KPF_ANON;
+		u |= BIT_ULL(KPF_ANON);
 	if (PageKsm(page))
-		u |= 1 << KPF_KSM;
+		u |= BIT_ULL(KPF_KSM);
 
 	/*
 	 * compound pages: export both head/tail info
 	 * they together define a compound page's start/end pos and order
 	 */
 	if (PageHead(page))
-		u |= 1 << KPF_COMPOUND_HEAD;
+		u |= BIT_ULL(KPF_COMPOUND_HEAD);
 	if (PageTail(page))
-		u |= 1 << KPF_COMPOUND_TAIL;
+		u |= BIT_ULL(KPF_COMPOUND_TAIL);
 	if (PageHuge(page))
-		u |= 1 << KPF_HUGE;
+		u |= BIT_ULL(KPF_HUGE);
 	/*
 	 * PageTransCompound can be true for non-huge compound pages (slab
 	 * pages or pages allocated by drivers with __GFP_COMP) because it
@@ -133,14 +133,13 @@ u64 stable_page_flags(struct page *page)
 		struct page *head = compound_head(page);
 
 		if (PageLRU(head) || PageAnon(head))
-			u |= 1 << KPF_THP;
+			u |= BIT_ULL(KPF_THP);
 		else if (is_huge_zero_page(head)) {
-			u |= 1 << KPF_ZERO_PAGE;
-			u |= 1 << KPF_THP;
+			u |= BIT_ULL(KPF_ZERO_PAGE);
+			u |= BIT_ULL(KPF_THP);
 		}
 	} else if (is_zero_pfn(page_to_pfn(page)))
-		u |= 1 << KPF_ZERO_PAGE;
-
+		u |= BIT_ULL(KPF_ZERO_PAGE);
 
 	/*
 	 * Caveats on high order pages: page->_refcount will only be set
@@ -148,23 +147,23 @@ u64 stable_page_flags(struct page *page)
 	 * SLOB won't set PG_slab at all on compound pages.
 	 */
 	if (PageBuddy(page))
-		u |= 1 << KPF_BUDDY;
+		u |= BIT_ULL(KPF_BUDDY);
 	else if (page_count(page) == 0 && is_free_buddy_page(page))
-		u |= 1 << KPF_BUDDY;
+		u |= BIT_ULL(KPF_BUDDY);
 
 	if (PageOffline(page))
-		u |= 1 << KPF_OFFLINE;
+		u |= BIT_ULL(KPF_OFFLINE);
 	if (PageTable(page))
-		u |= 1 << KPF_PGTABLE;
+		u |= BIT_ULL(KPF_PGTABLE);
 
 	if (page_is_idle(page))
-		u |= 1 << KPF_IDLE;
+		u |= BIT_ULL(KPF_IDLE);
 
 	u |= kpf_copy_bit(k, KPF_LOCKED,	PG_locked);
 
 	u |= kpf_copy_bit(k, KPF_SLAB,		PG_slab);
 	if (PageTail(page) && PageSlab(compound_head(page)))
-		u |= 1 << KPF_SLAB;
+		u |= BIT_ULL(KPF_SLAB);
 
 	u |= kpf_copy_bit(k, KPF_ERROR,		PG_error);
 	u |= kpf_copy_bit(k, KPF_DIRTY,		PG_dirty);
@@ -177,7 +176,7 @@ u64 stable_page_flags(struct page *page)
 	u |= kpf_copy_bit(k, KPF_RECLAIM,	PG_reclaim);
 
 	if (PageSwapCache(page))
-		u |= 1 << KPF_SWAPCACHE;
+		u |= BIT_ULL(KPF_SWAPCACHE);
 	u |= kpf_copy_bit(k, KPF_SWAPBACKED,	PG_swapbacked);
 
 	u |= kpf_copy_bit(k, KPF_UNEVICTABLE,	PG_unevictable);
-- 
1.8.3.1

[PATCH 2/2] /proc/kpageflags: do not use uninitialized struct pages

[Toshiki Fukasawa]

A kernel panic was observed during reading /proc/kpageflags for
first few pfns allocated by pmem namespace:

BUG: unable to handle page fault for address: fffffffffffffffe
[  114.495280] #PF: supervisor read access in kernel mode
[  114.495738] #PF: error_code(0x0000) - not-present page
[  114.496203] PGD 17120e067 P4D 17120e067 PUD 171210067 PMD 0
[  114.496713] Oops: 0000 [#1] SMP PTI
[  114.497037] CPU: 9 PID: 1202 Comm: page-types Not tainted 5.3.0-rc1 #1
[  114.497621] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS rel-1.11.0-0-g63451fca13-prebuilt.qemu-project.org 04/01/2014
[  114.498706] RIP: 0010:stable_page_flags+0x27/0x3f0
[  114.499142] Code: 82 66 90 66 66 66 66 90 48 85 ff 0f 84 d1 03 00 00 41 54 55 48 89 fd 53 48 8b 57 08 48 8b 1f 48 8d 42 ff 83 e2 01 48 0f 44 c7 <48> 8b 00 f6 c4 02 0f 84 57 03 00 00 45 31 e4 48 8b 55 08 48 89 ef
[  114.500788] RSP: 0018:ffffa5e601a0fe60 EFLAGS: 00010202
[  114.501373] RAX: fffffffffffffffe RBX: ffffffffffffffff RCX: 0000000000000000
[  114.502009] RDX: 0000000000000001 RSI: 00007ffca13a7310 RDI: ffffd07489000000
[  114.502637] RBP: ffffd07489000000 R08: 0000000000000001 R09: 0000000000000000
[  114.503270] R10: 0000000000000000 R11: 0000000000000000 R12: 0000000000240000
[  114.503896] R13: 0000000000080000 R14: 00007ffca13a7310 R15: ffffa5e601a0ff08
[  114.504530] FS:  00007f0266c7f540(0000) GS:ffff962dbbac0000(0000) knlGS:0000000000000000
[  114.505245] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[  114.505754] CR2: fffffffffffffffe CR3: 000000023a204000 CR4: 00000000000006e0
[  114.506401] Call Trace:
[  114.506660]  kpageflags_read+0xb1/0x130
[  114.507051]  proc_reg_read+0x39/0x60
[  114.507387]  vfs_read+0x8a/0x140
[  114.507686]  ksys_pread64+0x61/0xa0
[  114.508021]  do_syscall_64+0x5f/0x1a0
[  114.508372]  entry_SYSCALL_64_after_hwframe+0x44/0xa9
[  114.508844] RIP: 0033:0x7f0266ba426b

The reason for the panic is that stable_page_flags() which parses
the page flags uses uninitialized struct pages reserved by the
ZONE_DEVICE driver.

Earlier approach to fix this was discussed here:
https://marc.info/?l=linux-mm&m=152964770000672&w=2

This is another approach. To avoid using the uninitialized struct page,
immediately return with KPF_RESERVED at the beginning of
stable_page_flags() if the page is reserved by ZONE_DEVICE driver.

Cc: stable@vger.kernel.org
Signed-off-by: Toshiki Fukasawa <t-fukasawa@vx.jp.nec.com>
---
 fs/proc/page.c           |  3 +++
 include/linux/memremap.h |  6 ++++++
 kernel/memremap.c        | 20 ++++++++++++++++++++
 3 files changed, 29 insertions(+)

diff --git a/fs/proc/page.c b/fs/proc/page.c
index 69064ad..decd3fe 100644
--- a/fs/proc/page.c
+++ b/fs/proc/page.c
@@ -97,6 +97,9 @@ u64 stable_page_flags(struct page *page)
 	if (!page)
 		return BIT_ULL(KPF_NOPAGE);
 
+	if (pfn_zone_device_reserved(page_to_pfn(page)))
+		return BIT_ULL(KPF_RESERVED);
+
 	k = page->flags;
 	u = 0;
 
diff --git a/include/linux/memremap.h b/include/linux/memremap.h
index f8a5b2a..2cfc3c2 100644
--- a/include/linux/memremap.h
+++ b/include/linux/memremap.h
@@ -124,6 +124,7 @@ static inline struct vmem_altmap *pgmap_altmap(struct dev_pagemap *pgmap)
 }
 
 #ifdef CONFIG_ZONE_DEVICE
+bool pfn_zone_device_reserved(unsigned long pfn);
 void *devm_memremap_pages(struct device *dev, struct dev_pagemap *pgmap);
 void devm_memunmap_pages(struct device *dev, struct dev_pagemap *pgmap);
 struct dev_pagemap *get_dev_pagemap(unsigned long pfn,
@@ -132,6 +133,11 @@ struct dev_pagemap *get_dev_pagemap(unsigned long pfn,
 unsigned long vmem_altmap_offset(struct vmem_altmap *altmap);
 void vmem_altmap_free(struct vmem_altmap *altmap, unsigned long nr_pfns);
 #else
+static inline bool pfn_zone_device_reserved(unsigned long pfn)
+{
+	return false;
+}
+
 static inline void *devm_memremap_pages(struct device *dev,
 		struct dev_pagemap *pgmap)
 {
diff --git a/kernel/memremap.c b/kernel/memremap.c
index 6ee03a8..bc3471c 100644
--- a/kernel/memremap.c
+++ b/kernel/memremap.c
@@ -72,6 +72,26 @@ static unsigned long pfn_next(unsigned long pfn)
 	return pfn + 1;
 }
 
+/*
+ * This returns true if the page is reserved by ZONE_DEVICE driver.
+ */
+bool pfn_zone_device_reserved(unsigned long pfn)
+{
+	struct dev_pagemap *pgmap;
+	struct vmem_altmap *altmap;
+	bool ret = false;
+
+	pgmap = get_dev_pagemap(pfn, NULL);
+	if (!pgmap)
+		return ret;
+	altmap = pgmap_altmap(pgmap);
+	if (altmap && pfn < (altmap->base_pfn + altmap->reserve))
+		ret = true;
+	put_dev_pagemap(pgmap);
+
+	return ret;
+}
+
 #define for_each_device_pfn(pfn, map) \
 	for (pfn = pfn_first(map); pfn < pfn_end(map); pfn = pfn_next(pfn))
 
-- 
1.8.3.1

Re: [PATCH 1/2] /proc/kpageflags: prevent an integer overflow in stable_page_flags()

[Alexey Dobriyan]

On Thu, Jul 25, 2019 at 02:31:16AM +0000, Toshiki Fukasawa wrote:
> stable_page_flags() returns kpageflags info in u64, but it uses
> "1 << KPF_*" internally which is considered as int. This type mismatch
> causes no visible problem now, but it will if you set bit 32 or more as
> done in a subsequent patch. So use BIT_ULL in order to avoid future
> overflow issues.

> -		return 1 << KPF_NOPAGE;
> +		return BIT_ULL(KPF_NOPAGE);

This won't happen until bit 31 is used and all the flags are within int
currently and stable(!), so the problem doesn't exist for them.

Overflow implies some page flags are 64-bit only, which hopefully won't
happen.

Re: [PATCH 2/2] /proc/kpageflags: do not use uninitialized struct pages

[Michal Hocko]

On Thu 25-07-19 02:31:18, Toshiki Fukasawa wrote:
> A kernel panic was observed during reading /proc/kpageflags for
> first few pfns allocated by pmem namespace:
> 
> BUG: unable to handle page fault for address: fffffffffffffffe
> [  114.495280] #PF: supervisor read access in kernel mode
> [  114.495738] #PF: error_code(0x0000) - not-present page
> [  114.496203] PGD 17120e067 P4D 17120e067 PUD 171210067 PMD 0
> [  114.496713] Oops: 0000 [#1] SMP PTI
> [  114.497037] CPU: 9 PID: 1202 Comm: page-types Not tainted 5.3.0-rc1 #1
> [  114.497621] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS rel-1.11.0-0-g63451fca13-prebuilt.qemu-project.org 04/01/2014
> [  114.498706] RIP: 0010:stable_page_flags+0x27/0x3f0
> [  114.499142] Code: 82 66 90 66 66 66 66 90 48 85 ff 0f 84 d1 03 00 00 41 54 55 48 89 fd 53 48 8b 57 08 48 8b 1f 48 8d 42 ff 83 e2 01 48 0f 44 c7 <48> 8b 00 f6 c4 02 0f 84 57 03 00 00 45 31 e4 48 8b 55 08 48 89 ef
> [  114.500788] RSP: 0018:ffffa5e601a0fe60 EFLAGS: 00010202
> [  114.501373] RAX: fffffffffffffffe RBX: ffffffffffffffff RCX: 0000000000000000
> [  114.502009] RDX: 0000000000000001 RSI: 00007ffca13a7310 RDI: ffffd07489000000
> [  114.502637] RBP: ffffd07489000000 R08: 0000000000000001 R09: 0000000000000000
> [  114.503270] R10: 0000000000000000 R11: 0000000000000000 R12: 0000000000240000
> [  114.503896] R13: 0000000000080000 R14: 00007ffca13a7310 R15: ffffa5e601a0ff08
> [  114.504530] FS:  00007f0266c7f540(0000) GS:ffff962dbbac0000(0000) knlGS:0000000000000000
> [  114.505245] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [  114.505754] CR2: fffffffffffffffe CR3: 000000023a204000 CR4: 00000000000006e0
> [  114.506401] Call Trace:
> [  114.506660]  kpageflags_read+0xb1/0x130
> [  114.507051]  proc_reg_read+0x39/0x60
> [  114.507387]  vfs_read+0x8a/0x140
> [  114.507686]  ksys_pread64+0x61/0xa0
> [  114.508021]  do_syscall_64+0x5f/0x1a0
> [  114.508372]  entry_SYSCALL_64_after_hwframe+0x44/0xa9
> [  114.508844] RIP: 0033:0x7f0266ba426b
> 
> The reason for the panic is that stable_page_flags() which parses
> the page flags uses uninitialized struct pages reserved by the
> ZONE_DEVICE driver.

Why pmem hasn't initialized struct pages? Isn't that a bug that should
be addressed rather than paper over it like this?
-- 
Michal Hocko
SUSE Labs

Re: [PATCH 2/2] /proc/kpageflags: do not use uninitialized struct pages

[Toshiki Fukasawa]

On 2019/07/25 18:03, Michal Hocko wrote:
> On Thu 25-07-19 02:31:18, Toshiki Fukasawa wrote:
>> A kernel panic was observed during reading /proc/kpageflags for
>> first few pfns allocated by pmem namespace:
>>
>> BUG: unable to handle page fault for address: fffffffffffffffe
>> [  114.495280] #PF: supervisor read access in kernel mode
>> [  114.495738] #PF: error_code(0x0000) - not-present page
>> [  114.496203] PGD 17120e067 P4D 17120e067 PUD 171210067 PMD 0
>> [  114.496713] Oops: 0000 [#1] SMP PTI
>> [  114.497037] CPU: 9 PID: 1202 Comm: page-types Not tainted 5.3.0-rc1 #1
>> [  114.497621] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS rel-1.11.0-0-g63451fca13-prebuilt.qemu-project.org 04/01/2014
>> [  114.498706] RIP: 0010:stable_page_flags+0x27/0x3f0
>> [  114.499142] Code: 82 66 90 66 66 66 66 90 48 85 ff 0f 84 d1 03 00 00 41 54 55 48 89 fd 53 48 8b 57 08 48 8b 1f 48 8d 42 ff 83 e2 01 48 0f 44 c7 <48> 8b 00 f6 c4 02 0f 84 57 03 00 00 45 31 e4 48 8b 55 08 48 89 ef
>> [  114.500788] RSP: 0018:ffffa5e601a0fe60 EFLAGS: 00010202
>> [  114.501373] RAX: fffffffffffffffe RBX: ffffffffffffffff RCX: 0000000000000000
>> [  114.502009] RDX: 0000000000000001 RSI: 00007ffca13a7310 RDI: ffffd07489000000
>> [  114.502637] RBP: ffffd07489000000 R08: 0000000000000001 R09: 0000000000000000
>> [  114.503270] R10: 0000000000000000 R11: 0000000000000000 R12: 0000000000240000
>> [  114.503896] R13: 0000000000080000 R14: 00007ffca13a7310 R15: ffffa5e601a0ff08
>> [  114.504530] FS:  00007f0266c7f540(0000) GS:ffff962dbbac0000(0000) knlGS:0000000000000000
>> [  114.505245] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
>> [  114.505754] CR2: fffffffffffffffe CR3: 000000023a204000 CR4: 00000000000006e0
>> [  114.506401] Call Trace:
>> [  114.506660]  kpageflags_read+0xb1/0x130
>> [  114.507051]  proc_reg_read+0x39/0x60
>> [  114.507387]  vfs_read+0x8a/0x140
>> [  114.507686]  ksys_pread64+0x61/0xa0
>> [  114.508021]  do_syscall_64+0x5f/0x1a0
>> [  114.508372]  entry_SYSCALL_64_after_hwframe+0x44/0xa9
>> [  114.508844] RIP: 0033:0x7f0266ba426b
>>
>> The reason for the panic is that stable_page_flags() which parses
>> the page flags uses uninitialized struct pages reserved by the
>> ZONE_DEVICE driver.
> 
> Why pmem hasn't initialized struct pages? 

We proposed to initialize in previous approach but that wasn't merged.
(See https://marc.info/?l=linux-mm&m=152964792500739&w=2)

> Isn't that a bug that should be addressed rather than paper over it like this?

I'm not sure. What do you think, Dan?

Best regards,
Toshiki Fukasawa

Re: [PATCH 2/2] /proc/kpageflags: do not use uninitialized struct pages

[Michal Hocko]

On Fri 26-07-19 06:25:49, Toshiki Fukasawa wrote:
> 
> 
> On 2019/07/25 18:03, Michal Hocko wrote:
> > On Thu 25-07-19 02:31:18, Toshiki Fukasawa wrote:
> >> A kernel panic was observed during reading /proc/kpageflags for
> >> first few pfns allocated by pmem namespace:
> >>
> >> BUG: unable to handle page fault for address: fffffffffffffffe
> >> [  114.495280] #PF: supervisor read access in kernel mode
> >> [  114.495738] #PF: error_code(0x0000) - not-present page
> >> [  114.496203] PGD 17120e067 P4D 17120e067 PUD 171210067 PMD 0
> >> [  114.496713] Oops: 0000 [#1] SMP PTI
> >> [  114.497037] CPU: 9 PID: 1202 Comm: page-types Not tainted 5.3.0-rc1 #1
> >> [  114.497621] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS rel-1.11.0-0-g63451fca13-prebuilt.qemu-project.org 04/01/2014
> >> [  114.498706] RIP: 0010:stable_page_flags+0x27/0x3f0
> >> [  114.499142] Code: 82 66 90 66 66 66 66 90 48 85 ff 0f 84 d1 03 00 00 41 54 55 48 89 fd 53 48 8b 57 08 48 8b 1f 48 8d 42 ff 83 e2 01 48 0f 44 c7 <48> 8b 00 f6 c4 02 0f 84 57 03 00 00 45 31 e4 48 8b 55 08 48 89 ef
> >> [  114.500788] RSP: 0018:ffffa5e601a0fe60 EFLAGS: 00010202
> >> [  114.501373] RAX: fffffffffffffffe RBX: ffffffffffffffff RCX: 0000000000000000
> >> [  114.502009] RDX: 0000000000000001 RSI: 00007ffca13a7310 RDI: ffffd07489000000
> >> [  114.502637] RBP: ffffd07489000000 R08: 0000000000000001 R09: 0000000000000000
> >> [  114.503270] R10: 0000000000000000 R11: 0000000000000000 R12: 0000000000240000
> >> [  114.503896] R13: 0000000000080000 R14: 00007ffca13a7310 R15: ffffa5e601a0ff08
> >> [  114.504530] FS:  00007f0266c7f540(0000) GS:ffff962dbbac0000(0000) knlGS:0000000000000000
> >> [  114.505245] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> >> [  114.505754] CR2: fffffffffffffffe CR3: 000000023a204000 CR4: 00000000000006e0
> >> [  114.506401] Call Trace:
> >> [  114.506660]  kpageflags_read+0xb1/0x130
> >> [  114.507051]  proc_reg_read+0x39/0x60
> >> [  114.507387]  vfs_read+0x8a/0x140
> >> [  114.507686]  ksys_pread64+0x61/0xa0
> >> [  114.508021]  do_syscall_64+0x5f/0x1a0
> >> [  114.508372]  entry_SYSCALL_64_after_hwframe+0x44/0xa9
> >> [  114.508844] RIP: 0033:0x7f0266ba426b
> >>
> >> The reason for the panic is that stable_page_flags() which parses
> >> the page flags uses uninitialized struct pages reserved by the
> >> ZONE_DEVICE driver.
> > 
> > Why pmem hasn't initialized struct pages? 
> 
> We proposed to initialize in previous approach but that wasn't merged.
> (See https://marc.info/?l=linux-mm&m=152964792500739&w=2)
> 
> > Isn't that a bug that should be addressed rather than paper over it like this?
> 
> I'm not sure. What do you think, Dan?

Yeah, I am really curious about details. Why do we keep uninitialized
struct pages at all? What is a random pfn walker supposed to do? What
kind of metadata would be clobbered? In other words much more details
please.
-- 
Michal Hocko
SUSE Labs

Re: [PATCH 2/2] /proc/kpageflags: do not use uninitialized struct pages

[Toshiki Fukasawa]

On 2019/07/26 16:06, Michal Hocko wrote:
> On Fri 26-07-19 06:25:49, Toshiki Fukasawa wrote:
>>
>>
>> On 2019/07/25 18:03, Michal Hocko wrote:
>>> On Thu 25-07-19 02:31:18, Toshiki Fukasawa wrote:
>>>> A kernel panic was observed during reading /proc/kpageflags for
>>>> first few pfns allocated by pmem namespace:
>>>>
>>>> BUG: unable to handle page fault for address: fffffffffffffffe
>>>> [  114.495280] #PF: supervisor read access in kernel mode
>>>> [  114.495738] #PF: error_code(0x0000) - not-present page
>>>> [  114.496203] PGD 17120e067 P4D 17120e067 PUD 171210067 PMD 0
>>>> [  114.496713] Oops: 0000 [#1] SMP PTI
>>>> [  114.497037] CPU: 9 PID: 1202 Comm: page-types Not tainted 5.3.0-rc1 #1
>>>> [  114.497621] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS rel-1.11.0-0-g63451fca13-prebuilt.qemu-project.org 04/01/2014
>>>> [  114.498706] RIP: 0010:stable_page_flags+0x27/0x3f0
>>>> [  114.499142] Code: 82 66 90 66 66 66 66 90 48 85 ff 0f 84 d1 03 00 00 41 54 55 48 89 fd 53 48 8b 57 08 48 8b 1f 48 8d 42 ff 83 e2 01 48 0f 44 c7 <48> 8b 00 f6 c4 02 0f 84 57 03 00 00 45 31 e4 48 8b 55 08 48 89 ef
>>>> [  114.500788] RSP: 0018:ffffa5e601a0fe60 EFLAGS: 00010202
>>>> [  114.501373] RAX: fffffffffffffffe RBX: ffffffffffffffff RCX: 0000000000000000
>>>> [  114.502009] RDX: 0000000000000001 RSI: 00007ffca13a7310 RDI: ffffd07489000000
>>>> [  114.502637] RBP: ffffd07489000000 R08: 0000000000000001 R09: 0000000000000000
>>>> [  114.503270] R10: 0000000000000000 R11: 0000000000000000 R12: 0000000000240000
>>>> [  114.503896] R13: 0000000000080000 R14: 00007ffca13a7310 R15: ffffa5e601a0ff08
>>>> [  114.504530] FS:  00007f0266c7f540(0000) GS:ffff962dbbac0000(0000) knlGS:0000000000000000
>>>> [  114.505245] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
>>>> [  114.505754] CR2: fffffffffffffffe CR3: 000000023a204000 CR4: 00000000000006e0
>>>> [  114.506401] Call Trace:
>>>> [  114.506660]  kpageflags_read+0xb1/0x130
>>>> [  114.507051]  proc_reg_read+0x39/0x60
>>>> [  114.507387]  vfs_read+0x8a/0x140
>>>> [  114.507686]  ksys_pread64+0x61/0xa0
>>>> [  114.508021]  do_syscall_64+0x5f/0x1a0
>>>> [  114.508372]  entry_SYSCALL_64_after_hwframe+0x44/0xa9
>>>> [  114.508844] RIP: 0033:0x7f0266ba426b
>>>>
>>>> The reason for the panic is that stable_page_flags() which parses
>>>> the page flags uses uninitialized struct pages reserved by the
>>>> ZONE_DEVICE driver.
>>>
>>> Why pmem hasn't initialized struct pages?
>>
>> We proposed to initialize in previous approach but that wasn't merged.
>> (See https://marc.info/?l=linux-mm&m=152964792500739&w=2)
>>
>>> Isn't that a bug that should be addressed rather than paper over it like this?
>>
>> I'm not sure. What do you think, Dan?
> 
> Yeah, I am really curious about details. Why do we keep uninitialized
> struct pages at all? What is a random pfn walker supposed to do? What
> kind of metadata would be clobbered? In other words much more details
> please.
> 
I also want to know. I do not think that initializing struct pages will
clobber any metadata.

Best regards,
Toshiki Fukasawa

Re: [PATCH 2/2] /proc/kpageflags: do not use uninitialized struct pages

[Dan Williams]

On Sun, Aug 4, 2019 at 10:31 PM Toshiki Fukasawa
<t-fukasawa@vx.jp.nec.com> wrote:
>
> On 2019/07/26 16:06, Michal Hocko wrote:
> > On Fri 26-07-19 06:25:49, Toshiki Fukasawa wrote:
> >>
> >>
> >> On 2019/07/25 18:03, Michal Hocko wrote:
> >>> On Thu 25-07-19 02:31:18, Toshiki Fukasawa wrote:
> >>>> A kernel panic was observed during reading /proc/kpageflags for
> >>>> first few pfns allocated by pmem namespace:
> >>>>
> >>>> BUG: unable to handle page fault for address: fffffffffffffffe
> >>>> [  114.495280] #PF: supervisor read access in kernel mode
> >>>> [  114.495738] #PF: error_code(0x0000) - not-present page
> >>>> [  114.496203] PGD 17120e067 P4D 17120e067 PUD 171210067 PMD 0
> >>>> [  114.496713] Oops: 0000 [#1] SMP PTI
> >>>> [  114.497037] CPU: 9 PID: 1202 Comm: page-types Not tainted 5.3.0-rc1 #1
> >>>> [  114.497621] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS rel-1.11.0-0-g63451fca13-prebuilt.qemu-project.org 04/01/2014
> >>>> [  114.498706] RIP: 0010:stable_page_flags+0x27/0x3f0
> >>>> [  114.499142] Code: 82 66 90 66 66 66 66 90 48 85 ff 0f 84 d1 03 00 00 41 54 55 48 89 fd 53 48 8b 57 08 48 8b 1f 48 8d 42 ff 83 e2 01 48 0f 44 c7 <48> 8b 00 f6 c4 02 0f 84 57 03 00 00 45 31 e4 48 8b 55 08 48 89 ef
> >>>> [  114.500788] RSP: 0018:ffffa5e601a0fe60 EFLAGS: 00010202
> >>>> [  114.501373] RAX: fffffffffffffffe RBX: ffffffffffffffff RCX: 0000000000000000
> >>>> [  114.502009] RDX: 0000000000000001 RSI: 00007ffca13a7310 RDI: ffffd07489000000
> >>>> [  114.502637] RBP: ffffd07489000000 R08: 0000000000000001 R09: 0000000000000000
> >>>> [  114.503270] R10: 0000000000000000 R11: 0000000000000000 R12: 0000000000240000
> >>>> [  114.503896] R13: 0000000000080000 R14: 00007ffca13a7310 R15: ffffa5e601a0ff08
> >>>> [  114.504530] FS:  00007f0266c7f540(0000) GS:ffff962dbbac0000(0000) knlGS:0000000000000000
> >>>> [  114.505245] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> >>>> [  114.505754] CR2: fffffffffffffffe CR3: 000000023a204000 CR4: 00000000000006e0
> >>>> [  114.506401] Call Trace:
> >>>> [  114.506660]  kpageflags_read+0xb1/0x130
> >>>> [  114.507051]  proc_reg_read+0x39/0x60
> >>>> [  114.507387]  vfs_read+0x8a/0x140
> >>>> [  114.507686]  ksys_pread64+0x61/0xa0
> >>>> [  114.508021]  do_syscall_64+0x5f/0x1a0
> >>>> [  114.508372]  entry_SYSCALL_64_after_hwframe+0x44/0xa9
> >>>> [  114.508844] RIP: 0033:0x7f0266ba426b
> >>>>
> >>>> The reason for the panic is that stable_page_flags() which parses
> >>>> the page flags uses uninitialized struct pages reserved by the
> >>>> ZONE_DEVICE driver.
> >>>
> >>> Why pmem hasn't initialized struct pages?
> >>
> >> We proposed to initialize in previous approach but that wasn't merged.
> >> (See https://marc.info/?l=linux-mm&m=152964792500739&w=2)
> >>
> >>> Isn't that a bug that should be addressed rather than paper over it like this?
> >>
> >> I'm not sure. What do you think, Dan?
> >
> > Yeah, I am really curious about details. Why do we keep uninitialized
> > struct pages at all? What is a random pfn walker supposed to do? What
> > kind of metadata would be clobbered? In other words much more details
> > please.
> >
> I also want to know. I do not think that initializing struct pages will
> clobber any metadata.

The nvdimm implementation uses vmem_altmap to arrange for the 'struct
page' array to be allocated from a reservation of a pmem namespace. A
namespace in this mode contains an info-block that consumes the first
8K of the namespace capacity, capacity designated for page mapping,
capacity for padding the start of data to optionally 4K, 2MB, or 1GB
(on x86), and then the namespace data itself. The implementation
specifies a section aligned (now sub-section aligned) address to
arch_add_memory() to establish the linear mapping to map the metadata,
and then vmem_altmap indicates to memmap_init_zone() which pfns
represent data. The implementation only specifies enough 'struct page'
capacity for pfn_to_page() to operate on the data space, not the
namespace metadata space.

The proposal to validate ZONE_DEVICE pfns against the altmap seems the
right approach to me.

Re: [PATCH 2/2] /proc/kpageflags: do not use uninitialized struct pages

[Dan Williams]

On Wed, Jul 24, 2019 at 7:46 PM Toshiki Fukasawa
<t-fukasawa@vx.jp.nec.com> wrote:
>
> A kernel panic was observed during reading /proc/kpageflags for
> first few pfns allocated by pmem namespace:
>
> BUG: unable to handle page fault for address: fffffffffffffffe
> [  114.495280] #PF: supervisor read access in kernel mode
> [  114.495738] #PF: error_code(0x0000) - not-present page
> [  114.496203] PGD 17120e067 P4D 17120e067 PUD 171210067 PMD 0
> [  114.496713] Oops: 0000 [#1] SMP PTI
> [  114.497037] CPU: 9 PID: 1202 Comm: page-types Not tainted 5.3.0-rc1 #1
> [  114.497621] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS rel-1.11.0-0-g63451fca13-prebuilt.qemu-project.org 04/01/2014
> [  114.498706] RIP: 0010:stable_page_flags+0x27/0x3f0
> [  114.499142] Code: 82 66 90 66 66 66 66 90 48 85 ff 0f 84 d1 03 00 00 41 54 55 48 89 fd 53 48 8b 57 08 48 8b 1f 48 8d 42 ff 83 e2 01 48 0f 44 c7 <48> 8b 00 f6 c4 02 0f 84 57 03 00 00 45 31 e4 48 8b 55 08 48 89 ef
> [  114.500788] RSP: 0018:ffffa5e601a0fe60 EFLAGS: 00010202
> [  114.501373] RAX: fffffffffffffffe RBX: ffffffffffffffff RCX: 0000000000000000
> [  114.502009] RDX: 0000000000000001 RSI: 00007ffca13a7310 RDI: ffffd07489000000
> [  114.502637] RBP: ffffd07489000000 R08: 0000000000000001 R09: 0000000000000000
> [  114.503270] R10: 0000000000000000 R11: 0000000000000000 R12: 0000000000240000
> [  114.503896] R13: 0000000000080000 R14: 00007ffca13a7310 R15: ffffa5e601a0ff08
> [  114.504530] FS:  00007f0266c7f540(0000) GS:ffff962dbbac0000(0000) knlGS:0000000000000000
> [  114.505245] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [  114.505754] CR2: fffffffffffffffe CR3: 000000023a204000 CR4: 00000000000006e0
> [  114.506401] Call Trace:
> [  114.506660]  kpageflags_read+0xb1/0x130
> [  114.507051]  proc_reg_read+0x39/0x60
> [  114.507387]  vfs_read+0x8a/0x140
> [  114.507686]  ksys_pread64+0x61/0xa0
> [  114.508021]  do_syscall_64+0x5f/0x1a0
> [  114.508372]  entry_SYSCALL_64_after_hwframe+0x44/0xa9
> [  114.508844] RIP: 0033:0x7f0266ba426b
>
> The reason for the panic is that stable_page_flags() which parses
> the page flags uses uninitialized struct pages reserved by the
> ZONE_DEVICE driver.
>
> Earlier approach to fix this was discussed here:
> https://marc.info/?l=linux-mm&m=152964770000672&w=2
>
> This is another approach. To avoid using the uninitialized struct page,
> immediately return with KPF_RESERVED at the beginning of
> stable_page_flags() if the page is reserved by ZONE_DEVICE driver.
>
> Cc: stable@vger.kernel.org
> Signed-off-by: Toshiki Fukasawa <t-fukasawa@vx.jp.nec.com>
> ---
>  fs/proc/page.c           |  3 +++
>  include/linux/memremap.h |  6 ++++++
>  kernel/memremap.c        | 20 ++++++++++++++++++++
>  3 files changed, 29 insertions(+)
>
> diff --git a/fs/proc/page.c b/fs/proc/page.c
> index 69064ad..decd3fe 100644
> --- a/fs/proc/page.c
> +++ b/fs/proc/page.c
> @@ -97,6 +97,9 @@ u64 stable_page_flags(struct page *page)
>         if (!page)
>                 return BIT_ULL(KPF_NOPAGE);
>
> +       if (pfn_zone_device_reserved(page_to_pfn(page)))
> +               return BIT_ULL(KPF_RESERVED);

I think this should be KPF_NOPAGE. KPF_RESERVED implies a page is present.

> +
>         k = page->flags;
>         u = 0;
>
> diff --git a/include/linux/memremap.h b/include/linux/memremap.h
> index f8a5b2a..2cfc3c2 100644
> --- a/include/linux/memremap.h
> +++ b/include/linux/memremap.h
> @@ -124,6 +124,7 @@ static inline struct vmem_altmap *pgmap_altmap(struct dev_pagemap *pgmap)
>  }
>
>  #ifdef CONFIG_ZONE_DEVICE
> +bool pfn_zone_device_reserved(unsigned long pfn);
>  void *devm_memremap_pages(struct device *dev, struct dev_pagemap *pgmap);
>  void devm_memunmap_pages(struct device *dev, struct dev_pagemap *pgmap);
>  struct dev_pagemap *get_dev_pagemap(unsigned long pfn,
> @@ -132,6 +133,11 @@ struct dev_pagemap *get_dev_pagemap(unsigned long pfn,
>  unsigned long vmem_altmap_offset(struct vmem_altmap *altmap);
>  void vmem_altmap_free(struct vmem_altmap *altmap, unsigned long nr_pfns);
>  #else
> +static inline bool pfn_zone_device_reserved(unsigned long pfn)
> +{
> +       return false;
> +}
> +
>  static inline void *devm_memremap_pages(struct device *dev,
>                 struct dev_pagemap *pgmap)
>  {
> diff --git a/kernel/memremap.c b/kernel/memremap.c
> index 6ee03a8..bc3471c 100644
> --- a/kernel/memremap.c
> +++ b/kernel/memremap.c
> @@ -72,6 +72,26 @@ static unsigned long pfn_next(unsigned long pfn)
>         return pfn + 1;
>  }
>
> +/*
> + * This returns true if the page is reserved by ZONE_DEVICE driver.
> + */
> +bool pfn_zone_device_reserved(unsigned long pfn)
> +{
> +       struct dev_pagemap *pgmap;
> +       struct vmem_altmap *altmap;
> +       bool ret = false;
> +
> +       pgmap = get_dev_pagemap(pfn, NULL);

Ugh this will drastically slow down kpageflags_read() for all other
pfn ranges. What about burning another section flag to indicate
'device' sections so that we have a quick lookup for
pfn_is_zone_device()?

> +       if (!pgmap)
> +               return ret;

If pfn_is_zone_device() returns true than a failure to retrieve the
dev_pagemap should result in this routine returning true as well
because it means the driver hosting the device is in the process of
tearing down the mapping.

> +       altmap = pgmap_altmap(pgmap);
> +       if (altmap && pfn < (altmap->base_pfn + altmap->reserve))
> +               ret = true;
> +       put_dev_pagemap(pgmap);
> +
> +       return ret;
> +}
> +
>  #define for_each_device_pfn(pfn, map) \
>         for (pfn = pfn_first(map); pfn < pfn_end(map); pfn = pfn_next(pfn))
>
> --
> 1.8.3.1
>

Re: [PATCH 2/2] /proc/kpageflags: do not use uninitialized struct pages

[Michal Hocko]

On Mon 05-08-19 20:27:03, Dan Williams wrote:
> On Sun, Aug 4, 2019 at 10:31 PM Toshiki Fukasawa
> <t-fukasawa@vx.jp.nec.com> wrote:
> >
> > On 2019/07/26 16:06, Michal Hocko wrote:
> > > On Fri 26-07-19 06:25:49, Toshiki Fukasawa wrote:
> > >>
> > >>
> > >> On 2019/07/25 18:03, Michal Hocko wrote:
> > >>> On Thu 25-07-19 02:31:18, Toshiki Fukasawa wrote:
> > >>>> A kernel panic was observed during reading /proc/kpageflags for
> > >>>> first few pfns allocated by pmem namespace:
> > >>>>
> > >>>> BUG: unable to handle page fault for address: fffffffffffffffe
> > >>>> [  114.495280] #PF: supervisor read access in kernel mode
> > >>>> [  114.495738] #PF: error_code(0x0000) - not-present page
> > >>>> [  114.496203] PGD 17120e067 P4D 17120e067 PUD 171210067 PMD 0
> > >>>> [  114.496713] Oops: 0000 [#1] SMP PTI
> > >>>> [  114.497037] CPU: 9 PID: 1202 Comm: page-types Not tainted 5.3.0-rc1 #1
> > >>>> [  114.497621] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS rel-1.11.0-0-g63451fca13-prebuilt.qemu-project.org 04/01/2014
> > >>>> [  114.498706] RIP: 0010:stable_page_flags+0x27/0x3f0
> > >>>> [  114.499142] Code: 82 66 90 66 66 66 66 90 48 85 ff 0f 84 d1 03 00 00 41 54 55 48 89 fd 53 48 8b 57 08 48 8b 1f 48 8d 42 ff 83 e2 01 48 0f 44 c7 <48> 8b 00 f6 c4 02 0f 84 57 03 00 00 45 31 e4 48 8b 55 08 48 89 ef
> > >>>> [  114.500788] RSP: 0018:ffffa5e601a0fe60 EFLAGS: 00010202
> > >>>> [  114.501373] RAX: fffffffffffffffe RBX: ffffffffffffffff RCX: 0000000000000000
> > >>>> [  114.502009] RDX: 0000000000000001 RSI: 00007ffca13a7310 RDI: ffffd07489000000
> > >>>> [  114.502637] RBP: ffffd07489000000 R08: 0000000000000001 R09: 0000000000000000
> > >>>> [  114.503270] R10: 0000000000000000 R11: 0000000000000000 R12: 0000000000240000
> > >>>> [  114.503896] R13: 0000000000080000 R14: 00007ffca13a7310 R15: ffffa5e601a0ff08
> > >>>> [  114.504530] FS:  00007f0266c7f540(0000) GS:ffff962dbbac0000(0000) knlGS:0000000000000000
> > >>>> [  114.505245] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> > >>>> [  114.505754] CR2: fffffffffffffffe CR3: 000000023a204000 CR4: 00000000000006e0
> > >>>> [  114.506401] Call Trace:
> > >>>> [  114.506660]  kpageflags_read+0xb1/0x130
> > >>>> [  114.507051]  proc_reg_read+0x39/0x60
> > >>>> [  114.507387]  vfs_read+0x8a/0x140
> > >>>> [  114.507686]  ksys_pread64+0x61/0xa0
> > >>>> [  114.508021]  do_syscall_64+0x5f/0x1a0
> > >>>> [  114.508372]  entry_SYSCALL_64_after_hwframe+0x44/0xa9
> > >>>> [  114.508844] RIP: 0033:0x7f0266ba426b
> > >>>>
> > >>>> The reason for the panic is that stable_page_flags() which parses
> > >>>> the page flags uses uninitialized struct pages reserved by the
> > >>>> ZONE_DEVICE driver.
> > >>>
> > >>> Why pmem hasn't initialized struct pages?
> > >>
> > >> We proposed to initialize in previous approach but that wasn't merged.
> > >> (See https://marc.info/?l=linux-mm&m=152964792500739&w=2)
> > >>
> > >>> Isn't that a bug that should be addressed rather than paper over it like this?
> > >>
> > >> I'm not sure. What do you think, Dan?
> > >
> > > Yeah, I am really curious about details. Why do we keep uninitialized
> > > struct pages at all? What is a random pfn walker supposed to do? What
> > > kind of metadata would be clobbered? In other words much more details
> > > please.
> > >
> > I also want to know. I do not think that initializing struct pages will
> > clobber any metadata.
> 
> The nvdimm implementation uses vmem_altmap to arrange for the 'struct
> page' array to be allocated from a reservation of a pmem namespace. A
> namespace in this mode contains an info-block that consumes the first
> 8K of the namespace capacity, capacity designated for page mapping,
> capacity for padding the start of data to optionally 4K, 2MB, or 1GB
> (on x86), and then the namespace data itself. The implementation
> specifies a section aligned (now sub-section aligned) address to
> arch_add_memory() to establish the linear mapping to map the metadata,
> and then vmem_altmap indicates to memmap_init_zone() which pfns
> represent data. The implementation only specifies enough 'struct page'
> capacity for pfn_to_page() to operate on the data space, not the
> namespace metadata space.

Maybe I am dense but I do not really understand what prevents those
struct pages to be initialized to whatever state nvidimm subsystem
expects them to be? Is that a initialization speed up optimization?
 
> The proposal to validate ZONE_DEVICE pfns against the altmap seems the
> right approach to me.

This however means that all pfn walkers have to be aware of these
special struct pages somehow and that is error prone.

-- 
Michal Hocko
SUSE Labs

Re: [PATCH 2/2] /proc/kpageflags: do not use uninitialized struct pages

[Dan Williams]

On Mon, Aug 5, 2019 at 11:47 PM Michal Hocko <mhocko@kernel.org> wrote:
>
> On Mon 05-08-19 20:27:03, Dan Williams wrote:
> > On Sun, Aug 4, 2019 at 10:31 PM Toshiki Fukasawa
> > <t-fukasawa@vx.jp.nec.com> wrote:
> > >
> > > On 2019/07/26 16:06, Michal Hocko wrote:
> > > > On Fri 26-07-19 06:25:49, Toshiki Fukasawa wrote:
> > > >>
> > > >>
> > > >> On 2019/07/25 18:03, Michal Hocko wrote:
> > > >>> On Thu 25-07-19 02:31:18, Toshiki Fukasawa wrote:
> > > >>>> A kernel panic was observed during reading /proc/kpageflags for
> > > >>>> first few pfns allocated by pmem namespace:
> > > >>>>
> > > >>>> BUG: unable to handle page fault for address: fffffffffffffffe
> > > >>>> [  114.495280] #PF: supervisor read access in kernel mode
> > > >>>> [  114.495738] #PF: error_code(0x0000) - not-present page
> > > >>>> [  114.496203] PGD 17120e067 P4D 17120e067 PUD 171210067 PMD 0
> > > >>>> [  114.496713] Oops: 0000 [#1] SMP PTI
> > > >>>> [  114.497037] CPU: 9 PID: 1202 Comm: page-types Not tainted 5.3.0-rc1 #1
> > > >>>> [  114.497621] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS rel-1.11.0-0-g63451fca13-prebuilt.qemu-project.org 04/01/2014
> > > >>>> [  114.498706] RIP: 0010:stable_page_flags+0x27/0x3f0
> > > >>>> [  114.499142] Code: 82 66 90 66 66 66 66 90 48 85 ff 0f 84 d1 03 00 00 41 54 55 48 89 fd 53 48 8b 57 08 48 8b 1f 48 8d 42 ff 83 e2 01 48 0f 44 c7 <48> 8b 00 f6 c4 02 0f 84 57 03 00 00 45 31 e4 48 8b 55 08 48 89 ef
> > > >>>> [  114.500788] RSP: 0018:ffffa5e601a0fe60 EFLAGS: 00010202
> > > >>>> [  114.501373] RAX: fffffffffffffffe RBX: ffffffffffffffff RCX: 0000000000000000
> > > >>>> [  114.502009] RDX: 0000000000000001 RSI: 00007ffca13a7310 RDI: ffffd07489000000
> > > >>>> [  114.502637] RBP: ffffd07489000000 R08: 0000000000000001 R09: 0000000000000000
> > > >>>> [  114.503270] R10: 0000000000000000 R11: 0000000000000000 R12: 0000000000240000
> > > >>>> [  114.503896] R13: 0000000000080000 R14: 00007ffca13a7310 R15: ffffa5e601a0ff08
> > > >>>> [  114.504530] FS:  00007f0266c7f540(0000) GS:ffff962dbbac0000(0000) knlGS:0000000000000000
> > > >>>> [  114.505245] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> > > >>>> [  114.505754] CR2: fffffffffffffffe CR3: 000000023a204000 CR4: 00000000000006e0
> > > >>>> [  114.506401] Call Trace:
> > > >>>> [  114.506660]  kpageflags_read+0xb1/0x130
> > > >>>> [  114.507051]  proc_reg_read+0x39/0x60
> > > >>>> [  114.507387]  vfs_read+0x8a/0x140
> > > >>>> [  114.507686]  ksys_pread64+0x61/0xa0
> > > >>>> [  114.508021]  do_syscall_64+0x5f/0x1a0
> > > >>>> [  114.508372]  entry_SYSCALL_64_after_hwframe+0x44/0xa9
> > > >>>> [  114.508844] RIP: 0033:0x7f0266ba426b
> > > >>>>
> > > >>>> The reason for the panic is that stable_page_flags() which parses
> > > >>>> the page flags uses uninitialized struct pages reserved by the
> > > >>>> ZONE_DEVICE driver.
> > > >>>
> > > >>> Why pmem hasn't initialized struct pages?
> > > >>
> > > >> We proposed to initialize in previous approach but that wasn't merged.
> > > >> (See https://marc.info/?l=linux-mm&m=152964792500739&w=2)
> > > >>
> > > >>> Isn't that a bug that should be addressed rather than paper over it like this?
> > > >>
> > > >> I'm not sure. What do you think, Dan?
> > > >
> > > > Yeah, I am really curious about details. Why do we keep uninitialized
> > > > struct pages at all? What is a random pfn walker supposed to do? What
> > > > kind of metadata would be clobbered? In other words much more details
> > > > please.
> > > >
> > > I also want to know. I do not think that initializing struct pages will
> > > clobber any metadata.
> >
> > The nvdimm implementation uses vmem_altmap to arrange for the 'struct
> > page' array to be allocated from a reservation of a pmem namespace. A
> > namespace in this mode contains an info-block that consumes the first
> > 8K of the namespace capacity, capacity designated for page mapping,
> > capacity for padding the start of data to optionally 4K, 2MB, or 1GB
> > (on x86), and then the namespace data itself. The implementation
> > specifies a section aligned (now sub-section aligned) address to
> > arch_add_memory() to establish the linear mapping to map the metadata,
> > and then vmem_altmap indicates to memmap_init_zone() which pfns
> > represent data. The implementation only specifies enough 'struct page'
> > capacity for pfn_to_page() to operate on the data space, not the
> > namespace metadata space.
>
> Maybe I am dense but I do not really understand what prevents those
> struct pages to be initialized to whatever state nvidimm subsystem
> expects them to be? Is that a initialization speed up optimization?

No, not an optimization. If anything a regrettable choice in the
initial implementation to not reserve struct page space for the
metadata area. Certainly the kernel could fix this going forward, and
there are some configurations where even the existing allocation could
store those pfns, but there are others that need that reservation. So
there is a regression risk for some currently working configurations.

As always we could try making the reservation change and fail to
instantiate old namespaces that don't reserve enough capacity to see
who screams. I think the risk is low, but non-zero. That makes my
first choice to teach kpageflags_read() about the constraint.

> > The proposal to validate ZONE_DEVICE pfns against the altmap seems the
> > right approach to me.
>
> This however means that all pfn walkers have to be aware of these
> special struct pages somehow and that is error prone.

True, but what other blind pfn walkers do we have besides
kpageflags_read()? I expect most other pfn_to_page() code paths are
constrained to known pfns and avoid this surprise, but yes I need to
go audit those.

Re: [PATCH 2/2] /proc/kpageflags: do not use uninitialized struct pages

[Michal Hocko]

On Tue 06-08-19 09:15:25, Dan Williams wrote:
> On Mon, Aug 5, 2019 at 11:47 PM Michal Hocko <mhocko@kernel.org> wrote:
> >
> > On Mon 05-08-19 20:27:03, Dan Williams wrote:
> > > On Sun, Aug 4, 2019 at 10:31 PM Toshiki Fukasawa
> > > <t-fukasawa@vx.jp.nec.com> wrote:
> > > >
> > > > On 2019/07/26 16:06, Michal Hocko wrote:
> > > > > On Fri 26-07-19 06:25:49, Toshiki Fukasawa wrote:
> > > > >>
> > > > >>
> > > > >> On 2019/07/25 18:03, Michal Hocko wrote:
> > > > >>> On Thu 25-07-19 02:31:18, Toshiki Fukasawa wrote:
> > > > >>>> A kernel panic was observed during reading /proc/kpageflags for
> > > > >>>> first few pfns allocated by pmem namespace:
> > > > >>>>
> > > > >>>> BUG: unable to handle page fault for address: fffffffffffffffe
> > > > >>>> [  114.495280] #PF: supervisor read access in kernel mode
> > > > >>>> [  114.495738] #PF: error_code(0x0000) - not-present page
> > > > >>>> [  114.496203] PGD 17120e067 P4D 17120e067 PUD 171210067 PMD 0
> > > > >>>> [  114.496713] Oops: 0000 [#1] SMP PTI
> > > > >>>> [  114.497037] CPU: 9 PID: 1202 Comm: page-types Not tainted 5.3.0-rc1 #1
> > > > >>>> [  114.497621] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS rel-1.11.0-0-g63451fca13-prebuilt.qemu-project.org 04/01/2014
> > > > >>>> [  114.498706] RIP: 0010:stable_page_flags+0x27/0x3f0
> > > > >>>> [  114.499142] Code: 82 66 90 66 66 66 66 90 48 85 ff 0f 84 d1 03 00 00 41 54 55 48 89 fd 53 48 8b 57 08 48 8b 1f 48 8d 42 ff 83 e2 01 48 0f 44 c7 <48> 8b 00 f6 c4 02 0f 84 57 03 00 00 45 31 e4 48 8b 55 08 48 89 ef
> > > > >>>> [  114.500788] RSP: 0018:ffffa5e601a0fe60 EFLAGS: 00010202
> > > > >>>> [  114.501373] RAX: fffffffffffffffe RBX: ffffffffffffffff RCX: 0000000000000000
> > > > >>>> [  114.502009] RDX: 0000000000000001 RSI: 00007ffca13a7310 RDI: ffffd07489000000
> > > > >>>> [  114.502637] RBP: ffffd07489000000 R08: 0000000000000001 R09: 0000000000000000
> > > > >>>> [  114.503270] R10: 0000000000000000 R11: 0000000000000000 R12: 0000000000240000
> > > > >>>> [  114.503896] R13: 0000000000080000 R14: 00007ffca13a7310 R15: ffffa5e601a0ff08
> > > > >>>> [  114.504530] FS:  00007f0266c7f540(0000) GS:ffff962dbbac0000(0000) knlGS:0000000000000000
> > > > >>>> [  114.505245] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> > > > >>>> [  114.505754] CR2: fffffffffffffffe CR3: 000000023a204000 CR4: 00000000000006e0
> > > > >>>> [  114.506401] Call Trace:
> > > > >>>> [  114.506660]  kpageflags_read+0xb1/0x130
> > > > >>>> [  114.507051]  proc_reg_read+0x39/0x60
> > > > >>>> [  114.507387]  vfs_read+0x8a/0x140
> > > > >>>> [  114.507686]  ksys_pread64+0x61/0xa0
> > > > >>>> [  114.508021]  do_syscall_64+0x5f/0x1a0
> > > > >>>> [  114.508372]  entry_SYSCALL_64_after_hwframe+0x44/0xa9
> > > > >>>> [  114.508844] RIP: 0033:0x7f0266ba426b
> > > > >>>>
> > > > >>>> The reason for the panic is that stable_page_flags() which parses
> > > > >>>> the page flags uses uninitialized struct pages reserved by the
> > > > >>>> ZONE_DEVICE driver.
> > > > >>>
> > > > >>> Why pmem hasn't initialized struct pages?
> > > > >>
> > > > >> We proposed to initialize in previous approach but that wasn't merged.
> > > > >> (See https://marc.info/?l=linux-mm&m=152964792500739&w=2)
> > > > >>
> > > > >>> Isn't that a bug that should be addressed rather than paper over it like this?
> > > > >>
> > > > >> I'm not sure. What do you think, Dan?
> > > > >
> > > > > Yeah, I am really curious about details. Why do we keep uninitialized
> > > > > struct pages at all? What is a random pfn walker supposed to do? What
> > > > > kind of metadata would be clobbered? In other words much more details
> > > > > please.
> > > > >
> > > > I also want to know. I do not think that initializing struct pages will
> > > > clobber any metadata.
> > >
> > > The nvdimm implementation uses vmem_altmap to arrange for the 'struct
> > > page' array to be allocated from a reservation of a pmem namespace. A
> > > namespace in this mode contains an info-block that consumes the first
> > > 8K of the namespace capacity, capacity designated for page mapping,
> > > capacity for padding the start of data to optionally 4K, 2MB, or 1GB
> > > (on x86), and then the namespace data itself. The implementation
> > > specifies a section aligned (now sub-section aligned) address to
> > > arch_add_memory() to establish the linear mapping to map the metadata,
> > > and then vmem_altmap indicates to memmap_init_zone() which pfns
> > > represent data. The implementation only specifies enough 'struct page'
> > > capacity for pfn_to_page() to operate on the data space, not the
> > > namespace metadata space.
> >
> > Maybe I am dense but I do not really understand what prevents those
> > struct pages to be initialized to whatever state nvidimm subsystem
> > expects them to be? Is that a initialization speed up optimization?
> 
> No, not an optimization. If anything a regrettable choice in the
> initial implementation to not reserve struct page space for the
> metadata area. Certainly the kernel could fix this going forward, and
> there are some configurations where even the existing allocation could
> store those pfns, but there are others that need that reservation. So
> there is a regression risk for some currently working configurations.
> 
> As always we could try making the reservation change and fail to
> instantiate old namespaces that don't reserve enough capacity to see
> who screams. I think the risk is low, but non-zero. That makes my
> first choice to teach kpageflags_read() about the constraint.

Thanks for the explanation!

> > > The proposal to validate ZONE_DEVICE pfns against the altmap seems the
> > > right approach to me.
> >
> > This however means that all pfn walkers have to be aware of these
> > special struct pages somehow and that is error prone.
> 
> True, but what other blind pfn walkers do we have besides
> kpageflags_read()? I expect most other pfn_to_page() code paths are
> constrained to known pfns and avoid this surprise, but yes I need to
> go audit those.

Well, most pfn walkers in the MM code do go within a zone boundary. Many
check also the zone to ensure interleaving zones are handled properly. I
hope that these special zone device ranges are not going to interleave
with other normal zones. But as always having a subtle land mine like
this is really not nice. All valid pfns should have a real and
initialized struct pages.

-- 
Michal Hocko
SUSE Labs