2022-11-24 22:47:12

by Ricardo Ribalda

[permalink] [raw]
Subject: [PATCH] kexec: Enable runtime allocation of crash_image

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 <[email protected]>
---
kexec: Enable runtime allocation of crash_image

To: Eric Biederman <[email protected]>
Cc: [email protected]
Cc: [email protected]
Cc: Sergey Senozhatsky <[email protected]>
Cc: [email protected]
Cc: Ross Zwisler <[email protected]>
Cc: Philipp Rudo <[email protected]>
Cc: Baoquan He <[email protected]>
---
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 <[email protected]>


2022-11-25 03:29:38

by Baoquan He

[permalink] [raw]
Subject: Re: [PATCH] kexec: Enable runtime allocation of crash_image

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 <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
> Cc: Sergey Senozhatsky <[email protected]>
> Cc: [email protected]
> Cc: Ross Zwisler <[email protected]>
> Cc: Philipp Rudo <[email protected]>
> Cc: Baoquan He <[email protected]>
> ---
> 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 <[email protected]>
>

2022-11-25 06:22:03

by Ricardo Ribalda

[permalink] [raw]
Subject: Re: [PATCH] kexec: Enable runtime allocation of crash_image

Hi Baoquan

Thanks for your review!

On Fri, 25 Nov 2022 at 03:58, Baoquan He <[email protected]> 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 <[email protected]>
> > Cc: [email protected]
> > Cc: [email protected]
> > Cc: Sergey Senozhatsky <[email protected]>
> > Cc: [email protected]
> > Cc: Ross Zwisler <[email protected]>
> > Cc: Philipp Rudo <[email protected]>
> > Cc: Baoquan He <[email protected]>
> > ---
> > 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 <[email protected]>
> >
>


--
Ricardo Ribalda

2022-11-25 07:29:11

by Baoquan He

[permalink] [raw]
Subject: Re: [PATCH] kexec: Enable runtime allocation of crash_image

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 <[email protected]> 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"
"\"[email protected]\" 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 <[email protected]>
> > > Cc: [email protected]
> > > Cc: [email protected]
> > > Cc: Sergey Senozhatsky <[email protected]>
> > > Cc: [email protected]
> > > Cc: Ross Zwisler <[email protected]>
> > > Cc: Philipp Rudo <[email protected]>
> > > Cc: Baoquan He <[email protected]>
> > > ---
> > > 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 <[email protected]>
> > >
> >
>
>
> --
> Ricardo Ribalda
>

2022-11-25 07:34:34

by Ricardo Ribalda

[permalink] [raw]
Subject: Re: [PATCH] kexec: Enable runtime allocation of crash_image

Hi Baoquan

On Fri, 25 Nov 2022 at 08:15, Baoquan He <[email protected]> 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 <[email protected]> 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"
> "\"[email protected]\" 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 <[email protected]>
> > > > Cc: [email protected]
> > > > Cc: [email protected]
> > > > Cc: Sergey Senozhatsky <[email protected]>
> > > > Cc: [email protected]
> > > > Cc: Ross Zwisler <[email protected]>
> > > > Cc: Philipp Rudo <[email protected]>
> > > > Cc: Baoquan He <[email protected]>
> > > > ---
> > > > 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 <[email protected]>
> > > >
> > >
> >
> >
> > --
> > Ricardo Ribalda
> >
>


--
Ricardo Ribalda

2022-11-25 07:43:43

by Baoquan He

[permalink] [raw]
Subject: Re: [PATCH] kexec: Enable runtime allocation of crash_image

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 <[email protected]> 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.

2022-11-25 08:00:57

by Ricardo Ribalda

[permalink] [raw]
Subject: Re: [PATCH] kexec: Enable runtime allocation of crash_image

Hi

On Fri, 25 Nov 2022 at 08:27, Baoquan He <[email protected]> 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 <[email protected]> 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

2022-11-25 08:11:47

by Baoquan He

[permalink] [raw]
Subject: Re: [PATCH] kexec: Enable runtime allocation of crash_image

On 11/25/22 at 08:31am, Ricardo Ribalda wrote:
> Hi
>
> On Fri, 25 Nov 2022 at 08:27, Baoquan He <[email protected]> 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 <[email protected]> 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
>

2022-11-25 08:36:19

by Baoquan He

[permalink] [raw]
Subject: Re: [PATCH] kexec: Enable runtime allocation of crash_image

On 11/25/22 at 08:26am, Ricardo Ribalda wrote:
> Hi Baoquan
>
> On Fri, 25 Nov 2022 at 08:15, Baoquan He <[email protected]> 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 <[email protected]> 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"
> > "\"[email protected]\" 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 <[email protected]>
> > > > > Cc: [email protected]
> > > > > Cc: [email protected]
> > > > > Cc: Sergey Senozhatsky <[email protected]>
> > > > > Cc: [email protected]
> > > > > Cc: Ross Zwisler <[email protected]>
> > > > > Cc: Philipp Rudo <[email protected]>
> > > > > Cc: Baoquan He <[email protected]>
> > > > > ---
> > > > > 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 <[email protected]>
> > > > >
> > > >
> > >
> > >
> > > --
> > > Ricardo Ribalda
> > >
> >
>
>
> --
> Ricardo Ribalda
>

2022-11-25 08:52:27

by Ricardo Ribalda

[permalink] [raw]
Subject: Re: [PATCH] kexec: Enable runtime allocation of crash_image

Hi Baoquan

On Fri, 25 Nov 2022 at 08:45, Baoquan He <[email protected]> wrote:
>
> On 11/25/22 at 08:26am, Ricardo Ribalda wrote:
> > Hi Baoquan
> >
> > On Fri, 25 Nov 2022 at 08:15, Baoquan He <[email protected]> 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 <[email protected]> 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"
> > > "\"[email protected]\" 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 <[email protected]>
> > > > > > Cc: [email protected]
> > > > > > Cc: [email protected]
> > > > > > Cc: Sergey Senozhatsky <[email protected]>
> > > > > > Cc: [email protected]
> > > > > > Cc: Ross Zwisler <[email protected]>
> > > > > > Cc: Philipp Rudo <[email protected]>
> > > > > > Cc: Baoquan He <[email protected]>
> > > > > > ---
> > > > > > 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 <[email protected]>
> > > > > >
> > > > >
> > > >
> > > >
> > > > --
> > > > Ricardo Ribalda
> > > >
> > >
> >
> >
> > --
> > Ricardo Ribalda
> >
>


--
Ricardo Ribalda

2022-11-25 09:41:43

by Baoquan He

[permalink] [raw]
Subject: Re: [PATCH] kexec: Enable runtime allocation of crash_image

On 11/25/22 at 09:10am, Ricardo Ribalda wrote:
> Hi Baoquan
>
> On Fri, 25 Nov 2022 at 08:45, Baoquan He <[email protected]> wrote:
> >
> > On 11/25/22 at 08:26am, Ricardo Ribalda wrote:
> > > Hi Baoquan
> > >
> > > On Fri, 25 Nov 2022 at 08:15, Baoquan He <[email protected]> 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 <[email protected]> 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"
> > > > "\"[email protected]\" 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 <[email protected]>
> > > > > > > Cc: [email protected]
> > > > > > > Cc: [email protected]
> > > > > > > Cc: Sergey Senozhatsky <[email protected]>
> > > > > > > Cc: [email protected]rnel.org
> > > > > > > Cc: Ross Zwisler <[email protected]>
> > > > > > > Cc: Philipp Rudo <[email protected]>
> > > > > > > Cc: Baoquan He <[email protected]>
> > > > > > > ---
> > > > > > > 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 <[email protected]>
> > > > > > >
> > > > > >
> > > > >
> > > > >
> > > > > --
> > > > > Ricardo Ribalda
> > > > >
> > > >
> > >
> > >
> > > --
> > > Ricardo Ribalda
> > >
> >
>
>
> --
> Ricardo Ribalda
>