Message-ID: <20181001202244.GB8350@zn.tnic>
Date:   Mon, 1 Oct 2018 22:22:44 +0200
From:   Borislav Petkov <bp@...e.de>
To:     lijiang <lijiang@...hat.com>
Cc:     linux-kernel@...r.kernel.org, kexec@...ts.infradead.org,
        tglx@...utronix.de, mingo@...hat.com, hpa@...or.com,
        x86@...nel.org, akpm@...ux-foundation.org,
        dan.j.williams@...el.com, thomas.lendacky@....com,
        bhelgaas@...gle.com, baiyaowei@...s.chinamobile.com, tiwai@...e.de,
        brijesh.singh@....com, dyoung@...hat.com, bhe@...hat.com,
        jroedel@...e.de
Subject: Re: [PATCH v9 4/4] kdump/vmcore: support encrypted old memory with
 SME enabled

On Sun, Sep 30, 2018 at 04:37:41PM +0800, lijiang wrote:
> In kdump kernel, the old memory needs to be dumped into vmcore file.
> If SME is enabled in the first kernel, the old memory has to be
> remapped with the memory encryption mask, which will be automatically
> decrypted when read from DRAM.
> 
> For SME kdump, there are two cases that doesn't support:

Get rid of those two cases in the commit message.

> 
>  ----------------------------------------------
> | first-kernel | second-kernel | kdump support |
> |      (mem_encrypt=on|off)    |   (yes|no)    |
> |--------------+---------------+---------------|
> |     on       |     on        |     yes       |
> |     off      |     off       |     yes       |
> |     on       |     off       |     no        |
> |     off      |     on        |     no        |
> |______________|_______________|_______________|
> 
> 1. SME is enabled in the first kernel, but SME is disabled in kdump kernel
> In this case, because the old memory is encrypted, it can't be decrypted.
> The root cause is that the encryption key is not visible to any software
> runnint on the CPU cores(AMD cpu with SME), and is randomly generated on
> eache system reset. That is to say, kdump kernel won't have a chance to
> get the encryption key. So the encrypted memory can not be decrypted
> unless SME is active.
> 
> 2. SME is disabled in the first kernel, but SME is enabled in kdump kernel
> On the one hand, the old memory is decrypted, the old memory can be dumped
> as usual, so SME doesn't need to be enabled in kdump kernel; On the other
> hand, it will increase the complexity of the code, because that will have
> to consider how to pass the SME flag from the first kernel to the kdump
> kernel, it is really too expensive to do this.
> 
> This patches are only for SME kdump, the patches don't support SEV kdump.
> 
> Signed-off-by: Lianbo Jiang <lijiang@...hat.com>
> Reviewed-by: Tom Lendacky <thomas.lendacky@....com>

You cannot keep Reviewed-by: tags on patches which you change in a
non-trivial manner.

> ---
> Changes since v7:
> 1. Delete a file arch/x86/kernel/crash_dump_encrypt.c, and move the
> copy_oldmem_page_encrypted() to arch/x86/kernel/crash_dump_64.c, also
> rewrite some functions.(Suggested by Borislav)
> 2. Modify all code style issue.(Suggested by Borislav)
> 3. Remove a reduntant header file.(Suggested by Borislav)
> 4. Improve patch log.(Suggested by Borislav)
> 5. Modify compile error "fs/proc/vmcore.c:115: undefined reference
>    to `copy_oldmem_page_encrypted'"
> 6. Modify compile error "arch/x86//kernel/crash_dump_64.c:93:9:
>    error: redefinition of 'copy_oldmem_page_encrypted'"
> 
>  arch/x86/kernel/crash_dump_64.c | 65 ++++++++++++++++++++++++++++-----
>  fs/proc/vmcore.c                | 24 +++++++++---
>  include/linux/crash_dump.h      | 13 +++++++
>  3 files changed, 87 insertions(+), 15 deletions(-)
> 
> diff --git a/arch/x86/kernel/crash_dump_64.c b/arch/x86/kernel/crash_dump_64.c
> index 4f2e0778feac..6adbde592c44 100644
> --- a/arch/x86/kernel/crash_dump_64.c
> +++ b/arch/x86/kernel/crash_dump_64.c
> @@ -12,7 +12,7 @@
>  #include <linux/io.h>
>  
>  /**
> - * copy_oldmem_page - copy one page from "oldmem"
> + * __copy_oldmem_page - copy one page from "old memory encrypted or decrypted"

Dammit, what's it with those "old memory encrypted or decrypted" in quotation
marks?! What is wrong with simply saying:

Copy one page of the old kernel's memory. If @encrypted is set, the old
memory will be remapped with the encryption mask.

How hard is that?!

>   * @pfn: page frame number to be copied
>   * @buf: target memory address for the copy; this can be in kernel address
>   *	space or user address space (see @userbuf)
> @@ -20,31 +20,78 @@
>   * @offset: offset in bytes into the page (based on pfn) to begin the copy
>   * @userbuf: if set, @buf is in user address space, use copy_to_user(),
>   *	otherwise @buf is in kernel address space, use memcpy().
> + * @encrypted: if true, the old memory is encrypted.
> + *             if false, the old memory is decrypted.
>   *
> - * Copy a page from "oldmem". For this page, there is no pte mapped
> - * in the current kernel. We stitch up a pte, similar to kmap_atomic.
> + * Copy a page from "old memory encrypted or decrypted". For this page, there
> + * is no pte mapped in the current kernel. We stitch up a pte, similar to
> + * kmap_atomic.
>   */

This function is static now - why does it need to keep the comments
above it? And you've duplicated almost the same comment *three* times
now. Why?

Have the whole comment *once* and only one line sentences over the other
functions explaining the difference only.

> -ssize_t copy_oldmem_page(unsigned long pfn, char *buf,
> -		size_t csize, unsigned long offset, int userbuf)
> +static ssize_t __copy_oldmem_page(unsigned long pfn, char *buf, size_t csize,
> +				  unsigned long offset, int userbuf,
> +				  bool encrypted)
>  {
>  	void  *vaddr;

-- 
Regards/Gruss,
    Boris.

SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)

Powered by blists - more mailing lists