| Message ID | 20170814070849.20986-40-jgross@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
>>> On 14.08.17 at 09:08, <jgross@suse.com> wrote: > --- a/xen/common/kernel.c > +++ b/xen/common/kernel.c > @@ -23,9 +23,11 @@ enum system_state system_state = SYS_STATE_early_boot; > xen_commandline_t saved_cmdline; > static const char __initconst opt_builtin_cmdline[] = CONFIG_CMDLINE; > > -static void __init assign_integer_param( > +static int __init assign_integer_param( > const struct kernel_param *param, uint64_t val) > { > + unsigned int bits = param->len * BITS_PER_BYTE; > + > switch ( param->len ) > { > case sizeof(uint8_t): > @@ -43,14 +45,17 @@ static void __init assign_integer_param( > default: > BUG(); > } > + > + return ( (val & (~0ULL << bits)) && ~(val | (~0ULL >> (65 - bits))) ) ? The left part has undefined behavior when param->len == 8 (and on x86 I'd expect it to produce just "val"). The right part I guess is meant to be a sign check, but that's rather obscure. As iirc it is signed-to-unsigned conversion which has uniformly defined behavior it may end up being better for the parameter to be of signed type and to allow values in the range [<type>_MIN,U<type>_MAX]. Anything more precise would require signedness to be communicated from the *_param() users. Also - stray blanks inside the outermost parentheses. And finally, wouldn't it be better to check for overflow _before_ assigning to *param->var? > @@ -97,8 +102,9 @@ static void __init _cmdline_parse(const char *cmdline) > !strncmp(param->name, opt, q + 1 - opt) ) > { > optval[-1] = '='; > - ((void (*)(const char *))param->var)(q); > + rc = ((int (*)(const char *))param->var)(q); Neither here nor in the earlier "let custom parameter parsing routines return errno" nor in the overview you mention why this is safe - it is not a given that caller and callee disagreeing on return type is going to work. Just think of functions returning aggregates or (on ix86) ones returning floating point values in st(0). > optval[-1] = '\0'; > + break; Why? Applies to further break-s you add: At least in the past we had command line options with two handlers, where each of them needed to be invoked. I don't think we should make such impossible even if right now there aren't any such examples. Yet if you really mean to, then the behavioral change needs to be called out in the description. > @@ -106,24 +112,34 @@ static void __init _cmdline_parse(const char *cmdline) > switch ( param->type ) > { > case OPT_STR: > + rc = 0; > strlcpy(param->var, optval, param->len); > break; > case OPT_UINT: > - assign_integer_param( > + rc = assign_integer_param( > param, > - simple_strtoll(optval, NULL, 0)); > + simple_strtoll(optval, &s, 0)); > + if ( *s ) > + rc = -EINVAL; > break; > case OPT_BOOL: > - if ( !parse_bool(optval) ) > + rc = parse_bool(optval); > + if ( rc == -1 ) Maybe "rc < 0"? > @@ -131,13 +147,21 @@ static void __init _cmdline_parse(const char *cmdline) > safe_strcpy(opt, "no"); > optval = opt; > } > - ((void (*)(const char *))param->var)(optval); > + rc = ((int (*)(const char *))param->var)(optval); > break; > default: > BUG(); > break; > } > + > + break; > } > + > + if ( rc ) > + printk("parameter \"%s\" has invalid value \"%s\"!\n", optkey, > + optval); With the changes made to optval in OPT_CUSTOM handling this may end up being confusing. Jan
On 14/08/17 14:46, Jan Beulich wrote: >>>> On 14.08.17 at 09:08, <jgross@suse.com> wrote: >> --- a/xen/common/kernel.c >> +++ b/xen/common/kernel.c >> @@ -23,9 +23,11 @@ enum system_state system_state = SYS_STATE_early_boot; >> xen_commandline_t saved_cmdline; >> static const char __initconst opt_builtin_cmdline[] = CONFIG_CMDLINE; >> >> -static void __init assign_integer_param( >> +static int __init assign_integer_param( >> const struct kernel_param *param, uint64_t val) >> { >> + unsigned int bits = param->len * BITS_PER_BYTE; >> + >> switch ( param->len ) >> { >> case sizeof(uint8_t): >> @@ -43,14 +45,17 @@ static void __init assign_integer_param( >> default: >> BUG(); >> } >> + >> + return ( (val & (~0ULL << bits)) && ~(val | (~0ULL >> (65 - bits))) ) ? > > The left part has undefined behavior when param->len == 8 > (and on x86 I'd expect it to produce just "val"). The right part > I guess is meant to be a sign check, but that's rather obscure. Hmm, okay. > As iirc it is signed-to-unsigned conversion which has uniformly > defined behavior it may end up being better for the parameter > to be of signed type and to allow values in the range > [<type>_MIN,U<type>_MAX]. Anything more precise would > require signedness to be communicated from the *_param() > users. Okay, I'll have a try. > Also - stray blanks inside the outermost parentheses. > > And finally, wouldn't it be better to check for overflow _before_ > assigning to *param->var? I didn't want to change existing behavior. OTOH thinking twice you are right. Better using the default value than an unexpected small one. > >> @@ -97,8 +102,9 @@ static void __init _cmdline_parse(const char *cmdline) >> !strncmp(param->name, opt, q + 1 - opt) ) >> { >> optval[-1] = '='; >> - ((void (*)(const char *))param->var)(q); >> + rc = ((int (*)(const char *))param->var)(q); > > Neither here nor in the earlier "let custom parameter parsing > routines return errno" nor in the overview you mention why this > is safe - it is not a given that caller and callee disagreeing on > return type is going to work. Just think of functions returning > aggregates or (on ix86) ones returning floating point values in > st(0). I thought about using a union in struct kernel_param and removing above type cast. This would require modifying the initialization of the kernel_param struct via the *_param() macros, though. The other possibility would be using __builtin_types_compatible_p() to check the function to be of proper type. What would you like best? > >> optval[-1] = '\0'; >> + break; > > Why? Applies to further break-s you add: At least in the past we > had command line options with two handlers, where each of them > needed to be invoked. I don't think we should make such impossible > even if right now there aren't any such examples. Yet if you really > mean to, then the behavioral change needs to be called out in the > description. I wasn't aware of such a usage. I'm fine for both alternatives. As you seem to prefer to keep support for multiple handlers I'll modify the patch to allow that. >> @@ -106,24 +112,34 @@ static void __init _cmdline_parse(const char *cmdline) >> switch ( param->type ) >> { >> case OPT_STR: >> + rc = 0; >> strlcpy(param->var, optval, param->len); >> break; >> case OPT_UINT: >> - assign_integer_param( >> + rc = assign_integer_param( >> param, >> - simple_strtoll(optval, NULL, 0)); >> + simple_strtoll(optval, &s, 0)); >> + if ( *s ) >> + rc = -EINVAL; >> break; >> case OPT_BOOL: >> - if ( !parse_bool(optval) ) >> + rc = parse_bool(optval); >> + if ( rc == -1 ) > > Maybe "rc < 0"? Okay. > >> @@ -131,13 +147,21 @@ static void __init _cmdline_parse(const char *cmdline) >> safe_strcpy(opt, "no"); >> optval = opt; >> } >> - ((void (*)(const char *))param->var)(optval); >> + rc = ((int (*)(const char *))param->var)(optval); >> break; >> default: >> BUG(); >> break; >> } >> + >> + break; >> } >> + >> + if ( rc ) >> + printk("parameter \"%s\" has invalid value \"%s\"!\n", optkey, >> + optval); > > With the changes made to optval in OPT_CUSTOM handling this > may end up being confusing. Oh yes, good catch. Juergen
>>> On 14.08.17 at 15:31, <jgross@suse.com> wrote: > On 14/08/17 14:46, Jan Beulich wrote: >>>>> On 14.08.17 at 09:08, <jgross@suse.com> wrote: >>> @@ -97,8 +102,9 @@ static void __init _cmdline_parse(const char *cmdline) >>> !strncmp(param->name, opt, q + 1 - opt) ) >>> { >>> optval[-1] = '='; >>> - ((void (*)(const char *))param->var)(q); >>> + rc = ((int (*)(const char *))param->var)(q); >> >> Neither here nor in the earlier "let custom parameter parsing >> routines return errno" nor in the overview you mention why this >> is safe - it is not a given that caller and callee disagreeing on >> return type is going to work. Just think of functions returning >> aggregates or (on ix86) ones returning floating point values in >> st(0). > > I thought about using a union in struct kernel_param and removing > above type cast. This would require modifying the initialization of > the kernel_param struct via the *_param() macros, though. > > The other possibility would be using __builtin_types_compatible_p() > to check the function to be of proper type. > > What would you like best? I'd prefer the union approach; I was actually surprised to see we still only have a void * there. Jan
On 14/08/17 14:46, Jan Beulich wrote: >>>> On 14.08.17 at 09:08, <jgross@suse.com> wrote: >> --- a/xen/common/kernel.c >> +++ b/xen/common/kernel.c >> optval[-1] = '\0'; >> + break; > > Why? Applies to further break-s you add: At least in the past we > had command line options with two handlers, where each of them > needed to be invoked. I don't think we should make such impossible > even if right now there aren't any such examples. Yet if you really > mean to, then the behavioral change needs to be called out in the > description. While working on this I realized that this functionality has been working only in some cases. The custom parsing functions are being called with a copy of the option value, which they modify in some cases. So a second handler being called would see another value as the first handler, as long as modifying the option value keeps to be allowed. I see three possibilities here: 1. don't allow multiple handlers for the same parameter 2. restore the option value before calling each handler (as the error message I'm adding with this patch requires access to the whole option value this wouldn't be too hard) 3. don't allow a handler to modify the option value (solves my error message problem, too) Any preferences? Juergen
>>> On 15.08.17 at 14:54, <jgross@suse.com> wrote: > On 14/08/17 14:46, Jan Beulich wrote: >>>>> On 14.08.17 at 09:08, <jgross@suse.com> wrote: >>> --- a/xen/common/kernel.c >>> +++ b/xen/common/kernel.c >>> optval[-1] = '\0'; >>> + break; >> >> Why? Applies to further break-s you add: At least in the past we >> had command line options with two handlers, where each of them >> needed to be invoked. I don't think we should make such impossible >> even if right now there aren't any such examples. Yet if you really >> mean to, then the behavioral change needs to be called out in the >> description. > > While working on this I realized that this functionality has been > working only in some cases. The custom parsing functions are being > called with a copy of the option value, which they modify in some > cases. So a second handler being called would see another value as > the first handler, as long as modifying the option value keeps to be > allowed. > > I see three possibilities here: > > 1. don't allow multiple handlers for the same parameter > 2. restore the option value before calling each handler (as the > error message I'm adding with this patch requires access to the > whole option value this wouldn't be too hard) > 3. don't allow a handler to modify the option value (solves my error > message problem, too) > > Any preferences? I have no particular preference between 2 and 3, but both are better than 1. Jan
On Tue, Aug 15, 2017 at 02:54:07PM +0200, Juergen Gross wrote: > On 14/08/17 14:46, Jan Beulich wrote: > >>>> On 14.08.17 at 09:08, <jgross@suse.com> wrote: > >> --- a/xen/common/kernel.c > >> +++ b/xen/common/kernel.c > >> optval[-1] = '\0'; > >> + break; > > > > Why? Applies to further break-s you add: At least in the past we > > had command line options with two handlers, where each of them > > needed to be invoked. I don't think we should make such impossible > > even if right now there aren't any such examples. Yet if you really > > mean to, then the behavioral change needs to be called out in the > > description. > > While working on this I realized that this functionality has been > working only in some cases. The custom parsing functions are being > called with a copy of the option value, which they modify in some > cases. So a second handler being called would see another value as > the first handler, as long as modifying the option value keeps to be > allowed. > > I see three possibilities here: > > 1. don't allow multiple handlers for the same parameter > 2. restore the option value before calling each handler (as the > error message I'm adding with this patch requires access to the > whole option value this wouldn't be too hard) > 3. don't allow a handler to modify the option value (solves my error > message problem, too) Either 2 or 3 please.
diff --git a/xen/common/kernel.c b/xen/common/kernel.c index ce7cb8adb5..756380be5b 100644 --- a/xen/common/kernel.c +++ b/xen/common/kernel.c @@ -23,9 +23,11 @@ enum system_state system_state = SYS_STATE_early_boot; xen_commandline_t saved_cmdline; static const char __initconst opt_builtin_cmdline[] = CONFIG_CMDLINE; -static void __init assign_integer_param( +static int __init assign_integer_param( const struct kernel_param *param, uint64_t val) { + unsigned int bits = param->len * BITS_PER_BYTE; + switch ( param->len ) { case sizeof(uint8_t): @@ -43,14 +45,17 @@ static void __init assign_integer_param( default: BUG(); } + + return ( (val & (~0ULL << bits)) && ~(val | (~0ULL >> (65 - bits))) ) ? + -EOVERFLOW : 0; } static void __init _cmdline_parse(const char *cmdline) { char opt[128], *optval, *optkey, *q; - const char *p = cmdline; + const char *p = cmdline, *s; const struct kernel_param *param; - int bool_assert; + int bool_assert, rc = 0; for ( ; ; ) { @@ -97,8 +102,9 @@ static void __init _cmdline_parse(const char *cmdline) !strncmp(param->name, opt, q + 1 - opt) ) { optval[-1] = '='; - ((void (*)(const char *))param->var)(q); + rc = ((int (*)(const char *))param->var)(q); optval[-1] = '\0'; + break; } continue; } @@ -106,24 +112,34 @@ static void __init _cmdline_parse(const char *cmdline) switch ( param->type ) { case OPT_STR: + rc = 0; strlcpy(param->var, optval, param->len); break; case OPT_UINT: - assign_integer_param( + rc = assign_integer_param( param, - simple_strtoll(optval, NULL, 0)); + simple_strtoll(optval, &s, 0)); + if ( *s ) + rc = -EINVAL; break; case OPT_BOOL: - if ( !parse_bool(optval) ) + rc = parse_bool(optval); + if ( rc == -1 ) + break; + if ( !rc ) bool_assert = !bool_assert; + rc = 0; assign_integer_param(param, bool_assert); break; case OPT_SIZE: - assign_integer_param( + rc = assign_integer_param( param, - parse_size_and_unit(optval, NULL)); + parse_size_and_unit(optval, &s)); + if ( *s ) + rc = -EINVAL; break; case OPT_CUSTOM: + rc = -EINVAL; if ( !bool_assert ) { if ( *optval ) @@ -131,13 +147,21 @@ static void __init _cmdline_parse(const char *cmdline) safe_strcpy(opt, "no"); optval = opt; } - ((void (*)(const char *))param->var)(optval); + rc = ((int (*)(const char *))param->var)(optval); break; default: BUG(); break; } + + break; } + + if ( rc ) + printk("parameter \"%s\" has invalid value \"%s\"!\n", optkey, + optval); + if ( param >= __setup_end ) + printk("parameter \"%s\" unknown!\n", optkey); } } @@ -176,7 +200,8 @@ int __init parse_bool(const char *s) !strcmp("on", s) || !strcmp("true", s) || !strcmp("enable", s) || - !strcmp("1", s) ) + !strcmp("1", s) || + !*s ) return 1; return -1;