Re: [RFC][PATCH 07/14] printk: add kmsg_kmemdump_register

[Eugen Hristev <eugen.hristev@linaro.org>]

Hello Petr,

Thank you for your review.

On 5/5/25 18:25, Petr Mladek wrote:
> On Tue 2025-04-22 14:31:49, Eugen Hristev wrote:
>> Add kmsg_kmemdump_register, which registers prb, log_buf and infos/descs
>> to kmemdump.
>> This will allow kmemdump to be able to dump specific log buffer areas on
>> demand.
>>
>> --- a/kernel/printk/printk.c
>> +++ b/kernel/printk/printk.c
>> @@ -4650,6 +4651,18 @@ int kmsg_dump_register(struct kmsg_dumper *dumper)
>>  }
>>  EXPORT_SYMBOL_GPL(kmsg_dump_register);
>>  
>> +void kmsg_kmemdump_register(void)
>> +{
>> +	kmemdump_register("log_buf", (void *)log_buf_addr_get(), log_buf_len_get());
>> +	kmemdump_register("prb", (void *)&prb, sizeof(prb));
>> +	kmemdump_register("prb", (void *)prb, sizeof(*prb));
> 
> This looks strange. "prb" is a pointer to "struct printk_ringbuffer".
> It should be enough to register the memory with the structure.

Yes, from my perspective this should be also enough. However, when
loading the generated core dump into crash tool , the tool first looks
for the prb pointer itself, and then stops if the pointer is not readable.
After the prb pointer is being found, the crash tool dereferences it ,
and looks at the indicated address for the actual memory.
That is why the pointer is also saved as a kmemdump region in my proof
of concept.

> 
>> +	kmemdump_register("prb_descs", (void *)_printk_rb_static_descs,
>> +			  sizeof(_printk_rb_static_descs));
>> +	kmemdump_register("prb_infos", (void *)_printk_rb_static_infos,
>> +			  sizeof(_printk_rb_static_infos));
> 
> Also this looks wrong. These are static buffers which are used during
> early boot. They might later be replaced by dynamically allocated
> buffers when a bigger buffer is requested by "log_buf_len" command
> line parameter.
> 

I will double check whether the crash tool looks for these symbols or
only the memory, and come back with an answer

> I think that we need to register the memory of the structure
> and 3 more buffers. See how the bigger buffer is allocated in
> setup_log_buf().
> 
> I would expect something like:
> 
> 	unsigned int descs_count;
> 	unsigned long data_size;
> 
> 	descs_count = 2 << prb->desc_ring.count_bits;
> 	data_size = 2 << prb->data_ring.size_bits;
> 
> 	kmemdump_register("prb", (void *)prb, sizeof(*prb));
> 	kmemdump_register("prb_descs", (void *)prb->desc_ring->descs,
> 			  descs_count * sizeof(struct prb_desc));
> 	kmemdump_register("prb_infos", (void *)prb->desc_ring->infos,
> 			  descs_count * sizeof(struct printk_info));
> 	kmemdump_register("prb_data", (void *)prb->data_ring->data, data_size);
> 
> 
Thank you. It may be that in my test case, the buffer was not
extended/reallocated with a bigger one.

> But I wonder if this is enough. The current crash dump code also needs
> to export the format of the used structures, see
> log_buf_vmcoreinfo_setup().

It appears that crash tool looks for the structures into vmlinux
symbols. It can be that this information is not available to some tools,
or vmlinux not available, in which case all the used structures format
and sizes need to be exported. But right now, the crash tool does not
work without vmlinux.

> 
> Is the CONFIG_VMCORE_INFO code shared with the kmemdump, please?

I believe CONFIG_KMEMDUMP_COREIMAGE should select CONFIG_VMCORE_INFO
indeed, which is not done in my patches. Or I have not fully understood
your question ?


Eugen
> 
>> +}
>> +EXPORT_SYMBOL_GPL(kmsg_kmemdump_register);
>> +
> 
> Best Regards,
> Petr