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

[Coiby Xu]

On Thu, Dec 26, 2024 at 11:48:04AM +0800, Baoquan He wrote:
>On 12/23/24 at 09:16am, Coiby Xu wrote:
>> On Wed, Dec 11, 2024 at 08:55:52PM +0800, Baoquan He wrote:
>> > On 10/29/24 at 01:52pm, Coiby Xu wrote:
>......
>> > > 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?
>
>If crash_load_dm_crypt_keys() returned error, shouldn't we handle them
>separately? If disk encryption is not used, we return a specific value
>to indicate that, surely no need to pr_err(). However, if disk
>encryption is used for kdump but error is caused and failed
>crash_load_dm_crypt_keys(), why don't we error out for the case?
>
>Maybe below change can be made to differentiate them?
>
>diff --git a/kernel/crash_dump_dm_crypt.c b/kernel/crash_dump_dm_crypt.c
>index f72a88b7d106..327f84ea57c5 100644
>--- a/kernel/crash_dump_dm_crypt.c
>+++ b/kernel/crash_dump_dm_crypt.c
>@@ -400,7 +400,7 @@ int crash_load_dm_crypt_keys(struct kimage *image)
>
>        if (key_count <= 0) {
>                kexec_dprintk("No dm-crypt keys\n");
>-               return -EINVAL;
>+               return -ENOENT;
>        }

Now I see your point! Thanks for the explanation and suggestion! Yes,
it's better to fail early if crash_load_dm_crypt_keys failed when
encrypted disk is used for kdump . I'll apply the change to next
version.

-- 
Best regards,
Coiby