Fix buffer overflow in trans_stat_show().
Convert simple snprintf to the more secure scnprintf with size of
PAGE_SIZE.
Add condition checking if we are exceeding PAGE_SIZE and exit early from
loop. Also add at the end a warning that we exceeded PAGE_SIZE and that
stats is disabled.
Return -EFBIG in the case where we don't have enough space to write the
full transition table.
Also document in the ABI that this function can return -EFBIG error.
Cc: [email protected]
Closes: https://bugzilla.kernel.org/show_bug.cgi?id=218041
Fixes: e552bbaf5b98 ("PM / devfreq: Add sysfs node for representing frequency transition information.")
Signed-off-by: Christian Marangi <[email protected]>
---
Documentation/ABI/testing/sysfs-class-devfreq | 3 +
drivers/devfreq/devfreq.c | 57 +++++++++++++------
2 files changed, 42 insertions(+), 18 deletions(-)
diff --git a/Documentation/ABI/testing/sysfs-class-devfreq b/Documentation/ABI/testing/sysfs-class-devfreq
index 5e6b74f30406..1e7e0bb4c14e 100644
--- a/Documentation/ABI/testing/sysfs-class-devfreq
+++ b/Documentation/ABI/testing/sysfs-class-devfreq
@@ -52,6 +52,9 @@ Description:
echo 0 > /sys/class/devfreq/.../trans_stat
+ If the transition table is bigger than PAGE_SIZE, reading
+ this will return an -EFBIG error.
+
What: /sys/class/devfreq/.../available_frequencies
Date: October 2012
Contact: Nishanth Menon <[email protected]>
diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
index 63347a5ae599..8459512d9b07 100644
--- a/drivers/devfreq/devfreq.c
+++ b/drivers/devfreq/devfreq.c
@@ -1688,7 +1688,7 @@ static ssize_t trans_stat_show(struct device *dev,
struct device_attribute *attr, char *buf)
{
struct devfreq *df = to_devfreq(dev);
- ssize_t len;
+ ssize_t len = 0;
int i, j;
unsigned int max_state;
@@ -1697,7 +1697,7 @@ static ssize_t trans_stat_show(struct device *dev,
max_state = df->max_state;
if (max_state == 0)
- return sprintf(buf, "Not Supported.\n");
+ return scnprintf(buf, PAGE_SIZE, "Not Supported.\n");
mutex_lock(&df->lock);
if (!df->stop_polling &&
@@ -1707,31 +1707,52 @@ static ssize_t trans_stat_show(struct device *dev,
}
mutex_unlock(&df->lock);
- len = sprintf(buf, " From : To\n");
- len += sprintf(buf + len, " :");
- for (i = 0; i < max_state; i++)
- len += sprintf(buf + len, "%10lu",
- df->freq_table[i]);
+ len += scnprintf(buf + len, PAGE_SIZE - len, " From : To\n");
+ len += scnprintf(buf + len, PAGE_SIZE - len, " :");
+ for (i = 0; i < max_state; i++) {
+ if (len >= PAGE_SIZE - 1)
+ break;
+ len += scnprintf(buf + len, PAGE_SIZE - len, "%10lu",
+ df->freq_table[i]);
+ }
+ if (len >= PAGE_SIZE - 1)
+ return PAGE_SIZE - 1;
- len += sprintf(buf + len, " time(ms)\n");
+ len += scnprintf(buf + len, PAGE_SIZE - len, " time(ms)\n");
for (i = 0; i < max_state; i++) {
+ if (len >= PAGE_SIZE - 1)
+ break;
if (df->freq_table[i] == df->previous_freq)
- len += sprintf(buf + len, "*");
+ len += scnprintf(buf + len, PAGE_SIZE - len, "*");
else
- len += sprintf(buf + len, " ");
+ len += scnprintf(buf + len, PAGE_SIZE - len, " ");
+ if (len >= PAGE_SIZE - 1)
+ break;
+
+ len += scnprintf(buf + len, PAGE_SIZE - len, "%10lu:",
+ df->freq_table[i]);
+ for (j = 0; j < max_state; j++) {
+ if (len >= PAGE_SIZE - 1)
+ break;
+ len += scnprintf(buf + len, PAGE_SIZE - len, "%10u",
+ df->stats.trans_table[(i * max_state) + j]);
+ }
+ if (len >= PAGE_SIZE - 1)
+ break;
+ len += scnprintf(buf + len, PAGE_SIZE - len, "%10llu\n", (u64)
+ jiffies64_to_msecs(df->stats.time_in_state[i]));
+ }
- len += sprintf(buf + len, "%10lu:", df->freq_table[i]);
- for (j = 0; j < max_state; j++)
- len += sprintf(buf + len, "%10u",
- df->stats.trans_table[(i * max_state) + j]);
+ if (len < PAGE_SIZE - 1)
+ len += scnprintf(buf + len, PAGE_SIZE - len, "Total transition : %u\n",
+ df->stats.total_trans);
- len += sprintf(buf + len, "%10llu\n", (u64)
- jiffies64_to_msecs(df->stats.time_in_state[i]));
+ if (len >= PAGE_SIZE - 1) {
+ pr_warn_once("devfreq transition table exceeds PAGE_SIZE. Disabling\n");
+ return -EFBIG;
}
- len += sprintf(buf + len, "Total transition : %u\n",
- df->stats.total_trans);
return len;
}
--
2.43.0
Follow the advice of the Documentation/filesystems/sysfs.rst and show()
should only use sysfs_emit() or sysfs_emit_at() when formatting the
value to be returned to user space.
Signed-off-by: Christian Marangi <[email protected]>
---
drivers/devfreq/devfreq.c | 37 +++++++++++++++++--------------------
1 file changed, 17 insertions(+), 20 deletions(-)
diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
index 8459512d9b07..92f211774f9b 100644
--- a/drivers/devfreq/devfreq.c
+++ b/drivers/devfreq/devfreq.c
@@ -1697,7 +1697,7 @@ static ssize_t trans_stat_show(struct device *dev,
max_state = df->max_state;
if (max_state == 0)
- return scnprintf(buf, PAGE_SIZE, "Not Supported.\n");
+ return sysfs_emit(buf, "Not Supported.\n");
mutex_lock(&df->lock);
if (!df->stop_polling &&
@@ -1707,47 +1707,44 @@ static ssize_t trans_stat_show(struct device *dev,
}
mutex_unlock(&df->lock);
- len += scnprintf(buf + len, PAGE_SIZE - len, " From : To\n");
- len += scnprintf(buf + len, PAGE_SIZE - len, " :");
+ len += sysfs_emit_at(buf, len, " From : To\n");
+ len += sysfs_emit_at(buf, len, " :");
for (i = 0; i < max_state; i++) {
if (len >= PAGE_SIZE - 1)
break;
- len += scnprintf(buf + len, PAGE_SIZE - len, "%10lu",
- df->freq_table[i]);
+ len += sysfs_emit_at(buf, len, "%10lu",
+ df->freq_table[i]);
}
+
if (len >= PAGE_SIZE - 1)
return PAGE_SIZE - 1;
-
- len += scnprintf(buf + len, PAGE_SIZE - len, " time(ms)\n");
+ len += sysfs_emit_at(buf, len, " time(ms)\n");
for (i = 0; i < max_state; i++) {
if (len >= PAGE_SIZE - 1)
break;
- if (df->freq_table[i] == df->previous_freq)
- len += scnprintf(buf + len, PAGE_SIZE - len, "*");
+ if (df->freq_table[2] == df->previous_freq)
+ len += sysfs_emit_at(buf, len, "*");
else
- len += scnprintf(buf + len, PAGE_SIZE - len, " ");
+ len += sysfs_emit_at(buf, len, " ");
if (len >= PAGE_SIZE - 1)
break;
-
- len += scnprintf(buf + len, PAGE_SIZE - len, "%10lu:",
- df->freq_table[i]);
+ len += sysfs_emit_at(buf, len, "%10lu:", df->freq_table[i]);
for (j = 0; j < max_state; j++) {
if (len >= PAGE_SIZE - 1)
break;
- len += scnprintf(buf + len, PAGE_SIZE - len, "%10u",
- df->stats.trans_table[(i * max_state) + j]);
+ len += sysfs_emit_at(buf, len, "%10u",
+ df->stats.trans_table[(i * max_state) + j]);
}
if (len >= PAGE_SIZE - 1)
break;
- len += scnprintf(buf + len, PAGE_SIZE - len, "%10llu\n", (u64)
- jiffies64_to_msecs(df->stats.time_in_state[i]));
+ len += sysfs_emit_at(buf, len, "%10llu\n", (u64)
+ jiffies64_to_msecs(df->stats.time_in_state[i]));
}
if (len < PAGE_SIZE - 1)
- len += scnprintf(buf + len, PAGE_SIZE - len, "Total transition : %u\n",
- df->stats.total_trans);
-
+ len += sysfs_emit_at(buf, len, "Total transition : %u\n",
+ df->stats.total_trans);
if (len >= PAGE_SIZE - 1) {
pr_warn_once("devfreq transition table exceeds PAGE_SIZE. Disabling\n");
return -EFBIG;
--
2.43.0
Le 04/01/2024 à 22:55, Christian Marangi a écrit :
> Fix buffer overflow in trans_stat_show().
>
> Convert simple snprintf to the more secure scnprintf with size of
> PAGE_SIZE.
>
> Add condition checking if we are exceeding PAGE_SIZE and exit early from
> loop. Also add at the end a warning that we exceeded PAGE_SIZE and that
> stats is disabled.
>
> Return -EFBIG in the case where we don't have enough space to write the
> full transition table.
>
> Also document in the ABI that this function can return -EFBIG error.
>
> Cc: [email protected]
> Closes: https://bugzilla.kernel.org/show_bug.cgi?id=218041
> Fixes: e552bbaf5b98 ("PM / devfreq: Add sysfs node for representing frequency transition information.")
> Signed-off-by: Christian Marangi <[email protected]>
> ---
> Documentation/ABI/testing/sysfs-class-devfreq | 3 +
> drivers/devfreq/devfreq.c | 57 +++++++++++++------
> 2 files changed, 42 insertions(+), 18 deletions(-)
>
> diff --git a/Documentation/ABI/testing/sysfs-class-devfreq b/Documentation/ABI/testing/sysfs-class-devfreq
> index 5e6b74f30406..1e7e0bb4c14e 100644
> --- a/Documentation/ABI/testing/sysfs-class-devfreq
> +++ b/Documentation/ABI/testing/sysfs-class-devfreq
> @@ -52,6 +52,9 @@ Description:
>
> echo 0 > /sys/class/devfreq/.../trans_stat
>
> + If the transition table is bigger than PAGE_SIZE, reading
> + this will return an -EFBIG error.
> +
> What: /sys/class/devfreq/.../available_frequencies
> Date: October 2012
> Contact: Nishanth Menon <[email protected]>
> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
> index 63347a5ae599..8459512d9b07 100644
> --- a/drivers/devfreq/devfreq.c
> +++ b/drivers/devfreq/devfreq.c
> @@ -1688,7 +1688,7 @@ static ssize_t trans_stat_show(struct device *dev,
> struct device_attribute *attr, char *buf)
> {
> struct devfreq *df = to_devfreq(dev);
> - ssize_t len;
> + ssize_t len = 0;
> int i, j;
> unsigned int max_state;
>
> @@ -1697,7 +1697,7 @@ static ssize_t trans_stat_show(struct device *dev,
> max_state = df->max_state;
>
> if (max_state == 0)
> - return sprintf(buf, "Not Supported.\n");
> + return scnprintf(buf, PAGE_SIZE, "Not Supported.\n");
Hi,
maybe using sysfs_emit_at() could be even cleaner and less verbose?
>
> mutex_lock(&df->lock);
> if (!df->stop_polling &&
> @@ -1707,31 +1707,52 @@ static ssize_t trans_stat_show(struct device *dev,
> }
> mutex_unlock(&df->lock);
>
> - len = sprintf(buf, " From : To\n");
> - len += sprintf(buf + len, " :");
> - for (i = 0; i < max_state; i++)
> - len += sprintf(buf + len, "%10lu",
> - df->freq_table[i]);
> + len += scnprintf(buf + len, PAGE_SIZE - len, " From : To\n");
> + len += scnprintf(buf + len, PAGE_SIZE - len, " :");
> + for (i = 0; i < max_state; i++) {
> + if (len >= PAGE_SIZE - 1)
> + break;
> + len += scnprintf(buf + len, PAGE_SIZE - len, "%10lu",
> + df->freq_table[i]);
> + }
> + if (len >= PAGE_SIZE - 1)
> + return PAGE_SIZE - 1;
>
> - len += sprintf(buf + len, " time(ms)\n");
> + len += scnprintf(buf + len, PAGE_SIZE - len, " time(ms)\n");
>
> for (i = 0; i < max_state; i++) {
> + if (len >= PAGE_SIZE - 1)
> + break;
I'm not sure that adding all these tests is needed. It could save some
cycles in the worse case (when buf could overflow), but in fact wastes
cycles in the normel case.
CJ
> if (df->freq_table[i] == df->previous_freq)
> - len += sprintf(buf + len, "*");
> + len += scnprintf(buf + len, PAGE_SIZE - len, "*");
> else
> - len += sprintf(buf + len, " ");
> + len += scnprintf(buf + len, PAGE_SIZE - len, " ");
> + if (len >= PAGE_SIZE - 1)
> + break;
> +
> + len += scnprintf(buf + len, PAGE_SIZE - len, "%10lu:",
> + df->freq_table[i]);
> + for (j = 0; j < max_state; j++) {
> + if (len >= PAGE_SIZE - 1)
> + break;
> + len += scnprintf(buf + len, PAGE_SIZE - len, "%10u",
> + df->stats.trans_table[(i * max_state) + j]);
> + }
> + if (len >= PAGE_SIZE - 1)
> + break;
> + len += scnprintf(buf + len, PAGE_SIZE - len, "%10llu\n", (u64)
> + jiffies64_to_msecs(df->stats.time_in_state[i]));
> + }
>
> - len += sprintf(buf + len, "%10lu:", df->freq_table[i]);
> - for (j = 0; j < max_state; j++)
> - len += sprintf(buf + len, "%10u",
> - df->stats.trans_table[(i * max_state) + j]);
> + if (len < PAGE_SIZE - 1)
> + len += scnprintf(buf + len, PAGE_SIZE - len, "Total transition : %u\n",
> + df->stats.total_trans);
>
> - len += sprintf(buf + len, "%10llu\n", (u64)
> - jiffies64_to_msecs(df->stats.time_in_state[i]));
> + if (len >= PAGE_SIZE - 1) {
> + pr_warn_once("devfreq transition table exceeds PAGE_SIZE. Disabling\n");
> + return -EFBIG;
> }
>
> - len += sprintf(buf + len, "Total transition : %u\n",
> - df->stats.total_trans);
> return len;
> }
>
On Thu, Jan 04, 2024 at 11:19:44PM +0100, Christophe JAILLET wrote:
> Le 04/01/2024 ? 22:55, Christian Marangi a ?crit?:
> > Fix buffer overflow in trans_stat_show().
> >
> > Convert simple snprintf to the more secure scnprintf with size of
> > PAGE_SIZE.
> >
> > Add condition checking if we are exceeding PAGE_SIZE and exit early from
> > loop. Also add at the end a warning that we exceeded PAGE_SIZE and that
> > stats is disabled.
> >
> > Return -EFBIG in the case where we don't have enough space to write the
> > full transition table.
> >
> > Also document in the ABI that this function can return -EFBIG error.
> >
> > Cc: [email protected]
> > Closes: https://bugzilla.kernel.org/show_bug.cgi?id=218041
> > Fixes: e552bbaf5b98 ("PM / devfreq: Add sysfs node for representing frequency transition information.")
> > Signed-off-by: Christian Marangi <[email protected]>
> > ---
> > Documentation/ABI/testing/sysfs-class-devfreq | 3 +
> > drivers/devfreq/devfreq.c | 57 +++++++++++++------
> > 2 files changed, 42 insertions(+), 18 deletions(-)
> >
> > diff --git a/Documentation/ABI/testing/sysfs-class-devfreq b/Documentation/ABI/testing/sysfs-class-devfreq
> > index 5e6b74f30406..1e7e0bb4c14e 100644
> > --- a/Documentation/ABI/testing/sysfs-class-devfreq
> > +++ b/Documentation/ABI/testing/sysfs-class-devfreq
> > @@ -52,6 +52,9 @@ Description:
> > echo 0 > /sys/class/devfreq/.../trans_stat
> > + If the transition table is bigger than PAGE_SIZE, reading
> > + this will return an -EFBIG error.
> > +
> > What: /sys/class/devfreq/.../available_frequencies
> > Date: October 2012
> > Contact: Nishanth Menon <[email protected]>
> > diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
> > index 63347a5ae599..8459512d9b07 100644
> > --- a/drivers/devfreq/devfreq.c
> > +++ b/drivers/devfreq/devfreq.c
> > @@ -1688,7 +1688,7 @@ static ssize_t trans_stat_show(struct device *dev,
> > struct device_attribute *attr, char *buf)
> > {
> > struct devfreq *df = to_devfreq(dev);
> > - ssize_t len;
> > + ssize_t len = 0;
> > int i, j;
> > unsigned int max_state;
> > @@ -1697,7 +1697,7 @@ static ssize_t trans_stat_show(struct device *dev,
> > max_state = df->max_state;
> > if (max_state == 0)
> > - return sprintf(buf, "Not Supported.\n");
> > + return scnprintf(buf, PAGE_SIZE, "Not Supported.\n");
>
> Hi,
>
> maybe using sysfs_emit_at() could be even cleaner and less verbose?
>
If you notice this change is done in the second patch of the series.
This patch still use this more generic way to permit this to be
backported on stable kernel. (older kernel doesn't have sysfs_emit_at()
hence it can't be backported)
> > mutex_lock(&df->lock);
> > if (!df->stop_polling &&
> > @@ -1707,31 +1707,52 @@ static ssize_t trans_stat_show(struct device *dev,
> > }
> > mutex_unlock(&df->lock);
> > - len = sprintf(buf, " From : To\n");
> > - len += sprintf(buf + len, " :");
> > - for (i = 0; i < max_state; i++)
> > - len += sprintf(buf + len, "%10lu",
> > - df->freq_table[i]);
> > + len += scnprintf(buf + len, PAGE_SIZE - len, " From : To\n");
> > + len += scnprintf(buf + len, PAGE_SIZE - len, " :");
> > + for (i = 0; i < max_state; i++) {
> > + if (len >= PAGE_SIZE - 1)
> > + break;
> > + len += scnprintf(buf + len, PAGE_SIZE - len, "%10lu",
> > + df->freq_table[i]);
> > + }
> > + if (len >= PAGE_SIZE - 1)
> > + return PAGE_SIZE - 1;
> > - len += sprintf(buf + len, " time(ms)\n");
> > + len += scnprintf(buf + len, PAGE_SIZE - len, " time(ms)\n");
> > for (i = 0; i < max_state; i++) {
> > + if (len >= PAGE_SIZE - 1)
> > + break;
>
> I'm not sure that adding all these tests is needed. It could save some
> cycles in the worse case (when buf could overflow), but in fact wastes
> cycles in the normel case.
>
Consider that cpufreq stats does the same exact checks and I feel the 2
thing should be equal (given they do the same exact task)
Also with case of -EBIG, I would expact the thing to be very big and
exiting early might be beneficial, for normal stats I would expact only
a few cycle added. Myabe we can reduce them just for the for cycle?
> > if (df->freq_table[i] == df->previous_freq)
> > - len += sprintf(buf + len, "*");
> > + len += scnprintf(buf + len, PAGE_SIZE - len, "*");
> > else
> > - len += sprintf(buf + len, " ");
> > + len += scnprintf(buf + len, PAGE_SIZE - len, " ");
> > + if (len >= PAGE_SIZE - 1)
> > + break;
> > +
> > + len += scnprintf(buf + len, PAGE_SIZE - len, "%10lu:",
> > + df->freq_table[i]);
> > + for (j = 0; j < max_state; j++) {
> > + if (len >= PAGE_SIZE - 1)
> > + break;
> > + len += scnprintf(buf + len, PAGE_SIZE - len, "%10u",
> > + df->stats.trans_table[(i * max_state) + j]);
> > + }
> > + if (len >= PAGE_SIZE - 1)
> > + break;
> > + len += scnprintf(buf + len, PAGE_SIZE - len, "%10llu\n", (u64)
> > + jiffies64_to_msecs(df->stats.time_in_state[i]));
> > + }
> > - len += sprintf(buf + len, "%10lu:", df->freq_table[i]);
> > - for (j = 0; j < max_state; j++)
> > - len += sprintf(buf + len, "%10u",
> > - df->stats.trans_table[(i * max_state) + j]);
> > + if (len < PAGE_SIZE - 1)
> > + len += scnprintf(buf + len, PAGE_SIZE - len, "Total transition : %u\n",
> > + df->stats.total_trans);
> > - len += sprintf(buf + len, "%10llu\n", (u64)
> > - jiffies64_to_msecs(df->stats.time_in_state[i]));
> > + if (len >= PAGE_SIZE - 1) {
> > + pr_warn_once("devfreq transition table exceeds PAGE_SIZE. Disabling\n");
> > + return -EFBIG;
> > }
> > - len += sprintf(buf + len, "Total transition : %u\n",
> > - df->stats.total_trans);
> > return len;
> > }
>
--
Ansuel
On Thu, Jan 04, 2024 at 11:44:54PM +0100, Christian Marangi wrote:
> On Thu, Jan 04, 2024 at 11:19:44PM +0100, Christophe JAILLET wrote:
> > > @@ -1688,7 +1688,7 @@ static ssize_t trans_stat_show(struct device *dev,
> > > struct device_attribute *attr, char *buf)
> > > {
> > > struct devfreq *df = to_devfreq(dev);
> > > - ssize_t len;
> > > + ssize_t len = 0;
> > > int i, j;
> > > unsigned int max_state;
> > > @@ -1697,7 +1697,7 @@ static ssize_t trans_stat_show(struct device *dev,
> > > max_state = df->max_state;
> > > if (max_state == 0)
> > > - return sprintf(buf, "Not Supported.\n");
> > > + return scnprintf(buf, PAGE_SIZE, "Not Supported.\n");
> >
> > Hi,
> >
> > maybe using sysfs_emit_at() could be even cleaner and less verbose?
> >
>
> If you notice this change is done in the second patch of the series.
> This patch still use this more generic way to permit this to be
> backported on stable kernel. (older kernel doesn't have sysfs_emit_at()
> hence it can't be backported)
All activly supported kernels on the kernel.org front page have
sysfs_emit_at(), so this should not be an issue for anyone. Just do the
change here at the same time.
thanks,
greg k-h
Le 04/01/2024 à 23:44, Christian Marangi a écrit :
> On Thu, Jan 04, 2024 at 11:19:44PM +0100, Christophe JAILLET wrote:
>> Le 04/01/2024 à 22:55, Christian Marangi a écrit :
>>> Fix buffer overflow in trans_stat_show().
>>>
>>> Convert simple snprintf to the more secure scnprintf with size of
>>> PAGE_SIZE.
>>>
>>> Add condition checking if we are exceeding PAGE_SIZE and exit early from
>>> loop. Also add at the end a warning that we exceeded PAGE_SIZE and that
>>> stats is disabled.
>>>
>>> Return -EFBIG in the case where we don't have enough space to write the
>>> full transition table.
>>>
>>> Also document in the ABI that this function can return -EFBIG error.
>>>
>>> Cc: [email protected]
>>> Closes: https://bugzilla.kernel.org/show_bug.cgi?id=218041
>>> Fixes: e552bbaf5b98 ("PM / devfreq: Add sysfs node for representing frequency transition information.")
>>> Signed-off-by: Christian Marangi <[email protected]>
>>> ---
>>> Documentation/ABI/testing/sysfs-class-devfreq | 3 +
>>> drivers/devfreq/devfreq.c | 57 +++++++++++++------
>>> 2 files changed, 42 insertions(+), 18 deletions(-)
>>>
>>> diff --git a/Documentation/ABI/testing/sysfs-class-devfreq b/Documentation/ABI/testing/sysfs-class-devfreq
>>> index 5e6b74f30406..1e7e0bb4c14e 100644
>>> --- a/Documentation/ABI/testing/sysfs-class-devfreq
>>> +++ b/Documentation/ABI/testing/sysfs-class-devfreq
>>> @@ -52,6 +52,9 @@ Description:
>>> echo 0 > /sys/class/devfreq/.../trans_stat
>>> + If the transition table is bigger than PAGE_SIZE, reading
>>> + this will return an -EFBIG error.
>>> +
>>> What: /sys/class/devfreq/.../available_frequencies
>>> Date: October 2012
>>> Contact: Nishanth Menon <[email protected]>
>>> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
>>> index 63347a5ae599..8459512d9b07 100644
>>> --- a/drivers/devfreq/devfreq.c
>>> +++ b/drivers/devfreq/devfreq.c
>>> @@ -1688,7 +1688,7 @@ static ssize_t trans_stat_show(struct device *dev,
>>> struct device_attribute *attr, char *buf)
>>> {
>>> struct devfreq *df = to_devfreq(dev);
>>> - ssize_t len;
>>> + ssize_t len = 0;
>>> int i, j;
>>> unsigned int max_state;
>>> @@ -1697,7 +1697,7 @@ static ssize_t trans_stat_show(struct device *dev,
>>> max_state = df->max_state;
>>> if (max_state == 0)
>>> - return sprintf(buf, "Not Supported.\n");
>>> + return scnprintf(buf, PAGE_SIZE, "Not Supported.\n");
>>
>> Hi,
>>
>> maybe using sysfs_emit_at() could be even cleaner and less verbose?
>>
>
> If you notice this change is done in the second patch of the series.
> This patch still use this more generic way to permit this to be
> backported on stable kernel. (older kernel doesn't have sysfs_emit_at()
> hence it can't be backported)
Ok. Thanks for the clarification.
>
>>> mutex_lock(&df->lock);
>>> if (!df->stop_polling &&
>>> @@ -1707,31 +1707,52 @@ static ssize_t trans_stat_show(struct device *dev,
>>> }
>>> mutex_unlock(&df->lock);
>>> - len = sprintf(buf, " From : To\n");
>>> - len += sprintf(buf + len, " :");
>>> - for (i = 0; i < max_state; i++)
>>> - len += sprintf(buf + len, "%10lu",
>>> - df->freq_table[i]);
>>> + len += scnprintf(buf + len, PAGE_SIZE - len, " From : To\n");
>>> + len += scnprintf(buf + len, PAGE_SIZE - len, " :");
>>> + for (i = 0; i < max_state; i++) {
>>> + if (len >= PAGE_SIZE - 1)
>>> + break;
>>> + len += scnprintf(buf + len, PAGE_SIZE - len, "%10lu",
>>> + df->freq_table[i]);
>>> + }
>>> + if (len >= PAGE_SIZE - 1)
>>> + return PAGE_SIZE - 1;
[1]
>>> - len += sprintf(buf + len, " time(ms)\n");
>>> + len += scnprintf(buf + len, PAGE_SIZE - len, " time(ms)\n");
>>> for (i = 0; i < max_state; i++) {
>>> + if (len >= PAGE_SIZE - 1)
>>> + break;
>>
>> I'm not sure that adding all these tests is needed. It could save some
>> cycles in the worse case (when buf could overflow), but in fact wastes
>> cycles in the normel case.
>>
>
> Consider that cpufreq stats does the same exact checks and I feel the 2
> thing should be equal (given they do the same exact task)
Make sense.
But I think that show_trans_table(() could also save some tests.
I agree with you that limiting these tests to at most 1 per loop should
be already a first step.
Also surprising to me, is that if the output is too big, at [1] (above)
we silently truncate it, and at [2] (below) we return an error.
CJ
>
> Also with case of -EBIG, I would expact the thing to be very big and
> exiting early might be beneficial, for normal stats I would expact only
> a few cycle added. Myabe we can reduce them just for the for cycle?
>
>>> if (df->freq_table[i] == df->previous_freq)
>>> - len += sprintf(buf + len, "*");
>>> + len += scnprintf(buf + len, PAGE_SIZE - len, "*");
>>> else
>>> - len += sprintf(buf + len, " ");
>>> + len += scnprintf(buf + len, PAGE_SIZE - len, " ");
>>> + if (len >= PAGE_SIZE - 1)
>>> + break;
>>> +
>>> + len += scnprintf(buf + len, PAGE_SIZE - len, "%10lu:",
>>> + df->freq_table[i]);
>>> + for (j = 0; j < max_state; j++) {
>>> + if (len >= PAGE_SIZE - 1)
>>> + break;
>>> + len += scnprintf(buf + len, PAGE_SIZE - len, "%10u",
>>> + df->stats.trans_table[(i * max_state) + j]);
>>> + }
>>> + if (len >= PAGE_SIZE - 1)
>>> + break;
>>> + len += scnprintf(buf + len, PAGE_SIZE - len, "%10llu\n", (u64)
>>> + jiffies64_to_msecs(df->stats.time_in_state[i]));
>>> + }
>>> - len += sprintf(buf + len, "%10lu:", df->freq_table[i]);
>>> - for (j = 0; j < max_state; j++)
>>> - len += sprintf(buf + len, "%10u",
>>> - df->stats.trans_table[(i * max_state) + j]);
>>> + if (len < PAGE_SIZE - 1)
>>> + len += scnprintf(buf + len, PAGE_SIZE - len, "Total transition : %u\n",
>>> + df->stats.total_trans);
>>> - len += sprintf(buf + len, "%10llu\n", (u64)
>>> - jiffies64_to_msecs(df->stats.time_in_state[i]));
>>> + if (len >= PAGE_SIZE - 1) {
>>> + pr_warn_once("devfreq transition table exceeds PAGE_SIZE. Disabling\n");
>>> + return -EFBIG;
[2]
>>> }
>>> - len += sprintf(buf + len, "Total transition : %u\n",
>>> - df->stats.total_trans);
>>> return len;
>>> }
>>
>