Received-SPF: pass (google.com: domain of linux-kernel+bounces-188412-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Date: Fri, 24 May 2024 15:40:27 +0800
From: Coiby Xu <coxu@redhat.com>
To: Baoquan He <bhe@redhat.com>
Cc: kexec@lists.infradead.org, Ondrej Kozina <okozina@redhat.com>, 
	Milan Broz <gmazyland@gmail.com>, Thomas Staudt <tstaudt@de.ibm.com>, 
	Daniel P =?utf-8?B?LiBCZXJyYW5nw6k=?= <berrange@redhat.com>, Kairui Song <ryncsn@gmail.com>, 
	Jan Pazdziora <jpazdziora@redhat.com>, Pingfan Liu <kernelfans@gmail.com>, 
	Dave Young <dyoung@redhat.com>, linux-kernel@vger.kernel.org, x86@kernel.org, 
	Dave Hansen <dave.hansen@intel.com>, Vitaly Kuznetsov <vkuznets@redhat.com>, 
	Vivek Goyal <vgoyal@redhat.com>
Subject: Re: [PATCH v3 4/7] crash_dump: reuse saved dm crypt keys for
 CPU/memory hot-plugging
Message-ID: <c5xg5akvf5ikphio4osdbg2giclzvxu5dbsivrvtht6rxbgs52@hriqkr6kdui5>
References: <20240425100434.198925-1-coxu@redhat.com>
 <20240425100434.198925-5-coxu@redhat.com>
 <ZkwZfcfTOL1UGKJb@MiWiFi-R3L-srv>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii; format=flowed
Content-Disposition: inline
In-Reply-To: <ZkwZfcfTOL1UGKJb@MiWiFi-R3L-srv>

On Tue, May 21, 2024 at 11:48:49AM +0800, Baoquan He wrote:
>On 04/25/24 at 06:04pm, Coiby Xu wrote:
>> When there is CPU/memory hot-plugging, the kdump kernel image and initrd
>> will be reloaded. The user space can write the "reuse" command to
>> /sys/kernel/crash_dm_crypt_key so the stored keys can be re-saved again.
>>
>> Note currently only x86 (commit ea53ad9cf73b ("x86/crash: add x86 crash
>> hotplug support")) and ppc (WIP) supports the new infrastructure
>> (commit 247262756121 ("crash: add generic infrastructure for crash
>> hotplug support")). If the new infrastructure get extended to all arches,
>> this patch can be dropped.
>>
>> Signed-off-by: Coiby Xu <coxu@redhat.com>
>> ---
>>  kernel/crash_dump_dm_crypt.c | 30 ++++++++++++++++++++++++++----
>>  1 file changed, 26 insertions(+), 4 deletions(-)
>>
>> diff --git a/kernel/crash_dump_dm_crypt.c b/kernel/crash_dump_dm_crypt.c
>> index b9997fb53351..6ac1fabdb6cb 100644
>> --- a/kernel/crash_dump_dm_crypt.c
>> +++ b/kernel/crash_dump_dm_crypt.c
>> @@ -10,12 +10,13 @@
>>  // The key scription has the format: cryptsetup:UUID 11+36+1(NULL)=48
>>  #define KEY_DESC_LEN 48
>>
>> -static char *STATE_STR[] = {"fresh", "initialized", "recorded", "loaded"};
>> +static char *STATE_STR[] = {"fresh", "initialized", "recorded", "loaded", "reuse"};
>>  enum STATE_ENUM {
>>  	FRESH = 0,
>>  	INITIALIZED,
>>  	RECORDED,
>>  	LOADED,
>> +	REUSE,
>>  } state;
>>
>>  static unsigned int key_count;
>> @@ -90,12 +91,31 @@ static int record_key_desc(const char *buf, struct dm_crypt_key *dm_key)
>>  	return 0;
>>  }
>>
>> +static void get_keys_from_kdump_reserved_memory(void)
>> +{
>> +	struct keys_header *keys_header_loaded;
>> +
>> +	arch_kexec_unprotect_crashkres();
>
>I don't see the corresponging arch_kexec_protect_crashkres() in the
>following flow. Aren't they a pair when used?

Thanks for raising the concern! The user space is supposed to load the
kdump kernel as next step. But for code readability and security, it's
better to add arch_kexec_protect_crashkres.


-- 
Best regards,
Coiby