[2.6.38-rc7, patch] cpuinfo_cur_freq unreadable by non-root

[2.6.38-rc7, patch] cpuinfo_cur_freq unreadable by non-root

[Daniel J Blueman]

I failed to find why
/sys/devices/system/cpu/cpu0/cpufreq/cpuinfo_cur_freq is only readable
by root; the cost of reading it isn't sufficient to cause memory or
scheduling denial, but may have been in the past.

Thus, allow non-root users read permission, consistent with other
attributes, allowing it to be monitored.

Signed-off-by: Daniel J Blueman <daniel.blueman@gmail.com>

diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index 1109f68..0f491fe 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -664,7 +664,7 @@ static ssize_t show_bios_limit(struct
cpufreq_policy *policy, char *buf)
 	return sprintf(buf, "%u\n", policy->cpuinfo.max_freq);
 }

-cpufreq_freq_attr_ro_perm(cpuinfo_cur_freq, 0400);
+cpufreq_freq_attr_ro(cpuinfo_cur_freq);
 cpufreq_freq_attr_ro(cpuinfo_min_freq);
 cpufreq_freq_attr_ro(cpuinfo_max_freq);
 cpufreq_freq_attr_ro(cpuinfo_transition_latency);
-- 
Daniel J Blueman

Re: [2.6.38-rc7, patch] cpuinfo_cur_freq unreadable by non-root

[Thomas Renninger]

Hi,

<resend, because my stupid mailer added a strange content-type header
preventing the mail to be sent to the cpufreq list (hope it works now)>

On Wednesday, March 02, 2011 06:52:05 AM Daniel J Blueman wrote:
> I failed to find why
> /sys/devices/system/cpu/cpu0/cpufreq/cpuinfo_cur_freq is only readable
> by root; the cost of reading it isn't sufficient to cause memory or
> scheduling denial, but may have been in the past.
> 
> Thus, allow non-root users read permission, consistent with other
> attributes, allowing it to be monitored.
> 
> Signed-off-by: Daniel J Blueman <daniel.blueman@gmail.com>
> 
> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> index 1109f68..0f491fe 100644
> --- a/drivers/cpufreq/cpufreq.c
> +++ b/drivers/cpufreq/cpufreq.c
> @@ -664,7 +664,7 @@ static ssize_t show_bios_limit(struct
> cpufreq_policy *policy, char *buf)
>  	return sprintf(buf, "%u\n", policy->cpuinfo.max_freq);
>  }
> 
> -cpufreq_freq_attr_ro_perm(cpuinfo_cur_freq, 0400);
> +cpufreq_freq_attr_ro(cpuinfo_cur_freq);
>  cpufreq_freq_attr_ro(cpuinfo_min_freq);
Having this read only for root goes back to the beginning
of git history of this file in 2005...
It looks like this is done on purpose, but I do not see why
this should not be readable for others.

Maybe Dominik or Dave have an idea.
Please also remove the definition of cpufreq_freq_attr_ro_perm
in include/linux/cpufreq.h, cpuinfo_cur_freq is the only
instance using it.

Thanks,

   Thomas

[2.6.38-rc7, patch v2] cpuinfo_cur_freq unreadable by non-root

[Daniel J Blueman]

On 2 March 2011 16:59, Thomas Renninger <trenn@suse.de> wrote:
> On Wednesday, March 02, 2011 06:52:05 AM Daniel J Blueman wrote:
>
>> I failed to find why
>> /sys/devices/system/cpu/cpu0/cpufreq/cpuinfo_cur_freq is only readable
>> by root; the cost of reading it isn't sufficient to cause memory or
>> scheduling denial, but may have been in the past.
>>
>> Thus, allow non-root users read permission, consistent with other
>> attributes, allowing it to be monitored.
>>
>> Signed-off-by: Daniel J Blueman <daniel.blueman@gmail.com>
>>
>> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
>> index 1109f68..0f491fe 100644
>> --- a/drivers/cpufreq/cpufreq.c
>> +++ b/drivers/cpufreq/cpufreq.c
>> @@ -664,7 +664,7 @@ static ssize_t show_bios_limit(struct
>> cpufreq_policy *policy, char *buf)
>> return sprintf(buf, "%u\n", policy->cpuinfo.max_freq);
>> }
>>
>> -cpufreq_freq_attr_ro_perm(cpuinfo_cur_freq, 0400);
>> +cpufreq_freq_attr_ro(cpuinfo_cur_freq);
>
> Having this read only for root goes back to the beginning
> of git history of this file in 2005...
> It looks like this is done on purpose, but I do not see why
> this should not be readable for others.

> Maybe Dominik or Dave have an idea.
> Please also remove the definition of cpufreq_freq_attr_ro_perm
> in include/linux/cpufreq.h, cpuinfo_cur_freq is the only
> instance using it.

I got a feeling someone would ask. Updated:

/sys/devices/system/cpu/cpu0/cpufreq/cpuinfo_cur_freq is only readable
by root; the cost of reading it isn't sufficient to cause memory or
scheduling denial, but may have been in the past, so allow non-root
users read permission, consistent with other attributes, allowing
monitoring. Remove subsequently unused macro.

Signed-off-by: Daniel J Blueman <daniel.blueman@gmail.com>

diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index 1109f68..0f491fe 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -664,7 +664,7 @@ static ssize_t show_bios_limit(struct
cpufreq_policy *policy, char *buf)
 	return sprintf(buf, "%u\n", policy->cpuinfo.max_freq);
 }

-cpufreq_freq_attr_ro_perm(cpuinfo_cur_freq, 0400);
+cpufreq_freq_attr_ro(cpuinfo_cur_freq);
 cpufreq_freq_attr_ro(cpuinfo_min_freq);
 cpufreq_freq_attr_ro(cpuinfo_max_freq);
 cpufreq_freq_attr_ro(cpuinfo_transition_latency);
diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h
index c3e9de8..3a4cdc9 100644
--- a/include/linux/cpufreq.h
+++ b/include/linux/cpufreq.h
@@ -277,10 +277,6 @@ struct freq_attr {
 static struct freq_attr _name =			\
 __ATTR(_name, 0444, show_##_name, NULL)

-#define cpufreq_freq_attr_ro_perm(_name, _perm)	\
-static struct freq_attr _name =			\
-__ATTR(_name, _perm, show_##_name, NULL)
-
 #define cpufreq_freq_attr_ro_old(_name)		\
 static struct freq_attr _name##_old =		\
 __ATTR(_name, 0444, show_##_name##_old, NULL)
-- 
Daniel J Blueman

Re: [2.6.38-rc7, patch] cpuinfo_cur_freq unreadable by non-root

[Dominik Brodowski]

Hey,

On Wed, Mar 02, 2011 at 10:27:38AM +0100, Thomas Renninger wrote:
> > -cpufreq_freq_attr_ro_perm(cpuinfo_cur_freq, 0400);
> > +cpufreq_freq_attr_ro(cpuinfo_cur_freq);
> >  cpufreq_freq_attr_ro(cpuinfo_min_freq);
> Having this read only for root goes back to the beginning
> of git history of this file in 2005...
> It looks like this is done on purpose, but I do not see why
> this should not be readable for others.
> 
> Maybe Dominik or Dave have an idea.

This talks to hardware, and might (in some implementations) talk to the
BIOS, causing it to do nasty things, such as executing things in SMM. That
shouldn't be available to the world. However, as ACPI still does not provide
a ->get() method, making this attribute world readable _might_ not cause too
much harm, but it isn't worth the hassle IMO. Especially as everyone may
read out scaling_cur_freq, and scaling_cur_freq differs from
cpuinfo_cur_freq _only_ if something is broken.

Best,
	Dominik