Re: [PATCH v4 2/5] kexec: do not special-case the -s option
On Fri, Mar 16, 2018 at 12:38:13PM +0100, Michal Suchánek wrote:
> On Fri, 16 Mar 2018 12:20:25 +0100
> Simon Horman wrote:
>
> > On Thu, Mar 15, 2018 at 12:13:18PM +0100, Michal Suchánek wrote:
> > > On Thu, 15 Mar 2018 11:38:30 +0100
> > > Simon Horman wrote:
>
> > > >
> > > >
> > > > if (do_load && (kexec_flags & KEXEC_ON_CRASH) &&
> > > > !is_crashkernel_mem_reserved()) {
> > > > die("Memory for crashkernel is not reserved\n"
> > > > "Please reserve memory by passing"
> > > > "\"crashkernel=X@Y\" parameter to kernel\n"
> > > > "Then try to loading kdump kernel\n");
> > > > }
> > >
> > > Do you not need memory for kexec -s? This looks broken to start
> > > with.
> >
> > Could you propose a fix? I realise your patchset may not introduce
> > this problem. But it seems to me that it makes things slightly worse
> > or at the very least perpetuates the notion that the above is correct.
>
> Yes, it makes sense to fix the condition.
Thanks.
> > > > ...
> > > >
> > > > if ((result == 0) && do_load_jump_back_helper) {
> > >
> > > And yes, this should not be allowed with -s
> >
> > Is the simple fix here for your patch to add an extra condition
> > to the if statement above?
>
> This is not supported with -s but nothing prevents setting the flag. So
> a test for kexec_load should be added I guess.
That is what I was thinking too.
___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec
Re: [PATCH v4 2/5] kexec: do not special-case the -s option
On Fri, 16 Mar 2018 12:20:25 +0100
Simon Horman wrote:
> On Thu, Mar 15, 2018 at 12:13:18PM +0100, Michal Suchánek wrote:
> > On Thu, 15 Mar 2018 11:38:30 +0100
> > Simon Horman wrote:
> > >
> > >
> > > if (do_load && (kexec_flags & KEXEC_ON_CRASH) &&
> > > !is_crashkernel_mem_reserved()) {
> > > die("Memory for crashkernel is not reserved\n"
> > > "Please reserve memory by passing"
> > > "\"crashkernel=X@Y\" parameter to kernel\n"
> > > "Then try to loading kdump kernel\n");
> > > }
> >
> > Do you not need memory for kexec -s? This looks broken to start
> > with.
>
> Could you propose a fix? I realise your patchset may not introduce
> this problem. But it seems to me that it makes things slightly worse
> or at the very least perpetuates the notion that the above is correct.
Yes, it makes sense to fix the condition.
>
> > > ...
> > >
> > > if ((result == 0) && do_load_jump_back_helper) {
> >
> > And yes, this should not be allowed with -s
>
> Is the simple fix here for your patch to add an extra condition
> to the if statement above?
This is not supported with -s but nothing prevents setting the flag. So
a test for kexec_load should be added I guess.
Thanks
Michal
___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec
Re: [PATCH v4 2/5] kexec: do not special-case the -s option
On Thu, Mar 15, 2018 at 12:13:18PM +0100, Michal Suchánek wrote:
> On Thu, 15 Mar 2018 11:38:30 +0100
> Simon Horman wrote:
>
> > On Tue, Mar 06, 2018 at 02:15:53PM +0100, Michal Suchanek wrote:
> > > It is parsed separately to save a few CPU cycles when setting up
> > > other options but it just complicates the code. So fold it back and
> > > set up all flags both for KEXEC_LOAD and KEXEC_FILE_LOAD
> > >
> > > Signed-off-by: Michal Suchanek
> > > ---
> > > kexec/kexec.c | 25 -
> > > 1 file changed, 4 insertions(+), 21 deletions(-)
> > >
> > > diff --git a/kexec/kexec.c b/kexec/kexec.c
> > > index ab8cff7fe083..9ea102e1565a 100644
> > > --- a/kexec/kexec.c
> > > +++ b/kexec/kexec.c
> > > @@ -1256,19 +1256,6 @@ int main(int argc, char *argv[])
> > > };
> > > static const char short_options[] = KEXEC_ALL_OPT_STR;
> > >
> > > - /*
> > > - * First check if --use-kexec-file-syscall is set. That
> > > changes lot of
> > > - * things
> > > - */
> > > - while ((opt = getopt_long(argc, argv, short_options,
> > > - options, 0)) != -1) {
> > > - switch(opt) {
> > > - case OPT_KEXEC_FILE_SYSCALL:
> > > - do_kexec_file_syscall = 1;
> > > - break;
> > > - }
> > > - }
> > > -
> > > /* Reset getopt for the next pass. */
> > > opterr = 1;
> > > optind = 1;
> > > @@ -1310,8 +1297,7 @@ int main(int argc, char *argv[])
> > > do_shutdown = 0;
> > > do_sync = 0;
> > > do_unload = 1;
> > > - if (do_kexec_file_syscall)
> > > - kexec_file_flags |=
> > > KEXEC_FILE_UNLOAD;
> > > + kexec_file_flags |= KEXEC_FILE_UNLOAD;
> > > break;
> > > case OPT_EXEC:
> > > do_load = 0;
> > @@ -1354,11 +1340,8 @@ int main(int argc, char *argv[])
> >
> > The existing code has the following above the context shown in the
> > patch:
> >
> > do_load = 1;
> >
> >
> > > do_exec = 0;
> > > do_shutdown = 0;
> > > do_sync = 0;
> > > - if (do_kexec_file_syscall)
> > > - kexec_file_flags |=
> > > KEXEC_FILE_ON_CRASH;
> > > - else
> > > - kexec_flags = KEXEC_ON_CRASH;
> > > - break;
> > > + kexec_file_flags |= KEXEC_FILE_ON_CRASH;
> > > + kexec_flags = KEXEC_ON_CRASH;
> >
> > kexec_flags is now set regardless of the value of
> > do_kexec_file_syscall, which was not the case prior to this patch.
> > That seems to affect the following which appears later in the
> > function. Is that ok?
> >
> >
> > if (do_load && (kexec_flags & KEXEC_ON_CRASH) &&
> > !is_crashkernel_mem_reserved()) {
> > die("Memory for crashkernel is not reserved\n"
> > "Please reserve memory by passing"
> > "\"crashkernel=X@Y\" parameter to kernel\n"
> > "Then try to loading kdump kernel\n");
> > }
>
> Do you not need memory for kexec -s? This looks broken to start with.
Could you propose a fix? I realise your patchset may not introduce this
problem. But it seems to me that it makes things slightly worse or at
the very least perpetuates the notion that the above is correct.
> > ...
> >
> > if ((result == 0) && do_load_jump_back_helper) {
>
> And yes, this should not be allowed with -s
Is the simple fix here for your patch to add an extra condition
to the if statement above?
___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec
Re: [PATCH v4 2/5] kexec: do not special-case the -s option
On Thu, 15 Mar 2018 11:38:30 +0100
Simon Horman wrote:
> On Tue, Mar 06, 2018 at 02:15:53PM +0100, Michal Suchanek wrote:
> > It is parsed separately to save a few CPU cycles when setting up
> > other options but it just complicates the code. So fold it back and
> > set up all flags both for KEXEC_LOAD and KEXEC_FILE_LOAD
> >
> > Signed-off-by: Michal Suchanek
> > ---
> > kexec/kexec.c | 25 -
> > 1 file changed, 4 insertions(+), 21 deletions(-)
> >
> > diff --git a/kexec/kexec.c b/kexec/kexec.c
> > index ab8cff7fe083..9ea102e1565a 100644
> > --- a/kexec/kexec.c
> > +++ b/kexec/kexec.c
> > @@ -1256,19 +1256,6 @@ int main(int argc, char *argv[])
> > };
> > static const char short_options[] = KEXEC_ALL_OPT_STR;
> >
> > - /*
> > -* First check if --use-kexec-file-syscall is set. That
> > changes lot of
> > -* things
> > -*/
> > - while ((opt = getopt_long(argc, argv, short_options,
> > - options, 0)) != -1) {
> > - switch(opt) {
> > - case OPT_KEXEC_FILE_SYSCALL:
> > - do_kexec_file_syscall = 1;
> > - break;
> > - }
> > - }
> > -
> > /* Reset getopt for the next pass. */
> > opterr = 1;
> > optind = 1;
> > @@ -1310,8 +1297,7 @@ int main(int argc, char *argv[])
> > do_shutdown = 0;
> > do_sync = 0;
> > do_unload = 1;
> > - if (do_kexec_file_syscall)
> > - kexec_file_flags |=
> > KEXEC_FILE_UNLOAD;
> > + kexec_file_flags |= KEXEC_FILE_UNLOAD;
> > break;
> > case OPT_EXEC:
> > do_load = 0;
> @@ -1354,11 +1340,8 @@ int main(int argc, char *argv[])
>
> The existing code has the following above the context shown in the
> patch:
>
> do_load = 1;
>
>
> > do_exec = 0;
> > do_shutdown = 0;
> > do_sync = 0;
> > - if (do_kexec_file_syscall)
> > - kexec_file_flags |=
> > KEXEC_FILE_ON_CRASH;
> > - else
> > - kexec_flags = KEXEC_ON_CRASH;
> > - break;
> > + kexec_file_flags |= KEXEC_FILE_ON_CRASH;
> > + kexec_flags = KEXEC_ON_CRASH;
>
> kexec_flags is now set regardless of the value of
> do_kexec_file_syscall, which was not the case prior to this patch.
> That seems to affect the following which appears later in the
> function. Is that ok?
>
>
> if (do_load && (kexec_flags & KEXEC_ON_CRASH) &&
> !is_crashkernel_mem_reserved()) {
> die("Memory for crashkernel is not reserved\n"
> "Please reserve memory by passing"
> "\"crashkernel=X@Y\" parameter to kernel\n"
> "Then try to loading kdump kernel\n");
> }
Do you not need memory for kexec -s? This looks broken to start with.
>
> ...
>
> if ((result == 0) && do_load_jump_back_helper) {
And yes, this should not be allowed with -s
Thanks
Michal
___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec
Re: [PATCH v4 2/5] kexec: do not special-case the -s option
On Tue, Mar 06, 2018 at 02:15:53PM +0100, Michal Suchanek wrote:
> It is parsed separately to save a few CPU cycles when setting up other
> options but it just complicates the code. So fold it back and set up all
> flags both for KEXEC_LOAD and KEXEC_FILE_LOAD
>
> Signed-off-by: Michal Suchanek
> ---
> kexec/kexec.c | 25 -
> 1 file changed, 4 insertions(+), 21 deletions(-)
>
> diff --git a/kexec/kexec.c b/kexec/kexec.c
> index ab8cff7fe083..9ea102e1565a 100644
> --- a/kexec/kexec.c
> +++ b/kexec/kexec.c
> @@ -1256,19 +1256,6 @@ int main(int argc, char *argv[])
> };
> static const char short_options[] = KEXEC_ALL_OPT_STR;
>
> - /*
> - * First check if --use-kexec-file-syscall is set. That changes lot of
> - * things
> - */
> - while ((opt = getopt_long(argc, argv, short_options,
> - options, 0)) != -1) {
> - switch(opt) {
> - case OPT_KEXEC_FILE_SYSCALL:
> - do_kexec_file_syscall = 1;
> - break;
> - }
> - }
> -
> /* Reset getopt for the next pass. */
> opterr = 1;
> optind = 1;
> @@ -1310,8 +1297,7 @@ int main(int argc, char *argv[])
> do_shutdown = 0;
> do_sync = 0;
> do_unload = 1;
> - if (do_kexec_file_syscall)
> - kexec_file_flags |= KEXEC_FILE_UNLOAD;
> + kexec_file_flags |= KEXEC_FILE_UNLOAD;
> break;
> case OPT_EXEC:
> do_load = 0;
@@ -1354,11 +1340,8 @@ int main(int argc, char *argv[])
The existing code has the following above the context shown in the patch:
do_load = 1;
> do_exec = 0;
> do_shutdown = 0;
> do_sync = 0;
> - if (do_kexec_file_syscall)
> - kexec_file_flags |= KEXEC_FILE_ON_CRASH;
> - else
> - kexec_flags = KEXEC_ON_CRASH;
> - break;
> + kexec_file_flags |= KEXEC_FILE_ON_CRASH;
> + kexec_flags = KEXEC_ON_CRASH;
kexec_flags is now set regardless of the value of do_kexec_file_syscall,
which was not the case prior to this patch. That seems to affect the
following which appears later in the function. Is that ok?
if (do_load && (kexec_flags & KEXEC_ON_CRASH) &&
!is_crashkernel_mem_reserved()) {
die("Memory for crashkernel is not reserved\n"
"Please reserve memory by passing"
"\"crashkernel=X@Y\" parameter to kernel\n"
"Then try to loading kdump kernel\n");
}
...
if ((result == 0) && do_load_jump_back_helper) {
result = my_load_jump_back_helper(kexec_flags, entry);
}
> case OPT_MEM_MIN:
> mem_min = strtoul(optarg, &endptr, 0);
> if (*endptr) {
> @@ -1383,7 +1366,7 @@ int main(int argc, char *argv[])
> do_reuse_initrd = 1;
> break;
> case OPT_KEXEC_FILE_SYSCALL:
> - /* We already parsed it. Nothing to do. */
> + do_kexec_file_syscall = 1;
> break;
> case OPT_STATUS:
> do_status = 1;
> --
> 2.13.6
>
___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec
[PATCH v4 2/5] kexec: do not special-case the -s option
It is parsed separately to save a few CPU cycles when setting up other
options but it just complicates the code. So fold it back and set up all
flags both for KEXEC_LOAD and KEXEC_FILE_LOAD
Signed-off-by: Michal Suchanek
---
kexec/kexec.c | 25 -
1 file changed, 4 insertions(+), 21 deletions(-)
diff --git a/kexec/kexec.c b/kexec/kexec.c
index ab8cff7fe083..9ea102e1565a 100644
--- a/kexec/kexec.c
+++ b/kexec/kexec.c
@@ -1256,19 +1256,6 @@ int main(int argc, char *argv[])
};
static const char short_options[] = KEXEC_ALL_OPT_STR;
- /*
-* First check if --use-kexec-file-syscall is set. That changes lot of
-* things
-*/
- while ((opt = getopt_long(argc, argv, short_options,
- options, 0)) != -1) {
- switch(opt) {
- case OPT_KEXEC_FILE_SYSCALL:
- do_kexec_file_syscall = 1;
- break;
- }
- }
-
/* Reset getopt for the next pass. */
opterr = 1;
optind = 1;
@@ -1310,8 +1297,7 @@ int main(int argc, char *argv[])
do_shutdown = 0;
do_sync = 0;
do_unload = 1;
- if (do_kexec_file_syscall)
- kexec_file_flags |= KEXEC_FILE_UNLOAD;
+ kexec_file_flags |= KEXEC_FILE_UNLOAD;
break;
case OPT_EXEC:
do_load = 0;
@@ -1354,11 +1340,8 @@ int main(int argc, char *argv[])
do_exec = 0;
do_shutdown = 0;
do_sync = 0;
- if (do_kexec_file_syscall)
- kexec_file_flags |= KEXEC_FILE_ON_CRASH;
- else
- kexec_flags = KEXEC_ON_CRASH;
- break;
+ kexec_file_flags |= KEXEC_FILE_ON_CRASH;
+ kexec_flags = KEXEC_ON_CRASH;
case OPT_MEM_MIN:
mem_min = strtoul(optarg, &endptr, 0);
if (*endptr) {
@@ -1383,7 +1366,7 @@ int main(int argc, char *argv[])
do_reuse_initrd = 1;
break;
case OPT_KEXEC_FILE_SYSCALL:
- /* We already parsed it. Nothing to do. */
+ do_kexec_file_syscall = 1;
break;
case OPT_STATUS:
do_status = 1;
--
2.13.6
___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec
