Message-ID: <cf6f672fbaf645f780ae5eab1a955871@AcuMS.aculab.com>
Date:   Wed, 30 Mar 2022 09:20:31 +0000
From:   David Laight <David.Laight@...LAB.COM>
To:     'Guenter Roeck' <linux@...ck-us.net>,
        'Michael Walle' <michael@...le.cc>,
        Xu Yilun <yilun.xu@...el.com>, Tom Rix <trix@...hat.com>,
        Jean Delvare <jdelvare@...e.com>, Andrew Lunn <andrew@...n.ch>,
        Heiner Kallweit <hkallweit1@...il.com>,
        Russell King <linux@...linux.org.uk>,
        "David S . Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>
CC:     "linux-hwmon@...r.kernel.org" <linux-hwmon@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>
Subject: RE: [PATCH v2 1/5] hwmon: introduce hwmon_sanitize_name()

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)

Powered by blists - more mailing lists