[PATCH v4 2/7] crash_dump: make dm crypt keys persist for the kdump kernel

[Coiby Xu]

On Mon, Jun 10, 2024 at 09:18:53AM +0800, Baoquan He wrote:
>On 06/07/24 at 08:27pm, Coiby Xu wrote:
>> On Wed, Jun 05, 2024 at 04:22:12PM +0800, Baoquan He wrote:
>> > On 05/23/24 at 01:04pm, Coiby Xu wrote:
>> > .....
>> > > diff --git a/kernel/crash_dump_dm_crypt.c b/kernel/crash_dump_dm_crypt.c
>> > > new file mode 100644
>> > > index 000000000000..78809189084a
>> > > --- /dev/null
>> > > +++ b/kernel/crash_dump_dm_crypt.c
>> > > @@ -0,0 +1,113 @@
>> > > +// SPDX-License-Identifier: GPL-2.0-only
>> [...]
>> > > +
>> > > +static unsigned int key_count;
>> > > +static size_t keys_header_size;
>> >
>> > These two global variables seems not so necessary. Please see comment at
>> > below.
>>
>> Thanks for the comment! But I think it's better to keep these two static
>> variables for reasons as will be explained later.
>>
>> >
>> > > +
>> > > +struct dm_crypt_key {
>> > > +	unsigned int key_size;
>> > > +	char key_desc[KEY_DESC_LEN];
>> > > +	u8 data[KEY_SIZE_MAX];
>> > > +};
>> > > +
>> > > +static struct keys_header {
>> > > +	unsigned int key_count;
>> >                     ~~~~~~~~
>> >                     This is the max number a system have from init();
>> > You can add one field member to record how many key slots have been
>> > used.
>> > > +	struct dm_crypt_key keys[] __counted_by(key_count);
>> > > +} *keys_header;
>> >
>> > Maybe we can rearrange the keys_header like below, the name may not be
>> > very appropriate though.
>> >
>> > static struct keys_header {
>> > 	unsigned int max_key_slots;
>> > 	unsigned int used_key_slots;
>> > 	struct dm_crypt_key keys[] __counted_by(key_count);
>> > } *keys_header;
>>
>> Thanks for the suggestion! Since 1) KEY_NUM_MAX already defines the
>> maximum number of dm crypt keys 2) we only need to let the kdump kernel
>> now how many keys are saved, so I simply use total_keys instead of
>> key_count in struct keys_header in v5,
>>
>> static struct keys_header {
>> 	unsigned int total_keys;
>> 	struct dm_crypt_key keys[] __counted_by(total_keys);
>> } *keys_header;
>>
>> Hopefully this renaming will improve code readability.
>
>If you add key_count into keys_header, then kdump kernel will know how
>many keys are really saved and need be retrieved. What's your concern
>when you have to put key_count outside and take it as a global variable?

Yes, the kdump kernel can know how many keys by reading keys_header. But
1st kernel needs to know how many keys are recorded so far as user space
is expected to tell the kernel which keys are needed one by one and the
key_count will be increased witch each new key.

>
>>
>> >
>> > >
>> >
>> > > +
>> > > +static size_t get_keys_header_size(struct keys_header *keys_header,
>> > > +				   size_t key_count)
>> > > +{
>> > > +	return struct_size(keys_header, keys, key_count);
>> > > +}
>> >
>> > I personally don't think get_keys_header_size is so necessary. If we
>> > have to keep it, may be we can remove the global variable
>> > keys_header_size, we can call get_keys_header_size() and use local
>> > variable to record the value instead.
>>
>> Thanks for the suggestion! But the kdump kernel also need to call
>> get_keys_header_size in later patches.
>
>If so, you can remove keys_header_size now. You can define local
>variable to take the newly calculated value. keys_header_size seems not
>so necesary.

Sure, new version will have the global variable keys_header_size
removed which can make the code a bit easier to reason about, thanks!

>
>By the way, you don't need to rush to post v5. When people review
>patches, agreement need be reached after discussion. Then next version
>can be posted.

Thanks for the suggestion! I'll wait for the consensus to be truly
reached next time.

And sorry, it takes longer time than I have expected to evaluate all the
feedback. In retrospective, maybe it's better to respond to each
feedback in a parallel manner than pondering over all the feedback
first.

-- 
Best regards,
Coiby