[PATCH v4 2/5] kexec: do not special-case the -s option

[Simon Horman]

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 <horms at verge.net.au> 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 <msuchanek at suse.de>
> > > ---
> > >  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 at 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?