| Message ID | 20170816125219.5255-40-jgross@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
>>> On 16.08.17 at 14:52, <jgross@suse.com> wrote: > static void __init _cmdline_parse(const char *cmdline) > { > char opt[128], *optval, *optkey, *q; > - const char *p = cmdline; > + const char *p = cmdline, *s, *key; > const struct kernel_param *param; > - int bool_assert; > + int bool_assert, rctmp, rc; > + bool found; If you touch this anyway, I think bool_assert should become bool too. And perhaps worthwhile shrinking the scope of at least some of the variables you add/touch. > @@ -131,13 +157,21 @@ static void __init _cmdline_parse(const char *cmdline) > safe_strcpy(opt, "no"); > optval = opt; > } > - ((void (*)(const char *))param->var)(optval); > + rctmp = param->par.func(optval); > break; > default: > BUG(); > break; > } > + > + if ( !rc ) > + rc = rctmp; > } > + > + if ( rc ) > + printk("parameter \"%s\" has invalid value \"%s\"!\n", key, optval); Since a few different rc values are possible by now, it's perhaps worth also logging rc. > @@ -176,7 +210,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; Careful with this: Taking the "iommu=" example that I've commented on in the other patch already, much depends on what you mean to do about the problem there: "iommu=,..." should not end up meaning "iommu=on,...". > --- a/xen/include/xen/init.h > +++ b/xen/include/xen/init.h > @@ -83,7 +83,10 @@ struct kernel_param { > OPT_CUSTOM > } type; > unsigned int len; > - void *var; > + union { > + void *var; > + int (*func)(const char *); > + } par; > }; > > extern const struct kernel_param __setup_start[], __setup_end[]; > @@ -95,23 +98,38 @@ extern const struct kernel_param __setup_start[], > __setup_end[]; > > #define custom_param(_name, _var) \ > __setup_str __setup_str_##_var[] = _name; \ > - __kparam __setup_##_var = { __setup_str_##_var, OPT_CUSTOM, 0, _var } > + __kparam __setup_##_var = \ > + { .name = __setup_str_##_var, \ > + .type = OPT_CUSTOM, \ > + .par.func = _var } I much appreciate this, as it is only now that we can be sure all handler functions are of the intended type. Jan
On 22/08/17 13:24, Jan Beulich wrote: >>>> On 16.08.17 at 14:52, <jgross@suse.com> wrote: >> static void __init _cmdline_parse(const char *cmdline) >> { >> char opt[128], *optval, *optkey, *q; >> - const char *p = cmdline; >> + const char *p = cmdline, *s, *key; >> const struct kernel_param *param; >> - int bool_assert; >> + int bool_assert, rctmp, rc; >> + bool found; > > If you touch this anyway, I think bool_assert should become bool too. > And perhaps worthwhile shrinking the scope of at least some of the > variables you add/touch. Okay. > >> @@ -131,13 +157,21 @@ static void __init _cmdline_parse(const char *cmdline) >> safe_strcpy(opt, "no"); >> optval = opt; >> } >> - ((void (*)(const char *))param->var)(optval); >> + rctmp = param->par.func(optval); >> break; >> default: >> BUG(); >> break; >> } >> + >> + if ( !rc ) >> + rc = rctmp; >> } >> + >> + if ( rc ) >> + printk("parameter \"%s\" has invalid value \"%s\"!\n", key, optval); > > Since a few different rc values are possible by now, it's perhaps > worth also logging rc. > >> @@ -176,7 +210,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; > > Careful with this: Taking the "iommu=" example that I've commented > on in the other patch already, much depends on what you mean to > do about the problem there: "iommu=,..." should not end up > meaning "iommu=on,...". It won't. *s will be ',' in this case. Juergen
>>> On 23.08.17 at 11:30, <jgross@suse.com> wrote: > On 22/08/17 13:24, Jan Beulich wrote: >>>>> On 16.08.17 at 14:52, <jgross@suse.com> wrote: >>> @@ -176,7 +210,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; >> >> Careful with this: Taking the "iommu=" example that I've commented >> on in the other patch already, much depends on what you mean to >> do about the problem there: "iommu=,..." should not end up >> meaning "iommu=on,...". > > It won't. *s will be ',' in this case. Right, but as said - much depends on what you mean to do about the problem in the earlier patch. Jan
On 23/08/17 11:38, Jan Beulich wrote: >>>> On 23.08.17 at 11:30, <jgross@suse.com> wrote: >> On 22/08/17 13:24, Jan Beulich wrote: >>>>>> On 16.08.17 at 14:52, <jgross@suse.com> wrote: >>>> @@ -176,7 +210,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; >>> >>> Careful with this: Taking the "iommu=" example that I've commented >>> on in the other patch already, much depends on what you mean to >>> do about the problem there: "iommu=,..." should not end up >>> meaning "iommu=on,...". >> >> It won't. *s will be ',' in this case. > > Right, but as said - much depends on what you mean to do about > the problem in the earlier patch. So I just hit this. And looking more thoroughly into it: today it in fact has exactly this meaning. iommu_enable is "1" per default. So specifying "iommu=,..." won't change this and has the same semantics as "iommu=on,...". So should I change this or not? With the new parse_bool() parameter (end of the string pointer or NULL) I can easily tell the difference between a boolean with or without following options. Juergen
>>> On 23.08.17 at 14:42, <jgross@suse.com> wrote: > On 23/08/17 11:38, Jan Beulich wrote: >>>>> On 23.08.17 at 11:30, <jgross@suse.com> wrote: >>> On 22/08/17 13:24, Jan Beulich wrote: >>>>>>> On 16.08.17 at 14:52, <jgross@suse.com> wrote: >>>>> @@ -176,7 +210,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; >>>> >>>> Careful with this: Taking the "iommu=" example that I've commented >>>> on in the other patch already, much depends on what you mean to >>>> do about the problem there: "iommu=,..." should not end up >>>> meaning "iommu=on,...". >>> >>> It won't. *s will be ',' in this case. >> >> Right, but as said - much depends on what you mean to do about >> the problem in the earlier patch. > > So I just hit this. And looking more thoroughly into it: today it in > fact has exactly this meaning. iommu_enable is "1" per default. So > specifying "iommu=,..." won't change this and has the same semantics > as "iommu=on,...". But that's not the interesting case. Are you saying that "iommu=off iommu=,..." enables the IOMMU today? It doesn't look to me as if it would. Jan
On 23/08/17 15:18, Jan Beulich wrote: >>>> On 23.08.17 at 14:42, <jgross@suse.com> wrote: >> On 23/08/17 11:38, Jan Beulich wrote: >>>>>> On 23.08.17 at 11:30, <jgross@suse.com> wrote: >>>> On 22/08/17 13:24, Jan Beulich wrote: >>>>>>>> On 16.08.17 at 14:52, <jgross@suse.com> wrote: >>>>>> @@ -176,7 +210,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; >>>>> >>>>> Careful with this: Taking the "iommu=" example that I've commented >>>>> on in the other patch already, much depends on what you mean to >>>>> do about the problem there: "iommu=,..." should not end up >>>>> meaning "iommu=on,...". >>>> >>>> It won't. *s will be ',' in this case. >>> >>> Right, but as said - much depends on what you mean to do about >>> the problem in the earlier patch. >> >> So I just hit this. And looking more thoroughly into it: today it in >> fact has exactly this meaning. iommu_enable is "1" per default. So >> specifying "iommu=,..." won't change this and has the same semantics >> as "iommu=on,...". > > But that's not the interesting case. Are you saying that > "iommu=off iommu=,..." enables the IOMMU today? It doesn't > look to me as if it would. I guess this is a weird case after all: trying to be compatible to "iommu=off iommu=,..." while "iommu=off iommu=on,..." doesn't work correctly today is questionable. OTOH the easiest way to avoid all discussions might be to drop the parse_bool() modification to return "true" in case of an empty string and handle this case by not calling parse_bool() at all from _cmdline_parse() if no argument was given. Juergen
>>> On 23.08.17 at 16:21, <jgross@suse.com> wrote: > On 23/08/17 15:18, Jan Beulich wrote: >>>>> On 23.08.17 at 14:42, <jgross@suse.com> wrote: >>> On 23/08/17 11:38, Jan Beulich wrote: >>>>>>> On 23.08.17 at 11:30, <jgross@suse.com> wrote: >>>>> On 22/08/17 13:24, Jan Beulich wrote: >>>>>>>>> On 16.08.17 at 14:52, <jgross@suse.com> wrote: >>>>>>> @@ -176,7 +210,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; >>>>>> >>>>>> Careful with this: Taking the "iommu=" example that I've commented >>>>>> on in the other patch already, much depends on what you mean to >>>>>> do about the problem there: "iommu=,..." should not end up >>>>>> meaning "iommu=on,...". >>>>> >>>>> It won't. *s will be ',' in this case. >>>> >>>> Right, but as said - much depends on what you mean to do about >>>> the problem in the earlier patch. >>> >>> So I just hit this. And looking more thoroughly into it: today it in >>> fact has exactly this meaning. iommu_enable is "1" per default. So >>> specifying "iommu=,..." won't change this and has the same semantics >>> as "iommu=on,...". >> >> But that's not the interesting case. Are you saying that >> "iommu=off iommu=,..." enables the IOMMU today? It doesn't >> look to me as if it would. > > I guess this is a weird case after all: trying to be compatible to > "iommu=off iommu=,..." while "iommu=off iommu=on,..." doesn't work > correctly today is questionable. Yeah, it shouldn't be "compatibility", but "sane behavior". > OTOH the easiest way to avoid all discussions might be to drop the > parse_bool() modification to return "true" in case of an empty string > and handle this case by not calling parse_bool() at all from > _cmdline_parse() if no argument was given. Good idea. Jan
diff --git a/xen/common/kernel.c b/xen/common/kernel.c index ce7cb8adb5..c629ffa11c 100644 --- a/xen/common/kernel.c +++ b/xen/common/kernel.c @@ -23,34 +23,43 @@ 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) { switch ( param->len ) { case sizeof(uint8_t): - *(uint8_t *)param->var = val; + if ( val > UINT8_MAX && val < (uint64_t)INT8_MIN ) + return -EOVERFLOW; + *(uint8_t *)param->par.var = val; break; case sizeof(uint16_t): - *(uint16_t *)param->var = val; + if ( val > UINT16_MAX && val < (uint64_t)INT16_MIN ) + return -EOVERFLOW; + *(uint16_t *)param->par.var = val; break; case sizeof(uint32_t): - *(uint32_t *)param->var = val; + if ( val > UINT32_MAX && val < (uint64_t)INT32_MIN ) + return -EOVERFLOW; + *(uint32_t *)param->par.var = val; break; case sizeof(uint64_t): - *(uint64_t *)param->var = val; + *(uint64_t *)param->par.var = val; break; default: BUG(); } + + return 0; } static void __init _cmdline_parse(const char *cmdline) { char opt[128], *optval, *optkey, *q; - const char *p = cmdline; + const char *p = cmdline, *s, *key; const struct kernel_param *param; - int bool_assert; + int bool_assert, rctmp, rc; + bool found; for ( ; ; ) { @@ -84,10 +93,13 @@ static void __init _cmdline_parse(const char *cmdline) } /* Boolean parameters can be inverted with 'no-' prefix. */ + key = optkey; bool_assert = !!strncmp("no-", optkey, 3); if ( !bool_assert ) optkey += 3; + rc = 0; + found = false; for ( param = __setup_start; param < __setup_end; param++ ) { if ( strcmp(param->name, optkey) ) @@ -96,34 +108,48 @@ static void __init _cmdline_parse(const char *cmdline) strlen(param->name) == q + 1 - opt && !strncmp(param->name, opt, q + 1 - opt) ) { + found = true; optval[-1] = '='; - ((void (*)(const char *))param->var)(q); + rctmp = param->par.func(q); optval[-1] = '\0'; + if ( !rc ) + rc = rctmp; } continue; } + rctmp = 0; + found = true; switch ( param->type ) { case OPT_STR: - strlcpy(param->var, optval, param->len); + strlcpy(param->par.var, optval, param->len); break; case OPT_UINT: - assign_integer_param( + rctmp = assign_integer_param( param, - simple_strtoll(optval, NULL, 0)); + simple_strtoll(optval, &s, 0)); + if ( *s ) + rctmp = -EINVAL; break; case OPT_BOOL: - if ( !parse_bool(optval) ) + rctmp = parse_bool(optval); + if ( rctmp < 0 ) + break; + if ( !rctmp ) bool_assert = !bool_assert; + rctmp = 0; assign_integer_param(param, bool_assert); break; case OPT_SIZE: - assign_integer_param( + rctmp = assign_integer_param( param, - parse_size_and_unit(optval, NULL)); + parse_size_and_unit(optval, &s)); + if ( *s ) + rctmp = -EINVAL; break; case OPT_CUSTOM: + rctmp = -EINVAL; if ( !bool_assert ) { if ( *optval ) @@ -131,13 +157,21 @@ static void __init _cmdline_parse(const char *cmdline) safe_strcpy(opt, "no"); optval = opt; } - ((void (*)(const char *))param->var)(optval); + rctmp = param->par.func(optval); break; default: BUG(); break; } + + if ( !rc ) + rc = rctmp; } + + if ( rc ) + printk("parameter \"%s\" has invalid value \"%s\"!\n", key, optval); + if ( !found ) + printk("parameter \"%s\" unknown!\n", key); } } @@ -176,7 +210,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; diff --git a/xen/include/xen/init.h b/xen/include/xen/init.h index 25d2eef8dd..234ec25aae 100644 --- a/xen/include/xen/init.h +++ b/xen/include/xen/init.h @@ -83,7 +83,10 @@ struct kernel_param { OPT_CUSTOM } type; unsigned int len; - void *var; + union { + void *var; + int (*func)(const char *); + } par; }; extern const struct kernel_param __setup_start[], __setup_end[]; @@ -95,23 +98,38 @@ extern const struct kernel_param __setup_start[], __setup_end[]; #define custom_param(_name, _var) \ __setup_str __setup_str_##_var[] = _name; \ - __kparam __setup_##_var = { __setup_str_##_var, OPT_CUSTOM, 0, _var } + __kparam __setup_##_var = \ + { .name = __setup_str_##_var, \ + .type = OPT_CUSTOM, \ + .par.func = _var } #define boolean_param(_name, _var) \ __setup_str __setup_str_##_var[] = _name; \ __kparam __setup_##_var = \ - { __setup_str_##_var, OPT_BOOL, sizeof(_var), &_var } + { .name = __setup_str_##_var, \ + .type = OPT_BOOL, \ + .len = sizeof(_var), \ + .par.var = &_var } #define integer_param(_name, _var) \ __setup_str __setup_str_##_var[] = _name; \ __kparam __setup_##_var = \ - { __setup_str_##_var, OPT_UINT, sizeof(_var), &_var } + { .name = __setup_str_##_var, \ + .type = OPT_UINT, \ + .len = sizeof(_var), \ + .par.var = &_var } #define size_param(_name, _var) \ __setup_str __setup_str_##_var[] = _name; \ __kparam __setup_##_var = \ - { __setup_str_##_var, OPT_SIZE, sizeof(_var), &_var } + { .name = __setup_str_##_var, \ + .type = OPT_SIZE, \ + .len = sizeof(_var), \ + .par.var = &_var } #define string_param(_name, _var) \ __setup_str __setup_str_##_var[] = _name; \ __kparam __setup_##_var = \ - { __setup_str_##_var, OPT_STR, sizeof(_var), &_var } + { .name = __setup_str_##_var, \ + .type = OPT_STR, \ + .len = sizeof(_var), \ + .par.var = &_var } #endif /* __ASSEMBLY__ */ diff --git a/xen/include/xen/types.h b/xen/include/xen/types.h index b1dbb8720a..03f0fe612e 100644 --- a/xen/include/xen/types.h +++ b/xen/include/xen/types.h @@ -14,12 +14,15 @@ #define NULL ((void*)0) #endif +#define INT8_MIN (-127-1) #define INT16_MIN (-32767-1) #define INT32_MIN (-2147483647-1) +#define INT8_MAX (127) #define INT16_MAX (32767) #define INT32_MAX (2147483647) +#define UINT8_MAX (255) #define UINT16_MAX (65535) #define UINT32_MAX (4294967295U)
Where possible check validity of parameters in _cmdline_parse() and issue a warning message in case of an error detected. In order to make sure a custom parameter parsing function really returns a value (error or success), don't use a void pointer for storing the function address, but a proper typed function pointer. Cc: Andrew Cooper <andrew.cooper3@citrix.com> Cc: George Dunlap <George.Dunlap@eu.citrix.com> Cc: Ian Jackson <ian.jackson@eu.citrix.com> Cc: Jan Beulich <jbeulich@suse.com> Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> Cc: Stefano Stabellini <sstabellini@kernel.org> Cc: Tim Deegan <tim@xen.org> Cc: Wei Liu <wei.liu2@citrix.com> Signed-off-by: Juergen Gross <jgross@suse.com> --- V2: - replaced literal 8 by BITS_PER_BYTE (Wei Liu) - added test for empty string to parse_bool() V3: - use function pointer in struct kernel_param (Jan Beulich) - better range check in assign_integer_param() (Jan Beulich) - dont assign int values in case of overflow (Jan Beulich) - allow multiple handlers for a parameter (Jan Beulich) --- xen/common/kernel.c | 67 +++++++++++++++++++++++++++++++++++++------------ xen/include/xen/init.h | 30 +++++++++++++++++----- xen/include/xen/types.h | 3 +++ 3 files changed, 78 insertions(+), 22 deletions(-)