| Message ID | 20190605190053.19177-1-ezequiel@collabora.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
| Series | [1/2] PM / devfreq: Fix governor module load failure | expand |
Hi Ezequiel, Missatge de Ezequiel Garcia <ezequiel@collabora.com> del dia dc., 5 de juny 2019 a les 21:06: > > A bit unexpectedly (but still documented), request_module may > return a positive value, in case of a modprobe error. > This is currently causing issues in the devfreq framework. > > When a request_module exits with a positive value, we currently > return that via ERR_PTR. However, because the value is positive, > it's not a ERR_VALUE proper, and is therefore treated as a > valid struct devfreq_governor pointer, leading to a kernel oops. > > The right way to fix this is hinted in __request_module documentation: > > """ > [snip] The function returns > zero on success or a negative errno code or positive exit code from > "modprobe" on failure. Note that a successful module load does not mean > the module did not then unload and exit on an error of its own. Callers > must check that the service they requested is now available not blindly > invoke it. > """ > > Therefore, drop the return value check, which is not useful, and instead > just re-try to find the (hopefully now loaded) governor. > > Fixes: 23c7b54ca1cd1 ("PM / devfreq: Fix devfreq_add_device() when drivers are built as modules.") I think that what you really fixed is a bug introduced by: b53b0128052ff ("PM / devfreq: Fix static checker warning in try_then_request_governor") not the above commit. > Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com> > --- > drivers/devfreq/devfreq.c | 8 ++------ > 1 file changed, 2 insertions(+), 6 deletions(-) > > diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c > index 6b6991f0e873..8868ad9472d2 100644 > --- a/drivers/devfreq/devfreq.c > +++ b/drivers/devfreq/devfreq.c > @@ -236,7 +236,6 @@ static struct devfreq_governor *find_devfreq_governor(const char *name) > static struct devfreq_governor *try_then_request_governor(const char *name) > { > struct devfreq_governor *governor; > - int err = 0; > > if (IS_ERR_OR_NULL(name)) { > pr_err("DEVFREQ: %s: Invalid parameters\n", __func__); > @@ -251,13 +250,10 @@ static struct devfreq_governor *try_then_request_governor(const char *name) > > if (!strncmp(name, DEVFREQ_GOV_SIMPLE_ONDEMAND, > DEVFREQ_NAME_LEN)) > - err = request_module("governor_%s", "simpleondemand"); > + request_module("governor_%s", "simpleondemand"); > else > - err = request_module("governor_%s", name); > - /* Restore previous state before return */ > + request_module("governor_%s", name); > mutex_lock(&devfreq_list_lock); > - if (err) If you remove this check you'll iterate always over the full devfreq list of governors, I know should be quick and is not too long but ... > - return ERR_PTR(err); The fix can be simply: return ERR_PTR(-EINVAL); I don't think overlap the real error is a problem here. Thanks, Enric > > governor = find_devfreq_governor(name); > } > -- > 2.20.1 >
On Wed, 2019-06-05 at 23:46 +0200, Enric Balletbo Serra wrote: > Hi Ezequiel, > > Missatge de Ezequiel Garcia <ezequiel@collabora.com> del dia dc., 5 de > juny 2019 a les 21:06: > > A bit unexpectedly (but still documented), request_module may > > return a positive value, in case of a modprobe error. > > This is currently causing issues in the devfreq framework. > > > > When a request_module exits with a positive value, we currently > > return that via ERR_PTR. However, because the value is positive, > > it's not a ERR_VALUE proper, and is therefore treated as a > > valid struct devfreq_governor pointer, leading to a kernel oops. > > > > The right way to fix this is hinted in __request_module documentation: > > > > """ > > [snip] The function returns > > zero on success or a negative errno code or positive exit code from > > "modprobe" on failure. Note that a successful module load does not mean > > the module did not then unload and exit on an error of its own. Callers > > must check that the service they requested is now available not blindly > > invoke it. > > """ > > > > Therefore, drop the return value check, which is not useful, and instead > > just re-try to find the (hopefully now loaded) governor. > > > > Fixes: 23c7b54ca1cd1 ("PM / devfreq: Fix devfreq_add_device() when drivers are built as modules.") > > I think that what you really fixed is a bug introduced by: > > b53b0128052ff ("PM / devfreq: Fix static checker warning in > try_then_request_governor") > > not the above commit. > Oh, you are right of course. I looked for the commit introducing the request_module usage, and thought it was the culprit. > > Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com> > > --- > > drivers/devfreq/devfreq.c | 8 ++------ > > 1 file changed, 2 insertions(+), 6 deletions(-) > > > > diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c > > index 6b6991f0e873..8868ad9472d2 100644 > > --- a/drivers/devfreq/devfreq.c > > +++ b/drivers/devfreq/devfreq.c > > @@ -236,7 +236,6 @@ static struct devfreq_governor *find_devfreq_governor(const char *name) > > static struct devfreq_governor *try_then_request_governor(const char *name) > > { > > struct devfreq_governor *governor; > > - int err = 0; > > > > if (IS_ERR_OR_NULL(name)) { > > pr_err("DEVFREQ: %s: Invalid parameters\n", __func__); > > @@ -251,13 +250,10 @@ static struct devfreq_governor *try_then_request_governor(const char *name) > > > > if (!strncmp(name, DEVFREQ_GOV_SIMPLE_ONDEMAND, > > DEVFREQ_NAME_LEN)) > > - err = request_module("governor_%s", "simpleondemand"); > > + request_module("governor_%s", "simpleondemand"); > > else > > - err = request_module("governor_%s", name); > > - /* Restore previous state before return */ > > + request_module("governor_%s", name); > > mutex_lock(&devfreq_list_lock); > > - if (err) > > If you remove this check you'll iterate always over the full devfreq > list of governors, I know should be quick and is not too long but ... > Keep in mind that when the request_module succeeds, we need to iterate anyways to find the governor. > > - return ERR_PTR(err); > > The fix can be simply: > > return ERR_PTR(-EINVAL); > > I don't think overlap the real error is a problem here. > Yeah, I also thought about this, but somehow thought this was simpler. I don't have a strong opinion, so whatever you prefer is fine. Thanks, Eze > Thanks, > Enric > > > governor = find_devfreq_governor(name); > > } > > -- > > 2.20.1 > >
On 6/6/19 15:42, Ezequiel Garcia wrote: > On Wed, 2019-06-05 at 23:46 +0200, Enric Balletbo Serra wrote: >> Hi Ezequiel, >> >> Missatge de Ezequiel Garcia <ezequiel@collabora.com> del dia dc., 5 de >> juny 2019 a les 21:06: >>> A bit unexpectedly (but still documented), request_module may >>> return a positive value, in case of a modprobe error. >>> This is currently causing issues in the devfreq framework. >>> >>> When a request_module exits with a positive value, we currently >>> return that via ERR_PTR. However, because the value is positive, >>> it's not a ERR_VALUE proper, and is therefore treated as a >>> valid struct devfreq_governor pointer, leading to a kernel oops. >>> >>> The right way to fix this is hinted in __request_module documentation: >>> >>> """ >>> [snip] The function returns >>> zero on success or a negative errno code or positive exit code from >>> "modprobe" on failure. Note that a successful module load does not mean >>> the module did not then unload and exit on an error of its own. Callers >>> must check that the service they requested is now available not blindly >>> invoke it. >>> """ >>> >>> Therefore, drop the return value check, which is not useful, and instead >>> just re-try to find the (hopefully now loaded) governor. >>> >>> Fixes: 23c7b54ca1cd1 ("PM / devfreq: Fix devfreq_add_device() when drivers are built as modules.") >> >> I think that what you really fixed is a bug introduced by: >> >> b53b0128052ff ("PM / devfreq: Fix static checker warning in >> try_then_request_governor") >> >> not the above commit. >> > > Oh, you are right of course. I looked for the commit introducing the > request_module usage, and thought it was the culprit. > >>> Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com> >>> --- >>> drivers/devfreq/devfreq.c | 8 ++------ >>> 1 file changed, 2 insertions(+), 6 deletions(-) >>> >>> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c >>> index 6b6991f0e873..8868ad9472d2 100644 >>> --- a/drivers/devfreq/devfreq.c >>> +++ b/drivers/devfreq/devfreq.c >>> @@ -236,7 +236,6 @@ static struct devfreq_governor *find_devfreq_governor(const char *name) >>> static struct devfreq_governor *try_then_request_governor(const char *name) >>> { >>> struct devfreq_governor *governor; >>> - int err = 0; >>> >>> if (IS_ERR_OR_NULL(name)) { >>> pr_err("DEVFREQ: %s: Invalid parameters\n", __func__); >>> @@ -251,13 +250,10 @@ static struct devfreq_governor *try_then_request_governor(const char *name) >>> >>> if (!strncmp(name, DEVFREQ_GOV_SIMPLE_ONDEMAND, >>> DEVFREQ_NAME_LEN)) >>> - err = request_module("governor_%s", "simpleondemand"); >>> + request_module("governor_%s", "simpleondemand"); >>> else >>> - err = request_module("governor_%s", name); >>> - /* Restore previous state before return */ >>> + request_module("governor_%s", name); >>> mutex_lock(&devfreq_list_lock); >>> - if (err) >> >> If you remove this check you'll iterate always over the full devfreq >> list of governors, I know should be quick and is not too long but ... >> > > Keep in mind that when the request_module succeeds, we need > to iterate anyways to find the governor. > Well, the error path will be a micro-bit faster :-) >>> - return ERR_PTR(err); >> >> The fix can be simply: >> >> return ERR_PTR(-EINVAL); >> >> I don't think overlap the real error is a problem here. >> > > Yeah, I also thought about this, but somehow thought this > was simpler. > > I don't have a strong opinion, so whatever you prefer is fine. > Me neither, I think is more up to the maintainer :-) BTW, with the Fixes tag fixed Reviewed-by: Enric Balletbo i Serra <enric.balletbo@collabora.com> > Thanks, > Eze > >> Thanks, >> Enric >> >>> governor = find_devfreq_governor(name); >>> } >>> -- >>> 2.20.1 >>> > >
Hi, On 19. 6. 6. 오전 4:00, Ezequiel Garcia wrote: > A bit unexpectedly (but still documented), request_module may > return a positive value, in case of a modprobe error. > This is currently causing issues in the devfreq framework. > > When a request_module exits with a positive value, we currently > return that via ERR_PTR. However, because the value is positive, > it's not a ERR_VALUE proper, and is therefore treated as a > valid struct devfreq_governor pointer, leading to a kernel oops. > > The right way to fix this is hinted in __request_module documentation: > > """ > [snip] The function returns > zero on success or a negative errno code or positive exit code from > "modprobe" on failure. Note that a successful module load does not mean > the module did not then unload and exit on an error of its own. Callers > must check that the service they requested is now available not blindly > invoke it. > """ > > Therefore, drop the return value check, which is not useful, and instead > just re-try to find the (hopefully now loaded) governor. > > Fixes: 23c7b54ca1cd1 ("PM / devfreq: Fix devfreq_add_device() when drivers are built as modules.") > Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com> > --- > drivers/devfreq/devfreq.c | 8 ++------ > 1 file changed, 2 insertions(+), 6 deletions(-) > > diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c > index 6b6991f0e873..8868ad9472d2 100644 > --- a/drivers/devfreq/devfreq.c > +++ b/drivers/devfreq/devfreq.c > @@ -236,7 +236,6 @@ static struct devfreq_governor *find_devfreq_governor(const char *name) > static struct devfreq_governor *try_then_request_governor(const char *name) > { > struct devfreq_governor *governor; > - int err = 0; > > if (IS_ERR_OR_NULL(name)) { > pr_err("DEVFREQ: %s: Invalid parameters\n", __func__); > @@ -251,13 +250,10 @@ static struct devfreq_governor *try_then_request_governor(const char *name) > > if (!strncmp(name, DEVFREQ_GOV_SIMPLE_ONDEMAND, > DEVFREQ_NAME_LEN)) > - err = request_module("governor_%s", "simpleondemand"); > + request_module("governor_%s", "simpleondemand"); I don't agree to remove the exception handling. Even if request_module() returns positive value, it have to be handled the negative code for error case. > else > - err = request_module("governor_%s", name); > - /* Restore previous state before return */ > + request_module("governor_%s", name); ditto. > mutex_lock(&devfreq_list_lock); > - if (err)> - return ERR_PTR(err); You better to modify it as following: if (err < 0) return ERR_PTR(err); else if (err > 0) return ERR_PTR(-EINVAL); > > governor = find_devfreq_governor(name); > } >
On 19. 6. 20. 오후 4:59, Chanwoo Choi wrote: > Hi, > > On 19. 6. 6. 오전 4:00, Ezequiel Garcia wrote: >> A bit unexpectedly (but still documented), request_module may >> return a positive value, in case of a modprobe error. >> This is currently causing issues in the devfreq framework. >> >> When a request_module exits with a positive value, we currently >> return that via ERR_PTR. However, because the value is positive, >> it's not a ERR_VALUE proper, and is therefore treated as a >> valid struct devfreq_governor pointer, leading to a kernel oops. >> >> The right way to fix this is hinted in __request_module documentation: >> >> """ >> [snip] The function returns >> zero on success or a negative errno code or positive exit code from >> "modprobe" on failure. Note that a successful module load does not mean >> the module did not then unload and exit on an error of its own. Callers >> must check that the service they requested is now available not blindly >> invoke it. >> """ >> >> Therefore, drop the return value check, which is not useful, and instead >> just re-try to find the (hopefully now loaded) governor. >> >> Fixes: 23c7b54ca1cd1 ("PM / devfreq: Fix devfreq_add_device() when drivers are built as modules.") >> Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com> >> --- >> drivers/devfreq/devfreq.c | 8 ++------ >> 1 file changed, 2 insertions(+), 6 deletions(-) >> >> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c >> index 6b6991f0e873..8868ad9472d2 100644 >> --- a/drivers/devfreq/devfreq.c >> +++ b/drivers/devfreq/devfreq.c >> @@ -236,7 +236,6 @@ static struct devfreq_governor *find_devfreq_governor(const char *name) >> static struct devfreq_governor *try_then_request_governor(const char *name) >> { >> struct devfreq_governor *governor; >> - int err = 0; >> >> if (IS_ERR_OR_NULL(name)) { >> pr_err("DEVFREQ: %s: Invalid parameters\n", __func__); >> @@ -251,13 +250,10 @@ static struct devfreq_governor *try_then_request_governor(const char *name) >> >> if (!strncmp(name, DEVFREQ_GOV_SIMPLE_ONDEMAND, >> DEVFREQ_NAME_LEN)) >> - err = request_module("governor_%s", "simpleondemand"); >> + request_module("governor_%s", "simpleondemand"); > > I don't agree to remove the exception handling. Even if request_module() > returns positive value, Sorry, I wrote the wrong comment. It have to handle the positive return value for exception handling. > >> else >> - err = request_module("governor_%s", name); >> - /* Restore previous state before return */ >> + request_module("governor_%s", name); > > ditto. > >> mutex_lock(&devfreq_list_lock); >> - if (err)> - return ERR_PTR(err); > > You better to modify it as following: > > if (err < 0) > return ERR_PTR(err); > else if (err > 0) > return ERR_PTR(-EINVAL); > > >> >> governor = find_devfreq_governor(name); >> } >> > >
On Thu, 2019-06-20 at 17:04 +0900, Chanwoo Choi wrote: > On 19. 6. 20. 오후 4:59, Chanwoo Choi wrote: > > Hi, > > > > On 19. 6. 6. 오전 4:00, Ezequiel Garcia wrote: > > > A bit unexpectedly (but still documented), request_module may > > > return a positive value, in case of a modprobe error. > > > This is currently causing issues in the devfreq framework. > > > > > > When a request_module exits with a positive value, we currently > > > return that via ERR_PTR. However, because the value is positive, > > > it's not a ERR_VALUE proper, and is therefore treated as a > > > valid struct devfreq_governor pointer, leading to a kernel oops. > > > > > > The right way to fix this is hinted in __request_module documentation: > > > > > > """ > > > [snip] The function returns > > > zero on success or a negative errno code or positive exit code from > > > "modprobe" on failure. Note that a successful module load does not mean > > > the module did not then unload and exit on an error of its own. Callers > > > must check that the service they requested is now available not blindly > > > invoke it. > > > """ > > > > > > Therefore, drop the return value check, which is not useful, and instead > > > just re-try to find the (hopefully now loaded) governor. > > > > > > Fixes: 23c7b54ca1cd1 ("PM / devfreq: Fix devfreq_add_device() when drivers are built as modules.") > > > Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com> > > > --- > > > drivers/devfreq/devfreq.c | 8 ++------ > > > 1 file changed, 2 insertions(+), 6 deletions(-) > > > > > > diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c > > > index 6b6991f0e873..8868ad9472d2 100644 > > > --- a/drivers/devfreq/devfreq.c > > > +++ b/drivers/devfreq/devfreq.c > > > @@ -236,7 +236,6 @@ static struct devfreq_governor *find_devfreq_governor(const char *name) > > > static struct devfreq_governor *try_then_request_governor(const char *name) > > > { > > > struct devfreq_governor *governor; > > > - int err = 0; > > > > > > if (IS_ERR_OR_NULL(name)) { > > > pr_err("DEVFREQ: %s: Invalid parameters\n", __func__); > > > @@ -251,13 +250,10 @@ static struct devfreq_governor *try_then_request_governor(const char *name) > > > > > > if (!strncmp(name, DEVFREQ_GOV_SIMPLE_ONDEMAND, > > > DEVFREQ_NAME_LEN)) > > > - err = request_module("governor_%s", "simpleondemand"); > > > + request_module("governor_%s", "simpleondemand"); > > > > I don't agree to remove the exception handling. Even if request_module() > > returns positive value, > > Sorry, I wrote the wrong comment. It have to handle the positive return value > for exception handling. > OK, let me give this a new try. Thanks, Ezequiel
diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c index 6b6991f0e873..8868ad9472d2 100644 --- a/drivers/devfreq/devfreq.c +++ b/drivers/devfreq/devfreq.c @@ -236,7 +236,6 @@ static struct devfreq_governor *find_devfreq_governor(const char *name) static struct devfreq_governor *try_then_request_governor(const char *name) { struct devfreq_governor *governor; - int err = 0; if (IS_ERR_OR_NULL(name)) { pr_err("DEVFREQ: %s: Invalid parameters\n", __func__); @@ -251,13 +250,10 @@ static struct devfreq_governor *try_then_request_governor(const char *name) if (!strncmp(name, DEVFREQ_GOV_SIMPLE_ONDEMAND, DEVFREQ_NAME_LEN)) - err = request_module("governor_%s", "simpleondemand"); + request_module("governor_%s", "simpleondemand"); else - err = request_module("governor_%s", name); - /* Restore previous state before return */ + request_module("governor_%s", name); mutex_lock(&devfreq_list_lock); - if (err) - return ERR_PTR(err); governor = find_devfreq_governor(name); }
A bit unexpectedly (but still documented), request_module may return a positive value, in case of a modprobe error. This is currently causing issues in the devfreq framework. When a request_module exits with a positive value, we currently return that via ERR_PTR. However, because the value is positive, it's not a ERR_VALUE proper, and is therefore treated as a valid struct devfreq_governor pointer, leading to a kernel oops. The right way to fix this is hinted in __request_module documentation: """ [snip] The function returns zero on success or a negative errno code or positive exit code from "modprobe" on failure. Note that a successful module load does not mean the module did not then unload and exit on an error of its own. Callers must check that the service they requested is now available not blindly invoke it. """ Therefore, drop the return value check, which is not useful, and instead just re-try to find the (hopefully now loaded) governor. Fixes: 23c7b54ca1cd1 ("PM / devfreq: Fix devfreq_add_device() when drivers are built as modules.") Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com> --- drivers/devfreq/devfreq.c | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-)