| Message ID | 20220329160730.3265481-2-michael@walle.cc (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | hwmon: introduce hwmon_sanitize() | expand |
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 '/'? David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
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
On Tue, Mar 29, 2022 at 06:07:26PM +0200, Michael Walle wrote: > 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. > > Signed-off-by: Michael Walle <michael@walle.cc> > --- > Documentation/hwmon/hwmon-kernel-api.rst | 9 ++++- > drivers/hwmon/hwmon.c | 49 ++++++++++++++++++++++++ > include/linux/hwmon.h | 3 ++ > 3 files changed, 60 insertions(+), 1 deletion(-) > > diff --git a/Documentation/hwmon/hwmon-kernel-api.rst b/Documentation/hwmon/hwmon-kernel-api.rst > index c41eb6108103..12f4a9bcef04 100644 > --- a/Documentation/hwmon/hwmon-kernel-api.rst > +++ b/Documentation/hwmon/hwmon-kernel-api.rst > @@ -50,6 +50,10 @@ register/unregister functions:: > > void devm_hwmon_device_unregister(struct device *dev); > > + char *hwmon_sanitize_name(const char *name); > + > + char *devm_hwmon_sanitize_name(struct device *dev, const char *name); > + > hwmon_device_register_with_groups registers a hardware monitoring device. > The first parameter of this function is a pointer to the parent device. > The name parameter is a pointer to the hwmon device name. The registration > @@ -93,7 +97,10 @@ removal would be too late. > > All supported hwmon device registration functions only accept valid device > names. Device names including invalid characters (whitespace, '*', or '-') > -will be rejected. The 'name' parameter is mandatory. > +will be rejected. The 'name' parameter is mandatory. Before calling a > +register function you should either use hwmon_sanitize_name or > +devm_hwmon_sanitize_name to replace any invalid characters with an I suggest to duplicate the name and replace ... Thanks, Yilun > +underscore. > > Using devm_hwmon_device_register_with_info() > -------------------------------------------- > diff --git a/drivers/hwmon/hwmon.c b/drivers/hwmon/hwmon.c > index 989e2c8496dd..619ef9f9a16e 100644 > --- a/drivers/hwmon/hwmon.c > +++ b/drivers/hwmon/hwmon.c > @@ -1057,6 +1057,55 @@ void devm_hwmon_device_unregister(struct device *dev) > } > EXPORT_SYMBOL_GPL(devm_hwmon_device_unregister); > > +static char *__hwmon_sanitize_name(struct device *dev, const char *old_name) > +{ > + char *name, *p; > + > + if (dev) > + name = devm_kstrdup(dev, old_name, GFP_KERNEL); > + else > + name = kstrdup(old_name, GFP_KERNEL); > + if (!name) > + return NULL; > + > + for (p = name; *p; p++) > + if (hwmon_is_bad_char(*p)) > + *p = '_'; > + > + return name; > +} > + > +/** > + * hwmon_sanitize_name - Replaces invalid characters in a hwmon name > + * @name: NUL-terminated name > + * > + * Allocates a new string where any invalid characters will be replaced > + * by an underscore. > + * > + * Returns newly allocated name or %NULL in case of error. > + */ > +char *hwmon_sanitize_name(const char *name) > +{ > + return __hwmon_sanitize_name(NULL, name); > +} > +EXPORT_SYMBOL_GPL(hwmon_sanitize_name); > + > +/** > + * devm_hwmon_sanitize_name - resource managed hwmon_sanitize_name() > + * @dev: device to allocate memory for > + * @name: NUL-terminated name > + * > + * Allocates a new string where any invalid characters will be replaced > + * by an underscore. > + * > + * Returns newly allocated name or %NULL in case of error. > + */ > +char *devm_hwmon_sanitize_name(struct device *dev, const char *name) > +{ > + return __hwmon_sanitize_name(dev, name); > +} > +EXPORT_SYMBOL_GPL(devm_hwmon_sanitize_name); > + > static void __init hwmon_pci_quirks(void) > { > #if defined CONFIG_X86 && defined CONFIG_PCI > diff --git a/include/linux/hwmon.h b/include/linux/hwmon.h > index eba380b76d15..4efaf06fd2b8 100644 > --- a/include/linux/hwmon.h > +++ b/include/linux/hwmon.h > @@ -461,6 +461,9 @@ void devm_hwmon_device_unregister(struct device *dev); > int hwmon_notify_event(struct device *dev, enum hwmon_sensor_types type, > u32 attr, int channel); > > +char *hwmon_sanitize_name(const char *name); > +char *devm_hwmon_sanitize_name(struct device *dev, const char *name); > + > /** > * hwmon_is_bad_char - Is the char invalid in a hwmon name > * @ch: the char to be considered > -- > 2.30.2
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)
From: Xu Yilun > Sent: 30 March 2022 07:51 > > On Tue, Mar 29, 2022 at 06:07:26PM +0200, Michael Walle wrote: > > 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. > > > > Signed-off-by: Michael Walle <michael@walle.cc> > > --- > > Documentation/hwmon/hwmon-kernel-api.rst | 9 ++++- > > drivers/hwmon/hwmon.c | 49 ++++++++++++++++++++++++ > > include/linux/hwmon.h | 3 ++ > > 3 files changed, 60 insertions(+), 1 deletion(-) > > > > diff --git a/Documentation/hwmon/hwmon-kernel-api.rst b/Documentation/hwmon/hwmon-kernel-api.rst > > index c41eb6108103..12f4a9bcef04 100644 > > --- a/Documentation/hwmon/hwmon-kernel-api.rst > > +++ b/Documentation/hwmon/hwmon-kernel-api.rst > > @@ -50,6 +50,10 @@ register/unregister functions:: > > > > void devm_hwmon_device_unregister(struct device *dev); > > > > + char *hwmon_sanitize_name(const char *name); > > + > > + char *devm_hwmon_sanitize_name(struct device *dev, const char *name); > > + > > hwmon_device_register_with_groups registers a hardware monitoring device. > > The first parameter of this function is a pointer to the parent device. > > The name parameter is a pointer to the hwmon device name. The registration > > @@ -93,7 +97,10 @@ removal would be too late. > > > > All supported hwmon device registration functions only accept valid device > > names. Device names including invalid characters (whitespace, '*', or '-') > > -will be rejected. The 'name' parameter is mandatory. > > +will be rejected. The 'name' parameter is mandatory. Before calling a > > +register function you should either use hwmon_sanitize_name or > > +devm_hwmon_sanitize_name to replace any invalid characters with an > > I suggest to duplicate the name and replace ... You are now going to get code that passed in NULL when the kmalloc() fails. If 'sanitizing' the name is the correct thing to do then sanitize it when the copy is made into the allocated structure. (I'm assuming that the 'const char *name' parameter doesn't have to be persistent - that would be another bug just waiting to happen.) Seems really pointless to be do a kmalloc() just to pass a string into a function. David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
> 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. It is often not a literal string in the driver, but something based on the DT description of the hardware. Andrew
On 3/29/22 09:07, Michael Walle wrote: > 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. > > Signed-off-by: Michael Walle <michael@walle.cc> > --- > Documentation/hwmon/hwmon-kernel-api.rst | 9 ++++- > drivers/hwmon/hwmon.c | 49 ++++++++++++++++++++++++ > include/linux/hwmon.h | 3 ++ > 3 files changed, 60 insertions(+), 1 deletion(-) > > diff --git a/Documentation/hwmon/hwmon-kernel-api.rst b/Documentation/hwmon/hwmon-kernel-api.rst > index c41eb6108103..12f4a9bcef04 100644 > --- a/Documentation/hwmon/hwmon-kernel-api.rst > +++ b/Documentation/hwmon/hwmon-kernel-api.rst > @@ -50,6 +50,10 @@ register/unregister functions:: > > void devm_hwmon_device_unregister(struct device *dev); > > + char *hwmon_sanitize_name(const char *name); > + > + char *devm_hwmon_sanitize_name(struct device *dev, const char *name); > + > hwmon_device_register_with_groups registers a hardware monitoring device. > The first parameter of this function is a pointer to the parent device. > The name parameter is a pointer to the hwmon device name. The registration > @@ -93,7 +97,10 @@ removal would be too late. > > All supported hwmon device registration functions only accept valid device > names. Device names including invalid characters (whitespace, '*', or '-') > -will be rejected. The 'name' parameter is mandatory. > +will be rejected. The 'name' parameter is mandatory. Before calling a > +register function you should either use hwmon_sanitize_name or > +devm_hwmon_sanitize_name to replace any invalid characters with an > +underscore. That needs more details and deserves its own paragraph. Calling one of the functions is only necessary if the original name does or can include unsupported characters; an unconditional "should" is therefore a bit strong. Also, it is important to mention that the function duplicates the name, and that it is the responsibility of the caller to release the name if hwmon_sanitize_name() was called and the device is removed. Thanks, Guenter
From: Guenter Roeck > Sent: 30 March 2022 15:23 > On 3/29/22 09:07, Michael Walle wrote: > > 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. > > > > Signed-off-by: Michael Walle <michael@walle.cc> > > --- > > Documentation/hwmon/hwmon-kernel-api.rst | 9 ++++- > > drivers/hwmon/hwmon.c | 49 ++++++++++++++++++++++++ > > include/linux/hwmon.h | 3 ++ > > 3 files changed, 60 insertions(+), 1 deletion(-) > > > > diff --git a/Documentation/hwmon/hwmon-kernel-api.rst b/Documentation/hwmon/hwmon-kernel-api.rst > > index c41eb6108103..12f4a9bcef04 100644 > > --- a/Documentation/hwmon/hwmon-kernel-api.rst > > +++ b/Documentation/hwmon/hwmon-kernel-api.rst > > @@ -50,6 +50,10 @@ register/unregister functions:: > > > > void devm_hwmon_device_unregister(struct device *dev); > > > > + char *hwmon_sanitize_name(const char *name); > > + > > + char *devm_hwmon_sanitize_name(struct device *dev, const char *name); > > + > > hwmon_device_register_with_groups registers a hardware monitoring device. > > The first parameter of this function is a pointer to the parent device. > > The name parameter is a pointer to the hwmon device name. The registration > > @@ -93,7 +97,10 @@ removal would be too late. > > > > All supported hwmon device registration functions only accept valid device > > names. Device names including invalid characters (whitespace, '*', or '-') > > -will be rejected. The 'name' parameter is mandatory. > > +will be rejected. The 'name' parameter is mandatory. Before calling a > > +register function you should either use hwmon_sanitize_name or > > +devm_hwmon_sanitize_name to replace any invalid characters with an > > +underscore. > > That needs more details and deserves its own paragraph. Calling one of > the functions is only necessary if the original name does or can include > unsupported characters; an unconditional "should" is therefore a bit > strong. Also, it is important to mention that the function duplicates > the name, and that it is the responsibility of the caller to release > the name if hwmon_sanitize_name() was called and the device is removed. More worrying, and not documented, is that the buffer 'name' points to must persist. ISTM that the kmalloc() in __hwmon_device_register() should include space for a copy of the name. It can then do what it will with whatever is passed in. Oh yes, it has my 'favourite construct': if (!strlen(name)) ... (well str[strlen(str)] = 0 also happens!) David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
On Wed, Mar 30, 2022 at 10:11:39AM +0000, David Laight wrote: > From: Xu Yilun > > Sent: 30 March 2022 07:51 > > > > On Tue, Mar 29, 2022 at 06:07:26PM +0200, Michael Walle wrote: > > > 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. > > > > > > Signed-off-by: Michael Walle <michael@walle.cc> > > > --- > > > Documentation/hwmon/hwmon-kernel-api.rst | 9 ++++- > > > drivers/hwmon/hwmon.c | 49 ++++++++++++++++++++++++ > > > include/linux/hwmon.h | 3 ++ > > > 3 files changed, 60 insertions(+), 1 deletion(-) > > > > > > diff --git a/Documentation/hwmon/hwmon-kernel-api.rst b/Documentation/hwmon/hwmon-kernel-api.rst > > > index c41eb6108103..12f4a9bcef04 100644 > > > --- a/Documentation/hwmon/hwmon-kernel-api.rst > > > +++ b/Documentation/hwmon/hwmon-kernel-api.rst > > > @@ -50,6 +50,10 @@ register/unregister functions:: > > > > > > void devm_hwmon_device_unregister(struct device *dev); > > > > > > + char *hwmon_sanitize_name(const char *name); > > > + > > > + char *devm_hwmon_sanitize_name(struct device *dev, const char *name); > > > + > > > hwmon_device_register_with_groups registers a hardware monitoring device. > > > The first parameter of this function is a pointer to the parent device. > > > The name parameter is a pointer to the hwmon device name. The registration > > > @@ -93,7 +97,10 @@ removal would be too late. > > > > > > All supported hwmon device registration functions only accept valid device > > > names. Device names including invalid characters (whitespace, '*', or '-') > > > -will be rejected. The 'name' parameter is mandatory. > > > +will be rejected. The 'name' parameter is mandatory. Before calling a > > > +register function you should either use hwmon_sanitize_name or > > > +devm_hwmon_sanitize_name to replace any invalid characters with an > > > > I suggest to duplicate the name and replace ... > > You are now going to get code that passed in NULL when the kmalloc() fails. > If 'sanitizing' the name is the correct thing to do then sanitize it > when the copy is made into the allocated structure. Then the driver is unaware of the name change, which makes more confusing. > (I'm assuming that the 'const char *name' parameter doesn't have to > be persistent - that would be another bug just waiting to happen.) The hwmon core does require a persistent "name" parameter now. No name copy is made when hwmon dev register. > > Seems really pointless to be do a kmalloc() just to pass a string > into a function. Maybe we should not force a kmalloc() when the sanitizing is needed, let the driver decide whether to duplicate the string or not. Thanks, Yilun > > David > > - > Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK > Registration No: 1397386 (Wales)
On 3/30/22 07:50, David Laight wrote: > From: Guenter Roeck >> Sent: 30 March 2022 15:23 >> On 3/29/22 09:07, Michael Walle wrote: >>> 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. >>> >>> Signed-off-by: Michael Walle <michael@walle.cc> >>> --- >>> Documentation/hwmon/hwmon-kernel-api.rst | 9 ++++- >>> drivers/hwmon/hwmon.c | 49 ++++++++++++++++++++++++ >>> include/linux/hwmon.h | 3 ++ >>> 3 files changed, 60 insertions(+), 1 deletion(-) >>> >>> diff --git a/Documentation/hwmon/hwmon-kernel-api.rst b/Documentation/hwmon/hwmon-kernel-api.rst >>> index c41eb6108103..12f4a9bcef04 100644 >>> --- a/Documentation/hwmon/hwmon-kernel-api.rst >>> +++ b/Documentation/hwmon/hwmon-kernel-api.rst >>> @@ -50,6 +50,10 @@ register/unregister functions:: >>> >>> void devm_hwmon_device_unregister(struct device *dev); >>> >>> + char *hwmon_sanitize_name(const char *name); >>> + >>> + char *devm_hwmon_sanitize_name(struct device *dev, const char *name); >>> + >>> hwmon_device_register_with_groups registers a hardware monitoring device. >>> The first parameter of this function is a pointer to the parent device. >>> The name parameter is a pointer to the hwmon device name. The registration >>> @@ -93,7 +97,10 @@ removal would be too late. >>> >>> All supported hwmon device registration functions only accept valid device >>> names. Device names including invalid characters (whitespace, '*', or '-') >>> -will be rejected. The 'name' parameter is mandatory. >>> +will be rejected. The 'name' parameter is mandatory. Before calling a >>> +register function you should either use hwmon_sanitize_name or >>> +devm_hwmon_sanitize_name to replace any invalid characters with an >>> +underscore. >> >> That needs more details and deserves its own paragraph. Calling one of >> the functions is only necessary if the original name does or can include >> unsupported characters; an unconditional "should" is therefore a bit >> strong. Also, it is important to mention that the function duplicates >> the name, and that it is the responsibility of the caller to release >> the name if hwmon_sanitize_name() was called and the device is removed. > > More worrying, and not documented, is that the buffer 'name' points > to must persist. > You mean the name argument passed to the hwmon registration functions ? That has been the case forever, and I don't recall a single problem with it. If it disturbs you, please feel free to submit a patch adding more details to the documentation. I would not want to change the code and always copy the name because in almost all cases it _is_ a fixed string, and duplicating it would have no value. > ISTM that the kmalloc() in __hwmon_device_register() should include > space for a copy of the name. > It can then do what it will with whatever is passed in. > Whatever is passed in is what the user wants. Registration functions don't change the name. Providing a valid name is the responsibility of the caller. > Oh yes, it has my 'favourite construct': if (!strlen(name)) ... > (well str[strlen(str)] = 0 also happens!) > Sorry, I don't understand what the problem is here. Thanks, Guenter
On 3/30/22 07:51, Xu Yilun wrote: > On Wed, Mar 30, 2022 at 10:11:39AM +0000, David Laight wrote: >> From: Xu Yilun >>> Sent: 30 March 2022 07:51 >>> >>> On Tue, Mar 29, 2022 at 06:07:26PM +0200, Michael Walle wrote: >>>> 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. >>>> >>>> Signed-off-by: Michael Walle <michael@walle.cc> >>>> --- >>>> Documentation/hwmon/hwmon-kernel-api.rst | 9 ++++- >>>> drivers/hwmon/hwmon.c | 49 ++++++++++++++++++++++++ >>>> include/linux/hwmon.h | 3 ++ >>>> 3 files changed, 60 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/Documentation/hwmon/hwmon-kernel-api.rst b/Documentation/hwmon/hwmon-kernel-api.rst >>>> index c41eb6108103..12f4a9bcef04 100644 >>>> --- a/Documentation/hwmon/hwmon-kernel-api.rst >>>> +++ b/Documentation/hwmon/hwmon-kernel-api.rst >>>> @@ -50,6 +50,10 @@ register/unregister functions:: >>>> >>>> void devm_hwmon_device_unregister(struct device *dev); >>>> >>>> + char *hwmon_sanitize_name(const char *name); >>>> + >>>> + char *devm_hwmon_sanitize_name(struct device *dev, const char *name); >>>> + >>>> hwmon_device_register_with_groups registers a hardware monitoring device. >>>> The first parameter of this function is a pointer to the parent device. >>>> The name parameter is a pointer to the hwmon device name. The registration >>>> @@ -93,7 +97,10 @@ removal would be too late. >>>> >>>> All supported hwmon device registration functions only accept valid device >>>> names. Device names including invalid characters (whitespace, '*', or '-') >>>> -will be rejected. The 'name' parameter is mandatory. >>>> +will be rejected. The 'name' parameter is mandatory. Before calling a >>>> +register function you should either use hwmon_sanitize_name or >>>> +devm_hwmon_sanitize_name to replace any invalid characters with an >>> >>> I suggest to duplicate the name and replace ... >> >> You are now going to get code that passed in NULL when the kmalloc() fails. >> If 'sanitizing' the name is the correct thing to do then sanitize it >> when the copy is made into the allocated structure. > > Then the driver is unaware of the name change, which makes more > confusing. > >> (I'm assuming that the 'const char *name' parameter doesn't have to >> be persistent - that would be another bug just waiting to happen.) > > The hwmon core does require a persistent "name" parameter now. No name > copy is made when hwmon dev register. > >> >> Seems really pointless to be do a kmalloc() just to pass a string >> into a function. > > Maybe we should not force a kmalloc() when the sanitizing is needed, let > the driver decide whether to duplicate the string or not. > Drivers can do that today, and in all existing cases they do so (which is why I had suggested to handle the duplication in the convenience function in the first place). Drivers don't _have_ to use the provided convenience functions. At the same time, convenience functions should cover the most common use cases. Michael, let's just drop the changes outside drivers/hwmon from the series, and let's keep hwmon_is_bad_char() in the include file. Let's just document it, explaining its use case. Code outside drivers/hwmon can then be independently modified or not at a later time, based on driver author and/or maintainer preference. Thanks, Guenter
On Wed, Mar 30, 2022 at 08:23:35AM -0700, Guenter Roeck wrote: > Michael, let's just drop the changes outside drivers/hwmon from > the series, and let's keep hwmon_is_bad_char() in the include file. > Let's just document it, explaining its use case. Why? There hasn't been any objection to the change. All the discussion seems to be around the new function (this patch) rather than the actual conversions in drivers. I'm entirely in favour of cleaning this up - it irks me that we're doing exactly the same cleanup everywhere we have a hwmon. At the very least, I would be completely in favour of keeping the changes in the sfp and phy code.
Am 2022-03-31 16:45, schrieb Russell King (Oracle): > On Wed, Mar 30, 2022 at 08:23:35AM -0700, Guenter Roeck wrote: >> Michael, let's just drop the changes outside drivers/hwmon from >> the series, and let's keep hwmon_is_bad_char() in the include file. >> Let's just document it, explaining its use case. > > Why? There hasn't been any objection to the change. All the discussion > seems to be around the new function (this patch) rather than the actual > conversions in drivers. > > I'm entirely in favour of cleaning this up - it irks me that we're > doing > exactly the same cleanup everywhere we have a hwmon. > > At the very least, I would be completely in favour of keeping the > changes in the sfp and phy code. FWIW, my plan was to send the hwmon patches first, by then my other series (the polynomial_calc() one) will also be ready to be picked. Then I'd ask Guenter for a stable branch with these two series which hopefully get merged into net-next. Then I can repost the missing patches on net-next along with the new sensors support for the GPY and LAN8814 PHYs. For the last patch, if it should be applied or not, or when, that will be up to Guenter then. -michael
On Thu, Mar 31, 2022 at 04:51:47PM +0200, Michael Walle wrote: > Am 2022-03-31 16:45, schrieb Russell King (Oracle): > > On Wed, Mar 30, 2022 at 08:23:35AM -0700, Guenter Roeck wrote: > > > Michael, let's just drop the changes outside drivers/hwmon from > > > the series, and let's keep hwmon_is_bad_char() in the include file. > > > Let's just document it, explaining its use case. > > > > Why? There hasn't been any objection to the change. All the discussion > > seems to be around the new function (this patch) rather than the actual > > conversions in drivers. > > > > I'm entirely in favour of cleaning this up - it irks me that we're doing > > exactly the same cleanup everywhere we have a hwmon. > > > > At the very least, I would be completely in favour of keeping the > > changes in the sfp and phy code. > > FWIW, my plan was to send the hwmon patches first, by then my other > series (the polynomial_calc() one) will also be ready to be picked. > Then I'd ask Guenter for a stable branch with these two series which > hopefully get merged into net-next. Then I can repost the missing > patches on net-next along with the new sensors support for the GPY > and LAN8814 PHYs. Okay, that's fine. It just sounded like the conversion of other drivers outside drivers/hwmon was being dropped. Note that there's another "sanitisation" of hwmon names in drivers/net/phy/marvell.c - that converts any non-alnum character to an underscore. Not sure why the different approach was chosen there.
Am 2022-03-31 16:58, schrieb Russell King (Oracle): > Note that there's another "sanitisation" of hwmon names in > drivers/net/phy/marvell.c - that converts any non-alnum character to > an underscore. Not sure why the different approach was chosen there. I saw that, but I didn't touch it because it would change the name. I guess that will be an incompatible userspace change. -michael
On 3/31/22 07:58, Russell King (Oracle) wrote: > On Thu, Mar 31, 2022 at 04:51:47PM +0200, Michael Walle wrote: >> Am 2022-03-31 16:45, schrieb Russell King (Oracle): >>> On Wed, Mar 30, 2022 at 08:23:35AM -0700, Guenter Roeck wrote: >>>> Michael, let's just drop the changes outside drivers/hwmon from >>>> the series, and let's keep hwmon_is_bad_char() in the include file. >>>> Let's just document it, explaining its use case. >>> >>> Why? There hasn't been any objection to the change. All the discussion >>> seems to be around the new function (this patch) rather than the actual >>> conversions in drivers. >>> >>> I'm entirely in favour of cleaning this up - it irks me that we're doing >>> exactly the same cleanup everywhere we have a hwmon. >>> >>> At the very least, I would be completely in favour of keeping the >>> changes in the sfp and phy code. >> >> FWIW, my plan was to send the hwmon patches first, by then my other >> series (the polynomial_calc() one) will also be ready to be picked. >> Then I'd ask Guenter for a stable branch with these two series which >> hopefully get merged into net-next. Then I can repost the missing >> patches on net-next along with the new sensors support for the GPY >> and LAN8814 PHYs. > > Okay, that's fine. It just sounded like the conversion of other drivers > outside drivers/hwmon was being dropped. > Not dropped, just disconnected. From hwmon perspective, we want a certain set of characters to be dropped or replaced. Also, from hwmon perspective, it makes sense to have the helper function allocate the replacement data. There was disagreement about which characters should be replaced, and if the helper function should allocate the replacement string or not. I have no intention to change the set of characters without good reason, and I feel quite strongly about allocating the replacement in the helper function. Since potential callers don't _have_ to use the helper and don't _have_ to provide valid names (and are responsible for the consequences), I would like that discussion to be separate from hwmon changes. > Note that there's another "sanitisation" of hwmon names in > drivers/net/phy/marvell.c - that converts any non-alnum character to > an underscore. Not sure why the different approach was chosen there. > It actually drops non-alphanumeric characters. The name is derived from the phy device name, which I think is derived from the name field in struct phy_driver. That includes spaces and '(', ')'. I honestly have no idea what libsensors would do with '(' and ')'. Either case, even if that would create a hiccup in libsensors and we would add '(' and ')' to the 'forbidden' list of characters, the fact that the code doesn't replace but drop non-alphanumeric characters means it won't be able to use a helper anyway since that would result in a hwmon 'name' attribute change and thus not be backward compatible. Besides, "Marvell_88E1111__Finisar_" would look a bit odd anyway, and "Marvell88E1111Finisar" may be at least slightly better. Guenter
diff --git a/Documentation/hwmon/hwmon-kernel-api.rst b/Documentation/hwmon/hwmon-kernel-api.rst index c41eb6108103..12f4a9bcef04 100644 --- a/Documentation/hwmon/hwmon-kernel-api.rst +++ b/Documentation/hwmon/hwmon-kernel-api.rst @@ -50,6 +50,10 @@ register/unregister functions:: void devm_hwmon_device_unregister(struct device *dev); + char *hwmon_sanitize_name(const char *name); + + char *devm_hwmon_sanitize_name(struct device *dev, const char *name); + hwmon_device_register_with_groups registers a hardware monitoring device. The first parameter of this function is a pointer to the parent device. The name parameter is a pointer to the hwmon device name. The registration @@ -93,7 +97,10 @@ removal would be too late. All supported hwmon device registration functions only accept valid device names. Device names including invalid characters (whitespace, '*', or '-') -will be rejected. The 'name' parameter is mandatory. +will be rejected. The 'name' parameter is mandatory. Before calling a +register function you should either use hwmon_sanitize_name or +devm_hwmon_sanitize_name to replace any invalid characters with an +underscore. Using devm_hwmon_device_register_with_info() -------------------------------------------- diff --git a/drivers/hwmon/hwmon.c b/drivers/hwmon/hwmon.c index 989e2c8496dd..619ef9f9a16e 100644 --- a/drivers/hwmon/hwmon.c +++ b/drivers/hwmon/hwmon.c @@ -1057,6 +1057,55 @@ void devm_hwmon_device_unregister(struct device *dev) } EXPORT_SYMBOL_GPL(devm_hwmon_device_unregister); +static char *__hwmon_sanitize_name(struct device *dev, const char *old_name) +{ + char *name, *p; + + if (dev) + name = devm_kstrdup(dev, old_name, GFP_KERNEL); + else + name = kstrdup(old_name, GFP_KERNEL); + if (!name) + return NULL; + + for (p = name; *p; p++) + if (hwmon_is_bad_char(*p)) + *p = '_'; + + return name; +} + +/** + * hwmon_sanitize_name - Replaces invalid characters in a hwmon name + * @name: NUL-terminated name + * + * Allocates a new string where any invalid characters will be replaced + * by an underscore. + * + * Returns newly allocated name or %NULL in case of error. + */ +char *hwmon_sanitize_name(const char *name) +{ + return __hwmon_sanitize_name(NULL, name); +} +EXPORT_SYMBOL_GPL(hwmon_sanitize_name); + +/** + * devm_hwmon_sanitize_name - resource managed hwmon_sanitize_name() + * @dev: device to allocate memory for + * @name: NUL-terminated name + * + * Allocates a new string where any invalid characters will be replaced + * by an underscore. + * + * Returns newly allocated name or %NULL in case of error. + */ +char *devm_hwmon_sanitize_name(struct device *dev, const char *name) +{ + return __hwmon_sanitize_name(dev, name); +} +EXPORT_SYMBOL_GPL(devm_hwmon_sanitize_name); + static void __init hwmon_pci_quirks(void) { #if defined CONFIG_X86 && defined CONFIG_PCI diff --git a/include/linux/hwmon.h b/include/linux/hwmon.h index eba380b76d15..4efaf06fd2b8 100644 --- a/include/linux/hwmon.h +++ b/include/linux/hwmon.h @@ -461,6 +461,9 @@ void devm_hwmon_device_unregister(struct device *dev); int hwmon_notify_event(struct device *dev, enum hwmon_sensor_types type, u32 attr, int channel); +char *hwmon_sanitize_name(const char *name); +char *devm_hwmon_sanitize_name(struct device *dev, const char *name); + /** * hwmon_is_bad_char - Is the char invalid in a hwmon name * @ch: the char to be considered
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. Signed-off-by: Michael Walle <michael@walle.cc> --- Documentation/hwmon/hwmon-kernel-api.rst | 9 ++++- drivers/hwmon/hwmon.c | 49 ++++++++++++++++++++++++ include/linux/hwmon.h | 3 ++ 3 files changed, 60 insertions(+), 1 deletion(-)