[PATCH 0/4] arch, mm: improve robustness of direct map manipulation

From: Mike Rapoport <redacted>

Hi,

During recent discussion about KVM protected memory, David raised a concern
about usage of __kernel_map_pages() outside of DEBUG_PAGEALLOC scope [1].

Indeed, for architectures that define CONFIG_ARCH_HAS_SET_DIRECT_MAP it is
possible that __kernel_map_pages() would fail, but since this function is
void, the failure will go unnoticed.

Moreover, there's lack of consistency of __kernel_map_pages() semantics
across architectures as some guard this function with
#ifdef DEBUG_PAGEALLOC, some refuse to update the direct map if page
allocation debugging is disabled at run time and some allow modifying the
direct map regardless of DEBUG_PAGEALLOC settings.

This set straightens this out by restoring dependency of
__kernel_map_pages() on DEBUG_PAGEALLOC and updating the call sites
accordingly. 

[1] https://lore.kernel.org/lkml/2759b4bf-e1e3-d006-7d86-78a40348269d@redhat.com

Mike Rapoport (4):
  mm: introduce debug_pagealloc_map_pages() helper
  PM: hibernate: improve robustness of mapping pages in the direct map
  arch, mm: restore dependency of __kernel_map_pages() of DEBUG_PAGEALLOC
  arch, mm: make kernel_page_present() always available

 arch/Kconfig                        |  3 +++
 arch/arm64/Kconfig                  |  4 +---
 arch/arm64/include/asm/cacheflush.h |  1 +
 arch/arm64/mm/pageattr.c            |  6 +++--
 arch/powerpc/Kconfig                |  5 +----
 arch/riscv/Kconfig                  |  4 +---
 arch/riscv/include/asm/pgtable.h    |  2 --
 arch/riscv/include/asm/set_memory.h |  1 +
 arch/riscv/mm/pageattr.c            | 31 +++++++++++++++++++++++++
 arch/s390/Kconfig                   |  4 +---
 arch/sparc/Kconfig                  |  4 +---

14 more lines

From: Mike Rapoport <redacted>

When CONFIG_DEBUG_PAGEALLOC is enabled, it unmaps pages from the kernel
direct mapping after free_pages(). The pages than need to be mapped back
before they could be used. Theese mapping operations use
__kernel_map_pages() guarded with with debug_pagealloc_enabled().

The only place that calls __kernel_map_pages() without checking whether
DEBUG_PAGEALLOC is enabled is the hibernation code that presumes
availability of this function when ARCH_HAS_SET_DIRECT_MAP is set.
Still, on arm64, __kernel_map_pages() will bail out when DEBUG_PAGEALLOC is
not enabled but set_direct_map_invalid_noflush() may render some pages not
present in the direct map and hibernation code won't be able to save such
pages.

To make page allocation debugging and hibernation interaction more robust,
the dependency on DEBUG_PAGEALLOC or ARCH_HAS_SET_DIRECT_MAP has to be made
more explicit.

Start with combining the guard condition and the call to
__kernel_map_pages() into a single debug_pagealloc_map_pages() function to
emphasize that __kernel_map_pages() should not be called without
DEBUG_PAGEALLOC and use this new function to map/unmap pages when page
allocation debug is enabled.

As the only remaining user of kernel_map_pages() is the hibernation code,
mode that function into kernel/power/snapshot.c closer to a caller.

Signed-off-by: Mike Rapoport <redacted>
---
 include/linux/mm.h      | 16 +++++++---------
 kernel/power/snapshot.c | 11 +++++++++++
 mm/memory_hotplug.c     |  3 +--
 mm/page_alloc.c         |  6 ++----
 mm/slab.c               |  8 +++-----
 5 files changed, 24 insertions(+), 20 deletions(-)

diff --git a/include/linux/mm.h b/include/linux/mm.h
index ef360fe70aaf..14e397f3752c 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h

@@ -2927,21 +2927,19 @@ static inline bool debug_pagealloc_enabled_static(void)
 #if defined(CONFIG_DEBUG_PAGEALLOC) || defined(CONFIG_ARCH_HAS_SET_DIRECT_MAP)
 extern void __kernel_map_pages(struct page *page, int numpages, int enable);
 
-/*
- * When called in DEBUG_PAGEALLOC context, the call should most likely be
- * guarded by debug_pagealloc_enabled() or debug_pagealloc_enabled_static()
- */
-static inline void
-kernel_map_pages(struct page *page, int numpages, int enable)
+static inline void debug_pagealloc_map_pages(struct page *page,
+					     int numpages, int enable)
 {
-	__kernel_map_pages(page, numpages, enable);
+	if (debug_pagealloc_enabled_static())
+		__kernel_map_pages(page, numpages, enable);
 }
+
 #ifdef CONFIG_HIBERNATION
 extern bool kernel_page_present(struct page *page);
 #endif	/* CONFIG_HIBERNATION */
 #else	/* CONFIG_DEBUG_PAGEALLOC || CONFIG_ARCH_HAS_SET_DIRECT_MAP */
-static inline void
-kernel_map_pages(struct page *page, int numpages, int enable) {}
+static inline void debug_pagealloc_map_pages(struct page *page,
+					     int numpages, int enable) {}
 #ifdef CONFIG_HIBERNATION
 static inline bool kernel_page_present(struct page *page) { return true; }
 #endif	/* CONFIG_HIBERNATION */

diff --git a/kernel/power/snapshot.c b/kernel/power/snapshot.c
index 46b1804c1ddf..fa499466f645 100644
--- a/kernel/power/snapshot.c
+++ b/kernel/power/snapshot.c

@@ -76,6 +76,17 @@ static inline void hibernate_restore_protect_page(void *page_address) {}
 static inline void hibernate_restore_unprotect_page(void *page_address) {}
 #endif /* CONFIG_STRICT_KERNEL_RWX  && CONFIG_ARCH_HAS_SET_MEMORY */
 
+#if defined(CONFIG_DEBUG_PAGEALLOC) || defined(CONFIG_ARCH_HAS_SET_DIRECT_MAP)
+static inline void
+kernel_map_pages(struct page *page, int numpages, int enable)
+{
+	__kernel_map_pages(page, numpages, enable);
+}
+#else
+static inline void
+kernel_map_pages(struct page *page, int numpages, int enable) {}
+#endif
+
 static int swsusp_page_is_free(struct page *);
 static void swsusp_set_page_forbidden(struct page *);
 static void swsusp_unset_page_forbidden(struct page *);

diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
index b44d4c7ba73b..e2b6043a4428 100644
--- a/mm/memory_hotplug.c
+++ b/mm/memory_hotplug.c

@@ -614,8 +614,7 @@ void generic_online_page(struct page *page, unsigned int order)
 	 * so we should map it first. This is better than introducing a special
 	 * case in page freeing fast path.
 	 */
-	if (debug_pagealloc_enabled_static())
-		kernel_map_pages(page, 1 << order, 1);
+	debug_pagealloc_map_pages(page, 1 << order, 1);
 	__free_pages_core(page, order);
 	totalram_pages_add(1UL << order);
 #ifdef CONFIG_HIGHMEM

diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index 23f5066bd4a5..9a66a1ff9193 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c

@@ -1272,8 +1272,7 @@ static __always_inline bool free_pages_prepare(struct page *page,
 	 */
 	arch_free_page(page, order);
 
-	if (debug_pagealloc_enabled_static())
-		kernel_map_pages(page, 1 << order, 0);
+	debug_pagealloc_map_pages(page, 1 << order, 0);
 
 	kasan_free_nondeferred_pages(page, order);
 

@@ -2270,8 +2269,7 @@ inline void post_alloc_hook(struct page *page, unsigned int order,
 	set_page_refcounted(page);
 
 	arch_alloc_page(page, order);
-	if (debug_pagealloc_enabled_static())
-		kernel_map_pages(page, 1 << order, 1);
+	debug_pagealloc_map_pages(page, 1 << order, 1);
 	kasan_alloc_pages(page, order);
 	kernel_poison_pages(page, 1 << order, 1);
 	set_page_owner(page, order, gfp_flags);

diff --git a/mm/slab.c b/mm/slab.c
index b1113561b98b..340db0ce74c4 100644
--- a/mm/slab.c
+++ b/mm/slab.c

@@ -1431,10 +1431,8 @@ static bool is_debug_pagealloc_cache(struct kmem_cache *cachep)
 #ifdef CONFIG_DEBUG_PAGEALLOC
 static void slab_kernel_map(struct kmem_cache *cachep, void *objp, int map)
 {
-	if (!is_debug_pagealloc_cache(cachep))
-		return;
-
-	kernel_map_pages(virt_to_page(objp), cachep->size / PAGE_SIZE, map);
+	debug_pagealloc_map_pages(virt_to_page(objp),
+				  cachep->size / PAGE_SIZE, map);
 }
 
 #else

@@ -2062,7 +2060,7 @@ int __kmem_cache_create(struct kmem_cache *cachep, slab_flags_t flags)
 
 #if DEBUG
 	/*
-	 * If we're going to use the generic kernel_map_pages()
+	 * If we're going to use the generic debug_pagealloc_map_pages()
 	 * poisoning, then it's going to smash the contents of
 	 * the redzone and userword anyhow, so switch them off.
 	 */

-- 
2.28.0

From: Mike Rapoport <redacted>

When DEBUG_PAGEALLOC or ARCH_HAS_SET_DIRECT_MAP is enabled a page may be
not present in the direct map and has to be explicitly mapped before it
could be copied.

On arm64 it is possible that a page would be removed from the direct map
using set_direct_map_invalid_noflush() but __kernel_map_pages() will refuse
to map this page back if DEBUG_PAGEALLOC is disabled.

Explicitly use set_direct_map_{default,invalid}_noflush() for
ARCH_HAS_SET_DIRECT_MAP case and debug_pagealloc_map_pages() for
DEBUG_PAGEALLOC case.

While on that, rename kernel_map_pages() to hibernate_map_page() and drop
numpages parameter.

Signed-off-by: Mike Rapoport <redacted>
---
 kernel/power/snapshot.c | 29 +++++++++++++++++++----------
 1 file changed, 19 insertions(+), 10 deletions(-)

diff --git a/kernel/power/snapshot.c b/kernel/power/snapshot.c
index fa499466f645..ecb7b32ce77c 100644
--- a/kernel/power/snapshot.c
+++ b/kernel/power/snapshot.c

@@ -76,16 +76,25 @@ static inline void hibernate_restore_protect_page(void *page_address) {}
 static inline void hibernate_restore_unprotect_page(void *page_address) {}
 #endif /* CONFIG_STRICT_KERNEL_RWX  && CONFIG_ARCH_HAS_SET_MEMORY */
 
-#if defined(CONFIG_DEBUG_PAGEALLOC) || defined(CONFIG_ARCH_HAS_SET_DIRECT_MAP)
-static inline void
-kernel_map_pages(struct page *page, int numpages, int enable)
+static inline void hibernate_map_page(struct page *page, int enable)
 {
-	__kernel_map_pages(page, numpages, enable);
+	if (IS_ENABLED(CONFIG_ARCH_HAS_SET_DIRECT_MAP)) {
+		unsigned long addr = (unsigned long)page_address(page);
+		int ret;
+
+		if (enable)
+			ret = set_direct_map_default_noflush(page);
+		else
+			ret = set_direct_map_invalid_noflush(page);
+
+		if (WARN_ON(ret))
+			return;
+
+		flush_tlb_kernel_range(addr, addr + PAGE_SIZE);
+	} else {
+		debug_pagealloc_map_pages(page, 1, enable);
+	}
 }
-#else
-static inline void
-kernel_map_pages(struct page *page, int numpages, int enable) {}
-#endif
 
 static int swsusp_page_is_free(struct page *);
 static void swsusp_set_page_forbidden(struct page *);

@@ -1366,9 +1375,9 @@ static void safe_copy_page(void *dst, struct page *s_page)
 	if (kernel_page_present(s_page)) {
 		do_copy_page(dst, page_address(s_page));
 	} else {
-		kernel_map_pages(s_page, 1, 1);
+		hibernate_map_page(s_page, 1);
 		do_copy_page(dst, page_address(s_page));
-		kernel_map_pages(s_page, 1, 0);
+		hibernate_map_page(s_page, 0);
 	}
 }
 

-- 
2.28.0

From: Mike Rapoport <redacted>

The design of DEBUG_PAGEALLOC presumes that __kernel_map_pages() must never
fail. With this assumption is wouldn't be safe to allow general usage of
this function.

Moreover, some architectures that implement __kernel_map_pages() have this
function guarded by #ifdef DEBUG_PAGEALLOC and some refuse to map/unmap
pages when page allocation debugging is disabled at runtime.

As all the users of __kernel_map_pages() were converted to use
debug_pagealloc_map_pages() it is safe to make it available only when
DEBUG_PAGEALLOC is set.

Signed-off-by: Mike Rapoport <redacted>
---
 arch/Kconfig                     |  3 +++
 arch/arm64/Kconfig               |  4 +---
 arch/arm64/mm/pageattr.c         |  6 ++++--
 arch/powerpc/Kconfig             |  5 +----
 arch/riscv/Kconfig               |  4 +---
 arch/riscv/include/asm/pgtable.h |  2 --
 arch/riscv/mm/pageattr.c         |  2 ++
 arch/s390/Kconfig                |  4 +---
 arch/sparc/Kconfig               |  4 +---
 arch/x86/Kconfig                 |  4 +---
 arch/x86/mm/pat/set_memory.c     |  2 ++
 include/linux/mm.h               | 10 +++++++---
 12 files changed, 24 insertions(+), 26 deletions(-)

diff --git a/arch/Kconfig b/arch/Kconfig
index 56b6ccc0e32d..56d4752b6db6 100644
--- a/arch/Kconfig
+++ b/arch/Kconfig

@@ -1028,6 +1028,9 @@ config HAVE_STATIC_CALL_INLINE
 	bool
 	depends on HAVE_STATIC_CALL
 
+config ARCH_SUPPORTS_DEBUG_PAGEALLOC
+	bool
+
 source "kernel/gcov/Kconfig"
 
 source "scripts/gcc-plugins/Kconfig"

diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
index 08fa3a1c50f0..1d4da0843668 100644
--- a/arch/arm64/Kconfig
+++ b/arch/arm64/Kconfig

@@ -71,6 +71,7 @@ config ARM64
 	select ARCH_USE_QUEUED_RWLOCKS
 	select ARCH_USE_QUEUED_SPINLOCKS
 	select ARCH_USE_SYM_ANNOTATIONS
+	select ARCH_SUPPORTS_DEBUG_PAGEALLOC
 	select ARCH_SUPPORTS_MEMORY_FAILURE
 	select ARCH_SUPPORTS_SHADOW_CALL_STACK if CC_HAVE_SHADOW_CALL_STACK
 	select ARCH_SUPPORTS_ATOMIC_RMW

@@ -1004,9 +1005,6 @@ config HOLES_IN_ZONE
 
 source "kernel/Kconfig.hz"
 
-config ARCH_SUPPORTS_DEBUG_PAGEALLOC
-	def_bool y
-
 config ARCH_SPARSEMEM_ENABLE
 	def_bool y
 	select SPARSEMEM_VMEMMAP_ENABLE

diff --git a/arch/arm64/mm/pageattr.c b/arch/arm64/mm/pageattr.c
index 1b94f5b82654..18613d8834db 100644
--- a/arch/arm64/mm/pageattr.c
+++ b/arch/arm64/mm/pageattr.c

@@ -178,13 +178,15 @@ int set_direct_map_default_noflush(struct page *page)
 				   PAGE_SIZE, change_page_range, &data);
 }
 
+#ifdef CONFIG_DEBUG_PAGEALLOC
 void __kernel_map_pages(struct page *page, int numpages, int enable)
 {
-	if (!debug_pagealloc_enabled() && !rodata_full)
+	if (!rodata_full)
 		return;
 
 	set_memory_valid((unsigned long)page_address(page), numpages, enable);
 }
+#endif /* CONFIG_DEBUG_PAGEALLOC */
 
 /*
  * This function is used to determine if a linear map page has been marked as

@@ -204,7 +206,7 @@ bool kernel_page_present(struct page *page)
 	pte_t *ptep;
 	unsigned long addr = (unsigned long)page_address(page);
 
-	if (!debug_pagealloc_enabled() && !rodata_full)
+	if (!rodata_full)
 		return true;
 
 	pgdp = pgd_offset_k(addr);

diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
index e9f13fe08492..ad8a83f3ddca 100644
--- a/arch/powerpc/Kconfig
+++ b/arch/powerpc/Kconfig

@@ -146,6 +146,7 @@ config PPC
 	select ARCH_MIGHT_HAVE_PC_SERIO
 	select ARCH_OPTIONAL_KERNEL_RWX		if ARCH_HAS_STRICT_KERNEL_RWX
 	select ARCH_SUPPORTS_ATOMIC_RMW
+	select ARCH_SUPPORTS_DEBUG_PAGEALLOC	if PPC32 || PPC_BOOK3S_64
 	select ARCH_USE_BUILTIN_BSWAP
 	select ARCH_USE_CMPXCHG_LOCKREF		if PPC64
 	select ARCH_USE_QUEUED_RWLOCKS		if PPC_QUEUED_SPINLOCKS

@@ -355,10 +356,6 @@ config PPC_OF_PLATFORM_PCI
 	depends on PCI
 	depends on PPC64 # not supported on 32 bits yet
 
-config ARCH_SUPPORTS_DEBUG_PAGEALLOC
-	depends on PPC32 || PPC_BOOK3S_64
-	def_bool y
-
 config ARCH_SUPPORTS_UPROBES
 	def_bool y
 

diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
index d5e7ca08f22c..c704562ba45e 100644
--- a/arch/riscv/Kconfig
+++ b/arch/riscv/Kconfig

@@ -14,6 +14,7 @@ config RISCV
 	def_bool y
 	select ARCH_CLOCKSOURCE_INIT
 	select ARCH_SUPPORTS_ATOMIC_RMW
+	select ARCH_SUPPORTS_DEBUG_PAGEALLOC if MMU
 	select ARCH_HAS_BINFMT_FLAT
 	select ARCH_HAS_DEBUG_VM_PGTABLE
 	select ARCH_HAS_DEBUG_VIRTUAL if MMU

@@ -153,9 +154,6 @@ config ARCH_SELECT_MEMORY_MODEL
 config ARCH_WANT_GENERAL_HUGETLB
 	def_bool y
 
-config ARCH_SUPPORTS_DEBUG_PAGEALLOC
-	def_bool y
-
 config SYS_SUPPORTS_HUGETLBFS
 	depends on MMU
 	def_bool y

diff --git a/arch/riscv/include/asm/pgtable.h b/arch/riscv/include/asm/pgtable.h
index 183f1f4b2ae6..41a72861987c 100644
--- a/arch/riscv/include/asm/pgtable.h
+++ b/arch/riscv/include/asm/pgtable.h

@@ -461,8 +461,6 @@ static inline int ptep_clear_flush_young(struct vm_area_struct *vma,
 #define VMALLOC_START		0
 #define VMALLOC_END		TASK_SIZE
 
-static inline void __kernel_map_pages(struct page *page, int numpages, int enable) {}
-
 #endif /* !CONFIG_MMU */
 
 #define kern_addr_valid(addr)   (1) /* FIXME */

diff --git a/arch/riscv/mm/pageattr.c b/arch/riscv/mm/pageattr.c
index 19fecb362d81..321b09d2e2ea 100644
--- a/arch/riscv/mm/pageattr.c
+++ b/arch/riscv/mm/pageattr.c

@@ -184,6 +184,7 @@ int set_direct_map_default_noflush(struct page *page)
 	return ret;
 }
 
+#ifdef CONFIG_DEBUG_PAGEALLOC
 void __kernel_map_pages(struct page *page, int numpages, int enable)
 {
 	if (!debug_pagealloc_enabled())

@@ -196,3 +197,4 @@ void __kernel_map_pages(struct page *page, int numpages, int enable)
 		__set_memory((unsigned long)page_address(page), numpages,
 			     __pgprot(0), __pgprot(_PAGE_PRESENT));
 }
+#endif

diff --git a/arch/s390/Kconfig b/arch/s390/Kconfig
index 34371539a9b9..0a42d457bff4 100644
--- a/arch/s390/Kconfig
+++ b/arch/s390/Kconfig

@@ -35,9 +35,6 @@ config GENERIC_LOCKBREAK
 config PGSTE
 	def_bool y if KVM
 
-config ARCH_SUPPORTS_DEBUG_PAGEALLOC
-	def_bool y
-
 config AUDIT_ARCH
 	def_bool y
 

@@ -106,6 +103,7 @@ config S390
 	select ARCH_INLINE_WRITE_UNLOCK_IRQRESTORE
 	select ARCH_STACKWALK
 	select ARCH_SUPPORTS_ATOMIC_RMW
+	select ARCH_SUPPORTS_DEBUG_PAGEALLOC
 	select ARCH_SUPPORTS_NUMA_BALANCING
 	select ARCH_USE_BUILTIN_BSWAP
 	select ARCH_USE_CMPXCHG_LOCKREF

diff --git a/arch/sparc/Kconfig b/arch/sparc/Kconfig
index a6ca135442f9..2c729b8d097a 100644
--- a/arch/sparc/Kconfig
+++ b/arch/sparc/Kconfig

@@ -88,6 +88,7 @@ config SPARC64
 	select HAVE_C_RECORDMCOUNT
 	select HAVE_ARCH_AUDITSYSCALL
 	select ARCH_SUPPORTS_ATOMIC_RMW
+	select ARCH_SUPPORTS_DEBUG_PAGEALLOC
 	select HAVE_NMI
 	select HAVE_REGS_AND_STACK_ACCESS_API
 	select ARCH_USE_QUEUED_RWLOCKS

@@ -148,9 +149,6 @@ config GENERIC_ISA_DMA
 	bool
 	default y if SPARC32
 
-config ARCH_SUPPORTS_DEBUG_PAGEALLOC
-	def_bool y if SPARC64
-
 config PGTABLE_LEVELS
 	default 4 if 64BIT
 	default 3

diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index f6946b81f74a..0db3fb1da70c 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig

@@ -91,6 +91,7 @@ config X86
 	select ARCH_STACKWALK
 	select ARCH_SUPPORTS_ACPI
 	select ARCH_SUPPORTS_ATOMIC_RMW
+	select ARCH_SUPPORTS_DEBUG_PAGEALLOC
 	select ARCH_SUPPORTS_NUMA_BALANCING	if X86_64
 	select ARCH_USE_BUILTIN_BSWAP
 	select ARCH_USE_QUEUED_RWLOCKS

@@ -329,9 +330,6 @@ config ZONE_DMA32
 config AUDIT_ARCH
 	def_bool y if X86_64
 
-config ARCH_SUPPORTS_DEBUG_PAGEALLOC
-	def_bool y
-
 config KASAN_SHADOW_OFFSET
 	hex
 	depends on KASAN

diff --git a/arch/x86/mm/pat/set_memory.c b/arch/x86/mm/pat/set_memory.c
index 40baa90e74f4..7f248fc45317 100644
--- a/arch/x86/mm/pat/set_memory.c
+++ b/arch/x86/mm/pat/set_memory.c

@@ -2194,6 +2194,7 @@ int set_direct_map_default_noflush(struct page *page)
 	return __set_pages_p(page, 1);
 }
 
+#ifdef CONFIG_DEBUG_PAGEALLOC
 void __kernel_map_pages(struct page *page, int numpages, int enable)
 {
 	if (PageHighMem(page))

@@ -2225,6 +2226,7 @@ void __kernel_map_pages(struct page *page, int numpages, int enable)
 
 	arch_flush_lazy_mmu_mode();
 }
+#endif /* CONFIG_DEBUG_PAGEALLOC */
 
 #ifdef CONFIG_HIBERNATION
 bool kernel_page_present(struct page *page)

diff --git a/include/linux/mm.h b/include/linux/mm.h
index 14e397f3752c..ab0ef6bd351d 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h

@@ -2924,7 +2924,11 @@ static inline bool debug_pagealloc_enabled_static(void)
 	return static_branch_unlikely(&_debug_pagealloc_enabled);
 }
 
-#if defined(CONFIG_DEBUG_PAGEALLOC) || defined(CONFIG_ARCH_HAS_SET_DIRECT_MAP)
+#ifdef CONFIG_DEBUG_PAGEALLOC
+/*
+ * To support DEBUG_PAGEALLOC architecture must ensure that
+ * __kernel_map_pages() never fails
+ */
 extern void __kernel_map_pages(struct page *page, int numpages, int enable);
 
 static inline void debug_pagealloc_map_pages(struct page *page,

@@ -2937,13 +2941,13 @@ static inline void debug_pagealloc_map_pages(struct page *page,
 #ifdef CONFIG_HIBERNATION
 extern bool kernel_page_present(struct page *page);
 #endif	/* CONFIG_HIBERNATION */
-#else	/* CONFIG_DEBUG_PAGEALLOC || CONFIG_ARCH_HAS_SET_DIRECT_MAP */
+#else	/* CONFIG_DEBUG_PAGEALLOC */
 static inline void debug_pagealloc_map_pages(struct page *page,
 					     int numpages, int enable) {}
 #ifdef CONFIG_HIBERNATION
 static inline bool kernel_page_present(struct page *page) { return true; }
 #endif	/* CONFIG_HIBERNATION */
-#endif	/* CONFIG_DEBUG_PAGEALLOC || CONFIG_ARCH_HAS_SET_DIRECT_MAP */
+#endif	/* CONFIG_DEBUG_PAGEALLOC */
 
 #ifdef __HAVE_ARCH_GATE_AREA
 extern struct vm_area_struct *get_gate_vma(struct mm_struct *mm);

-- 
2.28.0

From: Mike Rapoport <redacted>

For architectures that enable ARCH_HAS_SET_MEMORY having the ability to
verify that a page is mapped in the kernel direct map can be useful
regardless of hibernation.

Add RISC-V implementation of kernel_page_present() and update its forward
declarations and stubs to be a part of set_memory API.

Signed-off-by: Mike Rapoport <redacted>
---
 arch/arm64/include/asm/cacheflush.h |  1 +
 arch/riscv/include/asm/set_memory.h |  1 +
 arch/riscv/mm/pageattr.c            | 29 +++++++++++++++++++++++++++++
 arch/x86/include/asm/set_memory.h   |  1 +
 arch/x86/mm/pat/set_memory.c        |  2 --
 include/linux/mm.h                  |  7 -------
 include/linux/set_memory.h          |  5 +++++
 7 files changed, 37 insertions(+), 9 deletions(-)

diff --git a/arch/arm64/include/asm/cacheflush.h b/arch/arm64/include/asm/cacheflush.h
index 9384fd8fc13c..45217f21f1fe 100644
--- a/arch/arm64/include/asm/cacheflush.h
+++ b/arch/arm64/include/asm/cacheflush.h

@@ -140,6 +140,7 @@ int set_memory_valid(unsigned long addr, int numpages, int enable);
 
 int set_direct_map_invalid_noflush(struct page *page);
 int set_direct_map_default_noflush(struct page *page);
+bool kernel_page_present(struct page *page);
 
 #include <asm-generic/cacheflush.h>
 

diff --git a/arch/riscv/include/asm/set_memory.h b/arch/riscv/include/asm/set_memory.h
index 4c5bae7ca01c..d690b08dff2a 100644
--- a/arch/riscv/include/asm/set_memory.h
+++ b/arch/riscv/include/asm/set_memory.h

@@ -24,6 +24,7 @@ static inline int set_memory_nx(unsigned long addr, int numpages) { return 0; }
 
 int set_direct_map_invalid_noflush(struct page *page);
 int set_direct_map_default_noflush(struct page *page);
+bool kernel_page_present(struct page *page);
 
 #endif /* __ASSEMBLY__ */
 

diff --git a/arch/riscv/mm/pageattr.c b/arch/riscv/mm/pageattr.c
index 321b09d2e2ea..87ba5a68bbb8 100644
--- a/arch/riscv/mm/pageattr.c
+++ b/arch/riscv/mm/pageattr.c

@@ -198,3 +198,32 @@ void __kernel_map_pages(struct page *page, int numpages, int enable)
 			     __pgprot(0), __pgprot(_PAGE_PRESENT));
 }
 #endif
+
+bool kernel_page_present(struct page *page)
+{
+	unsigned long addr = (unsigned long)page_address(page);
+	pgd_t *pgd;
+	pud_t *pud;
+	p4d_t *p4d;
+	pmd_t *pmd;
+	pte_t *pte;
+
+	pgd = pgd_offset_k(addr);
+	if (!pgd_present(*pgd))
+		return false;
+
+	p4d = p4d_offset(pgd, addr);
+	if (!p4d_present(*p4d))
+		return false;
+
+	pud = pud_offset(p4d, addr);
+	if (!pud_present(*pud))
+		return false;
+
+	pmd = pmd_offset(pud, addr);
+	if (!pmd_present(*pmd))
+		return false;
+
+	pte = pte_offset_kernel(pmd, addr);
+	return pte_present(*pte);
+}

diff --git a/arch/x86/include/asm/set_memory.h b/arch/x86/include/asm/set_memory.h
index 5948218f35c5..4352f08bfbb5 100644
--- a/arch/x86/include/asm/set_memory.h
+++ b/arch/x86/include/asm/set_memory.h

@@ -82,6 +82,7 @@ int set_pages_rw(struct page *page, int numpages);
 
 int set_direct_map_invalid_noflush(struct page *page);
 int set_direct_map_default_noflush(struct page *page);
+bool kernel_page_present(struct page *page);
 
 extern int kernel_set_to_readonly;
 

diff --git a/arch/x86/mm/pat/set_memory.c b/arch/x86/mm/pat/set_memory.c
index 7f248fc45317..16f878c26667 100644
--- a/arch/x86/mm/pat/set_memory.c
+++ b/arch/x86/mm/pat/set_memory.c

@@ -2228,7 +2228,6 @@ void __kernel_map_pages(struct page *page, int numpages, int enable)
 }
 #endif /* CONFIG_DEBUG_PAGEALLOC */
 
-#ifdef CONFIG_HIBERNATION
 bool kernel_page_present(struct page *page)
 {
 	unsigned int level;

@@ -2240,7 +2239,6 @@ bool kernel_page_present(struct page *page)
 	pte = lookup_address((unsigned long)page_address(page), &level);
 	return (pte_val(*pte) & _PAGE_PRESENT);
 }
-#endif /* CONFIG_HIBERNATION */
 
 int __init kernel_map_pages_in_pgd(pgd_t *pgd, u64 pfn, unsigned long address,
 				   unsigned numpages, unsigned long page_flags)

diff --git a/include/linux/mm.h b/include/linux/mm.h
index ab0ef6bd351d..44b82f22e76a 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h

@@ -2937,16 +2937,9 @@ static inline void debug_pagealloc_map_pages(struct page *page,
 	if (debug_pagealloc_enabled_static())
 		__kernel_map_pages(page, numpages, enable);
 }
-
-#ifdef CONFIG_HIBERNATION
-extern bool kernel_page_present(struct page *page);
-#endif	/* CONFIG_HIBERNATION */
 #else	/* CONFIG_DEBUG_PAGEALLOC */
 static inline void debug_pagealloc_map_pages(struct page *page,
 					     int numpages, int enable) {}
-#ifdef CONFIG_HIBERNATION
-static inline bool kernel_page_present(struct page *page) { return true; }
-#endif	/* CONFIG_HIBERNATION */
 #endif	/* CONFIG_DEBUG_PAGEALLOC */
 
 #ifdef __HAVE_ARCH_GATE_AREA

diff --git a/include/linux/set_memory.h b/include/linux/set_memory.h
index 860e0f843c12..fe1aa4e54680 100644
--- a/include/linux/set_memory.h
+++ b/include/linux/set_memory.h

@@ -23,6 +23,11 @@ static inline int set_direct_map_default_noflush(struct page *page)
 {
 	return 0;
 }
+
+static inline bool kernel_page_present(struct page *page)
+{
+	return true;
+}
 #endif
 
 #ifndef set_mce_nospec

-- 
2.28.0

On Sun, 2020-10-25 at 12:15 +0200, Mike Rapoport wrote:

From: Mike Rapoport <redacted>

When DEBUG_PAGEALLOC or ARCH_HAS_SET_DIRECT_MAP is enabled a page may
be
not present in the direct map and has to be explicitly mapped before
it
could be copied.

On arm64 it is possible that a page would be removed from the direct
map
using set_direct_map_invalid_noflush() but __kernel_map_pages() will
refuse
to map this page back if DEBUG_PAGEALLOC is disabled.

It looks to me that arm64 __kernel_map_pages() will still attempt to
map it if rodata_full is true, how does this happen?

27 more quoted lines

If somehow a page was unmapped such that
set_direct_map_default_noflush() would fail, then this code introduces
a WARN, but it will still try to read the unmapped page. Why not just
have the WARN's inside of __kernel_map_pages() if they fail and then
have a warning for the debug page alloc cases as well? Since logic
around both expects them not to fail.

On Sun, 2020-10-25 at 12:15 +0200, Mike Rapoport wrote:

This is only used by hibernate today right? Makes sense that it should
return a correct answer if someone starts to use it without looking too
closely at the header. But could we just remove the default static
inline return true implementation and let the linker fail if someone
starts to use it outside hibernate? Then we could leave it compiled out
until then.

Also it looks like riscv does not have ARCH_HIBERNATION_POSSIBLE so the
new function added here couldn't be used yet. You could also just let
the linker catch it if riscv ever enables hibernate?

On Sun, 2020-10-25 at 12:15 +0200, Mike Rapoport wrote:

Indeed, for architectures that define CONFIG_ARCH_HAS_SET_DIRECT_MAP
it is
possible that __kernel_map_pages() would fail, but since this
function is
void, the failure will go unnoticed.

Could you elaborate on how this could happen? Do you mean during
runtime today or if something new was introduced?

On Mon, Oct 26, 2020 at 01:13:52AM +0000, Edgecombe, Rick P wrote:

On Sun, 2020-10-25 at 12:15 +0200, Mike Rapoport wrote:

quoted

Indeed, for architectures that define CONFIG_ARCH_HAS_SET_DIRECT_MAP
it is
possible that __kernel_map_pages() would fail, but since this
function is
void, the failure will go unnoticed.

Could you elaborate on how this could happen? Do you mean during
runtime today or if something new was introduced?

A failure in__kernel_map_pages() may happen today. For instance, on x86
if the kernel is built with DEBUG_PAGEALLOC.

	__kernel_map_pages(page, 1, 0);

will need to split, say, 2M page and during the split an allocation of
page table could fail.

Currently, the only user of __kernel_map_pages() outside DEBUG_PAGEALLOC
is hibernation, but I think it would be safer to entirely prevent usage
of __kernel_map_pages() when DEBUG_PAGEALLOC=n.

-- 
Sincerely yours,
Mike.

On Mon, Oct 26, 2020 at 12:38:32AM +0000, Edgecombe, Rick P wrote:

On Sun, 2020-10-25 at 12:15 +0200, Mike Rapoport wrote:

quoted

From: Mike Rapoport <redacted>

When DEBUG_PAGEALLOC or ARCH_HAS_SET_DIRECT_MAP is enabled a page may
be
not present in the direct map and has to be explicitly mapped before
it
could be copied.

On arm64 it is possible that a page would be removed from the direct
map
using set_direct_map_invalid_noflush() but __kernel_map_pages() will
refuse
to map this page back if DEBUG_PAGEALLOC is disabled.

It looks to me that arm64 __kernel_map_pages() will still attempt to
map it if rodata_full is true, how does this happen?

Unless I misread the code, arm64 requires both rodata_full and
debug_pagealloc_enabled() to be true for __kernel_map_pages() to do
anything.
But rodata_full condition applies to set_direct_map_*_noflush() as well,
so with !rodata_full the linear map won't be ever changed.

quoted

Explicitly use set_direct_map_{default,invalid}_noflush() for
ARCH_HAS_SET_DIRECT_MAP case and debug_pagealloc_map_pages() for
DEBUG_PAGEALLOC case.

While on that, rename kernel_map_pages() to hibernate_map_page() and
drop
numpages parameter.

Signed-off-by: Mike Rapoport <redacted>
---
 kernel/power/snapshot.c | 29 +++++++++++++++++++----------
 1 file changed, 19 insertions(+), 10 deletions(-)

diff --git a/kernel/power/snapshot.c b/kernel/power/snapshot.c
index fa499466f645..ecb7b32ce77c 100644
--- a/kernel/power/snapshot.c
+++ b/kernel/power/snapshot.c

@@ -76,16 +76,25 @@ static inline void

hibernate_restore_protect_page(void *page_address) {}
 static inline void hibernate_restore_unprotect_page(void
*page_address) {}
 #endif /* CONFIG_STRICT_KERNEL_RWX  && CONFIG_ARCH_HAS_SET_MEMORY */
 
-#if defined(CONFIG_DEBUG_PAGEALLOC) ||
defined(CONFIG_ARCH_HAS_SET_DIRECT_MAP)
-static inline void
-kernel_map_pages(struct page *page, int numpages, int enable)
+static inline void hibernate_map_page(struct page *page, int enable)
 {
-	__kernel_map_pages(page, numpages, enable);
+	if (IS_ENABLED(CONFIG_ARCH_HAS_SET_DIRECT_MAP)) {
+		unsigned long addr = (unsigned long)page_address(page);
+		int ret;
+
+		if (enable)
+			ret = set_direct_map_default_noflush(page);
+		else
+			ret = set_direct_map_invalid_noflush(page);
+
+		if (WARN_ON(ret))

34 more quoted lines

The intention of this series is to disallow usage of
__kernel_map_pages() when DEBUG_PAGEALLOC=n. I'll update this patch to
better handle possible errors, but I still want to keep WARN in the
caller.

-- 
Sincerely yours,
Mike.

On Mon, Oct 26, 2020 at 12:54:01AM +0000, Edgecombe, Rick P wrote:

On Sun, 2020-10-25 at 12:15 +0200, Mike Rapoport wrote:

quoted

index 7f248fc45317..16f878c26667 100644

--- a/arch/x86/mm/pat/set_memory.c
+++ b/arch/x86/mm/pat/set_memory.c

@@ -2228,7 +2228,6 @@ void __kernel_map_pages(struct page *page, int

numpages, int enable)
 }
 #endif /* CONFIG_DEBUG_PAGEALLOC */
 
-#ifdef CONFIG_HIBERNATION
 bool kernel_page_present(struct page *page)
 {
        unsigned int level;

@@ -2240,7 +2239,6 @@ bool kernel_page_present(struct page *page)
        pte = lookup_address((unsigned long)page_address(page),

&level);
        return (pte_val(*pte) & _PAGE_PRESENT);
 }
-#endif /* CONFIG_HIBERNATION */

This is only used by hibernate today right? Makes sense that it should
return a correct answer if someone starts to use it without looking too
closely at the header. But could we just remove the default static
inline return true implementation and let the linker fail if someone
starts to use it outside hibernate? Then we could leave it compiled out
until then.

Hmm, I'm not sure I follow you here.
We'd need some stub for architectures that have
ARCH_HIBERNATION_POSSIBLE and do not have
CONFIG_ARCH_HAS_SET_DIRECT_MAP.

I don't see how the kernel would compile for ppc or sparc with
hibernation enabled if I remove the default implementation.

Also it looks like riscv does not have ARCH_HIBERNATION_POSSIBLE so the
new function added here couldn't be used yet. You could also just let
the linker catch it if riscv ever enables hibernate?

-- 
Sincerely yours,
Mike.

On 25.10.20 11:15, Mike Rapoport wrote:

From: Mike Rapoport <redacted>

When CONFIG_DEBUG_PAGEALLOC is enabled, it unmaps pages from the
kernel direct mapping after free_pages(). The pages than need to be
mapped back before they could be used. Theese mapping operations use 
__kernel_map_pages() guarded with with debug_pagealloc_enabled().

The only place that calls __kernel_map_pages() without checking
whether DEBUG_PAGEALLOC is enabled is the hibernation code that
presumes availability of this function when ARCH_HAS_SET_DIRECT_MAP
is set. Still, on arm64, __kernel_map_pages() will bail out when
DEBUG_PAGEALLOC is not enabled but set_direct_map_invalid_noflush()
may render some pages not present in the direct map and hibernation
code won't be able to save such pages.

To make page allocation debugging and hibernation interaction more
robust, the dependency on DEBUG_PAGEALLOC or ARCH_HAS_SET_DIRECT_MAP
has to be made more explicit.

Start with combining the guard condition and the call to 
__kernel_map_pages() into a single debug_pagealloc_map_pages()
function to emphasize that __kernel_map_pages() should not be called
without DEBUG_PAGEALLOC and use this new function to map/unmap pages
when page allocation debug is enabled.

As the only remaining user of kernel_map_pages() is the hibernation
code, mode that function into kernel/power/snapshot.c closer to a
caller.

s/mode/move/

4 more quoted lines

That change should go into a separate patch.


For the debug_pagealloc_map_pages() parts

Reviewed-by: David Hildenbrand <redacted>


-- 
Thanks,

David / dhildenb

On Mon, Oct 26, 2020 at 12:05:13PM +0100, David Hildenbrand wrote:

On 25.10.20 11:15, Mike Rapoport wrote:

quoted

From: Mike Rapoport <redacted>

When CONFIG_DEBUG_PAGEALLOC is enabled, it unmaps pages from the
kernel direct mapping after free_pages(). The pages than need to be
mapped back before they could be used. Theese mapping operations use 
__kernel_map_pages() guarded with with debug_pagealloc_enabled().

The only place that calls __kernel_map_pages() without checking
whether DEBUG_PAGEALLOC is enabled is the hibernation code that
presumes availability of this function when ARCH_HAS_SET_DIRECT_MAP
is set. Still, on arm64, __kernel_map_pages() will bail out when
DEBUG_PAGEALLOC is not enabled but set_direct_map_invalid_noflush()
may render some pages not present in the direct map and hibernation
code won't be able to save such pages.

To make page allocation debugging and hibernation interaction more
robust, the dependency on DEBUG_PAGEALLOC or ARCH_HAS_SET_DIRECT_MAP
has to be made more explicit.

Start with combining the guard condition and the call to 
__kernel_map_pages() into a single debug_pagealloc_map_pages()
function to emphasize that __kernel_map_pages() should not be called
without DEBUG_PAGEALLOC and use this new function to map/unmap pages
when page allocation debug is enabled.

As the only remaining user of kernel_map_pages() is the hibernation
code, mode that function into kernel/power/snapshot.c closer to a
caller.

s/mode/move/

quoted

Signed-off-by: Mike Rapoport <redacted> --- 
include/linux/mm.h      | 16 +++++++--------- kernel/power/snapshot.c
| 11 +++++++++++ mm/memory_hotplug.c     |  3 +-- mm/page_alloc.c
|  6 ++---- mm/slab.c               |  8 +++----- 5 files changed, 24
insertions(+), 20 deletions(-)

diff --git a/include/linux/mm.h b/include/linux/mm.h index

38 more quoted lines

 
Hmm, I beleive you refer to moving kernel_map_pages() to snapshot.c,
right?

For the debug_pagealloc_map_pages() parts

Reviewed-by: David Hildenbrand <redacted>

 
Thanks!

-- 
Thanks,

David / dhildenb

-- 
Sincerely yours,
Mike.

On 26.10.20 12:54, Mike Rapoport wrote:

On Mon, Oct 26, 2020 at 12:05:13PM +0100, David Hildenbrand wrote:

quoted

On 25.10.20 11:15, Mike Rapoport wrote:

quoted

From: Mike Rapoport <redacted>

When CONFIG_DEBUG_PAGEALLOC is enabled, it unmaps pages from the
kernel direct mapping after free_pages(). The pages than need to be
mapped back before they could be used. Theese mapping operations use 
__kernel_map_pages() guarded with with debug_pagealloc_enabled().

The only place that calls __kernel_map_pages() without checking
whether DEBUG_PAGEALLOC is enabled is the hibernation code that
presumes availability of this function when ARCH_HAS_SET_DIRECT_MAP
is set. Still, on arm64, __kernel_map_pages() will bail out when
DEBUG_PAGEALLOC is not enabled but set_direct_map_invalid_noflush()
may render some pages not present in the direct map and hibernation
code won't be able to save such pages.

To make page allocation debugging and hibernation interaction more
robust, the dependency on DEBUG_PAGEALLOC or ARCH_HAS_SET_DIRECT_MAP
has to be made more explicit.

Start with combining the guard condition and the call to 
__kernel_map_pages() into a single debug_pagealloc_map_pages()
function to emphasize that __kernel_map_pages() should not be called
without DEBUG_PAGEALLOC and use this new function to map/unmap pages
when page allocation debug is enabled.

As the only remaining user of kernel_map_pages() is the hibernation
code, mode that function into kernel/power/snapshot.c closer to a
caller.

s/mode/move/

quoted

Signed-off-by: Mike Rapoport <redacted> --- 
include/linux/mm.h      | 16 +++++++--------- kernel/power/snapshot.c
| 11 +++++++++++ mm/memory_hotplug.c     |  3 +-- mm/page_alloc.c
|  6 ++---- mm/slab.c               |  8 +++----- 5 files changed, 24
insertions(+), 20 deletions(-)

42 more quoted lines

Sorry, yes!


-- 
Thanks,

David / dhildenb

On Mon, 2020-10-26 at 11:05 +0200, Mike Rapoport wrote:

On Mon, Oct 26, 2020 at 01:13:52AM +0000, Edgecombe, Rick P wrote:

quoted

On Sun, 2020-10-25 at 12:15 +0200, Mike Rapoport wrote:

quoted

Indeed, for architectures that define
CONFIG_ARCH_HAS_SET_DIRECT_MAP
it is
possible that __kernel_map_pages() would fail, but since this
function is
void, the failure will go unnoticed.

Could you elaborate on how this could happen? Do you mean during
runtime today or if something new was introduced?

A failure in__kernel_map_pages() may happen today. For instance, on
x86
if the kernel is built with DEBUG_PAGEALLOC.

        __kernel_map_pages(page, 1, 0);

will need to split, say, 2M page and during the split an allocation
of
page table could fail.

On x86 at least, DEBUG_PAGEALLOC expects to never have to break a page
on the direct map and even disables locking in cpa because it assumes
this. If this is happening somehow anyway then we should probably fix
that. Even if it's a debug feature, it will not be as useful if it is
causing its own crashes.

I'm still wondering if there is something I'm missing here. It seems
like you are saying there is a bug in some arch's, so let's add a WARN
in cross-arch code to log it as it crashes. A warn and making things
clearer seem like good ideas, but if there is a bug we should fix it.
The code around the callers still functionally assume re-mapping can't
fail.

Currently, the only user of __kernel_map_pages() outside
DEBUG_PAGEALLOC
is hibernation, but I think it would be safer to entirely prevent
usage
of __kernel_map_pages() when DEBUG_PAGEALLOC=n.

I totally agree it's error prone FWIW. On x86, my mental model of how
it is supposed to work is: If a page is 4k and NP it cannot fail to be
remapped. set_direct_map_invalid_noflush() should result in 4k NP
pages, and DEBUG_PAGEALLOC should result in all 4k pages on the direct
map. Are you seeing this violated or do I have wrong assumptions?

Beyond whatever you are seeing, for the latter case of new things
getting introduced to an interface with hidden dependencies... Another
edge case could be a new caller to set_memory_np() could result in
large NP pages. None of the callers today should cause this AFAICT, but
it's not great to rely on the callers to know these details.

On Mon, 2020-10-26 at 11:15 +0200, Mike Rapoport wrote:

On Mon, Oct 26, 2020 at 12:38:32AM +0000, Edgecombe, Rick P wrote:

quoted

On Sun, 2020-10-25 at 12:15 +0200, Mike Rapoport wrote:

quoted

From: Mike Rapoport <redacted>

When DEBUG_PAGEALLOC or ARCH_HAS_SET_DIRECT_MAP is enabled a page
may
be
not present in the direct map and has to be explicitly mapped
before
it
could be copied.

On arm64 it is possible that a page would be removed from the
direct
map
using set_direct_map_invalid_noflush() but __kernel_map_pages()
will
refuse
to map this page back if DEBUG_PAGEALLOC is disabled.

It looks to me that arm64 __kernel_map_pages() will still attempt
to
map it if rodata_full is true, how does this happen?

Unless I misread the code, arm64 requires both rodata_full and
debug_pagealloc_enabled() to be true for __kernel_map_pages() to do
anything.
But rodata_full condition applies to set_direct_map_*_noflush() as
well,
so with !rodata_full the linear map won't be ever changed.

Hmm, looks to me that __kernel_map_pages() will only skip it if both
debug pagealloc and rodata_full are false.

But now I'm wondering if maybe we could simplify things by just moving
the hibernate unmapped page logic off of the direct map. On x86,
text_poke() used to use this reserved fixmap pte thing that it could
rely on to remap memory with. If hibernate had some separate pte for
remapping like that, then we could not have any direct map restrictions
caused by it/kernel_map_pages(), and it wouldn't have to worry about
relying on anything else.

On Mon, 2020-10-26 at 11:15 +0200, Mike Rapoport wrote:

The intention of this series is to disallow usage of
__kernel_map_pages() when DEBUG_PAGEALLOC=n. I'll update this patch
to
better handle possible errors, but I still want to keep WARN in the
caller.

Sorry, I missed this snippet at the bottom, that your intention was to
actually handle errors somehow in the hibernate code. Please ignore my
other comment that assumed you wanted to just add a warn.

On Mon, Oct 26, 2020 at 06:05:30PM +0000, Edgecombe, Rick P wrote:

On Mon, 2020-10-26 at 11:05 +0200, Mike Rapoport wrote:

quoted

On Mon, Oct 26, 2020 at 01:13:52AM +0000, Edgecombe, Rick P wrote:

quoted

On Sun, 2020-10-25 at 12:15 +0200, Mike Rapoport wrote:

quoted

Indeed, for architectures that define
CONFIG_ARCH_HAS_SET_DIRECT_MAP
it is
possible that __kernel_map_pages() would fail, but since this
function is
void, the failure will go unnoticed.

Could you elaborate on how this could happen? Do you mean during
runtime today or if something new was introduced?

A failure in__kernel_map_pages() may happen today. For instance, on
x86
if the kernel is built with DEBUG_PAGEALLOC.

        __kernel_map_pages(page, 1, 0);

will need to split, say, 2M page and during the split an allocation
of
page table could fail.

On x86 at least, DEBUG_PAGEALLOC expects to never have to break a page
on the direct map and even disables locking in cpa because it assumes
this. If this is happening somehow anyway then we should probably fix
that. Even if it's a debug feature, it will not be as useful if it is
causing its own crashes.

I'm still wondering if there is something I'm missing here. It seems
like you are saying there is a bug in some arch's, so let's add a WARN
in cross-arch code to log it as it crashes. A warn and making things
clearer seem like good ideas, but if there is a bug we should fix it.
The code around the callers still functionally assume re-mapping can't
fail.

Oh, I've meant x86 kernel *without* DEBUG_PAGEALLOC, and indeed the call
that unmaps pages back in safe_copy_page will just reset a 4K page to
NP because whatever made it NP at the first place already did the split.

Still, on arm64 with DEBUG_PAGEALLOC=n there is a possibility of a race
between map/unmap dance in __vunmap() and safe_copy_page() that may
cause access to unmapped memory:

__vunmap()
    vm_remove_mappings()
        set_direct_map_invalid()
					safe_copy_page()	
					    __kernel_map_pages()
					    	return
					    do_copy_page() -> fault
					   	
This is a theoretical bug, but it is still not nice :) 							

quoted

Currently, the only user of __kernel_map_pages() outside
DEBUG_PAGEALLOC
is hibernation, but I think it would be safer to entirely prevent
usage
of __kernel_map_pages() when DEBUG_PAGEALLOC=n.

I totally agree it's error prone FWIW. On x86, my mental model of how
it is supposed to work is: If a page is 4k and NP it cannot fail to be
remapped. set_direct_map_invalid_noflush() should result in 4k NP
pages, and DEBUG_PAGEALLOC should result in all 4k pages on the direct
map. Are you seeing this violated or do I have wrong assumptions?

You are right, there is a set of assumptions about the remapping of the
direct map pages that make it all work, at least on x86.
But this is very subtle and it's not easy to wrap one's head around
this.

That's why putting __kernel_map_pages() out of "common" use and
keep it only for DEBUG_PAGEALLOC would make things clearer.

Beyond whatever you are seeing, for the latter case of new things
getting introduced to an interface with hidden dependencies... Another
edge case could be a new caller to set_memory_np() could result in
large NP pages. None of the callers today should cause this AFAICT, but
it's not great to rely on the callers to know these details.

 
A caller of set_memory_*() or set_direct_map_*() should expect a failure
and be ready for that. So adding a WARN to safe_copy_page() is the first
step in that direction :)

-- 
Sincerely yours,
Mike.

On 27.10.20 09:38, Mike Rapoport wrote:

On Mon, Oct 26, 2020 at 06:05:30PM +0000, Edgecombe, Rick P wrote:

quoted

On Mon, 2020-10-26 at 11:05 +0200, Mike Rapoport wrote:

quoted

On Mon, Oct 26, 2020 at 01:13:52AM +0000, Edgecombe, Rick P wrote:

quoted

On Sun, 2020-10-25 at 12:15 +0200, Mike Rapoport wrote:

quoted

Indeed, for architectures that define
CONFIG_ARCH_HAS_SET_DIRECT_MAP
it is
possible that __kernel_map_pages() would fail, but since this
function is
void, the failure will go unnoticed.

Could you elaborate on how this could happen? Do you mean during
runtime today or if something new was introduced?

A failure in__kernel_map_pages() may happen today. For instance, on
x86
if the kernel is built with DEBUG_PAGEALLOC.

         __kernel_map_pages(page, 1, 0);

will need to split, say, 2M page and during the split an allocation
of
page table could fail.

On x86 at least, DEBUG_PAGEALLOC expects to never have to break a page
on the direct map and even disables locking in cpa because it assumes
this. If this is happening somehow anyway then we should probably fix
that. Even if it's a debug feature, it will not be as useful if it is
causing its own crashes.

I'm still wondering if there is something I'm missing here. It seems
like you are saying there is a bug in some arch's, so let's add a WARN
in cross-arch code to log it as it crashes. A warn and making things
clearer seem like good ideas, but if there is a bug we should fix it.
The code around the callers still functionally assume re-mapping can't
fail.

Oh, I've meant x86 kernel *without* DEBUG_PAGEALLOC, and indeed the call
that unmaps pages back in safe_copy_page will just reset a 4K page to
NP because whatever made it NP at the first place already did the split.

45 more quoted lines

I am probably missing something important, but why are we saving/restoring the content of pages that were explicitly removed from the identity mapping such that nobody will access them?

Pages that are not allocated should contain garbage or be zero (init_on_free). That should be easy to handle without ever reading the page content.

The other user seems to be vm_remove_mappings(), where we only *temporarily* remove the mapping - while hibernating, that code shouldn't be active anymore I guess - or we could protect it from happening.

As I expressed in another mail, secretmem pages should rather not be saved when hibernating - hibernation should be rather be disabled.

What am I missing?

-- 
Thanks,

David / dhildenb

On Mon, Oct 26, 2020 at 06:57:32PM +0000, Edgecombe, Rick P wrote:

On Mon, 2020-10-26 at 11:15 +0200, Mike Rapoport wrote:

quoted

On Mon, Oct 26, 2020 at 12:38:32AM +0000, Edgecombe, Rick P wrote:

quoted

On Sun, 2020-10-25 at 12:15 +0200, Mike Rapoport wrote:

quoted

From: Mike Rapoport <redacted>

When DEBUG_PAGEALLOC or ARCH_HAS_SET_DIRECT_MAP is enabled a page
may
be
not present in the direct map and has to be explicitly mapped
before
it
could be copied.

On arm64 it is possible that a page would be removed from the
direct
map
using set_direct_map_invalid_noflush() but __kernel_map_pages()
will
refuse
to map this page back if DEBUG_PAGEALLOC is disabled.

It looks to me that arm64 __kernel_map_pages() will still attempt
to
map it if rodata_full is true, how does this happen?

Unless I misread the code, arm64 requires both rodata_full and
debug_pagealloc_enabled() to be true for __kernel_map_pages() to do
anything.
But rodata_full condition applies to set_direct_map_*_noflush() as
well,
so with !rodata_full the linear map won't be ever changed.

Hmm, looks to me that __kernel_map_pages() will only skip it if both
debug pagealloc and rodata_full are false.

But now I'm wondering if maybe we could simplify things by just moving
the hibernate unmapped page logic off of the direct map. On x86,
text_poke() used to use this reserved fixmap pte thing that it could
rely on to remap memory with. If hibernate had some separate pte for
remapping like that, then we could not have any direct map restrictions

2 more quoted lines

Well, there is map_kernel_range() that can be used by hibernation as
there is no requirement for particular virtual address, but that would
be quite costly if done for every page.

Maybe we can do somthing like

	if (kernel_page_present(s_page)) {
		do_copy_page(dst, page_address(s_page));
	} else {
		map_kernel_range_noflush(page_address(page), PAGE_SIZE,
					 PROT_READ, &page);
		do_copy_page(dst, page_address(s_page));
		unmap_kernel_range_noflush(page_address(page), PAGE_SIZE);
	}

But it seems that a prerequisite for changing the way a page is mapped
in safe_copy_page() would be to teach hibernation that a mapping here
may fail.

-- 
Sincerely yours,
Mike.

On Tue, Oct 27, 2020 at 09:46:35AM +0100, David Hildenbrand wrote:

On 27.10.20 09:38, Mike Rapoport wrote:

quoted

On Mon, Oct 26, 2020 at 06:05:30PM +0000, Edgecombe, Rick P wrote:

quoted

On Mon, 2020-10-26 at 11:05 +0200, Mike Rapoport wrote:

quoted

On Mon, Oct 26, 2020 at 01:13:52AM +0000, Edgecombe, Rick P wrote:

quoted

On Sun, 2020-10-25 at 12:15 +0200, Mike Rapoport wrote:

quoted

Indeed, for architectures that define
CONFIG_ARCH_HAS_SET_DIRECT_MAP
it is
possible that __kernel_map_pages() would fail, but since this
function is
void, the failure will go unnoticed.

Could you elaborate on how this could happen? Do you mean during
runtime today or if something new was introduced?

A failure in__kernel_map_pages() may happen today. For instance, on
x86
if the kernel is built with DEBUG_PAGEALLOC.

         __kernel_map_pages(page, 1, 0);

will need to split, say, 2M page and during the split an allocation
of
page table could fail.

On x86 at least, DEBUG_PAGEALLOC expects to never have to break a page
on the direct map and even disables locking in cpa because it assumes
this. If this is happening somehow anyway then we should probably fix
that. Even if it's a debug feature, it will not be as useful if it is
causing its own crashes.

I'm still wondering if there is something I'm missing here. It seems
like you are saying there is a bug in some arch's, so let's add a WARN
in cross-arch code to log it as it crashes. A warn and making things
clearer seem like good ideas, but if there is a bug we should fix it.
The code around the callers still functionally assume re-mapping can't
fail.

Oh, I've meant x86 kernel *without* DEBUG_PAGEALLOC, and indeed the call
that unmaps pages back in safe_copy_page will just reset a 4K page to

53 more quoted lines

I'm not familiar with hibernation to say anything smart here, but the
help text of DEBUG_PAGEALLOC in Kconfig says:

	... this option cannot be enabled in combination with
	hibernation as that would result in incorrect warnings of memory
	corruption after a resume because free pages are not saved to
	the suspend image.

Probably you are right and free pages need to be handled differently,
but it does not seem the case now.

The other user seems to be vm_remove_mappings(), where we only *temporarily*
remove the mapping - while hibernating, that code shouldn't be active
anymore I guess - or we could protect it from happening.

Hmm, I _think_ vm_remove_mappings() shouldn't be active while
hibernating, but I'm not 100% sure.

As I expressed in another mail, secretmem pages should rather not be saved
when hibernating - hibernation should be rather be disabled.

Agree.

What am I missing?

I think I miscommunicated the purpose of this set, which was to hide
__kernel_map_pages() under DEBUG_PAGEALLOC and make hibernation use
set_direct_map_*() explictly without major rework of free pages handling
during hibernation.

Does it help?

-- 
Thanks,

David / dhildenb

-- 
Sincerely yours,
Mike.

On 27.10.20 10:47, Mike Rapoport wrote:

On Tue, Oct 27, 2020 at 09:46:35AM +0100, David Hildenbrand wrote:

quoted

On 27.10.20 09:38, Mike Rapoport wrote:

quoted

On Mon, Oct 26, 2020 at 06:05:30PM +0000, Edgecombe, Rick P wrote:

quoted

On Mon, 2020-10-26 at 11:05 +0200, Mike Rapoport wrote:

quoted

On Mon, Oct 26, 2020 at 01:13:52AM +0000, Edgecombe, Rick P wrote:

quoted

On Sun, 2020-10-25 at 12:15 +0200, Mike Rapoport wrote:

quoted

Indeed, for architectures that define
CONFIG_ARCH_HAS_SET_DIRECT_MAP
it is
possible that __kernel_map_pages() would fail, but since this
function is
void, the failure will go unnoticed.

Could you elaborate on how this could happen? Do you mean during
runtime today or if something new was introduced?

A failure in__kernel_map_pages() may happen today. For instance, on
x86
if the kernel is built with DEBUG_PAGEALLOC.

          __kernel_map_pages(page, 1, 0);

will need to split, say, 2M page and during the split an allocation
of
page table could fail.

On x86 at least, DEBUG_PAGEALLOC expects to never have to break a page
on the direct map and even disables locking in cpa because it assumes
this. If this is happening somehow anyway then we should probably fix
that. Even if it's a debug feature, it will not be as useful if it is
causing its own crashes.

I'm still wondering if there is something I'm missing here. It seems
like you are saying there is a bug in some arch's, so let's add a WARN
in cross-arch code to log it as it crashes. A warn and making things
clearer seem like good ideas, but if there is a bug we should fix it.
The code around the callers still functionally assume re-mapping can't
fail.

Oh, I've meant x86 kernel *without* DEBUG_PAGEALLOC, and indeed the call

87 more quoted lines

Heh, as always, once you touch questionable code, people will beg for proper cleanups instead :)


-- 
Thanks,

David / dhildenb

On Tue, 2020-10-27 at 10:49 +0200, Mike Rapoport wrote:

On Mon, Oct 26, 2020 at 06:57:32PM +0000, Edgecombe, Rick P wrote:

quoted

On Mon, 2020-10-26 at 11:15 +0200, Mike Rapoport wrote:

quoted

On Mon, Oct 26, 2020 at 12:38:32AM +0000, Edgecombe, Rick P
wrote:

quoted

On Sun, 2020-10-25 at 12:15 +0200, Mike Rapoport wrote:

quoted

From: Mike Rapoport <redacted>

When DEBUG_PAGEALLOC or ARCH_HAS_SET_DIRECT_MAP is enabled a
page
may
be
not present in the direct map and has to be explicitly mapped
before
it
could be copied.

On arm64 it is possible that a page would be removed from the
direct
map
using set_direct_map_invalid_noflush() but
__kernel_map_pages()
will
refuse
to map this page back if DEBUG_PAGEALLOC is disabled.

It looks to me that arm64 __kernel_map_pages() will still
attempt
to
map it if rodata_full is true, how does this happen?

Unless I misread the code, arm64 requires both rodata_full and
debug_pagealloc_enabled() to be true for __kernel_map_pages() to
do
anything.
But rodata_full condition applies to set_direct_map_*_noflush()
as
well,
so with !rodata_full the linear map won't be ever changed.

Hmm, looks to me that __kernel_map_pages() will only skip it if

38 more quoted lines

Yea that is what I meant, the direct map could still be used for mapped
pages.

But for the unmapped case it could have a pre-setup 4k pte for some non
direct map address. Then just change the pte to point to any unmapped
direct map page that was encountered. The point would be to give
hibernate some 4k pte of its own to manipulate so that it can't fail.

Yet another option would be have hibernate_map_page() just map large
pages if it finds them.

So we could teach hibernate to handle mapping failures, OR we could
change it so it doesn't rely on direct map page sizes in order to
succeed. The latter seems better to me since there isn't a reason why
it should have to fail and the resulting logic might be simpler. Both
seem like improvements in robustness though.

On Wed, 2020-10-28 at 13:30 +0200, Mike Rapoport wrote:

On Wed, Oct 28, 2020 at 11:20:12AM +0000, Will Deacon wrote:

quoted

On Tue, Oct 27, 2020 at 10:38:16AM +0200, Mike Rapoport wrote:

quoted

On Mon, Oct 26, 2020 at 06:05:30PM +0000, Edgecombe, Rick P
wrote:

quoted

On Mon, 2020-10-26 at 11:05 +0200, Mike Rapoport wrote:

quoted

On Mon, Oct 26, 2020 at 01:13:52AM +0000, Edgecombe, Rick P
wrote:

quoted

On Sun, 2020-10-25 at 12:15 +0200, Mike Rapoport wrote:

quoted

Indeed, for architectures that define
CONFIG_ARCH_HAS_SET_DIRECT_MAP
it is
possible that __kernel_map_pages() would fail, but since
this
function is
void, the failure will go unnoticed.

Could you elaborate on how this could happen? Do you mean
during
runtime today or if something new was introduced?

A failure in__kernel_map_pages() may happen today. For
instance, on
x86
if the kernel is built with DEBUG_PAGEALLOC.

        __kernel_map_pages(page, 1, 0);

will need to split, say, 2M page and during the split an
allocation
of
page table could fail.

On x86 at least, DEBUG_PAGEALLOC expects to never have to break
a page
on the direct map and even disables locking in cpa because it
assumes
this. If this is happening somehow anyway then we should
probably fix
that. Even if it's a debug feature, it will not be as useful if
it is

42 more quoted lines

Well, now I'm confused again.

As David pointed, __vunmap() should not be executing simultaneously
with the hibernate operation because hibernate can't snapshot while
data it needs to save is still updating. If a thread was paused when a
page was in an "invalid" state, it should be remapped by hibernate
before the copy.

To level set, before reading this mail, my takeaways from the
discussions on potential hibernate/debug page alloc problems were:

Potential RISC-V issue:
Doesn't have hibernate support

Potential ARM issue:
The logic around when it's cpa determines pages might be unmapped looks
correct for current callers.

Potential x86 page break issue:
Seems to be ok for now, but a new set_memory_np() caller could violate
assumptions in hibernate.

Non-obvious thorny logic: 
General agreement it would be good to separate dependencies.

Behavior of V1 of this patchset:
No functional change other than addition of a warn in hibernate.


So "does this fix the problem", "yes" leaves me a bit confused... Not
saying there couldn't be any problems, especially due to the thorniness
and cross arch stride, but what is it exactly and how does this series
fix it?

On Sun, 2020-10-25 at 12:15 +0200, Mike Rapoport wrote:

+       if (IS_ENABLED(CONFIG_ARCH_HAS_SET_DIRECT_MAP)) {
+               unsigned long addr = (unsigned
long)page_address(page);
+               int ret;
+
+               if (enable)
+                       ret = set_direct_map_default_noflush(page);
+               else
+                       ret = set_direct_map_invalid_noflush(page);
+
+               if (WARN_ON(ret))
+                       return;
+
+               flush_tlb_kernel_range(addr, addr + PAGE_SIZE);
+       } else {
+               debug_pagealloc_map_pages(page, 1, enable);
+       }

Looking at the arm side again, I think this might actually introduce a
regression for the arm/hibernate/DEBUG_PAGEALLOC combo.

Unlike __kernel_map_pages(), it looks like arm's cpa will always bail
in the set_direct_map_() functions if rodata_full is false. So if
rodata_full was disabled but debug page alloc is on, then this would
now skip remapping the pages. I guess the significance depends on
whether hibernate could actually try to save any DEBUG_PAGEALLOC
unmapped pages. Looks like it to me though.

On Tue, Oct 27, 2020 at 09:46:35AM +0100, David Hildenbrand wrote:

On 27.10.20 09:38, Mike Rapoport wrote:

quoted

On Mon, Oct 26, 2020 at 06:05:30PM +0000, Edgecombe, Rick P wrote:

quoted

Beyond whatever you are seeing, for the latter case of new things
getting introduced to an interface with hidden dependencies... Another
edge case could be a new caller to set_memory_np() could result in
large NP pages. None of the callers today should cause this AFAICT, but
it's not great to rely on the callers to know these details.

quoted

A caller of set_memory_*() or set_direct_map_*() should expect a failure
and be ready for that. So adding a WARN to safe_copy_page() is the first
step in that direction :)

I am probably missing something important, but why are we saving/restoring
the content of pages that were explicitly removed from the identity mapping
such that nobody will access them?

Actually, we should not be saving/restoring free pages during
hibernation as there are several calls to mark_free_pages() that should
exclude the free pages from the snapshot. I've tried to find why the fix
that maps/unmaps a page to save it was required at the first place, but
I could not find bug reports.

The closest I've got is an email from Rafael that asked to update
"hibernate: handle DEBUG_PAGEALLOC" patch:

https://lore.kernel.org/linux-pm/200802200133.44098.rjw@sisk.pl/

Could it be that safe_copy_page() tries to workaround a non-existent
problem?

-- 
Sincerely yours,
Mike.

On 28.10.20 12:09, Mike Rapoport wrote:

On Tue, Oct 27, 2020 at 09:46:35AM +0100, David Hildenbrand wrote:

quoted

On 27.10.20 09:38, Mike Rapoport wrote:

quoted

On Mon, Oct 26, 2020 at 06:05:30PM +0000, Edgecombe, Rick P wrote:

quoted

Beyond whatever you are seeing, for the latter case of new things
getting introduced to an interface with hidden dependencies... Another
edge case could be a new caller to set_memory_np() could result in
large NP pages. None of the callers today should cause this AFAICT, but
it's not great to rely on the callers to know these details.

quoted

quoted

A caller of set_memory_*() or set_direct_map_*() should expect a failure
and be ready for that. So adding a WARN to safe_copy_page() is the first
step in that direction :)

I am probably missing something important, but why are we saving/restoring
the content of pages that were explicitly removed from the identity mapping
such that nobody will access them?

Actually, we should not be saving/restoring free pages during
hibernation as there are several calls to mark_free_pages() that should
exclude the free pages from the snapshot. I've tried to find why the fix
that maps/unmaps a page to save it was required at the first place, but
I could not find bug reports.

The closest I've got is an email from Rafael that asked to update
"hibernate: handle DEBUG_PAGEALLOC" patch:

https://lore.kernel.org/linux-pm/200802200133.44098.rjw@sisk.pl/

Could it be that safe_copy_page() tries to workaround a non-existent
problem?

Clould be! Also see

https://lkml.kernel.org/r/38de5bb0-5559-d069-0ce0-daec66ef2746@suse.cz

which restores free page content based on more kernel parameters, not based on the original content.

-- 
Thanks,

David / dhildenb

On Wed, Oct 28, 2020 at 11:20:12AM +0000, Will Deacon wrote:

On Tue, Oct 27, 2020 at 10:38:16AM +0200, Mike Rapoport wrote:

quoted

On Mon, Oct 26, 2020 at 06:05:30PM +0000, Edgecombe, Rick P wrote:

quoted

On Mon, 2020-10-26 at 11:05 +0200, Mike Rapoport wrote:

quoted

On Mon, Oct 26, 2020 at 01:13:52AM +0000, Edgecombe, Rick P wrote:

quoted

On Sun, 2020-10-25 at 12:15 +0200, Mike Rapoport wrote:

quoted

Indeed, for architectures that define
CONFIG_ARCH_HAS_SET_DIRECT_MAP
it is
possible that __kernel_map_pages() would fail, but since this
function is
void, the failure will go unnoticed.

Could you elaborate on how this could happen? Do you mean during
runtime today or if something new was introduced?

A failure in__kernel_map_pages() may happen today. For instance, on
x86
if the kernel is built with DEBUG_PAGEALLOC.

        __kernel_map_pages(page, 1, 0);

will need to split, say, 2M page and during the split an allocation
of
page table could fail.

On x86 at least, DEBUG_PAGEALLOC expects to never have to break a page
on the direct map and even disables locking in cpa because it assumes
this. If this is happening somehow anyway then we should probably fix
that. Even if it's a debug feature, it will not be as useful if it is
causing its own crashes.

I'm still wondering if there is something I'm missing here. It seems
like you are saying there is a bug in some arch's, so let's add a WARN
in cross-arch code to log it as it crashes. A warn and making things
clearer seem like good ideas, but if there is a bug we should fix it.
The code around the callers still functionally assume re-mapping can't
fail.

Oh, I've meant x86 kernel *without* DEBUG_PAGEALLOC, and indeed the call
that unmaps pages back in safe_copy_page will just reset a 4K page to

17 more quoted lines

Yes.

Will

-- 
Sincerely yours,
Mike.

On Tue, Oct 27, 2020 at 10:44:21PM +0000, Edgecombe, Rick P wrote:

On Tue, 2020-10-27 at 10:49 +0200, Mike Rapoport wrote:

quoted

On Mon, Oct 26, 2020 at 06:57:32PM +0000, Edgecombe, Rick P wrote:

quoted

On Mon, 2020-10-26 at 11:15 +0200, Mike Rapoport wrote:

quoted

On Mon, Oct 26, 2020 at 12:38:32AM +0000, Edgecombe, Rick P
wrote:

quoted

On Sun, 2020-10-25 at 12:15 +0200, Mike Rapoport wrote:

quoted

From: Mike Rapoport <redacted>

When DEBUG_PAGEALLOC or ARCH_HAS_SET_DIRECT_MAP is enabled a
page
may
be
not present in the direct map and has to be explicitly mapped
before
it
could be copied.

On arm64 it is possible that a page would be removed from the
direct
map
using set_direct_map_invalid_noflush() but
__kernel_map_pages()
will
refuse
to map this page back if DEBUG_PAGEALLOC is disabled.

It looks to me that arm64 __kernel_map_pages() will still
attempt
to
map it if rodata_full is true, how does this happen?

Unless I misread the code, arm64 requires both rodata_full and
debug_pagealloc_enabled() to be true for __kernel_map_pages() to
do
anything.
But rodata_full condition applies to set_direct_map_*_noflush()
as
well,
so with !rodata_full the linear map won't be ever changed.

55 more quoted lines

That's correct, but as the purpose of this series is to prevent usage of
__kernel_map_pages() outside DEBUG_PAGALLOC, for now I'm going to update this
patch changelog and add a comment to hibernate_map_page().

-- 
Sincerely yours,
Mike.

On Wed, Oct 28, 2020 at 12:17:35PM +0100, David Hildenbrand wrote:

On 28.10.20 12:09, Mike Rapoport wrote:

quoted

On Tue, Oct 27, 2020 at 09:46:35AM +0100, David Hildenbrand wrote:

quoted

On 27.10.20 09:38, Mike Rapoport wrote:

quoted

On Mon, Oct 26, 2020 at 06:05:30PM +0000, Edgecombe, Rick P wrote:

quoted

Beyond whatever you are seeing, for the latter case of new things
getting introduced to an interface with hidden dependencies... Another
edge case could be a new caller to set_memory_np() could result in
large NP pages. None of the callers today should cause this AFAICT, but
it's not great to rely on the callers to know these details.

quoted

quoted

A caller of set_memory_*() or set_direct_map_*() should expect a failure
and be ready for that. So adding a WARN to safe_copy_page() is the first
step in that direction :)

I am probably missing something important, but why are we saving/restoring
the content of pages that were explicitly removed from the identity mapping
such that nobody will access them?

Actually, we should not be saving/restoring free pages during
hibernation as there are several calls to mark_free_pages() that should
exclude the free pages from the snapshot. I've tried to find why the fix
that maps/unmaps a page to save it was required at the first place, but
I could not find bug reports.

The closest I've got is an email from Rafael that asked to update
"hibernate: handle DEBUG_PAGEALLOC" patch:

https://lore.kernel.org/linux-pm/200802200133.44098.rjw@sisk.pl/

Could it be that safe_copy_page() tries to workaround a non-existent
problem?

Clould be! Also see

https://lkml.kernel.org/r/38de5bb0-5559-d069-0ce0-daec66ef2746@suse.cz

which restores free page content based on more kernel parameters, not based

1 more quoted lines

Ah, after looking at it now I've run kernel with DEBUG_PAGEALLOC=y and
CONFIG_INIT_ON_FREE_DEFAULT_ON=y and restore crahsed nicely.

[   27.210093] PM: Image successfully loaded
[   27.226709] Disabling non-boot CPUs ...                                      
[   27.231208] smpboot: CPU 1 is now offline                                    
[   27.363926] kvm-clock: cpu 0, msr 5c889001, primary cpu clock, resume        
[   27.363995] BUG: unable to handle page fault for address: ffff9f7a40108000   
[   27.367996] #PF: supervisor write access in kernel mode                      
[   27.369558] #PF: error_code(0x0002) - not-present page                       
[   27.371098] PGD 5ca01067 P4D 5ca01067 PUD 5ca02067 PMD 5ca03067 PTE 800ffffff
fef7060                                                                         
[   27.373421] Oops: 0002 [#1] SMP DEBUG_PAGEALLOC PTI                          
[   27.374905] CPU: 0 PID: 1200 Comm: bash Not tainted 5.10.0-rc1 #5            
[   27.376700] Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS rel-1.14
.0-0-g155821a1990b-prebuilt.qemu.org 04/01/2014                                 
[   27.379879] RIP: 0010:clear_page_rep+0x7/0x10          
[   27.381218] Code: e8 be 88 75 00 44 89 e2 48 89 ee 48 89 df e8 60 ff ff ff c6
 03 00 5b 5d 41 5c c3 cc cc cc cc cc cc cc cc b9 00 02 00 00 31 c0 <f3> 48 ab c3
 0f 1f 44 00 00 31 c0 b9 40 00 00 00 66 0f 1f 84 00 00                          
[   27.386457] RSP: 0018:ffffb6838046be08 EFLAGS: 00010046                      
[   27.388011] RAX: 0000000000000000 RBX: ffff9f7a487c0ec0 RCX: 0000000000000200
[   27.390082] RDX: ffff9f7a4c788000 RSI: 0000000000000000 RDI: ffff9f7a40108000
[   27.392138] RBP: ffffffff8629c860 R08: 0000000000000000 R09: 0000000000000007
[   27.394205] R10: 0000000000000004 R11: ffffb6838046bbf8 R12: 0000000000000000
[   27.396271] R13: ffff9f7a419a62a0 R14: 0000000000000005 R15: ffff9f7a484f4da0
[   27.398334] FS:  00007fe0c3f6a700(0000) GS:ffff9f7abf800000(0000) knlGS:0000000000000000                                                                     
[   27.400717] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033                
[   27.402432] CR2: ffff9f7a40108000 CR3: 000000000859a001 CR4: 0000000000060ef0
[   27.404485] Call Trace:                                                      
[   27.405326]  clear_free_pages+0xf5/0x150                                     
[   27.406568]  hibernation_snapshot+0x390/0x3d0                                
[   27.407908]  hibernate+0xdb/0x240                                            
[   27.408978]  state_store+0xd7/0xe0                                           
[   27.410078]  kernfs_fop_write+0x10e/0x1a0                                    
[   27.411333]  vfs_write+0xbb/0x210                                            
[   27.412423]  ksys_write+0x9c/0xd0                      
[   27.413488]  do_syscall_64+0x33/0x40                                         
[   27.414636]  entry_SYSCALL_64_after_hwframe+0x44/0xa9                        

16 more lines

-- 
Thanks,

David / dhildenb

-- 
Sincerely yours,
Mike.

On Tue, Oct 27, 2020 at 10:38:16AM +0200, Mike Rapoport wrote:

On Mon, Oct 26, 2020 at 06:05:30PM +0000, Edgecombe, Rick P wrote:

quoted

On Mon, 2020-10-26 at 11:05 +0200, Mike Rapoport wrote:

quoted

On Mon, Oct 26, 2020 at 01:13:52AM +0000, Edgecombe, Rick P wrote:

quoted

On Sun, 2020-10-25 at 12:15 +0200, Mike Rapoport wrote:

quoted

Indeed, for architectures that define
CONFIG_ARCH_HAS_SET_DIRECT_MAP
it is
possible that __kernel_map_pages() would fail, but since this
function is
void, the failure will go unnoticed.

Could you elaborate on how this could happen? Do you mean during
runtime today or if something new was introduced?

A failure in__kernel_map_pages() may happen today. For instance, on
x86
if the kernel is built with DEBUG_PAGEALLOC.

        __kernel_map_pages(page, 1, 0);

will need to split, say, 2M page and during the split an allocation
of
page table could fail.

On x86 at least, DEBUG_PAGEALLOC expects to never have to break a page
on the direct map and even disables locking in cpa because it assumes
this. If this is happening somehow anyway then we should probably fix
that. Even if it's a debug feature, it will not be as useful if it is
causing its own crashes.

I'm still wondering if there is something I'm missing here. It seems
like you are saying there is a bug in some arch's, so let's add a WARN
in cross-arch code to log it as it crashes. A warn and making things
clearer seem like good ideas, but if there is a bug we should fix it.
The code around the callers still functionally assume re-mapping can't
fail.

Oh, I've meant x86 kernel *without* DEBUG_PAGEALLOC, and indeed the call
that unmaps pages back in safe_copy_page will just reset a 4K page to
NP because whatever made it NP at the first place already did the split.

14 more quoted lines

Just to clarify: this patch series fixes this problem, right?

Will

On Wed, 2020-10-28 at 13:09 +0200, Mike Rapoport wrote:

On Tue, Oct 27, 2020 at 09:46:35AM +0100, David Hildenbrand wrote:

quoted

On 27.10.20 09:38, Mike Rapoport wrote:

quoted

On Mon, Oct 26, 2020 at 06:05:30PM +0000, Edgecombe, Rick P
wrote:

quoted

Beyond whatever you are seeing, for the latter case of new
things
getting introduced to an interface with hidden dependencies...
Another
edge case could be a new caller to set_memory_np() could result
in
large NP pages. None of the callers today should cause this
AFAICT, but
it's not great to rely on the callers to know these details.

A caller of set_memory_*() or set_direct_map_*() should expect a
failure
and be ready for that. So adding a WARN to safe_copy_page() is
the first
step in that direction :)

I am probably missing something important, but why are we
saving/restoring
the content of pages that were explicitly removed from the identity
mapping
such that nobody will access them?

Actually, we should not be saving/restoring free pages during
hibernation as there are several calls to mark_free_pages() that
should
exclude the free pages from the snapshot. I've tried to find why the
fix
that maps/unmaps a page to save it was required at the first place,
but
I could not find bug reports.

The closest I've got is an email from Rafael that asked to update
"hibernate: handle DEBUG_PAGEALLOC" patch:

https://lore.kernel.org/linux-pm/200802200133.44098.rjw@sisk.pl/

3 more quoted lines

It looks like inside page_alloc.c it unmaps the page before it actually
frees it, so to hibernate it could look like the page is still
allocated even though it's unmapped? Maybe that small window is what it
cared about initially.

There is also now the vmalloc case, which I am actually working on
expanding. So I think the re-mapping logic is needed.

On Wed, Oct 28, 2020 at 09:15:38PM +0000, Edgecombe, Rick P wrote:

On Sun, 2020-10-25 at 12:15 +0200, Mike Rapoport wrote:

quoted

+       if (IS_ENABLED(CONFIG_ARCH_HAS_SET_DIRECT_MAP)) {
+               unsigned long addr = (unsigned
long)page_address(page);
+               int ret;
+
+               if (enable)
+                       ret = set_direct_map_default_noflush(page);
+               else
+                       ret = set_direct_map_invalid_noflush(page);
+
+               if (WARN_ON(ret))
+                       return;
+
+               flush_tlb_kernel_range(addr, addr + PAGE_SIZE);
+       } else {
+               debug_pagealloc_map_pages(page, 1, enable);
+       }

Looking at the arm side again, I think this might actually introduce a
regression for the arm/hibernate/DEBUG_PAGEALLOC combo.

Unlike __kernel_map_pages(), it looks like arm's cpa will always bail
in the set_direct_map_() functions if rodata_full is false.

So if rodata_full was disabled but debug page alloc is on, then this
would now skip remapping the pages. I guess the significance depends
on whether hibernate could actually try to save any DEBUG_PAGEALLOC
unmapped pages. Looks like it to me though.

 
__kernel_map_pages() on arm64 will also bail out if rodata_full is
false:

void __kernel_map_pages(struct page *page, int numpages, int enable)
{
	if (!debug_pagealloc_enabled() && !rodata_full)
		return;

	set_memory_valid((unsigned long)page_address(page), numpages, enable);
}

So using set_direct_map() to map back pages removed from the direct map
with __kernel_map_pages() seems safe to me.

-- 
Sincerely yours,
Mike.

On Wed, Oct 28, 2020 at 09:03:31PM +0000, Edgecombe, Rick P wrote:

quoted

On Wed, Oct 28, 2020 at 11:20:12AM +0000, Will Deacon wrote:

quoted

On Tue, Oct 27, 2020 at 10:38:16AM +0200, Mike Rapoport wrote:

quoted

					   	
This is a theoretical bug, but it is still not nice :) 		
					

Just to clarify: this patch series fixes this problem, right?

Yes.

Well, now I'm confused again.

As David pointed, __vunmap() should not be executing simultaneously
with the hibernate operation because hibernate can't snapshot while
data it needs to save is still updating. If a thread was paused when a
page was in an "invalid" state, it should be remapped by hibernate
before the copy.

To level set, before reading this mail, my takeaways from the
discussions on potential hibernate/debug page alloc problems were:

Potential RISC-V issue:
Doesn't have hibernate support

Potential ARM issue:
The logic around when it's cpa determines pages might be unmapped looks
correct for current callers.

Potential x86 page break issue:
Seems to be ok for now, but a new set_memory_np() caller could violate
assumptions in hibernate.

Non-obvious thorny logic: 
General agreement it would be good to separate dependencies.

Behavior of V1 of this patchset:
No functional change other than addition of a warn in hibernate.

There is a change that adds explicit use of set_direct_map() to
hibernate. Currently, in case of arm64 with DEBUG_PAGEALLOC=n if a
thread was paused when a page was in an "invalid" state hibernate will
access an unmapped data because __kernel_map_pages() will bail out.
After the change set_direct_map_default_noflush() would be used and the
page will get mapped before copy.

So "does this fix the problem", "yes" leaves me a bit confused... Not
saying there couldn't be any problems, especially due to the thorniness
and cross arch stride, but what is it exactly and how does this series
fix it?

This series goal was primarily to separate dependincies and make it
clearer what DEBUG_PAGEALLOC and what SET_DIRECT_MAP are. As it turned
out, there is also some lack of consistency between architectures that
implement either of this so I tried to improve this as well.

Honestly, I don't know if a thread can be paused at the time __vunmap()
left invalid pages, but it could, there is an issue on arm64 with
DEBUG_PAGEALLOC=n and this set fixes it.

__vunmap()
    vm_remove_mappings()
        set_direct_map_invalid()
	/* thread is frozen */
 					safe_copy_page()	
 					    __kernel_map_pages()
						if (!debug_pagealloc())
 					    	    return
 					    do_copy_page() -> fault

-- 
Sincerely yours,
Mike.

On 25.10.20 11:15, Mike Rapoport wrote:

From: Mike Rapoport <redacted>

Hi,

During recent discussion about KVM protected memory, David raised a concern
about usage of __kernel_map_pages() outside of DEBUG_PAGEALLOC scope [1].

Indeed, for architectures that define CONFIG_ARCH_HAS_SET_DIRECT_MAP it is
possible that __kernel_map_pages() would fail, but since this function is
void, the failure will go unnoticed.

Moreover, there's lack of consistency of __kernel_map_pages() semantics
across architectures as some guard this function with
#ifdef DEBUG_PAGEALLOC, some refuse to update the direct map if page
allocation debugging is disabled at run time and some allow modifying the
direct map regardless of DEBUG_PAGEALLOC settings.

This set straightens this out by restoring dependency of
__kernel_map_pages() on DEBUG_PAGEALLOC and updating the call sites
accordingly.

So, I was primarily wondering if we really have to touch direct mappings in hibernation code, or if we can avoid doing that. I was wondering if we cannot simply do something like kmap() when trying to access a !mapped page. Similar to reading old-os memory after kexec when in kdump. Just a thought.

-- 
Thanks,

David / dhildenb

On Thu, 2020-10-29 at 09:54 +0200, Mike Rapoport wrote:

__kernel_map_pages() on arm64 will also bail out if rodata_full is
false:
void __kernel_map_pages(struct page *page, int numpages, int enable)
{
        if (!debug_pagealloc_enabled() && !rodata_full)
                return;

        set_memory_valid((unsigned long)page_address(page), numpages,
enable);
}

So using set_direct_map() to map back pages removed from the direct
map
with __kernel_map_pages() seems safe to me.

Heh, one of us must have some simple boolean error in our head. I hope
its not me! :) I'll try on more time.

__kernel_map_pages() will bail out if rodata_full is false **AND**
debug page alloc is off. So it will only bail under conditions where
there could be nothing unmapped on the direct map.

Equivalent logic would be:
	if (!(debug_pagealloc_enabled() || rodata_full))
		return;

Or:
	if (debug_pagealloc_enabled() || rodata_full)
		set_memory_valid(blah)

So if either is on, the existing code will try to re-map. But the
set_direct_map_()'s will only work if rodata_full is on. So switching
hibernate to set_direct_map() will cause the remap to be missed for the
debug page alloc case, with !rodata_full.

It also breaks normal debug page alloc usage with !rodata_full for
similar reasons after patch 3. The pages would never get unmapped.

On Thu, 2020-10-29 at 10:12 +0200, Mike Rapoport wrote:

This series goal was primarily to separate dependincies and make it
clearer what DEBUG_PAGEALLOC and what SET_DIRECT_MAP are. As it
turned
out, there is also some lack of consistency between architectures
that
implement either of this so I tried to improve this as well.

Honestly, I don't know if a thread can be paused at the time
__vunmap()
left invalid pages, but it could, there is an issue on arm64 with
DEBUG_PAGEALLOC=n and this set fixes it.

Ah, ok. So from this and the other thread, this is about the logic in
arm's cpa for when it will try the un/map operations. I think the logic
actually works currently. And this series introduces a problem on ARM
similar to the one you are saying preexists. I put the details in the
other thread.

On Thu, Oct 29, 2020 at 11:19:18PM +0000, Edgecombe, Rick P wrote:

On Thu, 2020-10-29 at 09:54 +0200, Mike Rapoport wrote:

quoted

__kernel_map_pages() on arm64 will also bail out if rodata_full is
false:
void __kernel_map_pages(struct page *page, int numpages, int enable)
{
        if (!debug_pagealloc_enabled() && !rodata_full)
                return;

        set_memory_valid((unsigned long)page_address(page), numpages,
enable);
}

So using set_direct_map() to map back pages removed from the direct
map
with __kernel_map_pages() seems safe to me.

Heh, one of us must have some simple boolean error in our head. I hope
its not me! :) I'll try on more time.

Well, then it's me :)
You are right, I misread this and I could not understand why
!rodata_full bothers you.

__kernel_map_pages() will bail out if rodata_full is false **AND**
debug page alloc is off. So it will only bail under conditions where
there could be nothing unmapped on the direct map.

Equivalent logic would be:
	if (!(debug_pagealloc_enabled() || rodata_full))
		return;

Or:
	if (debug_pagealloc_enabled() || rodata_full)
		set_memory_valid(blah)

So if either is on, the existing code will try to re-map. But the
set_direct_map_()'s will only work if rodata_full is on. So switching
hibernate to set_direct_map() will cause the remap to be missed for the
debug page alloc case, with !rodata_full.

It also breaks normal debug page alloc usage with !rodata_full for
similar reasons after patch 3. The pages would never get unmapped.

I've updated the patches, there should be no regression now.

-- 
Sincerely yours,
Mike.