| Message ID | 20201025101555.3057-3-rppt@kernel.org (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | arch, mm: improve robustness of direct map manipulation | expand |
On Sun, 2020-10-25 at 12:15 +0200, Mike Rapoport wrote: > From: Mike Rapoport <rppt@linux.ibm.com> > > 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? > 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 <rppt@linux.ibm.com> > --- > 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); > } > } > 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 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: > > From: Mike Rapoport <rppt@linux.ibm.com> > > > > 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. > > 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 <rppt@linux.ibm.com> > > --- > > 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); > > } > > } > > > > 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. 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.
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: > > On Sun, 2020-10-25 at 12:15 +0200, Mike Rapoport wrote: > > > From: Mike Rapoport <rppt@linux.ibm.com> > > > > > > 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:57:32PM +0000, Edgecombe, Rick P wrote: > 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: > > > On Sun, 2020-10-25 at 12:15 +0200, Mike Rapoport wrote: > > > > From: Mike Rapoport <rppt@linux.ibm.com> > > > > > > > > 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. 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.
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: > > 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: > > > > On Sun, 2020-10-25 at 12:15 +0200, Mike Rapoport wrote: > > > > > From: Mike Rapoport <rppt@linux.ibm.com> > > > > > > > > > > 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. > > 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. > 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 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: > > 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: > > > > 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: > > > > > > From: Mike Rapoport <rppt@linux.ibm.com> > > > > > > > > > > > > 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. > > > > 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. > > > 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. 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().
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 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: > > + 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.
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, Oct 29, 2020 at 11:19:18PM +0000, Edgecombe, Rick P wrote: > 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. 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.
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); } }