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:   Fri, 25 Nov 2022 10:58:20 +0800
From:   Baoquan He <bhe@redhat.com>
To:     Ricardo Ribalda <ribalda@chromium.org>
Cc:     Eric Biederman <ebiederm@xmission.com>,
        Philipp Rudo <prudo@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: <Y4AvTEZiNXfFU1Wv@MiWiFi-R3L-srv>
References: <20221124-kexec-noalloc-v1-0-d78361e99aec@chromium.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20221124-kexec-noalloc-v1-0-d78361e99aec@chromium.org>
Precedence: bulk

On 11/24/22 at 11:23pm, Ricardo Ribalda 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.

We don't check if the crashkernel memory region is valid in kernel, but
we do have done the check in kexec-tools utility. Since both kexec_load and
kexec_file_load need go through path of kexec-tools loading, we haven't
got problem with lack of the checking in kernel.

However, even though we want to do the check, doing like below is much
easier and more reasonable.

diff --git a/kernel/kexec_file.c b/kernel/kexec_file.c
index 45637511e0de..4d1339bd2ccf 100644
--- a/kernel/kexec_file.c
+++ b/kernel/kexec_file.c
@@ -344,6 +344,8 @@ SYSCALL_DEFINE5(kexec_file_load, int, kernel_fd, int, initrd_fd,
 
        dest_image = &kexec_image;
        if (flags & KEXEC_FILE_ON_CRASH) {
+               if (!crash_memory_valid())
+                       return -EINVAL;
                dest_image = &kexec_crash_image;
                if (kexec_crash_image)
                        arch_kexec_unprotect_crashkres();

So, I am wondering if there is an issue encountered if we don't do the
check in kernel.

Thanks
Baoquan

> 
> ---
> 
> 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,
> -- 
> Ricardo Ribalda <ribalda@chromium.org>
>