On Thu, Jun 28, 2018 at 10:52:07PM +0800, Yu Chen wrote: > Hi, > On Thu, Jun 28, 2018 at 10:28:56PM +0800, joeyli wrote: > > On Thu, Jun 28, 2018 at 09:50:17PM +0800, Yu Chen wrote: > > > Hi, > > > On Thu, Jun 28, 2018 at 09:07:20PM +0800, joeyli wrote: > > > > Hi Chen Yu, > > > > > > > > On Wed, Jun 20, 2018 at 05:40:32PM +0800, Chen Yu wrote: > > > > > Use the helper functions introduced previously to encrypt > > > > > the page data before they are submitted to the block device. > > > > > Besides, for the case of hibernation compression, the data > > > > > are firstly compressed and then encrypted, and vice versa > > > > > for the resume process. > > > > > > > > > > > > > I want to suggest my solution that it direct signs/encrypts the > > > > memory snapshot image. This solution is already shipped with > > > > SLE12 a couple of years: > > > > > > > > https://github.com/joeyli/linux-s4sign/commits/s4sign-hmac-encrypted-key-v0.2-v4.17-rc3 > > > > > > > I did not see image page encryption in above link, if I understand > > > > PM / hibernate: Generate and verify signature for snapshot image > > https://github.com/joeyli/linux-s4sign/commit/bae39460393ada4c0226dd07cd5e3afcef86b71f > > > > PM / hibernate: snapshot image encryption > > https://github.com/joeyli/linux-s4sign/commit/6a9a0113bb221c036ebd0f6321b7191283fe4929 > > > > The above patches sign and encrypt the data pages in snapshot image. > > It puts the signature to header. > > > It looks like your signature code can be simplyly added on top of the > solution we proposed here, how about we collaborating on this task?
OK, I will base on your user key solution to respin my signature patches. > just my 2 cents, > 1. The cryption code should be indepent of the snapshot code, and > this is why we implement it as a kernel module for that in PATCH[1/3]. Why the cryption code must be indepent of snapshot code? > 2. There's no need to traverse the snapshot image twice, if the > image is large(there's requirement on servers now) we can > simplyly do the encryption before the disk IO, and this is > why PATCH[2/3] looks like this. If the encryption solution is only for block device, then the uswsusp interface must be locked-down when kernel is in locked mode: uswsusp: Disable when the kernel is locked down https://git.kernel.org/pub/scm/linux/kernel/git/dhowells/linux-fs.git/commit/?h=lockdown-20180410&id=8732c1663d7c0305ae01ba5a1ee4d2299b7b4612 I still suggest to keep the solution to direct encript the snapshot image for uswsusp because the snapshot can be encrypted by kernel before user space get it. I mean that if the uswsusp be used, then kernel direct encrypts the snapshot image, otherwise kernel encrypts pages before block io. On the other hand, I have a question about asynchronous block io. Please see below... > > > > > Suggested-by: Rafael J. Wysocki <rafael.j.wyso...@intel.com> > > > > > Cc: Rafael J. Wysocki <rafael.j.wyso...@intel.com> > > > > > Cc: Pavel Machek <pa...@ucw.cz> > > > > > Cc: Len Brown <len.br...@intel.com> > > > > > Cc: Borislav Petkov <b...@alien8.de> > > > > > Cc: "Lee, Chun-Yi" <j...@suse.com> > > > > > Cc: linux...@vger.kernel.org > > > > > Cc: linux-kernel@vger.kernel.org > > > > > Signed-off-by: Chen Yu <yu.c.c...@intel.com> > > > > > --- > > > > > kernel/power/power.h | 1 + > > > > > kernel/power/swap.c | 215 > > > > > ++++++++++++++++++++++++++++++++++++++++++++++++--- > > > > > 2 files changed, 205 insertions(+), 11 deletions(-) [...snip] > > > > > /* kernel/power/hibernate.c */ > > > > > extern int swsusp_check(void); > > > > > diff --git a/kernel/power/swap.c b/kernel/power/swap.c > > > > > index c2bcf97..2b6b3d0 100644 > > > > > --- a/kernel/power/swap.c > > > > > +++ b/kernel/power/swap.c [...snip] > > > > > @@ -1069,18 +1171,42 @@ static int load_image(struct swap_map_handle > > > > > *handle, > > > > > if (!m) > > > > > m = 1; > > > > > nr_pages = 0; > > > > > + crypto_page_idx = 0; > > > > > + if (handle->crypto) { > > > > > + crypt_buf = (void *)get_zeroed_page(GFP_KERNEL); > > > > > + if (!crypt_buf) > > > > > + return -ENOMEM; > > > > > + } > > > > > start = ktime_get(); > > > > > for ( ; ; ) { > > > > > ret = snapshot_write_next(snapshot); > > > > > if (ret <= 0) > > > > > break; > > > > > - ret = swap_read_page(handle, data_of(*snapshot), &hb); > > > > > + if (handle->crypto) > > > > > + ret = swap_read_page(handle, crypt_buf, &hb); > > > > > + else > > > > > + ret = swap_read_page(handle, > > > > > data_of(*snapshot), &hb); > > > > > if (ret) > > > > > break; > > > > > if (snapshot->sync_read) > > > > > ret = hib_wait_io(&hb); In snapshot_write_next(), the logic will clean the snapshot->sync_read when the buffer page doesn't equal to the original page. Which means that the page can be read by asynchronous block io. Otherwise, kernel calls hib_wait_io() to wait until the block io was done. > > > > > if (ret) > > > > > break; > > > > > + if (handle->crypto) { > > > > > + /* > > > > > + * Need a decryption for the > > > > > + * data read from the block > > > > > + * device. > > > > > + */ > > > > > + ret = crypto_data(crypt_buf, PAGE_SIZE, > > > > > + data_of(*snapshot), > > > > > + PAGE_SIZE, > > > > > + false, > > > > > + crypto_page_idx); > > > > > + if (ret) > > > > > + break; > > > > > + crypto_page_idx++; > > > > > + } The decryption is here in the for-loop. But maybe the page is still in the block io queue for waiting the batch read? The page content is not really read to memory when the crypto_data be run? > > > > > if (!(nr_pages % m)) > > > > > pr_info("Image loading progress: %3d%%\n", > > > > > nr_pages / m * 10); nr_pages++; } err2 = hib_wait_io(&hb); stop = ktime_get(); When the for-loop is break, the above hib_wait_io(&hb) guarantees that all asynchronous block io are done. Then all pages are read to memory. I think that the decryption code must be moved after for-loop be break. Or there have any callback hook in the asynchronous block io that we can put the encryption code after the block io read the page. Thanks a lot! Joey Lee
