From f0c7183008b41e92fa676406d87f18773724b48b Mon Sep 17 00:00:00 2001 From: Brian Geffon Date: Thu, 21 Sep 2023 13:00:45 -0400 Subject: PM: hibernate: Use __get_safe_page() rather than touching the list We found at least one situation where the safe pages list was empty and get_buffer() would gladly try to use a NULL pointer. Signed-off-by: Brian Geffon Fixes: 8357376d3df2 ("[PATCH] swsusp: Improve handling of highmem") Cc: All applicable Signed-off-by: Rafael J. Wysocki --- kernel/power/snapshot.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) (limited to 'kernel/power') diff --git a/kernel/power/snapshot.c b/kernel/power/snapshot.c index 87e9f7e2bdc0..acd2e49b9ac8 100644 --- a/kernel/power/snapshot.c +++ b/kernel/power/snapshot.c @@ -2545,8 +2545,9 @@ static void *get_highmem_page_buffer(struct page *page, pbe->copy_page = tmp; } else { /* Copy of the page will be stored in normal memory */ - kaddr = safe_pages_list; - safe_pages_list = safe_pages_list->next; + kaddr = __get_safe_page(ca->gfp_mask); + if (!kaddr) + return ERR_PTR(-ENOMEM); pbe->copy_page = virt_to_page(kaddr); } pbe->next = highmem_pblist; @@ -2750,8 +2751,9 @@ static void *get_buffer(struct memory_bitmap *bm, struct chain_allocator *ca) return ERR_PTR(-ENOMEM); } pbe->orig_address = page_address(page); - pbe->address = safe_pages_list; - safe_pages_list = safe_pages_list->next; + pbe->address = __get_safe_page(ca->gfp_mask); + if (!pbe->address) + return ERR_PTR(-ENOMEM); pbe->next = restore_pblist; restore_pblist = pbe; return pbe->address; -- cgit v1.2.3 From d08970df1980476f27936e24d452550f3e9e92e1 Mon Sep 17 00:00:00 2001 From: Brian Geffon Date: Fri, 22 Sep 2023 12:07:04 -0400 Subject: PM: hibernate: Clean up sync_read handling in snapshot_write_next() In snapshot_write_next(), sync_read is set and unset in three different spots unnecessiarly. As a result there is a subtle bug where the first page after the meta data has been loaded unconditionally sets sync_read to 0. If this first PFN was actually a highmem page, then the returned buffer will be the global "buffer," and the page needs to be loaded synchronously. That is, I'm not sure we can always assume the following to be safe: handle->buffer = get_buffer(&orig_bm, &ca); handle->sync_read = 0; Because get_buffer() can call get_highmem_page_buffer() which can return 'buffer'. The easiest way to address this is just set sync_read before snapshot_write_next() returns if handle->buffer == buffer. Signed-off-by: Brian Geffon Fixes: 8357376d3df2 ("[PATCH] swsusp: Improve handling of highmem") Cc: All applicable [ rjw: Subject and changelog edits ] Signed-off-by: Rafael J. Wysocki --- kernel/power/snapshot.c | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) (limited to 'kernel/power') diff --git a/kernel/power/snapshot.c b/kernel/power/snapshot.c index acd2e49b9ac8..7f0e6870318b 100644 --- a/kernel/power/snapshot.c +++ b/kernel/power/snapshot.c @@ -2785,8 +2785,6 @@ next: if (handle->cur > 1 && handle->cur > nr_meta_pages + nr_copy_pages + nr_zero_pages) return 0; - handle->sync_read = 1; - if (!handle->cur) { if (!buffer) /* This makes the buffer be freed by swsusp_free() */ @@ -2829,7 +2827,6 @@ next: memory_bm_position_reset(&zero_bm); restore_pblist = NULL; handle->buffer = get_buffer(&orig_bm, &ca); - handle->sync_read = 0; if (IS_ERR(handle->buffer)) return PTR_ERR(handle->buffer); } @@ -2839,9 +2836,8 @@ next: handle->buffer = get_buffer(&orig_bm, &ca); if (IS_ERR(handle->buffer)) return PTR_ERR(handle->buffer); - if (handle->buffer != buffer) - handle->sync_read = 0; } + handle->sync_read = (handle->buffer == buffer); handle->cur++; /* Zero pages were not included in the image, memset it and move on. */ -- cgit v1.2.3 From 7bf770f74e850c0b8d21ac143b3b2bc18813a6eb Mon Sep 17 00:00:00 2001 From: Christoph Hellwig Date: Mon, 25 Sep 2023 09:40:58 +0200 Subject: PM: hibernate: fix the kerneldoc comment for swsusp_check() and swsusp_close() The comments for both swsusp_check() and swsusp_close() don't actually describe what they are doing. Just removing the comments would probably better, but as the file is full of useless kerneldoc comments for non-exported symbols this fits in better with the style. Signed-off-by: Christoph Hellwig Signed-off-by: Rafael J. Wysocki --- kernel/power/swap.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) (limited to 'kernel/power') diff --git a/kernel/power/swap.c b/kernel/power/swap.c index 74edbce2320b..a64af0a552f9 100644 --- a/kernel/power/swap.c +++ b/kernel/power/swap.c @@ -1513,7 +1513,7 @@ end: static void *swsusp_holder; /** - * swsusp_check - Check for swsusp signature in the resume device + * swsusp_check - Open the resume device and check for the swsusp signature. * @exclusive: Open the resume device exclusively. */ @@ -1564,7 +1564,7 @@ put: } /** - * swsusp_close - close swap device. + * swsusp_close - close resume device. * @exclusive: Close the resume device which is exclusively opened. */ -- cgit v1.2.3