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

Coiby Xu
2024-12-23T01:30:17Z

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@redhat.com>
>> ---
>> 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