[PATCH net-next 0/2] lib: string: add strreplace_nonalnum

[PATCH net-next 0/2] lib: string: add strreplace_nonalnum

[Heiner Kallweit]

Add a new function strreplace_nonalnum that replaces all
non-alphanumeric characters. Such functionality is needed e.g. when a
string is supposed to be used in a file name. If '\0' is given as new
character then non-alphanumeric characters are cut. 

There doesn't seem to be a maintainer or mailing list for lib/string.c
Therefore I hope it's ok to submit this through the netdev tree.

Heiner Kallweit (2):
  lib: string: add strreplace_nonalnum
  net: phy: aquantia: use new function strreplace_nonalnum

 drivers/net/phy/aquantia_hwmon.c | 10 +---------
 include/linux/string.h           |  1 +
 lib/string.c                     | 27 +++++++++++++++++++++++++++
 3 files changed, 29 insertions(+), 9 deletions(-)

-- 
2.21.0

[PATCH net-next 1/2] lib: string: add strreplace_nonalnum

[Heiner Kallweit]

Add a new function strreplace_nonalnum that replaces all
non-alphanumeric characters. Such functionality is needed e.g. when a
string is supposed to be used in a sysfs file name. If '\0' is given
as new character then non-alphanumeric characters are cut. 

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 include/linux/string.h |  1 +
 lib/string.c           | 27 +++++++++++++++++++++++++++
 2 files changed, 28 insertions(+)

diff --git a/include/linux/string.h b/include/linux/string.h
index 7927b875f..d827b0b0f 100644
--- a/include/linux/string.h
+++ b/include/linux/string.h
@@ -169,6 +169,7 @@ static inline void memcpy_flushcache(void *dst, const void *src, size_t cnt)
 #endif
 void *memchr_inv(const void *s, int c, size_t n);
 char *strreplace(char *s, char old, char new);
+char *strreplace_nonalnum(char *s, char new);
 
 extern void kfree_const(const void *x);
 
diff --git a/lib/string.c b/lib/string.c
index 38e4ca08e..f2b1baf96 100644
--- a/lib/string.c
+++ b/lib/string.c
@@ -1047,6 +1047,33 @@ char *strreplace(char *s, char old, char new)
 }
 EXPORT_SYMBOL(strreplace);
 
+/**
+ * strreplace_nonalnum - Replace all non-alphanumeric characters in a string.
+ * @s: The string to operate on.
+ * @new: The character non-alphanumeric characters are replaced with.
+ *
+ * If new is '\0' then non-alphanumeric characters are cut.
+ *
+ * Returns pointer to the nul byte at the end of the modified string.
+ */
+char *strreplace_nonalnum(char *s, char new)
+{
+	char *p = s;
+
+	for (; *s; ++s)
+		if (isalnum(*s)) {
+			if (p != s)
+				*p = *s;
+			++p;
+		} else if (new) {
+			*p++ = new;
+		}
+	*p = '\0';
+
+	return p;
+}
+EXPORT_SYMBOL(strreplace_nonalnum);
+
 void fortify_panic(const char *name)
 {
 	pr_emerg("detected buffer overflow in %s\n", name);
-- 
2.21.0

[PATCH net-next 2/2] net: phy: aquantia: use new function strreplace_nonalnum

[Heiner Kallweit]

Use new function strreplace_nonalnum to simplify the code.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/net/phy/aquantia_hwmon.c | 10 +---------
 1 file changed, 1 insertion(+), 9 deletions(-)

diff --git a/drivers/net/phy/aquantia_hwmon.c b/drivers/net/phy/aquantia_hwmon.c
index 19c4c280a..d47b3ed16 100644
--- a/drivers/net/phy/aquantia_hwmon.c
+++ b/drivers/net/phy/aquantia_hwmon.c
@@ -226,20 +226,12 @@ int aqr_hwmon_probe(struct phy_device *phydev)
 	struct device *dev = &phydev->mdio.dev;
 	struct device *hwmon_dev;
 	char *hwmon_name;
-	int i, j;
 
 	hwmon_name = devm_kstrdup(dev, dev_name(dev), GFP_KERNEL);
 	if (!hwmon_name)
 		return -ENOMEM;
 
-	for (i = j = 0; hwmon_name[i]; i++) {
-		if (isalnum(hwmon_name[i])) {
-			if (i != j)
-				hwmon_name[j] = hwmon_name[i];
-			j++;
-		}
-	}
-	hwmon_name[j] = '\0';
+	strreplace_nonalnum(hwmon_name, 0);
 
 	hwmon_dev = devm_hwmon_device_register_with_info(dev, hwmon_name,
 					phydev, &aqr_hwmon_chip_info, NULL);
-- 
2.21.0

Re: [PATCH net-next 2/2] net: phy: aquantia: use new function strreplace_nonalnum

[Andrew Lunn]

On Sun, Mar 03, 2019 at 06:21:53PM +0100, Heiner Kallweit wrote:
> Use new function strreplace_nonalnum to simplify the code.

Hi Heiner

Both Marvell PHY drivers could use this.

sfp.c has a variant of this as well.

     Andrew

Re: [PATCH net-next 0/2] lib: string: add strreplace_nonalnum

[Andrew Lunn]

On Sun, Mar 03, 2019 at 06:19:35PM +0100, Heiner Kallweit wrote:
> There doesn't seem to be a maintainer or mailing list for lib/string.c
> Therefore I hope it's ok to submit this through the netdev tree.

Hi Heiner

It is probably a good idea to Cc:

~/linux$ ./scripts/get_maintainer.pl -f lib/string.c
Alexander Shishkin <alexander.shishkin@linux.intel.com> (commit_signer:2/2=100%,authored:2/2=100%,added_lines:31/31=100%,removed_lines:30/30=100%)
Greg Kroah-Hartman <gregkh@linuxfoundation.org> (commit_signer:2/2=100%)
Andy Shevchenko <andriy.shevchenko@linux.intel.com> (commit_signer:1/2=50%)
linux-kernel@vger.kernel.org (open list)

And also Guenter Roeck who does most of the HWMON maintenance work.

    Andrew

Re: [PATCH net-next 0/2] lib: string: add strreplace_nonalnum

[Heiner Kallweit]

On 03.03.2019 18:34, Andrew Lunn wrote:
> On Sun, Mar 03, 2019 at 06:19:35PM +0100, Heiner Kallweit wrote:
>> There doesn't seem to be a maintainer or mailing list for lib/string.c
>> Therefore I hope it's ok to submit this through the netdev tree.
> 
> Hi Heiner
> 
> It is probably a good idea to Cc:
> 
> ~/linux$ ./scripts/get_maintainer.pl -f lib/string.c
> Alexander Shishkin <alexander.shishkin@linux.intel.com> (commit_signer:2/2=100%,authored:2/2=100%,added_lines:31/31=100%,removed_lines:30/30=100%)
> Greg Kroah-Hartman <gregkh@linuxfoundation.org> (commit_signer:2/2=100%)
> Andy Shevchenko <andriy.shevchenko@linux.intel.com> (commit_signer:1/2=50%)
> linux-kernel@vger.kernel.org (open list)
> 

Strange, when I use get_maintainer.pl with the patch I get the following only:

[root@hkvirt linux-next]# scripts/get_maintainer.pl 0001-lib-string-add-strreplace_nonalnum.patch
Stephen Rothwell <sfr@canb.auug.org.au> (commit_signer:1/1=100%,authored:1/1=100%,added_lines:479/479=100%,added_lines:1055/1055=100%)
linux-kernel@vger.kernel.org (open list)

Therefore I sent it to the usual suspects only.

> And also Guenter Roeck who does most of the HWMON maintenance work.
> 
Right.

>     Andrew
> 
Heiner

Re: [PATCH net-next 2/2] net: phy: aquantia: use new function strreplace_nonalnum

[Heiner Kallweit]

On 03.03.2019 18:31, Andrew Lunn wrote:
> On Sun, Mar 03, 2019 at 06:21:53PM +0100, Heiner Kallweit wrote:
>> Use new function strreplace_nonalnum to simplify the code.
> 
> Hi Heiner
> 
> Both Marvell PHY drivers could use this.
> 
> sfp.c has a variant of this as well.
> 
Thanks! My plan was to provide a first user so that the new
function gets accepted, and then look for more places where it
could be used. Most likely there are few HWMON drivers also
open-coding this functionality.

>      Andrew
> 
Heiner

Fwd: [PATCH net-next 1/2] lib: string: add strreplace_nonalnum

[Heiner Kallweit]

I submitted this through the netdev tree, maybe relevant for you as well.
See also here: https://marc.info/?t=155103900100003&r=1&w=2

-------- Forwarded Message --------
Subject: [PATCH net-next 1/2] lib: string: add strreplace_nonalnum
Date: Sun, 3 Mar 2019 18:20:50 +0100
From: Heiner Kallweit <hkallweit1@gmail.com>
To: Florian Fainelli <f.fainelli@gmail.com>, Andrew Lunn <andrew@lunn.ch>, David Miller <davem@davemloft.net>
CC: netdev@vger.kernel.org <netdev@vger.kernel.org>

Add a new function strreplace_nonalnum that replaces all
non-alphanumeric characters. Such functionality is needed e.g. when a
string is supposed to be used in a sysfs file name. If '\0' is given
as new character then non-alphanumeric characters are cut. 

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 include/linux/string.h |  1 +
 lib/string.c           | 27 +++++++++++++++++++++++++++
 2 files changed, 28 insertions(+)

diff --git a/include/linux/string.h b/include/linux/string.h
index 7927b875f..d827b0b0f 100644
--- a/include/linux/string.h
+++ b/include/linux/string.h
@@ -169,6 +169,7 @@ static inline void memcpy_flushcache(void *dst, const void *src, size_t cnt)
 #endif
 void *memchr_inv(const void *s, int c, size_t n);
 char *strreplace(char *s, char old, char new);
+char *strreplace_nonalnum(char *s, char new);
 
 extern void kfree_const(const void *x);
 
diff --git a/lib/string.c b/lib/string.c
index 38e4ca08e..f2b1baf96 100644
--- a/lib/string.c
+++ b/lib/string.c
@@ -1047,6 +1047,33 @@ char *strreplace(char *s, char old, char new)
 }
 EXPORT_SYMBOL(strreplace);
 
+/**
+ * strreplace_nonalnum - Replace all non-alphanumeric characters in a string.
+ * @s: The string to operate on.
+ * @new: The character non-alphanumeric characters are replaced with.
+ *
+ * If new is '\0' then non-alphanumeric characters are cut.
+ *
+ * Returns pointer to the nul byte at the end of the modified string.
+ */
+char *strreplace_nonalnum(char *s, char new)
+{
+	char *p = s;
+
+	for (; *s; ++s)
+		if (isalnum(*s)) {
+			if (p != s)
+				*p = *s;
+			++p;
+		} else if (new) {
+			*p++ = new;
+		}
+	*p = '\0';
+
+	return p;
+}
+EXPORT_SYMBOL(strreplace_nonalnum);
+
 void fortify_panic(const char *name)
 {
 	pr_emerg("detected buffer overflow in %s\n", name);
-- 
2.21.0

Re: Fwd: [PATCH net-next 1/2] lib: string: add strreplace_nonalnum

[Greg Kroah-Hartman]

On Sun, Mar 03, 2019 at 06:47:32PM +0100, Heiner Kallweit wrote:
> I submitted this through the netdev tree, maybe relevant for you as well.
> See also here: https://marc.info/?t=155103900100003&r=1&w=2
> 
> -------- Forwarded Message --------
> Subject: [PATCH net-next 1/2] lib: string: add strreplace_nonalnum
> Date: Sun, 3 Mar 2019 18:20:50 +0100
> From: Heiner Kallweit <hkallweit1@gmail.com>
> To: Florian Fainelli <f.fainelli@gmail.com>, Andrew Lunn <andrew@lunn.ch>, David Miller <davem@davemloft.net>
> CC: netdev@vger.kernel.org <netdev@vger.kernel.org>
> 
> Add a new function strreplace_nonalnum that replaces all
> non-alphanumeric characters. Such functionality is needed e.g. when a
> string is supposed to be used in a sysfs file name. If '\0' is given
> as new character then non-alphanumeric characters are cut. 

sysfs doesn't have any such requirements, it can use whatever you want
to give it for a filename.

So don't create a random kernel function for sysfs please.

> 
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> ---
>  include/linux/string.h |  1 +
>  lib/string.c           | 27 +++++++++++++++++++++++++++
>  2 files changed, 28 insertions(+)
> 
> diff --git a/include/linux/string.h b/include/linux/string.h
> index 7927b875f..d827b0b0f 100644
> --- a/include/linux/string.h
> +++ b/include/linux/string.h
> @@ -169,6 +169,7 @@ static inline void memcpy_flushcache(void *dst, const void *src, size_t cnt)
>  #endif
>  void *memchr_inv(const void *s, int c, size_t n);
>  char *strreplace(char *s, char old, char new);
> +char *strreplace_nonalnum(char *s, char new);
>  
>  extern void kfree_const(const void *x);
>  
> diff --git a/lib/string.c b/lib/string.c
> index 38e4ca08e..f2b1baf96 100644
> --- a/lib/string.c
> +++ b/lib/string.c
> @@ -1047,6 +1047,33 @@ char *strreplace(char *s, char old, char new)
>  }
>  EXPORT_SYMBOL(strreplace);
>  
> +/**
> + * strreplace_nonalnum - Replace all non-alphanumeric characters in a string.
> + * @s: The string to operate on.
> + * @new: The character non-alphanumeric characters are replaced with.
> + *
> + * If new is '\0' then non-alphanumeric characters are cut.
> + *
> + * Returns pointer to the nul byte at the end of the modified string.

Why do you need to point to the end of the string?


> + */
> +char *strreplace_nonalnum(char *s, char new)
> +{
> +	char *p = s;
> +
> +	for (; *s; ++s)
> +		if (isalnum(*s)) {
> +			if (p != s)
> +				*p = *s;
> +			++p;
> +		} else if (new) {
> +			*p++ = new;
> +		}
> +	*p = '\0';

No max length?  No error checking?  Surely we can do better, see the
long thread on the kernel-hardnening list about string functions please.

greg k-h

Re: Fwd: [PATCH net-next 1/2] lib: string: add strreplace_nonalnum

[Heiner Kallweit]

On 03.03.2019 18:55, Greg Kroah-Hartman wrote:
> On Sun, Mar 03, 2019 at 06:47:32PM +0100, Heiner Kallweit wrote:
>> I submitted this through the netdev tree, maybe relevant for you as well.
>> See also here: https://marc.info/?t=155103900100003&r=1&w=2
>>
>> -------- Forwarded Message --------
>> Subject: [PATCH net-next 1/2] lib: string: add strreplace_nonalnum
>> Date: Sun, 3 Mar 2019 18:20:50 +0100
>> From: Heiner Kallweit <hkallweit1@gmail.com>
>> To: Florian Fainelli <f.fainelli@gmail.com>, Andrew Lunn <andrew@lunn.ch>, David Miller <davem@davemloft.net>
>> CC: netdev@vger.kernel.org <netdev@vger.kernel.org>
>>
>> Add a new function strreplace_nonalnum that replaces all
>> non-alphanumeric characters. Such functionality is needed e.g. when a
>> string is supposed to be used in a sysfs file name. If '\0' is given
>> as new character then non-alphanumeric characters are cut. 
> 
> sysfs doesn't have any such requirements, it can use whatever you want
> to give it for a filename.
> 
Even a slash?
HWMON drivers is an example where such functionality occurs open-coded.

> So don't create a random kernel function for sysfs please.
> 
>>
>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
>> ---
>>  include/linux/string.h |  1 +
>>  lib/string.c           | 27 +++++++++++++++++++++++++++
>>  2 files changed, 28 insertions(+)
>>
>> diff --git a/include/linux/string.h b/include/linux/string.h
>> index 7927b875f..d827b0b0f 100644
>> --- a/include/linux/string.h
>> +++ b/include/linux/string.h
>> @@ -169,6 +169,7 @@ static inline void memcpy_flushcache(void *dst, const void *src, size_t cnt)
>>  #endif
>>  void *memchr_inv(const void *s, int c, size_t n);
>>  char *strreplace(char *s, char old, char new);
>> +char *strreplace_nonalnum(char *s, char new);
>>  
>>  extern void kfree_const(const void *x);
>>  
>> diff --git a/lib/string.c b/lib/string.c
>> index 38e4ca08e..f2b1baf96 100644
>> --- a/lib/string.c
>> +++ b/lib/string.c
>> @@ -1047,6 +1047,33 @@ char *strreplace(char *s, char old, char new)
>>  }
>>  EXPORT_SYMBOL(strreplace);
>>  
>> +/**
>> + * strreplace_nonalnum - Replace all non-alphanumeric characters in a string.
>> + * @s: The string to operate on.
>> + * @new: The character non-alphanumeric characters are replaced with.
>> + *
>> + * If new is '\0' then non-alphanumeric characters are cut.
>> + *
>> + * Returns pointer to the nul byte at the end of the modified string.
> 
> Why do you need to point to the end of the string?
> 
I don't have to. I just tried to keep as close as possible to the original
strreplace().

> 
>> + */
>> +char *strreplace_nonalnum(char *s, char new)
>> +{
>> +	char *p = s;
>> +
>> +	for (; *s; ++s)
>> +		if (isalnum(*s)) {
>> +			if (p != s)
>> +				*p = *s;
>> +			++p;
>> +		} else if (new) {
>> +			*p++ = new;
>> +		}
>> +	*p = '\0';
> 
> No max length?  No error checking?  Surely we can do better, see the
> long thread on the kernel-hardnening list about string functions please.
> 
As for the review comment before, I basically used strreplace() as
template and extended it. What you say about hardening is right,
but it's open for quite a few functions, isn't it?
I'll have a look at the mail thread you mentioned.

> greg k-h
> 
Heiner

Re: Fwd: [PATCH net-next 1/2] lib: string: add strreplace_nonalnum

[Greg Kroah-Hartman]

On Sun, Mar 03, 2019 at 07:04:21PM +0100, Heiner Kallweit wrote:
> On 03.03.2019 18:55, Greg Kroah-Hartman wrote:
> > On Sun, Mar 03, 2019 at 06:47:32PM +0100, Heiner Kallweit wrote:
> >> I submitted this through the netdev tree, maybe relevant for you as well.
> >> See also here: https://marc.info/?t=155103900100003&r=1&w=2
> >>
> >> -------- Forwarded Message --------
> >> Subject: [PATCH net-next 1/2] lib: string: add strreplace_nonalnum
> >> Date: Sun, 3 Mar 2019 18:20:50 +0100
> >> From: Heiner Kallweit <hkallweit1@gmail.com>
> >> To: Florian Fainelli <f.fainelli@gmail.com>, Andrew Lunn <andrew@lunn.ch>, David Miller <davem@davemloft.net>
> >> CC: netdev@vger.kernel.org <netdev@vger.kernel.org>
> >>
> >> Add a new function strreplace_nonalnum that replaces all
> >> non-alphanumeric characters. Such functionality is needed e.g. when a
> >> string is supposed to be used in a sysfs file name. If '\0' is given
> >> as new character then non-alphanumeric characters are cut. 
> > 
> > sysfs doesn't have any such requirements, it can use whatever you want
> > to give it for a filename.
> > 
> Even a slash?

Is a slash an illegal character for a file to have?  It's up to the vfs
to care about this, don't force random parts of the kernel to care :)

> HWMON drivers is an example where such functionality occurs open-coded.

Is that data coming from userspace or from a kernel driver?

thanks,

greg k-h

Re: Fwd: [PATCH net-next 1/2] lib: string: add strreplace_nonalnum

[Heiner Kallweit]

On 03.03.2019 19:15, Greg Kroah-Hartman wrote:
> On Sun, Mar 03, 2019 at 07:04:21PM +0100, Heiner Kallweit wrote:
>> On 03.03.2019 18:55, Greg Kroah-Hartman wrote:
>>> On Sun, Mar 03, 2019 at 06:47:32PM +0100, Heiner Kallweit wrote:
>>>> I submitted this through the netdev tree, maybe relevant for you as well.
>>>> See also here: https://marc.info/?t=155103900100003&r=1&w=2
>>>>
>>>> -------- Forwarded Message --------
>>>> Subject: [PATCH net-next 1/2] lib: string: add strreplace_nonalnum
>>>> Date: Sun, 3 Mar 2019 18:20:50 +0100
>>>> From: Heiner Kallweit <hkallweit1@gmail.com>
>>>> To: Florian Fainelli <f.fainelli@gmail.com>, Andrew Lunn <andrew@lunn.ch>, David Miller <davem@davemloft.net>
>>>> CC: netdev@vger.kernel.org <netdev@vger.kernel.org>
>>>>
>>>> Add a new function strreplace_nonalnum that replaces all
>>>> non-alphanumeric characters. Such functionality is needed e.g. when a
>>>> string is supposed to be used in a sysfs file name. If '\0' is given
>>>> as new character then non-alphanumeric characters are cut. 
>>>
>>> sysfs doesn't have any such requirements, it can use whatever you want
>>> to give it for a filename.
>>>
>> Even a slash?
> 
> Is a slash an illegal character for a file to have?  It's up to the vfs
> to care about this, don't force random parts of the kernel to care :)
> 
>> HWMON drivers is an example where such functionality occurs open-coded.
> 
> Is that data coming from userspace or from a kernel driver?
> 
Usually from a kernel driver. That's what
Documentation/hwmon/hwmon-kernel-api.txt says:

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.

The hwmon subsystem has an own function to check for such characters:
hwmon_is_bad_char()


> thanks,
> 
> greg k-h
> 
Heiner

Re: [PATCH net-next 0/2] lib: string: add strreplace_nonalnum

[Heiner Kallweit]

On 03.03.2019 18:19, Heiner Kallweit wrote:
> Add a new function strreplace_nonalnum that replaces all
> non-alphanumeric characters. Such functionality is needed e.g. when a
> string is supposed to be used in a file name. If '\0' is given as new
> character then non-alphanumeric characters are cut. 
> 
> There doesn't seem to be a maintainer or mailing list for lib/string.c
> Therefore I hope it's ok to submit this through the netdev tree.
> 
> Heiner Kallweit (2):
>   lib: string: add strreplace_nonalnum
>   net: phy: aquantia: use new function strreplace_nonalnum
> 
>  drivers/net/phy/aquantia_hwmon.c | 10 +---------
>  include/linux/string.h           |  1 +
>  lib/string.c                     | 27 +++++++++++++++++++++++++++
>  3 files changed, 29 insertions(+), 9 deletions(-)
> 
Greg still has some concerns, let's see what the outcome of this
discussion is.
https://lkml.org/lkml/2019/3/3/151

Re: Fwd: [PATCH net-next 1/2] lib: string: add strreplace_nonalnum

[Greg Kroah-Hartman]

On Sun, Mar 03, 2019 at 07:32:53PM +0100, Heiner Kallweit wrote:
> On 03.03.2019 19:15, Greg Kroah-Hartman wrote:
> > On Sun, Mar 03, 2019 at 07:04:21PM +0100, Heiner Kallweit wrote:
> >> On 03.03.2019 18:55, Greg Kroah-Hartman wrote:
> >>> On Sun, Mar 03, 2019 at 06:47:32PM +0100, Heiner Kallweit wrote:
> >>>> I submitted this through the netdev tree, maybe relevant for you as well.
> >>>> See also here: https://marc.info/?t=155103900100003&r=1&w=2
> >>>>
> >>>> -------- Forwarded Message --------
> >>>> Subject: [PATCH net-next 1/2] lib: string: add strreplace_nonalnum
> >>>> Date: Sun, 3 Mar 2019 18:20:50 +0100
> >>>> From: Heiner Kallweit <hkallweit1@gmail.com>
> >>>> To: Florian Fainelli <f.fainelli@gmail.com>, Andrew Lunn <andrew@lunn.ch>, David Miller <davem@davemloft.net>
> >>>> CC: netdev@vger.kernel.org <netdev@vger.kernel.org>
> >>>>
> >>>> Add a new function strreplace_nonalnum that replaces all
> >>>> non-alphanumeric characters. Such functionality is needed e.g. when a
> >>>> string is supposed to be used in a sysfs file name. If '\0' is given
> >>>> as new character then non-alphanumeric characters are cut. 
> >>>
> >>> sysfs doesn't have any such requirements, it can use whatever you want
> >>> to give it for a filename.
> >>>
> >> Even a slash?
> > 
> > Is a slash an illegal character for a file to have?  It's up to the vfs
> > to care about this, don't force random parts of the kernel to care :)
> > 
> >> HWMON drivers is an example where such functionality occurs open-coded.
> > 
> > Is that data coming from userspace or from a kernel driver?
> > 
> Usually from a kernel driver. That's what
> Documentation/hwmon/hwmon-kernel-api.txt says:

Usually?  So userspace can set the name?

> 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.
> 
> The hwmon subsystem has an own function to check for such characters:
> hwmon_is_bad_char()

It looks like hwmon is the only thing that cares about this then, why
do you want to make this a common function?

thanks,

greg k-h

Re: Fwd: [PATCH net-next 1/2] lib: string: add strreplace_nonalnum

[Heiner Kallweit]

On 03.03.2019 19:41, Greg Kroah-Hartman wrote:
> On Sun, Mar 03, 2019 at 07:32:53PM +0100, Heiner Kallweit wrote:
>> On 03.03.2019 19:15, Greg Kroah-Hartman wrote:
>>> On Sun, Mar 03, 2019 at 07:04:21PM +0100, Heiner Kallweit wrote:
>>>> On 03.03.2019 18:55, Greg Kroah-Hartman wrote:
>>>>> On Sun, Mar 03, 2019 at 06:47:32PM +0100, Heiner Kallweit wrote:
>>>>>> I submitted this through the netdev tree, maybe relevant for you as well.
>>>>>> See also here: https://marc.info/?t=155103900100003&r=1&w=2
>>>>>>
>>>>>> -------- Forwarded Message --------
>>>>>> Subject: [PATCH net-next 1/2] lib: string: add strreplace_nonalnum
>>>>>> Date: Sun, 3 Mar 2019 18:20:50 +0100
>>>>>> From: Heiner Kallweit <hkallweit1@gmail.com>
>>>>>> To: Florian Fainelli <f.fainelli@gmail.com>, Andrew Lunn <andrew@lunn.ch>, David Miller <davem@davemloft.net>
>>>>>> CC: netdev@vger.kernel.org <netdev@vger.kernel.org>
>>>>>>
>>>>>> Add a new function strreplace_nonalnum that replaces all
>>>>>> non-alphanumeric characters. Such functionality is needed e.g. when a
>>>>>> string is supposed to be used in a sysfs file name. If '\0' is given
>>>>>> as new character then non-alphanumeric characters are cut. 
>>>>>
>>>>> sysfs doesn't have any such requirements, it can use whatever you want
>>>>> to give it for a filename.
>>>>>
>>>> Even a slash?
>>>
>>> Is a slash an illegal character for a file to have?  It's up to the vfs
>>> to care about this, don't force random parts of the kernel to care :)
>>>
>>>> HWMON drivers is an example where such functionality occurs open-coded.
>>>
>>> Is that data coming from userspace or from a kernel driver?
>>>
>> Usually from a kernel driver. That's what
>> Documentation/hwmon/hwmon-kernel-api.txt says:
> 
> Usually?  So userspace can set the name?
> 
I'm not sure about that.

>> 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.
>>
>> The hwmon subsystem has an own function to check for such characters:
>> hwmon_is_bad_char()
> 
> It looks like hwmon is the only thing that cares about this then, why
> do you want to make this a common function?
> 
In phylib we have a similar issue, sysfs is broken if a PHY driver name
includes a slash, typically if some driver author uses "10 / 100 Mbit"
or similar.
IIRC for HWMON there has been some longer discussion about how to deal
with naming, I'd be curious to hear Guenter's opinion on the topic.

> thanks,
> 
> greg k-h
> 
Heiner

Re: Fwd: [PATCH net-next 1/2] lib: string: add strreplace_nonalnum

[Greg Kroah-Hartman]

On Sun, Mar 03, 2019 at 07:47:29PM +0100, Heiner Kallweit wrote:
> On 03.03.2019 19:41, Greg Kroah-Hartman wrote:
> > On Sun, Mar 03, 2019 at 07:32:53PM +0100, Heiner Kallweit wrote:
> >> On 03.03.2019 19:15, Greg Kroah-Hartman wrote:
> >>> On Sun, Mar 03, 2019 at 07:04:21PM +0100, Heiner Kallweit wrote:
> >>>> On 03.03.2019 18:55, Greg Kroah-Hartman wrote:
> >>>>> On Sun, Mar 03, 2019 at 06:47:32PM +0100, Heiner Kallweit wrote:
> >>>>>> I submitted this through the netdev tree, maybe relevant for you as well.
> >>>>>> See also here: https://marc.info/?t=155103900100003&r=1&w=2
> >>>>>>
> >>>>>> -------- Forwarded Message --------
> >>>>>> Subject: [PATCH net-next 1/2] lib: string: add strreplace_nonalnum
> >>>>>> Date: Sun, 3 Mar 2019 18:20:50 +0100
> >>>>>> From: Heiner Kallweit <hkallweit1@gmail.com>
> >>>>>> To: Florian Fainelli <f.fainelli@gmail.com>, Andrew Lunn <andrew@lunn.ch>, David Miller <davem@davemloft.net>
> >>>>>> CC: netdev@vger.kernel.org <netdev@vger.kernel.org>
> >>>>>>
> >>>>>> Add a new function strreplace_nonalnum that replaces all
> >>>>>> non-alphanumeric characters. Such functionality is needed e.g. when a
> >>>>>> string is supposed to be used in a sysfs file name. If '\0' is given
> >>>>>> as new character then non-alphanumeric characters are cut. 
> >>>>>
> >>>>> sysfs doesn't have any such requirements, it can use whatever you want
> >>>>> to give it for a filename.
> >>>>>
> >>>> Even a slash?
> >>>
> >>> Is a slash an illegal character for a file to have?  It's up to the vfs
> >>> to care about this, don't force random parts of the kernel to care :)
> >>>
> >>>> HWMON drivers is an example where such functionality occurs open-coded.
> >>>
> >>> Is that data coming from userspace or from a kernel driver?
> >>>
> >> Usually from a kernel driver. That's what
> >> Documentation/hwmon/hwmon-kernel-api.txt says:
> > 
> > Usually?  So userspace can set the name?
> > 
> I'm not sure about that.

You might want to check :)

> >> 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.
> >>
> >> The hwmon subsystem has an own function to check for such characters:
> >> hwmon_is_bad_char()
> > 
> > It looks like hwmon is the only thing that cares about this then, why
> > do you want to make this a common function?
> > 
> In phylib we have a similar issue, sysfs is broken if a PHY driver name
> includes a slash, typically if some driver author uses "10 / 100 Mbit"
> or similar.
> IIRC for HWMON there has been some longer discussion about how to deal
> with naming, I'd be curious to hear Guenter's opinion on the topic.

Making drivers "safe" from themselves is pretty foolish, as if they
don't know they are doing stupid things, they will continue to do those
stupid things :)

Having a driver name itself something bad like this is fine, nothing
breaks except no one can access any sysfs files from the driver, so the
driver needs to be fixed.

Now if userspace can set this string, that's a totaly different story,
that needs to be properly sanitized, because as we all know, "all input
is evil."

thanks,

greg k-h

Re: [PATCH net-next 0/2] lib: string: add strreplace_nonalnum

[Heiner Kallweit]

On 03.03.2019 19:36, Heiner Kallweit wrote:
> On 03.03.2019 18:19, Heiner Kallweit wrote:
>> Add a new function strreplace_nonalnum that replaces all
>> non-alphanumeric characters. Such functionality is needed e.g. when a
>> string is supposed to be used in a file name. If '\0' is given as new
>> character then non-alphanumeric characters are cut. 
>>
>> There doesn't seem to be a maintainer or mailing list for lib/string.c
>> Therefore I hope it's ok to submit this through the netdev tree.
>>
>> Heiner Kallweit (2):
>>   lib: string: add strreplace_nonalnum
>>   net: phy: aquantia: use new function strreplace_nonalnum
>>
>>  drivers/net/phy/aquantia_hwmon.c | 10 +---------
>>  include/linux/string.h           |  1 +
>>  lib/string.c                     | 27 +++++++++++++++++++++++++++
>>  3 files changed, 29 insertions(+), 9 deletions(-)
>>
> Greg still has some concerns, let's see what the outcome of this
> discussion is.
> https://lkml.org/lkml/2019/3/3/151
> 
This series should be deferred to next merge window.

Heiner

Re: [PATCH net-next 0/2] lib: string: add strreplace_nonalnum

[David Miller]

From: Heiner Kallweit <hkallweit1@gmail.com>
Date: Mon, 4 Mar 2019 19:54:16 +0100

> On 03.03.2019 19:36, Heiner Kallweit wrote:
>> On 03.03.2019 18:19, Heiner Kallweit wrote:
>>> Add a new function strreplace_nonalnum that replaces all
>>> non-alphanumeric characters. Such functionality is needed e.g. when a
>>> string is supposed to be used in a file name. If '\0' is given as new
>>> character then non-alphanumeric characters are cut. 
>>>
>>> There doesn't seem to be a maintainer or mailing list for lib/string.c
>>> Therefore I hope it's ok to submit this through the netdev tree.
>>>
>>> Heiner Kallweit (2):
>>>   lib: string: add strreplace_nonalnum
>>>   net: phy: aquantia: use new function strreplace_nonalnum
>>>
>>>  drivers/net/phy/aquantia_hwmon.c | 10 +---------
>>>  include/linux/string.h           |  1 +
>>>  lib/string.c                     | 27 +++++++++++++++++++++++++++
>>>  3 files changed, 29 insertions(+), 9 deletions(-)
>>>
>> Greg still has some concerns, let's see what the outcome of this
>> discussion is.
>> https://lkml.org/lkml/2019/3/3/151
>> 
> This series should be deferred to next merge window.

Ok I'll revert, sorry I just saw this.

Re: [PATCH net-next 0/2] lib: string: add strreplace_nonalnum

[Heiner Kallweit]

On 04.03.2019 20:22, David Miller wrote:
> From: Heiner Kallweit <hkallweit1@gmail.com>
> Date: Mon, 4 Mar 2019 19:54:16 +0100
> 
>> On 03.03.2019 19:36, Heiner Kallweit wrote:
>>> On 03.03.2019 18:19, Heiner Kallweit wrote:
>>>> Add a new function strreplace_nonalnum that replaces all
>>>> non-alphanumeric characters. Such functionality is needed e.g. when a
>>>> string is supposed to be used in a file name. If '\0' is given as new
>>>> character then non-alphanumeric characters are cut. 
>>>>
>>>> There doesn't seem to be a maintainer or mailing list for lib/string.c
>>>> Therefore I hope it's ok to submit this through the netdev tree.
>>>>
>>>> Heiner Kallweit (2):
>>>>   lib: string: add strreplace_nonalnum
>>>>   net: phy: aquantia: use new function strreplace_nonalnum
>>>>
>>>>  drivers/net/phy/aquantia_hwmon.c | 10 +---------
>>>>  include/linux/string.h           |  1 +
>>>>  lib/string.c                     | 27 +++++++++++++++++++++++++++
>>>>  3 files changed, 29 insertions(+), 9 deletions(-)
>>>>
>>> Greg still has some concerns, let's see what the outcome of this
>>> discussion is.
>>> https://lkml.org/lkml/2019/3/3/151
>>>
>> This series should be deferred to next merge window.
> 
> Ok I'll revert, sorry I just saw this.
> 
Thank you!