On Monday, July 06, 2020 12:34 CEST, Greg Kroah-Hartman wrote: > That's a lot of stack space, is it really needed? Can we just use a > static variable instead, or dynamically allocate this? It is very possible to statically or dynamically allocate this. Statically reserving an additional 128 bytes regardless of whether this feature is actually used feels a bit wasteful, so I'd prefer stack or dynamic allocation. An earlier draft of my patch did dynamically allocate this memory; early discussion (https://lkml.org/lkml/2020/7/3/248) suggested that dynamic allocation has the disadvantage of introducing a new obscure error condition: On Friday, July 03, 2020 10:13 CEST, David Laight wrote: > The problem with strdup() is you get the extra (unlikely) failure path. > 128 bytes of stack won't be a problem if the function is (essentially) > a leaf. So I rewrote my patch to use stack-based allocation instead. I've attached a patch using kstrdup at the end of this mail. I think that the new obscure error condition caused by kstrdup() isn't too bad since original code already had a call to kcalloc() anyway; the possibility for the function to fail due to out-of-memory errors already existed. In this version of the patch, there may however be a possible issue that different code paths are taken depending on when the out-of-memory error occurs, resulting in different side effects: val = kstrdup(value, GFP_KERNEL); if (!val) return -ENOMEM; err = param_set_copystring(val, kp); [...] if (quirk_list) { kfree(quirk_list); quirk_list = NULL; } quirk_list = kcalloc(quirk_count, sizeof(struct quirk_entry), GFP_KERNEL); If the OOM happens at the kstrdup() step (which I added), then the old value of the parameter will be retained. If the OOM happens at the kcalloc() step, then the kernel will remember the value of the new parameter (and return that value if asked what the parameter's current value is), but but neither the old nor the new parameter will be in effect: the quirk_list will be a NULL pointer and the quirks module will behave as if the usbcore.quirks parameter is empty. I could move my code to make an OOM at kstrdup() have the same side effects as an OOM at kcalloc(), but I'd personally think that the first kind of behaviour "setting the parameter failed, nothing happened" is more correct than the latter kind "setting the parameter failed, the parameter is now in limbo". --- drivers/usb/core/quirks.c | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-) diff --git a/drivers/usb/core/quirks.c b/drivers/usb/core/quirks.c index e0b77674869c..c96c50faccf7 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,10 +66,11 @@ 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; } - for (i = 0, p = (char *)val; p && *p;) { + for (i = 0, p = val; p && *p;) { /* Each entry consists of VID:PID:flags */ field = strsep(&p, ":"); if (!field) @@ -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
