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

[Petr Mladek <pmladek@suse.com>]

On Mon 2025-05-05 18:51:19, Eugen Hristev wrote:
> 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.

I see. It makes perfect sense to store the pointer as well after all.

> >> +	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.

I guess so. A bigger buffer is allocated either when explicitly
requested by "log_buf_len=" command line option. Or when the kernel
is running on a huge system with many CPUs and log_buf_add_cpu()
decides that the default buffer is not big enough for backtraces from
all CPUs.

> > 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 ?

I do not see CONFIG_VMCORE_INFO selected in drivers/debug/Kconfig.
But maybe the dependency is defined another way.

Honestly, I did not study all these details. I focused primary on
the printk-related interface and commented what came to my mind.

Also I was not sure how the dumped memory can be analyzed. I expected
that it should be readable by the "crash" tool. But I did not see it explained
in Documentation/debug/kmemdump.rst.

Best Regards,
Petr