* [PATCH] kexec: Enable runtime allocation of crash_image
@ 2022-11-24 22:23 ` Ricardo Ribalda
0 siblings, 0 replies; 28+ messages in thread
From: Ricardo Ribalda @ 2022-11-24 22:23 UTC (permalink / raw)
To: Eric Biederman
Cc: Baoquan He, Philipp Rudo, Sergey Senozhatsky, Ross Zwisler,
kexec, linux-kernel, Ricardo Ribalda
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>
---
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,
--
Ricardo Ribalda <ribalda@chromium.org>
_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec
^ permalink raw reply related [flat|nested] 28+ messages in thread* Re: [PATCH] kexec: Enable runtime allocation of crash_image
2022-11-24 22:23 ` Ricardo Ribalda
@ 2022-11-25 2:58 ` Baoquan He
-1 siblings, 0 replies; 28+ messages in thread
From: Baoquan He @ 2022-11-25 2:58 UTC (permalink / raw)
To: Ricardo Ribalda
Cc: Eric Biederman, Philipp Rudo, Sergey Senozhatsky, Ross Zwisler,
kexec, linux-kernel
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>
>
^ permalink raw reply related [flat|nested] 28+ messages in thread* Re: [PATCH] kexec: Enable runtime allocation of crash_image
@ 2022-11-25 2:58 ` Baoquan He
0 siblings, 0 replies; 28+ messages in thread
From: Baoquan He @ 2022-11-25 2:58 UTC (permalink / raw)
To: Ricardo Ribalda
Cc: Eric Biederman, Philipp Rudo, Sergey Senozhatsky, Ross Zwisler,
kexec, linux-kernel
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>
>
_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec
^ permalink raw reply related [flat|nested] 28+ messages in thread* Re: [PATCH] kexec: Enable runtime allocation of crash_image
2022-11-25 2:58 ` Baoquan He
@ 2022-11-25 5:52 ` Ricardo Ribalda
-1 siblings, 0 replies; 28+ messages in thread
From: Ricardo Ribalda @ 2022-11-25 5:52 UTC (permalink / raw)
To: Baoquan He
Cc: Eric Biederman, Philipp Rudo, Sergey Senozhatsky, Ross Zwisler,
kexec, linux-kernel
Hi Baoquan
Thanks for your review!
On Fri, 25 Nov 2022 at 03:58, Baoquan He <bhe@redhat.com> wrote:
>
> 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.
Not sure if I follow you.
We currently check if the crash kernel is in the right place at
sanity_check_segment_list()
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/kernel/kexec_core.c#n239
>
> 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>
> >
>
--
Ricardo Ribalda
^ permalink raw reply [flat|nested] 28+ messages in thread* Re: [PATCH] kexec: Enable runtime allocation of crash_image
@ 2022-11-25 5:52 ` Ricardo Ribalda
0 siblings, 0 replies; 28+ messages in thread
From: Ricardo Ribalda @ 2022-11-25 5:52 UTC (permalink / raw)
To: Baoquan He
Cc: Eric Biederman, Philipp Rudo, Sergey Senozhatsky, Ross Zwisler,
kexec, linux-kernel
Hi Baoquan
Thanks for your review!
On Fri, 25 Nov 2022 at 03:58, Baoquan He <bhe@redhat.com> wrote:
>
> 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.
Not sure if I follow you.
We currently check if the crash kernel is in the right place at
sanity_check_segment_list()
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/kernel/kexec_core.c#n239
>
> 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>
> >
>
--
Ricardo Ribalda
_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec
^ permalink raw reply [flat|nested] 28+ messages in thread* Re: [PATCH] kexec: Enable runtime allocation of crash_image
2022-11-25 5:52 ` Ricardo Ribalda
@ 2022-11-25 7:15 ` Baoquan He
-1 siblings, 0 replies; 28+ messages in thread
From: Baoquan He @ 2022-11-25 7:15 UTC (permalink / raw)
To: Ricardo Ribalda
Cc: Eric Biederman, Philipp Rudo, Sergey Senozhatsky, Ross Zwisler,
kexec, linux-kernel
On 11/25/22 at 06:52am, Ricardo Ribalda wrote:
> Hi Baoquan
>
> Thanks for your review!
>
> On Fri, 25 Nov 2022 at 03:58, Baoquan He <bhe@redhat.com> wrote:
> >
> > 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.
>
> Not sure if I follow you.
>
> We currently check if the crash kernel is in the right place at
> sanity_check_segment_list()
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/kernel/kexec_core.c#n239
Please check below code in kexec-tools utility, currently we have to use
kexec -p to enter into kexec_load or kexec_file_load system call. Before
entering system call, we have below code:
https://kernel.googlesource.com/pub/scm/utils/kernel/kexec/kexec-tools.git/+/refs/heads/master/kexec/kexec.c
int main(int argc, char *argv[])
{
......
if (do_load &&
((kexec_flags & KEXEC_ON_CRASH) ||
(kexec_file_flags & KEXEC_FILE_ON_CRASH)) &&
!is_crashkernel_mem_reserved()) {
die("Memory for crashkernel is not reserved\n"
"Please reserve memory by passing"
"\"crashkernel=Y@X\" parameter to kernel\n"
"Then try to loading kdump kernel\n");
}
......
}
>
>
> >
> > 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>
> > >
> >
>
>
> --
> Ricardo Ribalda
>
_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec
^ permalink raw reply [flat|nested] 28+ messages in thread* Re: [PATCH] kexec: Enable runtime allocation of crash_image
@ 2022-11-25 7:15 ` Baoquan He
0 siblings, 0 replies; 28+ messages in thread
From: Baoquan He @ 2022-11-25 7:15 UTC (permalink / raw)
To: Ricardo Ribalda
Cc: Eric Biederman, Philipp Rudo, Sergey Senozhatsky, Ross Zwisler,
kexec, linux-kernel
On 11/25/22 at 06:52am, Ricardo Ribalda wrote:
> Hi Baoquan
>
> Thanks for your review!
>
> On Fri, 25 Nov 2022 at 03:58, Baoquan He <bhe@redhat.com> wrote:
> >
> > 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.
>
> Not sure if I follow you.
>
> We currently check if the crash kernel is in the right place at
> sanity_check_segment_list()
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/kernel/kexec_core.c#n239
Please check below code in kexec-tools utility, currently we have to use
kexec -p to enter into kexec_load or kexec_file_load system call. Before
entering system call, we have below code:
https://kernel.googlesource.com/pub/scm/utils/kernel/kexec/kexec-tools.git/+/refs/heads/master/kexec/kexec.c
int main(int argc, char *argv[])
{
......
if (do_load &&
((kexec_flags & KEXEC_ON_CRASH) ||
(kexec_file_flags & KEXEC_FILE_ON_CRASH)) &&
!is_crashkernel_mem_reserved()) {
die("Memory for crashkernel is not reserved\n"
"Please reserve memory by passing"
"\"crashkernel=Y@X\" parameter to kernel\n"
"Then try to loading kdump kernel\n");
}
......
}
>
>
> >
> > 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>
> > >
> >
>
>
> --
> Ricardo Ribalda
>
^ permalink raw reply [flat|nested] 28+ messages in thread* Re: [PATCH] kexec: Enable runtime allocation of crash_image
2022-11-25 7:15 ` Baoquan He
@ 2022-11-25 7:26 ` Ricardo Ribalda
-1 siblings, 0 replies; 28+ messages in thread
From: Ricardo Ribalda @ 2022-11-25 7:26 UTC (permalink / raw)
To: Baoquan He
Cc: Eric Biederman, Philipp Rudo, Sergey Senozhatsky, Ross Zwisler,
kexec, linux-kernel
Hi Baoquan
On Fri, 25 Nov 2022 at 08:15, Baoquan He <bhe@redhat.com> wrote:
>
> On 11/25/22 at 06:52am, Ricardo Ribalda wrote:
> > Hi Baoquan
> >
> > Thanks for your review!
> >
> > On Fri, 25 Nov 2022 at 03:58, Baoquan He <bhe@redhat.com> wrote:
> > >
> > > 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.
> >
> > Not sure if I follow you.
> >
> > We currently check if the crash kernel is in the right place at
> > sanity_check_segment_list()
> > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/kernel/kexec_core.c#n239
>
> Please check below code in kexec-tools utility, currently we have to use
> kexec -p to enter into kexec_load or kexec_file_load system call. Before
> entering system call, we have below code:
So your concern is that the current kexec-tools does not let you pass
a crashkernel unless there is memory reserved for it?
Once the changes land in the kernel I can make a patch for that. I am
currently using this to test the code:
https://chromium-review.googlesource.com/c/chromiumos/platform2/+/3953579/4/kexec-lite/kexec-lite.c
>
> https://kernel.googlesource.com/pub/scm/utils/kernel/kexec/kexec-tools.git/+/refs/heads/master/kexec/kexec.c
>
> int main(int argc, char *argv[])
> {
> ......
> if (do_load &&
> ((kexec_flags & KEXEC_ON_CRASH) ||
> (kexec_file_flags & KEXEC_FILE_ON_CRASH)) &&
> !is_crashkernel_mem_reserved()) {
> die("Memory for crashkernel is not reserved\n"
> "Please reserve memory by passing"
> "\"crashkernel=Y@X\" parameter to kernel\n"
> "Then try to loading kdump kernel\n");
> }
>
> ......
> }
>
> >
> >
> > >
> > > 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>
> > > >
> > >
> >
> >
> > --
> > Ricardo Ribalda
> >
>
--
Ricardo Ribalda
^ permalink raw reply [flat|nested] 28+ messages in thread* Re: [PATCH] kexec: Enable runtime allocation of crash_image
@ 2022-11-25 7:26 ` Ricardo Ribalda
0 siblings, 0 replies; 28+ messages in thread
From: Ricardo Ribalda @ 2022-11-25 7:26 UTC (permalink / raw)
To: Baoquan He
Cc: Eric Biederman, Philipp Rudo, Sergey Senozhatsky, Ross Zwisler,
kexec, linux-kernel
Hi Baoquan
On Fri, 25 Nov 2022 at 08:15, Baoquan He <bhe@redhat.com> wrote:
>
> On 11/25/22 at 06:52am, Ricardo Ribalda wrote:
> > Hi Baoquan
> >
> > Thanks for your review!
> >
> > On Fri, 25 Nov 2022 at 03:58, Baoquan He <bhe@redhat.com> wrote:
> > >
> > > 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.
> >
> > Not sure if I follow you.
> >
> > We currently check if the crash kernel is in the right place at
> > sanity_check_segment_list()
> > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/kernel/kexec_core.c#n239
>
> Please check below code in kexec-tools utility, currently we have to use
> kexec -p to enter into kexec_load or kexec_file_load system call. Before
> entering system call, we have below code:
So your concern is that the current kexec-tools does not let you pass
a crashkernel unless there is memory reserved for it?
Once the changes land in the kernel I can make a patch for that. I am
currently using this to test the code:
https://chromium-review.googlesource.com/c/chromiumos/platform2/+/3953579/4/kexec-lite/kexec-lite.c
>
> https://kernel.googlesource.com/pub/scm/utils/kernel/kexec/kexec-tools.git/+/refs/heads/master/kexec/kexec.c
>
> int main(int argc, char *argv[])
> {
> ......
> if (do_load &&
> ((kexec_flags & KEXEC_ON_CRASH) ||
> (kexec_file_flags & KEXEC_FILE_ON_CRASH)) &&
> !is_crashkernel_mem_reserved()) {
> die("Memory for crashkernel is not reserved\n"
> "Please reserve memory by passing"
> "\"crashkernel=Y@X\" parameter to kernel\n"
> "Then try to loading kdump kernel\n");
> }
>
> ......
> }
>
> >
> >
> > >
> > > 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>
> > > >
> > >
> >
> >
> > --
> > Ricardo Ribalda
> >
>
--
Ricardo Ribalda
_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec
^ permalink raw reply [flat|nested] 28+ messages in thread* Re: [PATCH] kexec: Enable runtime allocation of crash_image
2022-11-25 7:26 ` Ricardo Ribalda
@ 2022-11-25 7:44 ` Baoquan He
-1 siblings, 0 replies; 28+ messages in thread
From: Baoquan He @ 2022-11-25 7:44 UTC (permalink / raw)
To: Ricardo Ribalda
Cc: Eric Biederman, Philipp Rudo, Sergey Senozhatsky, Ross Zwisler,
kexec, linux-kernel
On 11/25/22 at 08:26am, Ricardo Ribalda wrote:
> Hi Baoquan
>
> On Fri, 25 Nov 2022 at 08:15, Baoquan He <bhe@redhat.com> wrote:
> >
> > On 11/25/22 at 06:52am, Ricardo Ribalda wrote:
> > > Hi Baoquan
> > >
> > > Thanks for your review!
> > >
> > > On Fri, 25 Nov 2022 at 03:58, Baoquan He <bhe@redhat.com> wrote:
> > > >
> > > > 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.
> > >
> > > Not sure if I follow you.
> > >
> > > We currently check if the crash kernel is in the right place at
> > > sanity_check_segment_list()
> > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/kernel/kexec_core.c#n239
> >
> > Please check below code in kexec-tools utility, currently we have to use
> > kexec -p to enter into kexec_load or kexec_file_load system call. Before
> > entering system call, we have below code:
>
> So your concern is that the current kexec-tools does not let you pass
> a crashkernel unless there is memory reserved for it?
No, my concern is why we have to do the check in kernel if we have done
that in kexec-tools utility. You didn't say your kexec-lite need this
until now. I think it's fine to add the check in kernel if you prefer to
do the check in kernel, but not in kexec-lite.
The motivation or reason you want to make the change is very important.
>
> Once the changes land in the kernel I can make a patch for that. I am
> currently using this to test the code:
>
> https://chromium-review.googlesource.com/c/chromiumos/platform2/+/3953579/4/kexec-lite/kexec-lite.c
>
> >
> > https://kernel.googlesource.com/pub/scm/utils/kernel/kexec/kexec-tools.git/+/refs/heads/master/kexec/kexec.c
> >
> > int main(int argc, char *argv[])
> > {
> > ......
> > if (do_load &&
> > ((kexec_flags & KEXEC_ON_CRASH) ||
> > (kexec_file_flags & KEXEC_FILE_ON_CRASH)) &&
> > !is_crashkernel_mem_reserved()) {
> > die("Memory for crashkernel is not reserved\n"
> > "Please reserve memory by passing"
> > "\"crashkernel=Y@X\" parameter to kernel\n"
> > "Then try to loading kdump kernel\n");
> > }
> >
> > ......
> > }
> >
> > >
> > >
> > > >
> > > > 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>
> > > > >
> > > >
> > >
> > >
> > > --
> > > Ricardo Ribalda
> > >
> >
>
>
> --
> Ricardo Ribalda
>
_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec
^ permalink raw reply [flat|nested] 28+ messages in thread* Re: [PATCH] kexec: Enable runtime allocation of crash_image
@ 2022-11-25 7:44 ` Baoquan He
0 siblings, 0 replies; 28+ messages in thread
From: Baoquan He @ 2022-11-25 7:44 UTC (permalink / raw)
To: Ricardo Ribalda
Cc: Eric Biederman, Philipp Rudo, Sergey Senozhatsky, Ross Zwisler,
kexec, linux-kernel
On 11/25/22 at 08:26am, Ricardo Ribalda wrote:
> Hi Baoquan
>
> On Fri, 25 Nov 2022 at 08:15, Baoquan He <bhe@redhat.com> wrote:
> >
> > On 11/25/22 at 06:52am, Ricardo Ribalda wrote:
> > > Hi Baoquan
> > >
> > > Thanks for your review!
> > >
> > > On Fri, 25 Nov 2022 at 03:58, Baoquan He <bhe@redhat.com> wrote:
> > > >
> > > > 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.
> > >
> > > Not sure if I follow you.
> > >
> > > We currently check if the crash kernel is in the right place at
> > > sanity_check_segment_list()
> > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/kernel/kexec_core.c#n239
> >
> > Please check below code in kexec-tools utility, currently we have to use
> > kexec -p to enter into kexec_load or kexec_file_load system call. Before
> > entering system call, we have below code:
>
> So your concern is that the current kexec-tools does not let you pass
> a crashkernel unless there is memory reserved for it?
No, my concern is why we have to do the check in kernel if we have done
that in kexec-tools utility. You didn't say your kexec-lite need this
until now. I think it's fine to add the check in kernel if you prefer to
do the check in kernel, but not in kexec-lite.
The motivation or reason you want to make the change is very important.
>
> Once the changes land in the kernel I can make a patch for that. I am
> currently using this to test the code:
>
> https://chromium-review.googlesource.com/c/chromiumos/platform2/+/3953579/4/kexec-lite/kexec-lite.c
>
> >
> > https://kernel.googlesource.com/pub/scm/utils/kernel/kexec/kexec-tools.git/+/refs/heads/master/kexec/kexec.c
> >
> > int main(int argc, char *argv[])
> > {
> > ......
> > if (do_load &&
> > ((kexec_flags & KEXEC_ON_CRASH) ||
> > (kexec_file_flags & KEXEC_FILE_ON_CRASH)) &&
> > !is_crashkernel_mem_reserved()) {
> > die("Memory for crashkernel is not reserved\n"
> > "Please reserve memory by passing"
> > "\"crashkernel=Y@X\" parameter to kernel\n"
> > "Then try to loading kdump kernel\n");
> > }
> >
> > ......
> > }
> >
> > >
> > >
> > > >
> > > > 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>
> > > > >
> > > >
> > >
> > >
> > > --
> > > Ricardo Ribalda
> > >
> >
>
>
> --
> Ricardo Ribalda
>
^ permalink raw reply [flat|nested] 28+ messages in thread* Re: [PATCH] kexec: Enable runtime allocation of crash_image
2022-11-25 7:44 ` Baoquan He
@ 2022-11-25 8:10 ` Ricardo Ribalda
-1 siblings, 0 replies; 28+ messages in thread
From: Ricardo Ribalda @ 2022-11-25 8:10 UTC (permalink / raw)
To: Baoquan He
Cc: Eric Biederman, Philipp Rudo, Sergey Senozhatsky, Ross Zwisler,
kexec, linux-kernel
Hi Baoquan
On Fri, 25 Nov 2022 at 08:45, Baoquan He <bhe@redhat.com> wrote:
>
> On 11/25/22 at 08:26am, Ricardo Ribalda wrote:
> > Hi Baoquan
> >
> > On Fri, 25 Nov 2022 at 08:15, Baoquan He <bhe@redhat.com> wrote:
> > >
> > > On 11/25/22 at 06:52am, Ricardo Ribalda wrote:
> > > > Hi Baoquan
> > > >
> > > > Thanks for your review!
> > > >
> > > > On Fri, 25 Nov 2022 at 03:58, Baoquan He <bhe@redhat.com> wrote:
> > > > >
> > > > > 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.
> > > >
> > > > Not sure if I follow you.
> > > >
> > > > We currently check if the crash kernel is in the right place at
> > > > sanity_check_segment_list()
> > > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/kernel/kexec_core.c#n239
> > >
> > > Please check below code in kexec-tools utility, currently we have to use
> > > kexec -p to enter into kexec_load or kexec_file_load system call. Before
> > > entering system call, we have below code:
> >
> > So your concern is that the current kexec-tools does not let you pass
> > a crashkernel unless there is memory reserved for it?
>
> No, my concern is why we have to do the check in kernel if we have done
> that in kexec-tools utility. You didn't say your kexec-lite need this
> until now. I think it's fine to add the check in kernel if you prefer to
> do the check in kernel, but not in kexec-lite.
>
> The motivation or reason you want to make the change is very important.
kexec-lite is just to test the kernel code. It is easier to follow
than kexec-utils and supports 32bit userspace on a 64bit kernel.
I think it was clear. The motivation is to enable the use of
crashkernel when it is not statically predefined.
Any suggestions on how I can make it more clear?
>
>
> >
> > Once the changes land in the kernel I can make a patch for that. I am
> > currently using this to test the code:
> >
> > https://chromium-review.googlesource.com/c/chromiumos/platform2/+/3953579/4/kexec-lite/kexec-lite.c
> >
> > >
> > > https://kernel.googlesource.com/pub/scm/utils/kernel/kexec/kexec-tools.git/+/refs/heads/master/kexec/kexec.c
> > >
> > > int main(int argc, char *argv[])
> > > {
> > > ......
> > > if (do_load &&
> > > ((kexec_flags & KEXEC_ON_CRASH) ||
> > > (kexec_file_flags & KEXEC_FILE_ON_CRASH)) &&
> > > !is_crashkernel_mem_reserved()) {
> > > die("Memory for crashkernel is not reserved\n"
> > > "Please reserve memory by passing"
> > > "\"crashkernel=Y@X\" parameter to kernel\n"
> > > "Then try to loading kdump kernel\n");
Having that check ALSO is unserspace is fine. It lets kexec show a
more meaningful error message. But we should not rely on userspace
checks.
This patch is not about adding an extra check on the kernel, but to
enable extra functionaliry.
> > > }
> > >
> > > ......
> > > }
> > >
> > > >
> > > >
> > > > >
> > > > > 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>
> > > > > >
> > > > >
> > > >
> > > >
> > > > --
> > > > Ricardo Ribalda
> > > >
> > >
> >
> >
> > --
> > Ricardo Ribalda
> >
>
--
Ricardo Ribalda
^ permalink raw reply [flat|nested] 28+ messages in thread* Re: [PATCH] kexec: Enable runtime allocation of crash_image
@ 2022-11-25 8:10 ` Ricardo Ribalda
0 siblings, 0 replies; 28+ messages in thread
From: Ricardo Ribalda @ 2022-11-25 8:10 UTC (permalink / raw)
To: Baoquan He
Cc: Eric Biederman, Philipp Rudo, Sergey Senozhatsky, Ross Zwisler,
kexec, linux-kernel
Hi Baoquan
On Fri, 25 Nov 2022 at 08:45, Baoquan He <bhe@redhat.com> wrote:
>
> On 11/25/22 at 08:26am, Ricardo Ribalda wrote:
> > Hi Baoquan
> >
> > On Fri, 25 Nov 2022 at 08:15, Baoquan He <bhe@redhat.com> wrote:
> > >
> > > On 11/25/22 at 06:52am, Ricardo Ribalda wrote:
> > > > Hi Baoquan
> > > >
> > > > Thanks for your review!
> > > >
> > > > On Fri, 25 Nov 2022 at 03:58, Baoquan He <bhe@redhat.com> wrote:
> > > > >
> > > > > 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.
> > > >
> > > > Not sure if I follow you.
> > > >
> > > > We currently check if the crash kernel is in the right place at
> > > > sanity_check_segment_list()
> > > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/kernel/kexec_core.c#n239
> > >
> > > Please check below code in kexec-tools utility, currently we have to use
> > > kexec -p to enter into kexec_load or kexec_file_load system call. Before
> > > entering system call, we have below code:
> >
> > So your concern is that the current kexec-tools does not let you pass
> > a crashkernel unless there is memory reserved for it?
>
> No, my concern is why we have to do the check in kernel if we have done
> that in kexec-tools utility. You didn't say your kexec-lite need this
> until now. I think it's fine to add the check in kernel if you prefer to
> do the check in kernel, but not in kexec-lite.
>
> The motivation or reason you want to make the change is very important.
kexec-lite is just to test the kernel code. It is easier to follow
than kexec-utils and supports 32bit userspace on a 64bit kernel.
I think it was clear. The motivation is to enable the use of
crashkernel when it is not statically predefined.
Any suggestions on how I can make it more clear?
>
>
> >
> > Once the changes land in the kernel I can make a patch for that. I am
> > currently using this to test the code:
> >
> > https://chromium-review.googlesource.com/c/chromiumos/platform2/+/3953579/4/kexec-lite/kexec-lite.c
> >
> > >
> > > https://kernel.googlesource.com/pub/scm/utils/kernel/kexec/kexec-tools.git/+/refs/heads/master/kexec/kexec.c
> > >
> > > int main(int argc, char *argv[])
> > > {
> > > ......
> > > if (do_load &&
> > > ((kexec_flags & KEXEC_ON_CRASH) ||
> > > (kexec_file_flags & KEXEC_FILE_ON_CRASH)) &&
> > > !is_crashkernel_mem_reserved()) {
> > > die("Memory for crashkernel is not reserved\n"
> > > "Please reserve memory by passing"
> > > "\"crashkernel=Y@X\" parameter to kernel\n"
> > > "Then try to loading kdump kernel\n");
Having that check ALSO is unserspace is fine. It lets kexec show a
more meaningful error message. But we should not rely on userspace
checks.
This patch is not about adding an extra check on the kernel, but to
enable extra functionaliry.
> > > }
> > >
> > > ......
> > > }
> > >
> > > >
> > > >
> > > > >
> > > > > 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>
> > > > > >
> > > > >
> > > >
> > > >
> > > > --
> > > > Ricardo Ribalda
> > > >
> > >
> >
> >
> > --
> > Ricardo Ribalda
> >
>
--
Ricardo Ribalda
_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec
^ permalink raw reply [flat|nested] 28+ messages in thread* Re: [PATCH] kexec: Enable runtime allocation of crash_image
2022-11-25 8:10 ` Ricardo Ribalda
@ 2022-11-25 9:27 ` Baoquan He
-1 siblings, 0 replies; 28+ messages in thread
From: Baoquan He @ 2022-11-25 9:27 UTC (permalink / raw)
To: Ricardo Ribalda
Cc: Eric Biederman, Philipp Rudo, Sergey Senozhatsky, Ross Zwisler,
kexec, linux-kernel
On 11/25/22 at 09:10am, Ricardo Ribalda wrote:
> Hi Baoquan
>
> On Fri, 25 Nov 2022 at 08:45, Baoquan He <bhe@redhat.com> wrote:
> >
> > On 11/25/22 at 08:26am, Ricardo Ribalda wrote:
> > > Hi Baoquan
> > >
> > > On Fri, 25 Nov 2022 at 08:15, Baoquan He <bhe@redhat.com> wrote:
> > > >
> > > > On 11/25/22 at 06:52am, Ricardo Ribalda wrote:
> > > > > Hi Baoquan
> > > > >
> > > > > Thanks for your review!
> > > > >
> > > > > On Fri, 25 Nov 2022 at 03:58, Baoquan He <bhe@redhat.com> wrote:
> > > > > >
> > > > > > 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.
> > > > >
> > > > > Not sure if I follow you.
> > > > >
> > > > > We currently check if the crash kernel is in the right place at
> > > > > sanity_check_segment_list()
> > > > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/kernel/kexec_core.c#n239
> > > >
> > > > Please check below code in kexec-tools utility, currently we have to use
> > > > kexec -p to enter into kexec_load or kexec_file_load system call. Before
> > > > entering system call, we have below code:
> > >
> > > So your concern is that the current kexec-tools does not let you pass
> > > a crashkernel unless there is memory reserved for it?
> >
> > No, my concern is why we have to do the check in kernel if we have done
> > that in kexec-tools utility. You didn't say your kexec-lite need this
> > until now. I think it's fine to add the check in kernel if you prefer to
> > do the check in kernel, but not in kexec-lite.
> >
> > The motivation or reason you want to make the change is very important.
>
> kexec-lite is just to test the kernel code. It is easier to follow
> than kexec-utils and supports 32bit userspace on a 64bit kernel.
>
>
> I think it was clear. The motivation is to enable the use of
> crashkernel when it is not statically predefined.
I thought you are adding a check on crashkernel region validation. Now I
am totally lost how this patch can enable the use of crashkernel. I will
wait a while to see if other people understand it.
>
> Any suggestions on how I can make it more clear?
>
>
>
> >
> >
> > >
> > > Once the changes land in the kernel I can make a patch for that. I am
> > > currently using this to test the code:
> > >
> > > https://chromium-review.googlesource.com/c/chromiumos/platform2/+/3953579/4/kexec-lite/kexec-lite.c
> > >
> > > >
> > > > https://kernel.googlesource.com/pub/scm/utils/kernel/kexec/kexec-tools.git/+/refs/heads/master/kexec/kexec.c
> > > >
> > > > int main(int argc, char *argv[])
> > > > {
> > > > ......
> > > > if (do_load &&
> > > > ((kexec_flags & KEXEC_ON_CRASH) ||
> > > > (kexec_file_flags & KEXEC_FILE_ON_CRASH)) &&
> > > > !is_crashkernel_mem_reserved()) {
> > > > die("Memory for crashkernel is not reserved\n"
> > > > "Please reserve memory by passing"
> > > > "\"crashkernel=Y@X\" parameter to kernel\n"
> > > > "Then try to loading kdump kernel\n");
>
> Having that check ALSO is unserspace is fine. It lets kexec show a
> more meaningful error message. But we should not rely on userspace
> checks.
>
> This patch is not about adding an extra check on the kernel, but to
> enable extra functionaliry.
>
> > > > }
> > > >
> > > > ......
> > > > }
> > > >
> > > > >
> > > > >
> > > > > >
> > > > > > 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>
> > > > > > >
> > > > > >
> > > > >
> > > > >
> > > > > --
> > > > > Ricardo Ribalda
> > > > >
> > > >
> > >
> > >
> > > --
> > > Ricardo Ribalda
> > >
> >
>
>
> --
> Ricardo Ribalda
>
^ permalink raw reply [flat|nested] 28+ messages in thread* Re: [PATCH] kexec: Enable runtime allocation of crash_image
@ 2022-11-25 9:27 ` Baoquan He
0 siblings, 0 replies; 28+ messages in thread
From: Baoquan He @ 2022-11-25 9:27 UTC (permalink / raw)
To: Ricardo Ribalda
Cc: Eric Biederman, Philipp Rudo, Sergey Senozhatsky, Ross Zwisler,
kexec, linux-kernel
On 11/25/22 at 09:10am, Ricardo Ribalda wrote:
> Hi Baoquan
>
> On Fri, 25 Nov 2022 at 08:45, Baoquan He <bhe@redhat.com> wrote:
> >
> > On 11/25/22 at 08:26am, Ricardo Ribalda wrote:
> > > Hi Baoquan
> > >
> > > On Fri, 25 Nov 2022 at 08:15, Baoquan He <bhe@redhat.com> wrote:
> > > >
> > > > On 11/25/22 at 06:52am, Ricardo Ribalda wrote:
> > > > > Hi Baoquan
> > > > >
> > > > > Thanks for your review!
> > > > >
> > > > > On Fri, 25 Nov 2022 at 03:58, Baoquan He <bhe@redhat.com> wrote:
> > > > > >
> > > > > > 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.
> > > > >
> > > > > Not sure if I follow you.
> > > > >
> > > > > We currently check if the crash kernel is in the right place at
> > > > > sanity_check_segment_list()
> > > > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/kernel/kexec_core.c#n239
> > > >
> > > > Please check below code in kexec-tools utility, currently we have to use
> > > > kexec -p to enter into kexec_load or kexec_file_load system call. Before
> > > > entering system call, we have below code:
> > >
> > > So your concern is that the current kexec-tools does not let you pass
> > > a crashkernel unless there is memory reserved for it?
> >
> > No, my concern is why we have to do the check in kernel if we have done
> > that in kexec-tools utility. You didn't say your kexec-lite need this
> > until now. I think it's fine to add the check in kernel if you prefer to
> > do the check in kernel, but not in kexec-lite.
> >
> > The motivation or reason you want to make the change is very important.
>
> kexec-lite is just to test the kernel code. It is easier to follow
> than kexec-utils and supports 32bit userspace on a 64bit kernel.
>
>
> I think it was clear. The motivation is to enable the use of
> crashkernel when it is not statically predefined.
I thought you are adding a check on crashkernel region validation. Now I
am totally lost how this patch can enable the use of crashkernel. I will
wait a while to see if other people understand it.
>
> Any suggestions on how I can make it more clear?
>
>
>
> >
> >
> > >
> > > Once the changes land in the kernel I can make a patch for that. I am
> > > currently using this to test the code:
> > >
> > > https://chromium-review.googlesource.com/c/chromiumos/platform2/+/3953579/4/kexec-lite/kexec-lite.c
> > >
> > > >
> > > > https://kernel.googlesource.com/pub/scm/utils/kernel/kexec/kexec-tools.git/+/refs/heads/master/kexec/kexec.c
> > > >
> > > > int main(int argc, char *argv[])
> > > > {
> > > > ......
> > > > if (do_load &&
> > > > ((kexec_flags & KEXEC_ON_CRASH) ||
> > > > (kexec_file_flags & KEXEC_FILE_ON_CRASH)) &&
> > > > !is_crashkernel_mem_reserved()) {
> > > > die("Memory for crashkernel is not reserved\n"
> > > > "Please reserve memory by passing"
> > > > "\"crashkernel=Y@X\" parameter to kernel\n"
> > > > "Then try to loading kdump kernel\n");
>
> Having that check ALSO is unserspace is fine. It lets kexec show a
> more meaningful error message. But we should not rely on userspace
> checks.
>
> This patch is not about adding an extra check on the kernel, but to
> enable extra functionaliry.
>
> > > > }
> > > >
> > > > ......
> > > > }
> > > >
> > > > >
> > > > >
> > > > > >
> > > > > > 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>
> > > > > > >
> > > > > >
> > > > >
> > > > >
> > > > > --
> > > > > Ricardo Ribalda
> > > > >
> > > >
> > >
> > >
> > > --
> > > Ricardo Ribalda
> > >
> >
>
>
> --
> Ricardo Ribalda
>
_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH] kexec: Enable runtime allocation of crash_image
2022-11-25 5:52 ` Ricardo Ribalda
@ 2022-11-25 7:27 ` Baoquan He
-1 siblings, 0 replies; 28+ messages in thread
From: Baoquan He @ 2022-11-25 7:27 UTC (permalink / raw)
To: Ricardo Ribalda
Cc: Eric Biederman, Philipp Rudo, Sergey Senozhatsky, Ross Zwisler,
kexec, linux-kernel
On 11/25/22 at 06:52am, Ricardo Ribalda wrote:
> Hi Baoquan
>
> Thanks for your review!
>
> On Fri, 25 Nov 2022 at 03:58, Baoquan He <bhe@redhat.com> wrote:
> >
> > 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.
>
> Not sure if I follow you.
>
> We currently check if the crash kernel is in the right place at
> sanity_check_segment_list()
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/kernel/kexec_core.c#n239
And it's not checking if crashkernel memory is valid in
sanity_check_segment_list(), right? It's checking if the segments
are placed correctly.
_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH] kexec: Enable runtime allocation of crash_image
@ 2022-11-25 7:27 ` Baoquan He
0 siblings, 0 replies; 28+ messages in thread
From: Baoquan He @ 2022-11-25 7:27 UTC (permalink / raw)
To: Ricardo Ribalda
Cc: Eric Biederman, Philipp Rudo, Sergey Senozhatsky, Ross Zwisler,
kexec, linux-kernel
On 11/25/22 at 06:52am, Ricardo Ribalda wrote:
> Hi Baoquan
>
> Thanks for your review!
>
> On Fri, 25 Nov 2022 at 03:58, Baoquan He <bhe@redhat.com> wrote:
> >
> > 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.
>
> Not sure if I follow you.
>
> We currently check if the crash kernel is in the right place at
> sanity_check_segment_list()
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/kernel/kexec_core.c#n239
And it's not checking if crashkernel memory is valid in
sanity_check_segment_list(), right? It's checking if the segments
are placed correctly.
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH] kexec: Enable runtime allocation of crash_image
2022-11-25 7:27 ` Baoquan He
@ 2022-11-25 7:31 ` Ricardo Ribalda
-1 siblings, 0 replies; 28+ messages in thread
From: Ricardo Ribalda @ 2022-11-25 7:31 UTC (permalink / raw)
To: Baoquan He
Cc: Eric Biederman, Philipp Rudo, Sergey Senozhatsky, Ross Zwisler,
kexec, linux-kernel
Hi
On Fri, 25 Nov 2022 at 08:27, Baoquan He <bhe@redhat.com> wrote:
>
> On 11/25/22 at 06:52am, Ricardo Ribalda wrote:
> > Hi Baoquan
> >
> > Thanks for your review!
> >
> > On Fri, 25 Nov 2022 at 03:58, Baoquan He <bhe@redhat.com> wrote:
> > >
> > > 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.
> >
> > Not sure if I follow you.
> >
> > We currently check if the crash kernel is in the right place at
> > sanity_check_segment_list()
> > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/kernel/kexec_core.c#n239
>
> And it's not checking if crashkernel memory is valid in
> sanity_check_segment_list(), right? It's checking if the segments
> are placed correctly.
If it is not valid, then this condition is not met.
/* Ensure we are within the crash kernel limits */
if ((mstart < phys_to_boot_phys(crashk_res.start)) ||
(mend > phys_to_boot_phys(crashk_res.end)))
return -EADDRNOTAVAIL;
>
--
Ricardo Ribalda
^ permalink raw reply [flat|nested] 28+ messages in thread* Re: [PATCH] kexec: Enable runtime allocation of crash_image
@ 2022-11-25 7:31 ` Ricardo Ribalda
0 siblings, 0 replies; 28+ messages in thread
From: Ricardo Ribalda @ 2022-11-25 7:31 UTC (permalink / raw)
To: Baoquan He
Cc: Eric Biederman, Philipp Rudo, Sergey Senozhatsky, Ross Zwisler,
kexec, linux-kernel
Hi
On Fri, 25 Nov 2022 at 08:27, Baoquan He <bhe@redhat.com> wrote:
>
> On 11/25/22 at 06:52am, Ricardo Ribalda wrote:
> > Hi Baoquan
> >
> > Thanks for your review!
> >
> > On Fri, 25 Nov 2022 at 03:58, Baoquan He <bhe@redhat.com> wrote:
> > >
> > > 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.
> >
> > Not sure if I follow you.
> >
> > We currently check if the crash kernel is in the right place at
> > sanity_check_segment_list()
> > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/kernel/kexec_core.c#n239
>
> And it's not checking if crashkernel memory is valid in
> sanity_check_segment_list(), right? It's checking if the segments
> are placed correctly.
If it is not valid, then this condition is not met.
/* Ensure we are within the crash kernel limits */
if ((mstart < phys_to_boot_phys(crashk_res.start)) ||
(mend > phys_to_boot_phys(crashk_res.end)))
return -EADDRNOTAVAIL;
>
--
Ricardo Ribalda
_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec
^ permalink raw reply [flat|nested] 28+ messages in thread* Re: [PATCH] kexec: Enable runtime allocation of crash_image
2022-11-25 7:31 ` Ricardo Ribalda
@ 2022-11-25 7:48 ` Baoquan He
-1 siblings, 0 replies; 28+ messages in thread
From: Baoquan He @ 2022-11-25 7:48 UTC (permalink / raw)
To: Ricardo Ribalda
Cc: Eric Biederman, Philipp Rudo, Sergey Senozhatsky, Ross Zwisler,
kexec, linux-kernel
On 11/25/22 at 08:31am, Ricardo Ribalda wrote:
> Hi
>
> On Fri, 25 Nov 2022 at 08:27, Baoquan He <bhe@redhat.com> wrote:
> >
> > On 11/25/22 at 06:52am, Ricardo Ribalda wrote:
> > > Hi Baoquan
> > >
> > > Thanks for your review!
> > >
> > > On Fri, 25 Nov 2022 at 03:58, Baoquan He <bhe@redhat.com> wrote:
> > > >
> > > > 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.
> > >
> > > Not sure if I follow you.
> > >
> > > We currently check if the crash kernel is in the right place at
> > > sanity_check_segment_list()
> > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/kernel/kexec_core.c#n239
> >
> > And it's not checking if crashkernel memory is valid in
> > sanity_check_segment_list(), right? It's checking if the segments
> > are placed correctly.
>
> If it is not valid, then this condition is not met.
Yeah. BUT, even though it's valid, below condition could not be met
either. They are not the same thing.
>
> /* Ensure we are within the crash kernel limits */
> if ((mstart < phys_to_boot_phys(crashk_res.start)) ||
> (mend > phys_to_boot_phys(crashk_res.end)))
> return -EADDRNOTAVAIL;
>
>
> >
>
>
> --
> Ricardo Ribalda
>
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH] kexec: Enable runtime allocation of crash_image
@ 2022-11-25 7:48 ` Baoquan He
0 siblings, 0 replies; 28+ messages in thread
From: Baoquan He @ 2022-11-25 7:48 UTC (permalink / raw)
To: Ricardo Ribalda
Cc: Eric Biederman, Philipp Rudo, Sergey Senozhatsky, Ross Zwisler,
kexec, linux-kernel
On 11/25/22 at 08:31am, Ricardo Ribalda wrote:
> Hi
>
> On Fri, 25 Nov 2022 at 08:27, Baoquan He <bhe@redhat.com> wrote:
> >
> > On 11/25/22 at 06:52am, Ricardo Ribalda wrote:
> > > Hi Baoquan
> > >
> > > Thanks for your review!
> > >
> > > On Fri, 25 Nov 2022 at 03:58, Baoquan He <bhe@redhat.com> wrote:
> > > >
> > > > 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.
> > >
> > > Not sure if I follow you.
> > >
> > > We currently check if the crash kernel is in the right place at
> > > sanity_check_segment_list()
> > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/kernel/kexec_core.c#n239
> >
> > And it's not checking if crashkernel memory is valid in
> > sanity_check_segment_list(), right? It's checking if the segments
> > are placed correctly.
>
> If it is not valid, then this condition is not met.
Yeah. BUT, even though it's valid, below condition could not be met
either. They are not the same thing.
>
> /* Ensure we are within the crash kernel limits */
> if ((mstart < phys_to_boot_phys(crashk_res.start)) ||
> (mend > phys_to_boot_phys(crashk_res.end)))
> return -EADDRNOTAVAIL;
>
>
> >
>
>
> --
> Ricardo Ribalda
>
_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH] kexec: Enable runtime allocation of crash_image
2022-11-24 22:23 ` Ricardo Ribalda
@ 2022-11-28 17:00 ` Philipp Rudo
-1 siblings, 0 replies; 28+ messages in thread
From: Philipp Rudo @ 2022-11-28 17:00 UTC (permalink / raw)
To: Ricardo Ribalda
Cc: Eric Biederman, Baoquan He, Sergey Senozhatsky, Ross Zwisler,
kexec, linux-kernel
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,
_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec
^ permalink raw reply [flat|nested] 28+ messages in thread* Re: [PATCH] kexec: Enable runtime allocation of crash_image
@ 2022-11-28 17:00 ` Philipp Rudo
0 siblings, 0 replies; 28+ messages in thread
From: Philipp Rudo @ 2022-11-28 17:00 UTC (permalink / raw)
To: Ricardo Ribalda
Cc: Eric Biederman, Baoquan He, Sergey Senozhatsky, Ross Zwisler,
kexec, linux-kernel
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,
^ permalink raw reply [flat|nested] 28+ messages in thread* Re: [PATCH] kexec: Enable runtime allocation of crash_image
2022-11-28 17:00 ` Philipp Rudo
@ 2022-11-28 17:07 ` Ricardo Ribalda
-1 siblings, 0 replies; 28+ messages in thread
From: Ricardo Ribalda @ 2022-11-28 17:07 UTC (permalink / raw)
To: Philipp Rudo
Cc: Eric Biederman, Baoquan He, Sergey Senozhatsky, Ross Zwisler,
kexec, linux-kernel
Hi Philipp
Thanks for your review.
On Mon, 28 Nov 2022 at 18:00, Philipp Rudo <prudo@redhat.com> wrote:
>
> 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.
Right now, I do not intend to use it to fetch a kdump, I am using it
as the image that will run when the system crashes.
It seems to work fine on the two devices that I am using for tests.
>
> 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,
>
--
Ricardo Ribalda
^ permalink raw reply [flat|nested] 28+ messages in thread* Re: [PATCH] kexec: Enable runtime allocation of crash_image
@ 2022-11-28 17:07 ` Ricardo Ribalda
0 siblings, 0 replies; 28+ messages in thread
From: Ricardo Ribalda @ 2022-11-28 17:07 UTC (permalink / raw)
To: Philipp Rudo
Cc: Eric Biederman, Baoquan He, Sergey Senozhatsky, Ross Zwisler,
kexec, linux-kernel
Hi Philipp
Thanks for your review.
On Mon, 28 Nov 2022 at 18:00, Philipp Rudo <prudo@redhat.com> wrote:
>
> 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.
Right now, I do not intend to use it to fetch a kdump, I am using it
as the image that will run when the system crashes.
It seems to work fine on the two devices that I am using for tests.
>
> 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,
>
--
Ricardo Ribalda
_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec
^ permalink raw reply [flat|nested] 28+ messages in thread* Re: [PATCH] kexec: Enable runtime allocation of crash_image
2022-11-28 17:07 ` Ricardo Ribalda
@ 2022-11-29 15:12 ` Philipp Rudo
-1 siblings, 0 replies; 28+ messages in thread
From: Philipp Rudo @ 2022-11-29 15:12 UTC (permalink / raw)
To: Ricardo Ribalda
Cc: Eric Biederman, Baoquan He, Sergey Senozhatsky, Ross Zwisler,
kexec, linux-kernel
Ricardo,
On Mon, 28 Nov 2022 18:07:06 +0100
Ricardo Ribalda <ribalda@chromium.org> wrote:
> Hi Philipp
>
>
> Thanks for your review.
>
>
> On Mon, 28 Nov 2022 at 18:00, Philipp Rudo <prudo@redhat.com> wrote:
> >
> > 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.
>
> Right now, I do not intend to use it to fetch a kdump, I am using it
> as the image that will run when the system crashes.
the crash_image is currently all about creating a dump. If you want to
change that you need to discuss the new behavior in the commit message!
Please update the commit message.
Thanks
Philipp
>
> It seems to work fine on the two devices that I am using for tests.
>
> >
> > 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,
> >
>
>
^ permalink raw reply [flat|nested] 28+ messages in thread* Re: [PATCH] kexec: Enable runtime allocation of crash_image
@ 2022-11-29 15:12 ` Philipp Rudo
0 siblings, 0 replies; 28+ messages in thread
From: Philipp Rudo @ 2022-11-29 15:12 UTC (permalink / raw)
To: Ricardo Ribalda
Cc: Eric Biederman, Baoquan He, Sergey Senozhatsky, Ross Zwisler,
kexec, linux-kernel
Ricardo,
On Mon, 28 Nov 2022 18:07:06 +0100
Ricardo Ribalda <ribalda@chromium.org> wrote:
> Hi Philipp
>
>
> Thanks for your review.
>
>
> On Mon, 28 Nov 2022 at 18:00, Philipp Rudo <prudo@redhat.com> wrote:
> >
> > 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.
>
> Right now, I do not intend to use it to fetch a kdump, I am using it
> as the image that will run when the system crashes.
the crash_image is currently all about creating a dump. If you want to
change that you need to discuss the new behavior in the commit message!
Please update the commit message.
Thanks
Philipp
>
> It seems to work fine on the two devices that I am using for tests.
>
> >
> > 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,
> >
>
>
_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec
^ permalink raw reply [flat|nested] 28+ messages in thread