* Re: [lm-sensors] [PATCH] hwmon: Do not accept name attributes which include '-'
2014-02-28 18:40 [lm-sensors] [PATCH] hwmon: Do not accept name attributes which include '-' Guenter Roeck
@ 2014-03-02 13:17 ` Jean Delvare
2014-03-02 13:54 ` Jean Delvare
` (4 subsequent siblings)
5 siblings, 0 replies; 7+ messages in thread
From: Jean Delvare @ 2014-03-02 13:17 UTC (permalink / raw)
To: lm-sensors
Hi Guenter,
On Fri, 28 Feb 2014 10:40:37 -0800, Guenter Roeck wrote:
> hwmon name attributes must not include '-', as specified in
> Documentation/hwmon/sysfs-interface. Validate the name attribute
> and return an error if it is invalid.
>
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> ---
> drivers/hwmon/hwmon.c | 5 +++++
> 1 file changed, 5 insertions(+)
>
> diff --git a/drivers/hwmon/hwmon.c b/drivers/hwmon/hwmon.c
> index e176a43..6a84df4 100644
> --- a/drivers/hwmon/hwmon.c
> +++ b/drivers/hwmon/hwmon.c
> @@ -22,6 +22,7 @@
> #include <linux/gfp.h>
> #include <linux/spinlock.h>
> #include <linux/pci.h>
> +#include <linux/string.h>
>
> #define HWMON_ID_PREFIX "hwmon"
> #define HWMON_ID_FORMAT HWMON_ID_PREFIX "%d"
> @@ -99,6 +100,10 @@ hwmon_device_register_with_groups(struct device *dev, const char *name,
> struct hwmon_device *hwdev;
> int err, id;
>
> + /* Do not accept '-' in hwmon name attribute */
> + if (name && strchr(name, '-'))
> + return ERR_PTR(-EINVAL);
> +
I like it, thanks for doing that. Maybe we could check for spaces too?
That would break libsensors as well, and I vaguely recalled that
someone attempted that once already (caught during code review,
thankfully.)
> id = ida_simple_get(&hwmon_ida, 0, 0, GFP_KERNEL);
> if (id < 0)
> return ERR_PTR(id);
Also I think we want to discuss the x86_pkg_temp_thermal driver case
with the guys responsible for it. The driver creates a thermal zone
with type "pkg-temp-0" (and "pkg-temp-1" etc. if you have more than one
CPU.) The thermal-to-hwmon bridge puts "pkg-temp-0" in the name
attribute, which causes libsensors to have to deal with a device named
"pkg-temp-0-virtual-0". Horrible.
Changing "pkg-temp" to "pkg_temp" isn't enough. We also need to teach
the thermal-to-hwmon bridge how to deal with multiple instances of the
same thermal device. And we need to find a way to transmit the
information to libsensors (presumably trough a new sysfs attribute?),
and then implement the support in libsensors.
In the specific case of x86_pkg_temp_thermal though, the sanest thing
to do IMHO would be to _not_ have it exposed as a hwmon device at all.
The temperature it reports is already reported by the coretemp driver,
and I see no point in reporting it twice.
I seem to recall there was a plan to have a flag to skip certain
thermal zones in the thermal-to-hwmon bridge, was it ever implemented?
We'd need exactly that.
--
Jean Delvare
Suse L3 Support
_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [lm-sensors] [PATCH] hwmon: Do not accept name attributes which include '-'
2014-02-28 18:40 [lm-sensors] [PATCH] hwmon: Do not accept name attributes which include '-' Guenter Roeck
2014-03-02 13:17 ` Jean Delvare
@ 2014-03-02 13:54 ` Jean Delvare
2014-03-02 14:21 ` Jean Delvare
` (3 subsequent siblings)
5 siblings, 0 replies; 7+ messages in thread
From: Jean Delvare @ 2014-03-02 13:54 UTC (permalink / raw)
To: lm-sensors
On Sun, 2 Mar 2014 14:17:43 +0100, Jean Delvare wrote:
> Also I think we want to discuss the x86_pkg_temp_thermal driver case
> with the guys responsible for it. The driver creates a thermal zone
> with type "pkg-temp-0" (and "pkg-temp-1" etc. if you have more than one
> CPU.) The thermal-to-hwmon bridge puts "pkg-temp-0" in the name
> attribute, which causes libsensors to have to deal with a device named
> "pkg-temp-0-virtual-0". Horrible.
>
> Changing "pkg-temp" to "pkg_temp" isn't enough. We also need to teach
> the thermal-to-hwmon bridge how to deal with multiple instances of the
> same thermal device. And we need to find a way to transmit the
> information to libsensors (presumably trough a new sysfs attribute?),
> and then implement the support in libsensors.
Correction: the thermal-to-hwmon bridge consolidates the thermal zones
of the same type into a single hwmon device. So there will never be
multiple instances of the same type, and libsensors is fine as it is.
--
Jean Delvare
SUSE L3 Support
_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [lm-sensors] [PATCH] hwmon: Do not accept name attributes which include '-'
2014-02-28 18:40 [lm-sensors] [PATCH] hwmon: Do not accept name attributes which include '-' Guenter Roeck
2014-03-02 13:17 ` Jean Delvare
2014-03-02 13:54 ` Jean Delvare
@ 2014-03-02 14:21 ` Jean Delvare
2014-03-02 16:24 ` Guenter Roeck
` (2 subsequent siblings)
5 siblings, 0 replies; 7+ messages in thread
From: Jean Delvare @ 2014-03-02 14:21 UTC (permalink / raw)
To: lm-sensors
Replying to myself...
On Sun, 2 Mar 2014 14:17:43 +0100, Jean Delvare wrote:
> I seem to recall there was a plan to have a flag to skip certain
> thermal zones in the thermal-to-hwmon bridge, was it ever implemented?
> We'd need exactly that.
Yes, it was implemented. I'll submit a patch for the
x86_pkg_temp_thermal driver as soon as I am done testing.
--
Jean Delvare
SUSE L3 Support
_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [lm-sensors] [PATCH] hwmon: Do not accept name attributes which include '-'
2014-02-28 18:40 [lm-sensors] [PATCH] hwmon: Do not accept name attributes which include '-' Guenter Roeck
` (2 preceding siblings ...)
2014-03-02 14:21 ` Jean Delvare
@ 2014-03-02 16:24 ` Guenter Roeck
2014-03-02 16:29 ` Guenter Roeck
2014-03-02 20:59 ` Jean Delvare
5 siblings, 0 replies; 7+ messages in thread
From: Guenter Roeck @ 2014-03-02 16:24 UTC (permalink / raw)
To: lm-sensors
Hi Jean,
On 03/02/2014 05:17 AM, Jean Delvare wrote:
> Hi Guenter,
>
> On Fri, 28 Feb 2014 10:40:37 -0800, Guenter Roeck wrote:
>> hwmon name attributes must not include '-', as specified in
>> Documentation/hwmon/sysfs-interface. Validate the name attribute
>> and return an error if it is invalid.
>>
>> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
>> ---
>> drivers/hwmon/hwmon.c | 5 +++++
>> 1 file changed, 5 insertions(+)
>>
>> diff --git a/drivers/hwmon/hwmon.c b/drivers/hwmon/hwmon.c
>> index e176a43..6a84df4 100644
>> --- a/drivers/hwmon/hwmon.c
>> +++ b/drivers/hwmon/hwmon.c
>> @@ -22,6 +22,7 @@
>> #include <linux/gfp.h>
>> #include <linux/spinlock.h>
>> #include <linux/pci.h>
>> +#include <linux/string.h>
>>
>> #define HWMON_ID_PREFIX "hwmon"
>> #define HWMON_ID_FORMAT HWMON_ID_PREFIX "%d"
>> @@ -99,6 +100,10 @@ hwmon_device_register_with_groups(struct device *dev, const char *name,
>> struct hwmon_device *hwdev;
>> int err, id;
>>
>> + /* Do not accept '-' in hwmon name attribute */
>> + if (name && strchr(name, '-'))
>> + return ERR_PTR(-EINVAL);
>> +
>
> I like it, thanks for doing that. Maybe we could check for spaces too?
> That would break libsensors as well, and I vaguely recalled that
> someone attempted that once already (caught during code review,
> thankfully.)
>
Makes sense. Any other invalid characters we should look out for
since we are at it ?
>> id = ida_simple_get(&hwmon_ida, 0, 0, GFP_KERNEL);
>> if (id < 0)
>> return ERR_PTR(id);
>
> Also I think we want to discuss the x86_pkg_temp_thermal driver case
> with the guys responsible for it. The driver creates a thermal zone
> with type "pkg-temp-0" (and "pkg-temp-1" etc. if you have more than one
> CPU.) The thermal-to-hwmon bridge puts "pkg-temp-0" in the name
> attribute, which causes libsensors to have to deal with a device named
> "pkg-temp-0-virtual-0". Horrible.
>
> Changing "pkg-temp" to "pkg_temp" isn't enough. We also need to teach
> the thermal-to-hwmon bridge how to deal with multiple instances of the
> same thermal device. And we need to find a way to transmit the
> information to libsensors (presumably trough a new sysfs attribute?),
> and then implement the support in libsensors.
>
> In the specific case of x86_pkg_temp_thermal though, the sanest thing
> to do IMHO would be to _not_ have it exposed as a hwmon device at all.
> The temperature it reports is already reported by the coretemp driver,
> and I see no point in reporting it twice.
>
> I seem to recall there was a plan to have a flag to skip certain
> thermal zones in the thermal-to-hwmon bridge, was it ever implemented?
> We'd need exactly that.
>
Guess you already solved that. Thanks for taking care of it!
Guenter
_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [lm-sensors] [PATCH] hwmon: Do not accept name attributes which include '-'
2014-02-28 18:40 [lm-sensors] [PATCH] hwmon: Do not accept name attributes which include '-' Guenter Roeck
` (3 preceding siblings ...)
2014-03-02 16:24 ` Guenter Roeck
@ 2014-03-02 16:29 ` Guenter Roeck
2014-03-02 20:59 ` Jean Delvare
5 siblings, 0 replies; 7+ messages in thread
From: Guenter Roeck @ 2014-03-02 16:29 UTC (permalink / raw)
To: lm-sensors
On 03/02/2014 08:24 AM, Guenter Roeck wrote:
> Hi Jean,
>
> On 03/02/2014 05:17 AM, Jean Delvare wrote:
>> Hi Guenter,
>>
>> On Fri, 28 Feb 2014 10:40:37 -0800, Guenter Roeck wrote:
>>> hwmon name attributes must not include '-', as specified in
>>> Documentation/hwmon/sysfs-interface. Validate the name attribute
>>> and return an error if it is invalid.
>>>
>>> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
>>> ---
>>> drivers/hwmon/hwmon.c | 5 +++++
>>> 1 file changed, 5 insertions(+)
>>>
>>> diff --git a/drivers/hwmon/hwmon.c b/drivers/hwmon/hwmon.c
>>> index e176a43..6a84df4 100644
>>> --- a/drivers/hwmon/hwmon.c
>>> +++ b/drivers/hwmon/hwmon.c
>>> @@ -22,6 +22,7 @@
>>> #include <linux/gfp.h>
>>> #include <linux/spinlock.h>
>>> #include <linux/pci.h>
>>> +#include <linux/string.h>
>>>
>>> #define HWMON_ID_PREFIX "hwmon"
>>> #define HWMON_ID_FORMAT HWMON_ID_PREFIX "%d"
>>> @@ -99,6 +100,10 @@ hwmon_device_register_with_groups(struct device *dev, const char *name,
>>> struct hwmon_device *hwdev;
>>> int err, id;
>>>
>>> + /* Do not accept '-' in hwmon name attribute */
>>> + if (name && strchr(name, '-'))
>>> + return ERR_PTR(-EINVAL);
>>> +
>>
>> I like it, thanks for doing that. Maybe we could check for spaces too?
>> That would break libsensors as well, and I vaguely recalled that
>> someone attempted that once already (caught during code review,
>> thankfully.)
>>
>
> Makes sense. Any other invalid characters we should look out for
> since we are at it ?
>
Newline, '\', '?', '*' comes into mind, and possibly quotes (single
and double).
Guenter
_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [lm-sensors] [PATCH] hwmon: Do not accept name attributes which include '-'
2014-02-28 18:40 [lm-sensors] [PATCH] hwmon: Do not accept name attributes which include '-' Guenter Roeck
` (4 preceding siblings ...)
2014-03-02 16:29 ` Guenter Roeck
@ 2014-03-02 20:59 ` Jean Delvare
5 siblings, 0 replies; 7+ messages in thread
From: Jean Delvare @ 2014-03-02 20:59 UTC (permalink / raw)
To: lm-sensors
On Sun, 02 Mar 2014 08:29:11 -0800, Guenter Roeck wrote:
> On 03/02/2014 08:24 AM, Guenter Roeck wrote:
> > On 03/02/2014 05:17 AM, Jean Delvare wrote:
> >> I like it, thanks for doing that. Maybe we could check for spaces too?
> >> That would break libsensors as well, and I vaguely recalled that
> >> someone attempted that once already (caught during code review,
> >> thankfully.)
> >
> > Makes sense. Any other invalid characters we should look out for
> > since we are at it ?
>
> Newline, '\', '?', '*' comes into mind, and possibly quotes (single
> and double).
FWIW I don't think libsensors would mind about quotes or '?'. It would
certainly be bothered by '*' because it is used as a wildcard when
matching chips names. Newlines would be an issue with sscanf probably.
'\' shouldn't matter unless it triggers a bug.
Now, except for newline, I don't really expect anyone to include any of
thee in a chip name, and I don't recall it ever happening. Newline
could accidentally be added at the end of the name, but that wouldn't
matter because libsensors only reads the first line from the file IIRC.
--
Jean Delvare
SUSE L3 Support
_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
^ permalink raw reply [flat|nested] 7+ messages in thread