Re: [PATCH v6 6/7] x86/crash: pass dm crypt keys to kdump kernel

[Coiby Xu]

On Wed, Dec 11, 2024 at 08:55:52PM +0800, Baoquan He wrote:
> On 10/29/24 at 01:52pm, Coiby Xu wrote:
> > 1st kernel will build up the kernel command parameter dmcryptkeys as
> > similar to elfcorehdr to pass the memory address of the stored info of
> > dm crypt key to kdump kernel.
> >
> > Signed-off-by: Coiby Xu <coxu@xxxxxxxxxx>
> > ---
> >  arch/x86/kernel/crash.c           | 20 ++++++++++++++++++--
> >  arch/x86/kernel/kexec-bzimage64.c |  7 +++++++
> >  2 files changed, 25 insertions(+), 2 deletions(-)
> >
> > diff --git a/arch/x86/kernel/crash.c b/arch/x86/kernel/crash.c
> > index 340af8155658..99d50c31db02 100644
> > --- a/arch/x86/kernel/crash.c
> > +++ b/arch/x86/kernel/crash.c
> > @@ -278,6 +278,7 @@ static int memmap_exclude_ranges(struct kimage *image, struct crash_mem *cmem,
> >  				 unsigned long long mend)
> >  {
> >  	unsigned long start, end;
> > +	int ret;
> >
> >  	cmem->ranges[0].start = mstart;
> >  	cmem->ranges[0].end = mend;
> > @@ -286,22 +287,37 @@ static int memmap_exclude_ranges(struct kimage *image, struct crash_mem *cmem,
> >  	/* Exclude elf header region */
> >  	start = image->elf_load_addr;
> >  	end = start + image->elf_headers_sz - 1;
> > -	return crash_exclude_mem_range(cmem, start, end);
> > +	ret = crash_exclude_mem_range(cmem, start, end);
> > +
> > +	if (ret)
> > +		return ret;
> > +
> > +	/* Exclude dm crypt keys region */
> > +	if (image->dm_crypt_keys_addr) {
> > +		start = image->dm_crypt_keys_addr;
> > +		end = start + image->dm_crypt_keys_sz - 1;
> > +		return crash_exclude_mem_range(cmem, start, end);
> > +	}
> > +
> > +	return ret;
> >  }
> >
> >  /* Prepare memory map for crash dump kernel */
> >  int crash_setup_memmap_entries(struct kimage *image, struct boot_params *params)
> >  {
> > +	unsigned int max_nr_ranges = 3;
>
> Define a macro and add code comment to explain why this value is taken?

Good suggestion! While drafting the reason for max_nr_ranges=3, I
realize max_nr_ranges=2 is sufficient. I also notice the fill_up_crash_elf_data
function in the same source file has similar comment. So next version
I'll add the following change,


     int crash_setup_memmap_entries(struct kimage *image, struct boot_params *params)
     {
    +       unsigned int nr_ranges = 0;
            int i, ret = 0;
            unsigned long flags;
            struct e820_entry ei;
            struct crash_memmap_data cmd;
            struct crash_mem *cmem;
     
    -       cmem = vzalloc(struct_size(cmem, ranges, 1));
    +       /*
    +        * Using random kexec_buf for passing dm crypt keys may cause a range
    +        * split. So use two slots here.
    +        */
    +       nr_ranges = 2;
    +       cmem = vzalloc(struct_size(cmem, ranges, nr_ranges));
            if (!cmem)
                    return -ENOMEM;
     
    +       cmem->max_nr_ranges = nr_ranges;
    +       cmem->nr_ranges = 0;

> >  	int i, ret = 0;
> >  	unsigned long flags;
> >  	struct e820_entry ei;
> >  	struct crash_memmap_data cmd;
> >  	struct crash_mem *cmem;
> >
> > -	cmem = vzalloc(struct_size(cmem, ranges, 1));
> > +	cmem = vzalloc(struct_size(cmem, ranges, max_nr_ranges));
> >  	if (!cmem)
> >  		return -ENOMEM;
> >
> > +	cmem->max_nr_ranges = max_nr_ranges;
> > +
> >  	memset(&cmd, 0, sizeof(struct crash_memmap_data));
> >  	cmd.params = params;
> >
> > diff --git a/arch/x86/kernel/kexec-bzimage64.c b/arch/x86/kernel/kexec-bzimage64.c
> > index 68530fad05f7..9c94428927bd 100644
> > --- a/arch/x86/kernel/kexec-bzimage64.c
> > +++ b/arch/x86/kernel/kexec-bzimage64.c
> > @@ -76,6 +76,10 @@ static int setup_cmdline(struct kimage *image, struct boot_params *params,
> >  	if (image->type == KEXEC_TYPE_CRASH) {
> >  		len = sprintf(cmdline_ptr,
> >  			"elfcorehdr=0x%lx ", image->elf_load_addr);
> > +
> > +		if (image->dm_crypt_keys_addr != 0)
> > +			len += sprintf(cmdline_ptr + len,
> > +					"dmcryptkeys=0x%lx ", image->dm_crypt_keys_addr);
> >  	}
> >  	memcpy(cmdline_ptr + len, cmdline, cmdline_len);
> >  	cmdline_len += len;
> > @@ -441,6 +445,9 @@ static void *bzImage64_load(struct kimage *image, char *kernel,
> >  		ret = crash_load_segments(image);
> >  		if (ret)
> >  			return ERR_PTR(ret);
> > +		ret = crash_load_dm_crypt_keys(image);
> > +		if (ret)
> > +			pr_debug("Either no dm crypt key or error to retrieve the dm crypt key\n");
>
> If it's error, do we need change ti to pr_debug?

Thanks for raising the concern! I think it's OK to let
crash_load_dm_crypt_keys fail since disk encryption may not be used for
kdump, thus pr_debug is sufficient. Or have I misunderstood your comment?

> >  	}
> >  #endif
> >
> > --
> > 2.47.0

--
Best regards,
Coiby