[lm-sensors] Proposal: howto handle sysfs attribute writes with

[lm-sensors] Proposal: howto handle sysfs attribute writes with

[Hans de Goede]

Hi All,

Since there was some discussion about how I'm handling sysfs attr writes with 
invalid values in the f71882fg driver, and since I've seen other discussions 
about it, here is a proposal to try and create a standard way to handle this.

This is intended to become a part of Documentation/hwmon/sysfs eventually.

---

Howto check the validity of user written values to sysfs attributes:

hwmon sysfs attributes always contain numbers, so the first thing todo is to
convert the input to a number, there are 2 ways todo this depending wether
the number can be negative or not:
unsigned long u = simple_strtoul(buf, NULL, 10);
long s = simple_strtol(buf, NULL, 10);

With buf being the buffer with the user input being passed by the kernel.
Notice that we do not use the second argument of strto[u]l, and thus cannot
tell when 0 is returned, if this was really 0 or is caused by invalid input.
This is done deliberately as checking this everywhere would add a lot of
code to the kernel. We do need to document clearly that writing a non-number 
will be seen as writing 0.

Notice that it is important to always store the converted value in an unsigned 
long or long, so that no wrap around can happen before any further checking.

After conversion and storing the converted value in the right type, and 
preferably before any conversions on the value, the value should be checked if 
it its acceptable. For example if its a temperature limit being stored in an 
unsigned 8 bit register, we should have something like this:

unsigned long u = simple_strtoul(buf, NULL, 10);
if (u > 255000)
	return -EINVAL;

One could argue to clamp instead of returning EINVAL, but what todo then when 
something that is not a continues range like temp sensor type gets written? In 
the not a continues range scenario returning -EINVAL is the only thing that 
makes sense, so we do this in the continues range scenario too to be consistent.

---

So does this make sense? I know that many drivers are currently doing this 
different, some clamp, some don't check at all, thus effectively wrapping 
around in most cases, all these different ways are exactly the reason for me 
writing this proposal.

Regards,

Hans




_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] Proposal: howto handle sysfs attribute writes with

[Jean Delvare]

Hi Hans,

On Thu, 19 Jul 2007 17:02:20 +0200, Hans de Goede wrote:
> Since there was some discussion about how I'm handling sysfs attr writes with 
> invalid values in the f71882fg driver, and since I've seen other discussions 
> about it, here is a proposal to try and create a standard way to handle this.
> 
> This is intended to become a part of Documentation/hwmon/sysfs eventually.
> 
> ---
> 
> Howto check the validity of user written values to sysfs attributes:
> 
> hwmon sysfs attributes always contain numbers, so the first thing todo is to
> convert the input to a number, there are 2 ways todo this depending wether

s/todo/to do/, s/wether/whether/

> the number can be negative or not:
> unsigned long u = simple_strtoul(buf, NULL, 10);
> long s = simple_strtol(buf, NULL, 10);
> 
> With buf being the buffer with the user input being passed by the kernel.
> Notice that we do not use the second argument of strto[u]l, and thus cannot
> tell when 0 is returned, if this was really 0 or is caused by invalid input.
> This is done deliberately as checking this everywhere would add a lot of
> code to the kernel. We do need to document clearly that writing a non-number 
> will be seen as writing 0.

Agreed.

> Notice that it is important to always store the converted value in an unsigned 
> long or long, so that no wrap around can happen before any further checking.

True.

> After conversion and storing the converted value in the right type, and 
> preferably before any conversions on the value, the value should be checked if 

Not sure about the "preferably before any conversions on the value". In
some cases it's easy to get it wrong that way, and converting first is
more obvious.

> it its acceptable. For example if its a temperature limit being stored in an 

   s/its/is/                      s/its/it is/

> unsigned 8 bit register, we should have something like this:

Probably not a very good example, as temperature values are almost
never u8. s8 is much more frequent.

> 
> unsigned long u = simple_strtoul(buf, NULL, 10);
> if (u > 255000)
> 	return -EINVAL;
> 
> One could argue to clamp instead of returning EINVAL, but what todo then when 

As said it another thread, I indeed believe that continuous values
should be clamped. We even have a helper function for this,
SENSORS_LIMIT(), which you may want to mention. This is how the
majority of hwmon drivers behave right now.

> something that is not a continues range like temp sensor type gets written? In 
> the not a continues range scenario returning -EINVAL is the only thing that 
> makes sense, so we do this in the continues range scenario too to be consistent.

I see no problem making different rules for different case. The lists
aren't that long so we could even be explicit:

* temperature, fan and voltage limits: clamp.
* fan div, pwm... well, all the rest: error.

> 
> ---
> 
> So does this make sense? I know that many drivers are currently doing this 
> different, some clamp, some don't check at all, thus effectively wrapping 
> around in most cases, all these different ways are exactly the reason for me 
> writing this proposal.

Yes, I think this is a good idea.

-- 
Jean Delvare

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] Proposal: howto handle sysfs attribute writes with

[Hans de Goede]

Jean Delvare wrote:
> Hi Hans,
> 
> On Thu, 19 Jul 2007 17:02:20 +0200, Hans de Goede wrote:
>> Since there was some discussion about how I'm handling sysfs attr writes with 
>> invalid values in the f71882fg driver, and since I've seen other discussions 
>> about it, here is a proposal to try and create a standard way to handle this.
>>
>> This is intended to become a part of Documentation/hwmon/sysfs eventually.
>>

Thanks for the feedback Jean, here is a reworded version with all your feedback 
incorporated.

---

Howto check the validity of user written values to sysfs attributes:

hwmon sysfs attributes always contain numbers, so the first thing to do is to
convert the input to a number, there are 2 ways todo this depending whether
the number can be negative or not:
unsigned long u = simple_strtoul(buf, NULL, 10);
long s = simple_strtol(buf, NULL, 10);

With buf being the buffer with the user input being passed by the kernel.
Notice that we do not use the second argument of strto[u]l, and thus cannot
tell when 0 is returned, if this was really 0 or is caused by invalid input.
This is done deliberately as checking this everywhere would add a lot of
code to the kernel. We do need to document clearly that writing a non-number
will be seen as writing 0.

Notice that it is important to always store the converted value in an unsigned
long or long, so that no wrap around can happen before any further checking.

After conversion and storing the converted value in the right type, the value
should be checked if its acceptable. Be carefull with further conversions on the
value before checking it for validity, as these conversions could still cause a
wrap around before the check. For example do not multiply the result, and only add
/ substract if it has been divided before the add / substract.

What to do if a value is found to be invalid, depends on the type of the sysfs 
attribute that is being set. If its a continuous setting like a tempX_max or 
inX_max attribute, then the value should be clamped to its limits using 
SENSORS_LIMIT(value, min_limit, max_limit). If its not continuous like for 
example a tempX_type, then when an invalid value is written, -EINVAL should be 
returned.

Example1, temp1_max, register presents -128 - 127 degrees as 0 - 255:
long v = simple_strtol(buf, NULL, 10) / 1000;
SENSORS_LIMIT(v, -128, 127);
v += 128;
/* write v to register */

Example2, fan divider setting, valid values 2, 4 and 8
unsigned long v = simple_strtoul(buf, NULL, 10);

switch (v) {
        case 2: v = 1; break;
        case 4: v = 2; break;
        case 8: v = 3; break;
        default:
                return -EINVAL;
}
/* write v to register */

---

So, let me know what you think of the new version above and if its acceptable 
I'll do a patch adding this to the sysfs interface doc, or should it be in 
another doc?

Regards,

Hans

p.s.

Tomorrow I'm going on vacation for 6 days, so don't expect a follow up on any 
replies soon.

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] Proposal: howto handle sysfs attribute writes with

[Jean Delvare]

Hi Hans,

On Sun, 29 Jul 2007 09:13:03 +0200, Hans de Goede wrote:
> Howto check the validity of user written values to sysfs attributes:
> 
> hwmon sysfs attributes always contain numbers, so the first thing to do is to
> convert the input to a number, there are 2 ways todo this depending whether
> the number can be negative or not:
> unsigned long u = simple_strtoul(buf, NULL, 10);
> long s = simple_strtol(buf, NULL, 10);
> 
> With buf being the buffer with the user input being passed by the kernel.
> Notice that we do not use the second argument of strto[u]l, and thus cannot
> tell when 0 is returned, if this was really 0 or is caused by invalid input.
> This is done deliberately as checking this everywhere would add a lot of
> code to the kernel. We do need to document clearly that writing a non-number
> will be seen as writing 0.

This last sentence obviously can't be left verbatim in the final text.

> Notice that it is important to always store the converted value in an unsigned
> long or long, so that no wrap around can happen before any further checking.
> 
> After conversion and storing the converted value in the right type, the value

"After converting and storing"?

> should be checked if its acceptable. Be carefull with further conversions on the

Spelling: careful.

> value before checking it for validity, as these conversions could still cause a
> wrap around before the check. For example do not multiply the result, and only add
> / substract if it has been divided before the add / substract.

Spelling: subtract (I had to double-check this one, I would have had it
wrong myself.)

> What to do if a value is found to be invalid, depends on the type of the sysfs 
> attribute that is being set. If its a continuous setting like a tempX_max or 

Spelling: it's.

> inX_max attribute, then the value should be clamped to its limits using 
> SENSORS_LIMIT(value, min_limit, max_limit). If its not continuous like for 

Ditto.

> example a tempX_type, then when an invalid value is written, -EINVAL should be 
> returned.
> 
> Example1, temp1_max, register presents -128 - 127 degrees as 0 - 255:
> long v = simple_strtol(buf, NULL, 10) / 1000;
> SENSORS_LIMIT(v, -128, 127);
> v += 128;
> /* write v to register */

This example is confusing IMHO, as most chips present -128 - 127
degrees as 0 - 255, but using a signed (2's complement) value, not
using an offset.

> Example2, fan divider setting, valid values 2, 4 and 8
> unsigned long v = simple_strtoul(buf, NULL, 10);
> 
> switch (v) {
>         case 2: v = 1; break;
>         case 4: v = 2; break;
>         case 8: v = 3; break;
>         default:
>                 return -EINVAL;
> }
> /* write v to register */
> 
> ---
> 
> So, let me know what you think of the new version above and if its acceptable 
> I'll do a patch adding this to the sysfs interface doc, or should it be in 
> another doc?

sysfs-interface seems to be the right place.

-- 
Jean Delvare

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors