Re: [PATCH] kexec/crash: no crash update when kexec in progress

[Baoquan He <bhe@redhat.com>]

On 08/20/24 at 12:10pm, Sourabh Jain wrote:
> Hello Baoquan,
> 
> On 19/08/24 11:45, Baoquan He wrote:
> > On 08/19/24 at 09:45am, Sourabh Jain wrote:
> > > Hello Michael and Boaquan
> > > 
> > > On 01/08/24 12:21, Sourabh Jain wrote:
> > > > Hello Michael,
> > > > 
> > > > On 01/08/24 08:04, Michael Ellerman wrote:
> > > > > Sourabh Jain <sourabhjain@linux.ibm.com> writes:
> > > > > > The following errors are observed when kexec is done with SMT=off on
> > > > > > powerpc.
> > > > > > 
> > > > > > [  358.458385] Removing IBM Power 842 compression device
> > > > > > [  374.795734] kexec_core: Starting new kernel
> > > > > > [  374.795748] kexec: Waking offline cpu 1.
> > > > > > [  374.875695] crash hp: kexec_trylock() failed, elfcorehdr may
> > > > > > be inaccurate
> > > > > > [  374.935833] kexec: Waking offline cpu 2.
> > > > > > [  375.015664] crash hp: kexec_trylock() failed, elfcorehdr may
> > > > > > be inaccurate
> > > > > > snip..
> > > > > > [  375.515823] kexec: Waking offline cpu 6.
> > > > > > [  375.635667] crash hp: kexec_trylock() failed, elfcorehdr may
> > > > > > be inaccurate
> > > > > > [  375.695836] kexec: Waking offline cpu 7.
> > > > > Are they actually errors though? Do they block the actual kexec from
> > > > > happening? Or are they just warnings in dmesg?
> > > > The kexec kernel boots fine.
> > > > 
> > > > This warning appears regardless of whether the kdump kernel is loaded.
> > > > 
> > > > However, when the kdump kernel is loaded, we will not be able to update
> > > > the kdump image (FDT).
> > > > I think this should be fine given that kexec is in progress.
> > > > 
> > > > Please let me know your opinion.
> > > > 
> > > > > Because the fix looks like it could be racy.
> > > > It seems like it is racy, but given that kexec takes the lock first and
> > > > then
> > > > brings the CPU up, which triggers the kdump image, which always fails to
> > > > update the kdump image because it could not take the same lock.
> > > > 
> > > > Note: the kexec lock is not released unless kexec boot fails.
> > > Any comments or suggestions on this fix?
> > Is this a little better?
> > 
> > diff --git a/kernel/crash_core.c b/kernel/crash_core.c
> > index 63cf89393c6e..0355ffb712f4 100644
> > --- a/kernel/crash_core.c
> > +++ b/kernel/crash_core.c
> > @@ -504,7 +504,7 @@ int crash_check_hotplug_support(void)
> >   	crash_hotplug_lock();
> >   	/* Obtain lock while reading crash information */
> > -	if (!kexec_trylock()) {
> > +	if (!kexec_trylock() && kexec_in_progress) {
> >   		pr_info("kexec_trylock() failed, elfcorehdr may be inaccurate\n");
> >   		crash_hotplug_unlock();
> >   		return 0;
> > @@ -539,7 +539,7 @@ static void crash_handle_hotplug_event(unsigned int hp_action, unsigned int cpu,
> >   	crash_hotplug_lock();
> >   	/* Obtain lock while changing crash information */
> > -	if (!kexec_trylock()) {
> > +	if (!kexec_trylock() && kexec_in_progress) {
> >   		pr_info("kexec_trylock() failed, elfcorehdr may be inaccurate\n");
> >   		crash_hotplug_unlock();
> >   		return;
> 
> Ideally, when `kexec_in_progress` is True, there should be no way to acquire
> the kexec lock.
> Therefore, calling `kexec_trylock()` before checking `kexec_in_progress` is
> not helpful.
> The kernel will print the error message "kexec_trylock() failed, elfcorehdr
> may be inaccurate."
> So, with the above changes, the original problem remains unsolved.
> 
> However, after closely inspecting the `kernel/kexec_core.c:kernel_kexec()`
> function, I discovered
> an exceptional case where my patch needs an update. The issue arises when
> the system returns
> from the `machine_kexec()` function, which indicates that kexec has failed.
> 
> In this scenario, the kexec lock is released, but `kexec_in_progress`
> remains True.
> 
> I am unsure why `kexec_in_progress` is NOT set to False when kexec fails.
> Was this by design,
> or was it an oversight because returning from the `machine_kexec()` function
> is highly unlikely?
> 
> Here is my proposal to address the original problem along with the
> exceptional case I described
> above.
> 
> Let's implement two patches:
> 
> 1. A patch that sets `kexec_in_progress` to False if the system returns from
> `machine_kexec()` before

I don't think we have chance to return from machine_kexec() after
triggering kexec/kdump jumping. The KEXEC_JUMP could return, but I'v
never heard people using it.

>    unlocking the kexec lock in the `kernel_kexec()` function.
> 
>    ```
>    diff --git a/kernel/kexec_core.c b/kernel/kexec_core.c
>    index c0caa14880c3..b41277183455 100644
>    --- a/kernel/kexec_core.c
>    +++ b/kernel/kexec_core.c
>    @@ -1069,6 +1069,7 @@ int kernel_kexec(void)
>    #endif
> 
>     Unlock:
>    +      kexec_in_progress = false;
>            kexec_unlock();
>            return error;
>     ```
> 
> 2. A patch to return early from the `crash_handle_hotplug_event()` function
> if `kexec_in_progress` is
>    set to True. This is essentially my original patch.

There's a race gap between the kexec_in_progress checking and the
setting it to true which Michael has mentioned. That's why I think
maybe checking kexec_in_progress after failing to retriving
__kexec_lock is a little better, not very sure.

> 
> Please share your comments on the new approach.
> 
> Thank you for review.
> 
> - Sourabh Jain
>