Re: [PATCH v5 1/3] powerpc: make fadump resilient with memory add/remove events

[Sourabh Jain <sourabhjain@linux.ibm.com>]

Hello Michael,


On 09/11/23 17:44, Michael Ellerman wrote:
> Hi Sourabh,
>
> This seems like a good change to the design, but I'm confused by
> some things, more below ...

Thanks.

> Sourabh Jain <sourabhjain@linux.ibm.com> writes:
> ...
>> Table 1 below illustrates kernel's ability to collect dump if either the
>> first/crashed kernel or the second/fadump kernel does not have the
>> changes introduced here. Consider the 'old kernel' as the kernel without
>> this patch, and the 'new kernel' as the kernel with this patch included.
>>
>> +----------+------------------------+------------------------+-------+
>> | scenario |  first/crashed kernel  |  second/fadump kernel  |  Dump |
>> +----------+------------------------+------------------------+-------+
>> |    1     |       old kernel       |        new kernel      |  Yes  |
>> +----------+------------------------+------------------------+-------+
>> |    2     |       new kernel       |        old kernel      |  No   |
>> +----------+------------------------+------------------------+-------+
>>
>> 			      Table 1
>>
>> Scenario 1:
>> -----------
>> Since the magic number of fadump header is updated, the second kernel
>> can differentiate the crashed kernel is of type 'new kernel' or
>> 'old kernel' and act accordingly. In this scenario, since the crashed
>> kernel is of type 'old kernel,' the fadump kernel skips elfcorehdr
>> creation and uses the one prepared in the first kernel itself to collect
>> the dump.
>>
>> Scenario 2:
>> -----------
>> Since 'old kernel' as the fadump kernel is NOT capable of processing
>> fadump header with updated magic number from 'new kernel' hence it
>> gracefully fails with the below error and dump collection fails in this
>> scenario.
>>
>> [    0.007365] rtas fadump: Crash info header is not valid.
>>
>> Add a version field to the fadump_crash_info_header structure to avoid
>> the need to change its magic number in the future. Adding a version
>> field to the fadump header was one of the TODO items listed in
>> Documentation/powerpc/firmware-assisted-dump.rst.
> This is a good analysis of the issues with different kernel versions,
> and seems like an OK trade off, ie. that old kernels can't process dumps
> from new kernels.
>
> But do we actually support using different kernel versions for the
> crash/dump kernel?
>
> Because AFAICS the fadump_crash_info_header is not safe across kernel
> versions, in its current form or with your changes.

Yeah, I was also under the impression that it is not supported, but I 
was not aware
that the size of pt_regs and cpumask can change based on the configuration.

>
>> diff --git a/arch/powerpc/include/asm/fadump-internal.h b/arch/powerpc/include/asm/fadump-internal.h
>> index 27f9e11eda28..7be3d8894520 100644
>> --- a/arch/powerpc/include/asm/fadump-internal.h
>> +++ b/arch/powerpc/include/asm/fadump-internal.h
>> @@ -42,7 +42,25 @@ static inline u64 fadump_str_to_u64(const char *str)
>>   
>>   #define FADUMP_CPU_UNKNOWN		(~((u32)0))
>>   
>> -#define FADUMP_CRASH_INFO_MAGIC		fadump_str_to_u64("FADMPINF")
>> +/*
>> + * The introduction of new fields in the fadump crash info header has
>> + * led to a change in the magic key, from `FADMPINF` to `FADMPSIG`.
>> + * This alteration ensures backward compatibility, enabling the kernel
>> + * with the updated fadump crash info to handle kernel dumps from older
>> + * kernels.
>> + *
>> + * To prevent the need for further changes to the magic number in the
>> + * event of future modifications to the fadump header, a version field
>> + * has been introduced to track the fadump crash info header version.
>> + *
>> + * Historically, there was no connection between the magic number and
>> + * the fadump crash info header version. However, moving forward, the
>> + * `FADMPINF` magic number in header will be treated as version 0, while
>> + * the `FADMPSIG` magic number in header will include a version field to
>> + * determine its version.
>> + */
>> +#define FADUMP_CRASH_INFO_MAGIC		fadump_str_to_u64("FADMPSIG")
>> +#define FADUMP_VERSION			1
>>   
>>   /* fadump crash info structure */
>>   struct fadump_crash_info_header {
>> @@ -51,6 +69,10 @@ struct fadump_crash_info_header {
>>
> struct fadump_crash_info_header {
> 	u64		magic_number;
> 	u64		elfcorehdr_addr;
>
>>   	u32		crashing_cpu;
>>   	struct pt_regs	regs;
>>   	struct cpumask	cpu_mask;
>> +	u32		version;
>> +	u64		elfcorehdr_size;
>> +	u64		vmcoreinfo_raddr;
>> +	u64		vmcoreinfo_size;
>>   };
> The reason I say it's not safe is because pt_regs and especially cpumask
> can change size depending on the kernel configuration.
>
> pt_regs probably doesn't change size in practice for common kernel
> configurations, but some of the fields are under #ifdef.
>
> cpumask on the other hand is directly controlled by CONFIG_NR_CPUS. So
> if the first and second kernel have a different value for NR_CPUS they
> will disagree on the size of the struct.
>
> That has presumably worked OK so far because folks tend to use the same, or
> similar kernels for the first/second kernel. And also the cpumask is the
> last element of the struct, so a disagreement about it size doesn't
> affect the location of other fields.
>
> However with your new design the version field will move based on the
> cpumask size, which will potentially break detection of the version by
> the second kernel.
>
> At least that's how it looks to me, I don't see any checks anywhere that
> will save us, or did I miss something?
I agree with you. If the sizes of pt_regs and cpu_mask are different between
the first/crash kernel and the fadump/second kernel, the dump collection
might not work correctly.

Given that dump capture is not supported across kernel versions, I think 
it is
better to keep new attributes introduced in this patch to struct 
fadump_crash_info_header
before pt_regs and cpumask. For example:

/* fadump crash info structure */
struct fadump_crash_info_header {
     u64        magic_number;
+  u32        version;
+  u64        vmcoreinfo_raddr;
+  u64        vmcoreinfo_size;
     u64        elfcorehdr_addr;
+  u64        elfcorehdr_size;
     u32        crashing_cpu;
     struct pt_regs    regs;
     struct cpumask    cpu_mask;
};

Kernel with this patch included will not process the dump if 
fadump_crash_info_header
holds old magic number.

Something like:

     if (fdh->magic_number == fadump_str_to_u64("FADMPINF")) {
         pr_err("Incompatible fadump header, can't process the dump");
     }

Does this approach look good to you?

Thanks,
Sourabh