Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Mon, 28 Nov 2022 18:00:03 +0100
From:   Philipp Rudo <prudo@redhat.com>
To:     Ricardo Ribalda <ribalda@chromium.org>
Cc:     Eric Biederman <ebiederm@xmission.com>,
        Baoquan He <bhe@redhat.com>,
        Sergey Senozhatsky <senozhatsky@chromium.org>,
        Ross Zwisler <zwisler@kernel.org>, kexec@lists.infradead.org,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH] kexec: Enable runtime allocation of crash_image
Message-ID: <20221128180003.49747650@rotkaeppchen>
In-Reply-To: <20221124-kexec-noalloc-v1-0-d78361e99aec@chromium.org>
References: <20221124-kexec-noalloc-v1-0-d78361e99aec@chromium.org>
Organization: Red Hat inc.
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Precedence: bulk

Hi Ricardo,

On Thu, 24 Nov 2022 23:23:36 +0100
Ricardo Ribalda <ribalda@chromium.org> wrote:

> Usually crash_image is defined statically via the crashkernel parameter
> or DT.
> 
> But if the crash kernel is not used, or is smaller than then
> area pre-allocated that memory is wasted.
> 
> Also, if the crash kernel was not defined at bootime, there is no way to
> use the crash kernel.
> 
> Enable runtime allocation of the crash_image if the crash_image is not
> defined statically. Following the same memory allocation/validation path
> that for the reboot kexec kernel.
> 
> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>

I don't think this patch will work as intended. For one you omit
setting the image->type to KEXEC_TYPE_CRASH. But when you grep for that
type you will find that there is a lot of special handling done for it.
I don't believe that this can simply be skipped without causing
problems.

Furthermore I think you have missed one important detail. The memory
reserved for the crash kernel is not just a buffer for the image but
the memory it runs in! For that it has to be a continuous piece of
physical memory with usually some additional arch specific limitations.
When allocated dynamically all those limitations need to be considered.
But a standard kexec doesn't care about those limitations as it doesn't
care about the os running before itself. It can simply overwrite the
memory when booting. But if the crash kernel does the same it will
corrupt the dump it is supposed to generate.

Thanks
Philipp

> ---
> kexec: Enable runtime allocation of crash_image
> 
> To: Eric Biederman <ebiederm@xmission.com>
> Cc: kexec@lists.infradead.org
> Cc: linux-kernel@vger.kernel.org
> Cc: Sergey Senozhatsky <senozhatsky@chromium.org>
> Cc: linux-kernel@vger.kernel.org
> Cc: Ross Zwisler <zwisler@kernel.org>
> Cc: Philipp Rudo <prudo@redhat.com>
> Cc: Baoquan He <bhe@redhat.com>
> ---
>  include/linux/kexec.h | 1 +
>  kernel/kexec.c        | 9 +++++----
>  kernel/kexec_core.c   | 5 +++++
>  kernel/kexec_file.c   | 7 ++++---
>  4 files changed, 15 insertions(+), 7 deletions(-)
> 
> diff --git a/include/linux/kexec.h b/include/linux/kexec.h
> index 41a686996aaa..98ca9a32bc8e 100644
> --- a/include/linux/kexec.h
> +++ b/include/linux/kexec.h
> @@ -427,6 +427,7 @@ extern int kexec_load_disabled;
>  extern bool kexec_in_progress;
>  
>  int crash_shrink_memory(unsigned long new_size);
> +bool __crash_memory_valid(void);
>  ssize_t crash_get_memory_size(void);
>  
>  #ifndef arch_kexec_protect_crashkres
> diff --git a/kernel/kexec.c b/kernel/kexec.c
> index cb8e6e6f983c..b5c17db25e88 100644
> --- a/kernel/kexec.c
> +++ b/kernel/kexec.c
> @@ -28,7 +28,7 @@ static int kimage_alloc_init(struct kimage **rimage, unsigned long entry,
>  	struct kimage *image;
>  	bool kexec_on_panic = flags & KEXEC_ON_CRASH;
>  
> -	if (kexec_on_panic) {
> +	if (kexec_on_panic && __crash_memory_valid()) {
>  		/* Verify we have a valid entry point */
>  		if ((entry < phys_to_boot_phys(crashk_res.start)) ||
>  		    (entry > phys_to_boot_phys(crashk_res.end)))
> @@ -44,7 +44,7 @@ static int kimage_alloc_init(struct kimage **rimage, unsigned long entry,
>  	image->nr_segments = nr_segments;
>  	memcpy(image->segment, segments, nr_segments * sizeof(*segments));
>  
> -	if (kexec_on_panic) {
> +	if (kexec_on_panic && __crash_memory_valid()) {
>  		/* Enable special crash kernel control page alloc policy. */
>  		image->control_page = crashk_res.start;
>  		image->type = KEXEC_TYPE_CRASH;
> @@ -101,7 +101,7 @@ static int do_kexec_load(unsigned long entry, unsigned long nr_segments,
>  
>  	if (flags & KEXEC_ON_CRASH) {
>  		dest_image = &kexec_crash_image;
> -		if (kexec_crash_image)
> +		if (kexec_crash_image && __crash_memory_valid())
>  			arch_kexec_unprotect_crashkres();
>  	} else {
>  		dest_image = &kexec_image;
> @@ -157,7 +157,8 @@ static int do_kexec_load(unsigned long entry, unsigned long nr_segments,
>  	image = xchg(dest_image, image);
>  
>  out:
> -	if ((flags & KEXEC_ON_CRASH) && kexec_crash_image)
> +	if ((flags & KEXEC_ON_CRASH) && kexec_crash_image &&
> +	    __crash_memory_valid())
>  		arch_kexec_protect_crashkres();
>  
>  	kimage_free(image);
> diff --git a/kernel/kexec_core.c b/kernel/kexec_core.c
> index ca2743f9c634..77083c9760fb 100644
> --- a/kernel/kexec_core.c
> +++ b/kernel/kexec_core.c
> @@ -1004,6 +1004,11 @@ void crash_kexec(struct pt_regs *regs)
>  	}
>  }
>  
> +bool __crash_memory_valid(void)
> +{
> +	return crashk_res.end != crashk_res.start;
> +}
> +
>  ssize_t crash_get_memory_size(void)
>  {
>  	ssize_t size = 0;
> diff --git a/kernel/kexec_file.c b/kernel/kexec_file.c
> index 45637511e0de..0671f4f370ff 100644
> --- a/kernel/kexec_file.c
> +++ b/kernel/kexec_file.c
> @@ -280,7 +280,7 @@ kimage_file_alloc_init(struct kimage **rimage, int kernel_fd,
>  
>  	image->file_mode = 1;
>  
> -	if (kexec_on_panic) {
> +	if (kexec_on_panic && __crash_memory_valid()) {
>  		/* Enable special crash kernel control page alloc policy. */
>  		image->control_page = crashk_res.start;
>  		image->type = KEXEC_TYPE_CRASH;
> @@ -345,7 +345,7 @@ SYSCALL_DEFINE5(kexec_file_load, int, kernel_fd, int, initrd_fd,
>  	dest_image = &kexec_image;
>  	if (flags & KEXEC_FILE_ON_CRASH) {
>  		dest_image = &kexec_crash_image;
> -		if (kexec_crash_image)
> +		if (kexec_crash_image && __crash_memory_valid())
>  			arch_kexec_unprotect_crashkres();
>  	}
>  
> @@ -408,7 +408,8 @@ SYSCALL_DEFINE5(kexec_file_load, int, kernel_fd, int, initrd_fd,
>  exchange:
>  	image = xchg(dest_image, image);
>  out:
> -	if ((flags & KEXEC_FILE_ON_CRASH) && kexec_crash_image)
> +	if ((flags & KEXEC_FILE_ON_CRASH) && kexec_crash_image &&
> +	    __crash_memory_valid())
>  		arch_kexec_protect_crashkres();
>  
>  	kexec_unlock();
> 
> ---
> base-commit: 4312098baf37ee17a8350725e6e0d0e8590252d4
> change-id: 20221124-kexec-noalloc-3cab3cbe000f
> 
> Best regards,