On Thursday, July 02, 2020 09:55 CEST, David Laight wrote: > Hmm... sscanf() is also horrid. > Surprisingly difficult to use correctly. > > It is usually best to use strchr() (and maybe str[c]scn()) > to parse strings. > For numbers use whatever the kernels current 'favourite' implementation > of strtoul() is called.
I thought that using sscanf would clean up the code a bit compared to several haphazard calls, but I can see your point about sscanf being difficult to use correctly. The kernel functions kstrtou16 seem to expect a null-terminated string as argument. Since there are no null-bytes after the numbers we want to parse, it becomes necessary to copy at least part of the strings to a buffer. If we're copying strings to buffers anyway, I think the simplest solution would be to just kstrdup the entire parameter and not touch the rest of the string parsing code. This has the disadvantage of having an extra memory allocation to keep track of. Since the parameter is currently restricted to 128 characters at most, it may alternatively be possible to copy the parameter to a 128-byte buffer on the stack. This has the advantage of having to keep track of one less memory allocation, but the disadvantage of using 128 bytes more stack space; I'm not sure whether that's acceptable. Here's a sample patch involving kstrdup: --- drivers/usb/core/quirks.c | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-) diff --git a/drivers/usb/core/quirks.c b/drivers/usb/core/quirks.c index e0b77674869c..3b64b0be2563 100644 --- a/drivers/usb/core/quirks.c +++ b/drivers/usb/core/quirks.c @@ -25,17 +25,23 @@ static unsigned int quirk_count; static char quirks_param[128]; -static int quirks_param_set(const char *val, const struct kernel_param *kp) +static int quirks_param_set(const char *value, const struct kernel_param *kp) { - char *p, *field; + char *val, *p, *field; u16 vid, pid; u32 flags; size_t i; int err; + val = kstrdup(value, GFP_KERNEL); + if (!val) + return -ENOMEM; + err = param_set_copystring(val, kp); - if (err) + if (err) { + kfree(val); return err; + } mutex_lock(&quirk_mutex); @@ -60,6 +66,7 @@ static int quirks_param_set(const char *val, const struct kernel_param *kp) if (!quirk_list) { quirk_count = 0; mutex_unlock(&quirk_mutex); + kfree(val); return -ENOMEM; } @@ -144,6 +151,7 @@ static int quirks_param_set(const char *val, const struct kernel_param *kp) unlock: mutex_unlock(&quirk_mutex); + kfree(val); return 0; } -- 2.27.0