| Message ID | 96f459015e6418cee4fa20fdbdb80c4caf417c19.1569256298.git.leonard.crestez@nxp.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | PM / devfreq: Check NULL governor in available_governors_show | expand |
On Mon, Sep 23, 2019 at 07:34:43PM +0300, Leonard Crestez wrote: > The governor is initialized after sysfs attributes become visible so in > theory the governor field can be NULL here. > > Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com> > --- > drivers/devfreq/devfreq.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > Found this by hacking device core to call attribute "show" functions > from inside device_add. > > diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c > index 00fc23fea5b2..896fb2312f2f 100644 > --- a/drivers/devfreq/devfreq.c > +++ b/drivers/devfreq/devfreq.c > @@ -1322,11 +1322,11 @@ static ssize_t available_governors_show(struct device *d, > > /* > * The devfreq with immutable governor (e.g., passive) shows > * only own governor. > */ > - if (df->governor->immutable) { > + if (df->governor && df->governor->immutable) { > count = scnprintf(&buf[count], DEVFREQ_NAME_LEN, > "%s ", df->governor_name); > /* > * The devfreq device shows the registered governor except for > * immutable governors such as passive governor . Reviewed-by: Matthias Kaehlcke <mka@chromium.org>
Hi, On 19. 9. 24. 오전 1:34, Leonard Crestez wrote: > The governor is initialized after sysfs attributes become visible so in > theory the governor field can be NULL here. > > Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com> > --- > drivers/devfreq/devfreq.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > Found this by hacking device core to call attribute "show" functions > from inside device_add. > > diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c > index 00fc23fea5b2..896fb2312f2f 100644 > --- a/drivers/devfreq/devfreq.c > +++ b/drivers/devfreq/devfreq.c > @@ -1322,11 +1322,11 @@ static ssize_t available_governors_show(struct device *d, > > /* > * The devfreq with immutable governor (e.g., passive) shows > * only own governor. > */ > - if (df->governor->immutable) { > + if (df->governor && df->governor->immutable) { > count = scnprintf(&buf[count], DEVFREQ_NAME_LEN, > "%s ", df->governor_name); > /* > * The devfreq device shows the registered governor except for > * immutable governors such as passive governor . > As you mentioned, it create sysfs and then initialize the governor instance as following: device_register() device_add() device_add_attrs() creating sysfs entries. governor = try_then_request_governor(...) Thanks for fix-up. Reviewed-by: Chanwoo Choi <cw00.choi@samsung.com> Additionally, you have to add the following 'fixes' tag and then send it to stable mailing list(stable@vger.kernel.org). - Fixes: bcf23c79c4e46 ("PM / devfreq: Fix available_governor sysfs")
On 2019-09-24 4:48 AM, Chanwoo Choi wrote: > On 19. 9. 24. 오전 1:34, Leonard Crestez wrote: >> The governor is initialized after sysfs attributes become visible so in >> theory the governor field can be NULL here. >> >> Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com> >> --- >> drivers/devfreq/devfreq.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> Found this by hacking device core to call attribute "show" functions >> from inside device_add. >> >> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c >> index 00fc23fea5b2..896fb2312f2f 100644 >> --- a/drivers/devfreq/devfreq.c >> +++ b/drivers/devfreq/devfreq.c >> @@ -1322,11 +1322,11 @@ static ssize_t available_governors_show(struct device *d, >> >> /* >> * The devfreq with immutable governor (e.g., passive) shows >> * only own governor. >> */ >> - if (df->governor->immutable) { >> + if (df->governor && df->governor->immutable) { >> count = scnprintf(&buf[count], DEVFREQ_NAME_LEN, >> "%s ", df->governor_name); >> /* >> * The devfreq device shows the registered governor except for >> * immutable governors such as passive governor . >> > > As you mentioned, it create sysfs and then initialize the governor instance > as following: > > device_register() > device_add() > device_add_attrs() > creating sysfs entries. > governor = try_then_request_governor(...) > > > Thanks for fix-up. > Reviewed-by: Chanwoo Choi <cw00.choi@samsung.com> > > Additionally, you have to add the following 'fixes' tag > and then send it to stable mailing list(stable@vger.kernel.org). > - Fixes: bcf23c79c4e46 ("PM / devfreq: Fix available_governor sysfs") OK, but I'm not sure this meets the criteria for inclusion into linux stable: * It must fix a real bug that bothers people (not a, "This could be a problem..." type thing). * No "theoretical race condition" issues, unless an explanation of how the race can be exploited is also provided. This patch fixes a theoretical race condition which has been there since the start. -- Regards, Leonard
On 19. 9. 24. 오후 4:17, Leonard Crestez wrote: > On 2019-09-24 4:48 AM, Chanwoo Choi wrote: >> On 19. 9. 24. 오전 1:34, Leonard Crestez wrote: >>> The governor is initialized after sysfs attributes become visible so in >>> theory the governor field can be NULL here. >>> >>> Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com> >>> --- >>> drivers/devfreq/devfreq.c | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> Found this by hacking device core to call attribute "show" functions >>> from inside device_add. >>> >>> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c >>> index 00fc23fea5b2..896fb2312f2f 100644 >>> --- a/drivers/devfreq/devfreq.c >>> +++ b/drivers/devfreq/devfreq.c >>> @@ -1322,11 +1322,11 @@ static ssize_t available_governors_show(struct device *d, >>> >>> /* >>> * The devfreq with immutable governor (e.g., passive) shows >>> * only own governor. >>> */ >>> - if (df->governor->immutable) { >>> + if (df->governor && df->governor->immutable) { >>> count = scnprintf(&buf[count], DEVFREQ_NAME_LEN, >>> "%s ", df->governor_name); >>> /* >>> * The devfreq device shows the registered governor except for >>> * immutable governors such as passive governor . >>> >> >> As you mentioned, it create sysfs and then initialize the governor instance >> as following: >> >> device_register() >> device_add() >> device_add_attrs() >> creating sysfs entries. >> governor = try_then_request_governor(...) >> >> >> Thanks for fix-up. >> Reviewed-by: Chanwoo Choi <cw00.choi@samsung.com> >> >> Additionally, you have to add the following 'fixes' tag >> and then send it to stable mailing list(stable@vger.kernel.org). >> - Fixes: bcf23c79c4e46 ("PM / devfreq: Fix available_governor sysfs") > > OK, but I'm not sure this meets the criteria for inclusion into linux > stable: > > * It must fix a real bug that bothers people (not a, "This could be a > problem..." type thing). > * No "theoretical race condition" issues, unless an explanation of how > the race can be exploited is also provided. OK. If you think that it is not necessary to send it to stable mailing list, don't send it. Thanks. > > This patch fixes a theoretical race condition which has been there since > the start. > > -- > Regards, > Leonard >
diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c index 00fc23fea5b2..896fb2312f2f 100644 --- a/drivers/devfreq/devfreq.c +++ b/drivers/devfreq/devfreq.c @@ -1322,11 +1322,11 @@ static ssize_t available_governors_show(struct device *d, /* * The devfreq with immutable governor (e.g., passive) shows * only own governor. */ - if (df->governor->immutable) { + if (df->governor && df->governor->immutable) { count = scnprintf(&buf[count], DEVFREQ_NAME_LEN, "%s ", df->governor_name); /* * The devfreq device shows the registered governor except for * immutable governors such as passive governor .
The governor is initialized after sysfs attributes become visible so in theory the governor field can be NULL here. Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com> --- drivers/devfreq/devfreq.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) Found this by hacking device core to call attribute "show" functions from inside device_add.