On 3/29/22 19:57, David Laight wrote:
> From: Michael Walle
>> Sent: 29 March 2022 17:07
>>
>> More and more drivers will check for bad characters in the hwmon name
>> and all are using the same code snippet. Consolidate that code by adding
>> a new hwmon_sanitize_name() function.
>
> I'm assuming these 'bad' hwmon names come from userspace?
> Like ethernet interface names??
>
> Is silently changing the name of the hwmon entries the right
> thing to do at all?
>
> What happens if the user tries to create both "foo_bar" and "foo-bar"?
> I'm sure that is going to go horribly wrong somewhere.
>
> It would certainly make sense to have a function to verify the name
> is actually valid.
> Then bad names can be rejected earlier on.
>
> I'm also intrigued about the list of invalid characters:
>
> +static bool hwmon_is_bad_char(const char ch)
> +{
> + switch (ch) {
> + case '-':
> + case '*':
> + case ' ':
> + case '\t':
> + case '\n':
> + return true;
> + default:
> + return false;
> + }
> +}
>
> If '\t' and '\n' are invalid why are all the other control characters
> allowed?
> I'm guessing '*' is disallowed because it is the shell wildcard?
> So what about '?'.
> Then I'd expect '/' to be invalid - but that isn't checked.
> Never mind all the values 0x80 to 0xff - they are probably worse
> than whitespace.
>
> OTOH why are any characters invalid at all - except '/'?
>
The name is supposed to reflect a driver name. Usually driver names
are not defined by userspace but by driver authors. The name is used
by libsensors to distinguish a driver from its instantiation.
libsensors uses wildcards in /etc/sensors3.conf. Duplicate names
are expected; there can be many instances of the same driver in
the system. For example, on the system I am typing this on, I have:
/sys/class/hwmon/hwmon0/name:nvme
/sys/class/hwmon/hwmon1/name:nvme
/sys/class/hwmon/hwmon2/name:nouveau
/sys/class/hwmon/hwmon3/name:nct6797
/sys/class/hwmon/hwmon4/name:jc42
/sys/class/hwmon/hwmon5/name:jc42
/sys/class/hwmon/hwmon6/name:jc42
/sys/class/hwmon/hwmon7/name:jc42
/sys/class/hwmon/hwmon8/name:k10temp
hwmon_is_bad_char() filters out characters which interfere with
libsensor's view of driver instances and the configuration data
in /etc/sensors3.conf. For example, again on my system, the
"sensors" command reports the following jc42 and nvme sensors.
jc42-i2c-0-1a
jc42-i2c-0-18
jc42-i2c-0-1b
jc42-i2c-0-19
nvme-pci-0100
nvme-pci-2500
In /etc/sensors3.conf, there might be entries for "jc42-*" or "nvme-*".
I don't think libsensors cares if a driver is named "this/is/my/driver".
That driver would then, assuming it is an i2c driver, show up
with the sensors command as "this/is/my/driver-i2c-0-25" or similar.
If it is named "this%is%my%driver", it would be something like
"this%is%my%driver-i2c-0-25". And so on. We can not permit "jc-42"
because libsensors would not be able to parse something like
"jc-42-*" or "jc-42-i2c-*".
Taking your example, if driver authors implement two drivers, one
named foo-bar and the other foo_bar, it would be the driver authors'
responsibility to provide valid driver names to the hwmon subsystem,
whatever those names might be. If both end up named "foo_bar" and can
as result not be distinguished from each other by libsensors,
or a user of the "sensors" command, that would be entirely the
responsibility of the driver authors. The only involvement of the
hwmon subsystem - and that is optional - would be to provide means
to the drivers to help them ensure that the names are valid, but
not that they are unique.
If there is ever a driver with a driver name that interferes with
libsensors' ability to distinguish the driver name from interface/port
information, we'll be happy to add the offending character(s)
to hwmon_is_bad_char(). Until then, being picky doesn't really
add any value and appears pointless.
Thanks,
Guenter
From: Guenter Roeck
> Sent: 30 March 2022 04:47
>
> On 3/29/22 19:57, David Laight wrote:
> > From: Michael Walle
> >> Sent: 29 March 2022 17:07
> >>
> >> More and more drivers will check for bad characters in the hwmon name
> >> and all are using the same code snippet. Consolidate that code by adding
> >> a new hwmon_sanitize_name() function.
> >
> > I'm assuming these 'bad' hwmon names come from userspace?
> > Like ethernet interface names??
> >
> > Is silently changing the name of the hwmon entries the right
> > thing to do at all?
> >
> > What happens if the user tries to create both "foo_bar" and "foo-bar"?
> > I'm sure that is going to go horribly wrong somewhere.
> >
> > It would certainly make sense to have a function to verify the name
> > is actually valid.
> > Then bad names can be rejected earlier on.
> >
> > I'm also intrigued about the list of invalid characters:
> >
> > +static bool hwmon_is_bad_char(const char ch)
> > +{
> > + switch (ch) {
> > + case '-':
> > + case '*':
> > + case ' ':
> > + case '\t':
> > + case '\n':
> > + return true;
> > + default:
> > + return false;
> > + }
> > +}
> >
> > If '\t' and '\n' are invalid why are all the other control characters
> > allowed?
> > I'm guessing '*' is disallowed because it is the shell wildcard?
> > So what about '?'.
> > Then I'd expect '/' to be invalid - but that isn't checked.
> > Never mind all the values 0x80 to 0xff - they are probably worse
> > than whitespace.
> >
> > OTOH why are any characters invalid at all - except '/'?
> >
>
> The name is supposed to reflect a driver name. Usually driver names
> are not defined by userspace but by driver authors. The name is used
> by libsensors to distinguish a driver from its instantiation.
> libsensors uses wildcards in /etc/sensors3.conf. Duplicate names
> are expected; there can be many instances of the same driver in
> the system. For example, on the system I am typing this on, I have:
>
> /sys/class/hwmon/hwmon0/name:nvme
> /sys/class/hwmon/hwmon1/name:nvme
> /sys/class/hwmon/hwmon2/name:nouveau
> /sys/class/hwmon/hwmon3/name:nct6797
> /sys/class/hwmon/hwmon4/name:jc42
> /sys/class/hwmon/hwmon5/name:jc42
> /sys/class/hwmon/hwmon6/name:jc42
> /sys/class/hwmon/hwmon7/name:jc42
> /sys/class/hwmon/hwmon8/name:k10temp
>
> hwmon_is_bad_char() filters out characters which interfere with
> libsensor's view of driver instances and the configuration data
> in /etc/sensors3.conf. For example, again on my system, the
> "sensors" command reports the following jc42 and nvme sensors.
>
> jc42-i2c-0-1a
> jc42-i2c-0-18
> jc42-i2c-0-1b
> jc42-i2c-0-19
> nvme-pci-0100
> nvme-pci-2500
>
> In /etc/sensors3.conf, there might be entries for "jc42-*" or "nvme-*".
> I don't think libsensors cares if a driver is named "this/is/my/driver".
> That driver would then, assuming it is an i2c driver, show up
> with the sensors command as "this/is/my/driver-i2c-0-25" or similar.
> If it is named "this%is%my%driver", it would be something like
> "this%is%my%driver-i2c-0-25". And so on. We can not permit "jc-42"
> because libsensors would not be able to parse something like
> "jc-42-*" or "jc-42-i2c-*".
>
> Taking your example, if driver authors implement two drivers, one
> named foo-bar and the other foo_bar, it would be the driver authors'
> responsibility to provide valid driver names to the hwmon subsystem,
> whatever those names might be. If both end up named "foo_bar" and can
> as result not be distinguished from each other by libsensors,
> or a user of the "sensors" command, that would be entirely the
> responsibility of the driver authors. The only involvement of the
> hwmon subsystem - and that is optional - would be to provide means
> to the drivers to help them ensure that the names are valid, but
> not that they are unique.
>
> If there is ever a driver with a driver name that interferes with
> libsensors' ability to distinguish the driver name from interface/port
> information, we'll be happy to add the offending character(s)
> to hwmon_is_bad_char(). Until then, being picky doesn't really
> add any value and appears pointless.
So actually, the only one of the characters that is actually
likely at all is '-'.
And even that can be deemed to be an error in the caller?
Or a 'bug' in the libsensors code - which could itself treat '-' as '_'.
So why not error the request to created the hwmon device with
an invalid name.
The name supplied will soon get fixed - since it is a literal
string in the calling driver.
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)