[PATCH v2 1/1] ARM: orion5x: use mac_pton() helper

[PATCH v2 1/1] ARM: orion5x: use mac_pton() helper

[Andy Shevchenko]

Instead of custom approach let's use generic helper function.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Cc: Detlef Vollmann <dv@vollmann.ch>
Cc: Gregory CLEMENT <gregory.clement@free-electrons.com>
---
Changelog v2:
- select GENERIC_NET_UTILS when appropriate
- compile test only
 arch/arm/mach-orion5x/Kconfig        |  3 ++
 arch/arm/mach-orion5x/dns323-setup.c | 53 ++----------------------------------
 arch/arm/mach-orion5x/tsx09-common.c | 49 +++------------------------------
 3 files changed, 10 insertions(+), 95 deletions(-)

diff --git a/arch/arm/mach-orion5x/Kconfig b/arch/arm/mach-orion5x/Kconfig
index 08d2be2..66f1c95 100644
--- a/arch/arm/mach-orion5x/Kconfig
+++ b/arch/arm/mach-orion5x/Kconfig
@@ -45,6 +45,7 @@ config MACH_KUROBOX_PRO
 
 config MACH_DNS323
 	bool "D-Link DNS-323"
+	select GENERIC_NET_UTILS
 	select I2C_BOARDINFO
 	help
 	  Say 'Y' here if you want your kernel to support the
@@ -52,6 +53,7 @@ config MACH_DNS323
 
 config MACH_TS209
 	bool "QNAP TS-109/TS-209"
+	select GENERIC_NET_UTILS
 	help
 	  Say 'Y' here if you want your kernel to support the
 	  QNAP TS-109/TS-209 platform.
@@ -93,6 +95,7 @@ config MACH_LINKSTATION_LS_HGL
 
 config MACH_TS409
 	bool "QNAP TS-409"
+	select GENERIC_NET_UTILS
 	help
 	  Say 'Y' here if you want your kernel to support the
 	  QNAP TS-409 platform.
diff --git a/arch/arm/mach-orion5x/dns323-setup.c b/arch/arm/mach-orion5x/dns323-setup.c
index f267e58..bc279a8 100644
--- a/arch/arm/mach-orion5x/dns323-setup.c
+++ b/arch/arm/mach-orion5x/dns323-setup.c
@@ -173,42 +173,10 @@ static struct mv643xx_eth_platform_data dns323_eth_data = {
 	.phy_addr = MV643XX_ETH_PHY_ADDR(8),
 };
 
-/* dns323_parse_hex_*() taken from tsx09-common.c; should a common copy of these
- * functions be kept somewhere?
- */
-static int __init dns323_parse_hex_nibble(char n)
-{
-	if (n >= '0' && n <= '9')
-		return n - '0';
-
-	if (n >= 'A' && n <= 'F')
-		return n - 'A' + 10;
-
-	if (n >= 'a' && n <= 'f')
-		return n - 'a' + 10;
-
-	return -1;
-}
-
-static int __init dns323_parse_hex_byte(const char *b)
-{
-	int hi;
-	int lo;
-
-	hi = dns323_parse_hex_nibble(b[0]);
-	lo = dns323_parse_hex_nibble(b[1]);
-
-	if (hi < 0 || lo < 0)
-		return -1;
-
-	return (hi << 4) | lo;
-}
-
 static int __init dns323_read_mac_addr(void)
 {
 	u_int8_t addr[6];
-	int i;
-	char *mac_page;
+	void __iomem *mac_page;
 
 	/* MAC address is stored as a regular ol' string in /dev/mtdblock4
 	 * (0x007d0000-0x00800000) starting at offset 196480 (0x2ff80).
@@ -217,23 +185,8 @@ static int __init dns323_read_mac_addr(void)
 	if (!mac_page)
 		return -ENOMEM;
 
-	/* Sanity check the string we're looking@*/
-	for (i = 0; i < 5; i++) {
-		if (*(mac_page + (i * 3) + 2) != ':') {
-			goto error_fail;
-		}
-	}
-
-	for (i = 0; i < 6; i++)	{
-		int byte;
-
-		byte = dns323_parse_hex_byte(mac_page + (i * 3));
-		if (byte < 0) {
-			goto error_fail;
-		}
-
-		addr[i] = byte;
-	}
+	if (!mac_pton((__force const char *) mac_page, addr))
+		goto error_fail;
 
 	iounmap(mac_page);
 	printk("DNS-323: Found ethernet MAC address: %pM\n", addr);
diff --git a/arch/arm/mach-orion5x/tsx09-common.c b/arch/arm/mach-orion5x/tsx09-common.c
index 24b2959..d42e006 100644
--- a/arch/arm/mach-orion5x/tsx09-common.c
+++ b/arch/arm/mach-orion5x/tsx09-common.c
@@ -53,53 +53,12 @@ struct mv643xx_eth_platform_data qnap_tsx09_eth_data = {
 	.phy_addr	= MV643XX_ETH_PHY_ADDR(8),
 };
 
-static int __init qnap_tsx09_parse_hex_nibble(char n)
-{
-	if (n >= '0' && n <= '9')
-		return n - '0';
-
-	if (n >= 'A' && n <= 'F')
-		return n - 'A' + 10;
-
-	if (n >= 'a' && n <= 'f')
-		return n - 'a' + 10;
-
-	return -1;
-}
-
-static int __init qnap_tsx09_parse_hex_byte(const char *b)
-{
-	int hi;
-	int lo;
-
-	hi = qnap_tsx09_parse_hex_nibble(b[0]);
-	lo = qnap_tsx09_parse_hex_nibble(b[1]);
-
-	if (hi < 0 || lo < 0)
-		return -1;
-
-	return (hi << 4) | lo;
-}
-
 static int __init qnap_tsx09_check_mac_addr(const char *addr_str)
 {
 	u_int8_t addr[6];
-	int i;
 
-	for (i = 0; i < 6; i++) {
-		int byte;
-
-		/*
-		 * Enforce "xx:xx:xx:xx:xx:xx\n" format.
-		 */
-		if (addr_str[(i * 3) + 2] != ((i < 5) ? ':' : '\n'))
-			return -1;
-
-		byte = qnap_tsx09_parse_hex_byte(addr_str + (i * 3));
-		if (byte < 0)
-			return -1;
-		addr[i] = byte;
-	}
+	if (!mac_pton(addr_str, addr))
+		return -1;
 
 	printk(KERN_INFO "tsx09: found ethernet mac address %pM\n", addr);
 
@@ -118,12 +77,12 @@ void __init qnap_tsx09_find_mac_addr(u32 mem_base, u32 size)
 	unsigned long addr;
 
 	for (addr = mem_base; addr < (mem_base + size); addr += 1024) {
-		char *nor_page;
+		void __iomem *nor_page;
 		int ret = 0;
 
 		nor_page = ioremap(addr, 1024);
 		if (nor_page != NULL) {
-			ret = qnap_tsx09_check_mac_addr(nor_page);
+			ret = qnap_tsx09_check_mac_addr((__force const char *)nor_page);
 			iounmap(nor_page);
 		}
 
-- 
2.5.1

[PATCH v2 1/1] ARM: orion5x: use mac_pton() helper

[Detlef Vollmann]

On 10/02/15 16:12, Andy Shevchenko wrote:
> Instead of custom approach let's use generic helper function.
>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Cc: Detlef Vollmann <dv@vollmann.ch>
> Cc: Gregory CLEMENT <gregory.clement@free-electrons.com>
> ---
> Changelog v2:
> - select GENERIC_NET_UTILS when appropriate
> - compile test only
>   arch/arm/mach-orion5x/Kconfig        |  3 ++
>   arch/arm/mach-orion5x/dns323-setup.c | 53 ++----------------------------------
>   arch/arm/mach-orion5x/tsx09-common.c | 49 +++------------------------------
>   3 files changed, 10 insertions(+), 95 deletions(-)
At last I found some time to test it on DNS-323.
Applied to Russell's tree and used orion5x_defconfig.
Works nicely, and I like that the final (compressed) kernel
binary is now 200 byte smaller!

Sorry, can't test on a TS-x09.

Thanks,
   Detlef

[PATCH v2 1/1] ARM: orion5x: use mac_pton() helper

[Gregory CLEMENT]

Hi Detlef,
 
 On mar., oct. 13 2015, Detlef Vollmann <dv@vollmann.ch> wrote:

> On 10/02/15 16:12, Andy Shevchenko wrote:
>> Instead of custom approach let's use generic helper function.
>>
>> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
>> Cc: Detlef Vollmann <dv@vollmann.ch>
>> Cc: Gregory CLEMENT <gregory.clement@free-electrons.com>


Applied on mvebu/soc

With the tested-by flag from Detlef

Thanks,

Gregory


>> ---
>> Changelog v2:
>> - select GENERIC_NET_UTILS when appropriate
>> - compile test only
>>   arch/arm/mach-orion5x/Kconfig        |  3 ++
>>   arch/arm/mach-orion5x/dns323-setup.c | 53 ++----------------------------------
>>   arch/arm/mach-orion5x/tsx09-common.c | 49 +++------------------------------
>>   3 files changed, 10 insertions(+), 95 deletions(-)
> At last I found some time to test it on DNS-323.
> Applied to Russell's tree and used orion5x_defconfig.
> Works nicely, and I like that the final (compressed) kernel
> binary is now 200 byte smaller!
>
> Sorry, can't test on a TS-x09.
>
> Thanks,
>   Detlef
>

-- 
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

[v2,1/1] ARM: orion5x: use mac_pton() helper

[Stefan Hellermann]

Hi!

My QNAP TS-209 NAS Device is crashing with the following commit, which 
went in the kernel as commit 4904dbda41c860fd117b20f3c48adb2780eee37e

I cannot provide a boot log, the device panics before enabling the 
serial console. I had a tough time with git bisect, but after 2 days of 
work I can revert commit 4904dbda41c860fd117b20f3c48adb2780eee37e on 
kernel 4.4.116 or 4.9.82 and everything is fine.

Can I provide anything to help debug this further? The device is 
currently running a custom openwrt, Debian is also ported to the device. 
Current Debian installer crashes early caused by this commit.

This is the uboot-config:
# fw_printenv
bootargs=console=ttyS0,115200
baudrate=115200
loads_echo=0
ipaddr=172.17.21.244
serverip=172.17.21.8
rootpath=/mnt/ARM_FS/
stdin=serial
stdout=serial
stderr=serial
cpuName=926
CASset=min
enaMonExt=no
enaFlashBuf=yes
enaCpuStream=no
MALLOC_len=4
ethprime=egiga0
bootargs_root=root=/dev/nfs rw
bootargs_end=:::DB88FXX81:egiga0:none
image_name=uImage
standalone=fsload 0x400000 $(image_name);setenv bootargs $(bootargs) 
root=/dev/mtdblock0 rw ip=$(ipaddr):$(serverip)$(bootargs_end); bootm 
0x400000;
bootdelay=1
disaMvPnp=no
usb0Mode=host
usb1Mode=host
ethact=egiga0
bootcmd=bootm 0xff000000
overEthAddr=no
ethaddr=00:08:9B:AC:DA:A6





Am 02.10.2015 um 16:12 schrieb Andy Shevchenko:
> Instead of custom approach let's use generic helper function.
>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Cc: Detlef Vollmann <dv@vollmann.ch>
> Cc: Gregory CLEMENT <gregory.clement@free-electrons.com>
> ---
> Changelog v2:
> - select GENERIC_NET_UTILS when appropriate
> - compile test only
>   arch/arm/mach-orion5x/Kconfig        |  3 ++
>   arch/arm/mach-orion5x/dns323-setup.c | 53 ++----------------------------------
>   arch/arm/mach-orion5x/tsx09-common.c | 49 +++------------------------------
>   3 files changed, 10 insertions(+), 95 deletions(-)
>
> diff --git a/arch/arm/mach-orion5x/Kconfig b/arch/arm/mach-orion5x/Kconfig
> index 08d2be2..66f1c95 100644
> --- a/arch/arm/mach-orion5x/Kconfig
> +++ b/arch/arm/mach-orion5x/Kconfig
> @@ -45,6 +45,7 @@ config MACH_KUROBOX_PRO
>   
>   config MACH_DNS323
>   	bool "D-Link DNS-323"
> +	select GENERIC_NET_UTILS
>   	select I2C_BOARDINFO
>   	help
>   	  Say 'Y' here if you want your kernel to support the
> @@ -52,6 +53,7 @@ config MACH_DNS323
>   
>   config MACH_TS209
>   	bool "QNAP TS-109/TS-209"
> +	select GENERIC_NET_UTILS
>   	help
>   	  Say 'Y' here if you want your kernel to support the
>   	  QNAP TS-109/TS-209 platform.
> @@ -93,6 +95,7 @@ config MACH_LINKSTATION_LS_HGL
>   
>   config MACH_TS409
>   	bool "QNAP TS-409"
> +	select GENERIC_NET_UTILS
>   	help
>   	  Say 'Y' here if you want your kernel to support the
>   	  QNAP TS-409 platform.
> diff --git a/arch/arm/mach-orion5x/dns323-setup.c b/arch/arm/mach-orion5x/dns323-setup.c
> index f267e58..bc279a8 100644
> --- a/arch/arm/mach-orion5x/dns323-setup.c
> +++ b/arch/arm/mach-orion5x/dns323-setup.c
> @@ -173,42 +173,10 @@ static struct mv643xx_eth_platform_data dns323_eth_data = {
>   	.phy_addr = MV643XX_ETH_PHY_ADDR(8),
>   };
>   
> -/* dns323_parse_hex_*() taken from tsx09-common.c; should a common copy of these
> - * functions be kept somewhere?
> - */
> -static int __init dns323_parse_hex_nibble(char n)
> -{
> -	if (n >= '0' && n <= '9')
> -		return n - '0';
> -
> -	if (n >= 'A' && n <= 'F')
> -		return n - 'A' + 10;
> -
> -	if (n >= 'a' && n <= 'f')
> -		return n - 'a' + 10;
> -
> -	return -1;
> -}
> -
> -static int __init dns323_parse_hex_byte(const char *b)
> -{
> -	int hi;
> -	int lo;
> -
> -	hi = dns323_parse_hex_nibble(b[0]);
> -	lo = dns323_parse_hex_nibble(b[1]);
> -
> -	if (hi < 0 || lo < 0)
> -		return -1;
> -
> -	return (hi << 4) | lo;
> -}
> -
>   static int __init dns323_read_mac_addr(void)
>   {
>   	u_int8_t addr[6];
> -	int i;
> -	char *mac_page;
> +	void __iomem *mac_page;
>   
>   	/* MAC address is stored as a regular ol' string in /dev/mtdblock4
>   	 * (0x007d0000-0x00800000) starting at offset 196480 (0x2ff80).
> @@ -217,23 +185,8 @@ static int __init dns323_read_mac_addr(void)
>   	if (!mac_page)
>   		return -ENOMEM;
>   
> -	/* Sanity check the string we're looking at */
> -	for (i = 0; i < 5; i++) {
> -		if (*(mac_page + (i * 3) + 2) != ':') {
> -			goto error_fail;
> -		}
> -	}
> -
> -	for (i = 0; i < 6; i++)	{
> -		int byte;
> -
> -		byte = dns323_parse_hex_byte(mac_page + (i * 3));
> -		if (byte < 0) {
> -			goto error_fail;
> -		}
> -
> -		addr[i] = byte;
> -	}
> +	if (!mac_pton((__force const char *) mac_page, addr))
> +		goto error_fail;
>   
>   	iounmap(mac_page);
>   	printk("DNS-323: Found ethernet MAC address: %pM\n", addr);
> diff --git a/arch/arm/mach-orion5x/tsx09-common.c b/arch/arm/mach-orion5x/tsx09-common.c
> index 24b2959..d42e006 100644
> --- a/arch/arm/mach-orion5x/tsx09-common.c
> +++ b/arch/arm/mach-orion5x/tsx09-common.c
> @@ -53,53 +53,12 @@ struct mv643xx_eth_platform_data qnap_tsx09_eth_data = {
>   	.phy_addr	= MV643XX_ETH_PHY_ADDR(8),
>   };
>   
> -static int __init qnap_tsx09_parse_hex_nibble(char n)
> -{
> -	if (n >= '0' && n <= '9')
> -		return n - '0';
> -
> -	if (n >= 'A' && n <= 'F')
> -		return n - 'A' + 10;
> -
> -	if (n >= 'a' && n <= 'f')
> -		return n - 'a' + 10;
> -
> -	return -1;
> -}
> -
> -static int __init qnap_tsx09_parse_hex_byte(const char *b)
> -{
> -	int hi;
> -	int lo;
> -
> -	hi = qnap_tsx09_parse_hex_nibble(b[0]);
> -	lo = qnap_tsx09_parse_hex_nibble(b[1]);
> -
> -	if (hi < 0 || lo < 0)
> -		return -1;
> -
> -	return (hi << 4) | lo;
> -}
> -
>   static int __init qnap_tsx09_check_mac_addr(const char *addr_str)
>   {
>   	u_int8_t addr[6];
> -	int i;
>   
> -	for (i = 0; i < 6; i++) {
> -		int byte;
> -
> -		/*
> -		 * Enforce "xx:xx:xx:xx:xx:xx\n" format.
> -		 */
> -		if (addr_str[(i * 3) + 2] != ((i < 5) ? ':' : '\n'))
> -			return -1;
> -
> -		byte = qnap_tsx09_parse_hex_byte(addr_str + (i * 3));
> -		if (byte < 0)
> -			return -1;
> -		addr[i] = byte;
> -	}
> +	if (!mac_pton(addr_str, addr))
> +		return -1;
>   
>   	printk(KERN_INFO "tsx09: found ethernet mac address %pM\n", addr);
>   
> @@ -118,12 +77,12 @@ void __init qnap_tsx09_find_mac_addr(u32 mem_base, u32 size)
>   	unsigned long addr;
>   
>   	for (addr = mem_base; addr < (mem_base + size); addr += 1024) {
> -		char *nor_page;
> +		void __iomem *nor_page;
>   		int ret = 0;
>   
>   		nor_page = ioremap(addr, 1024);
>   		if (nor_page != NULL) {
> -			ret = qnap_tsx09_check_mac_addr(nor_page);
> +			ret = qnap_tsx09_check_mac_addr((__force const char *)nor_page);
>   			iounmap(nor_page);
>   		}
>   

[v2,1/1] ARM: orion5x: use mac_pton() helper

[Andrew Lunn]

On Thu, Feb 22, 2018 at 06:45:51PM +0100, Stefan Hellermann wrote:
> Hi!
> 
> My QNAP TS-209 NAS Device is crashing with the following commit, which went
> in the kernel as commit 4904dbda41c860fd117b20f3c48adb2780eee37e
> 
> I cannot provide a boot log, the device panics before enabling the serial
> console.

Hi Stefan

Did you try earlyprintk? You might need to recompile your kernel to
enable it.

Looking at the code, i don't see anything obviously wrong. So i think
i would start by looking how many times it goes through the loop in
qnap_tsx09_find_mac_addr() with this patch reverted, and what address
it finds the MAC address at.

Then see what happens with the current crashing code. Is it failing to
recognise the MAC address, and so keep looping around?

Thanks
	Andrew

[v2,1/1] ARM: orion5x: use mac_pton() helper

[Stefan Hellermann]

Am 22.02.2018 um 22:42 schrieb Andrew Lunn:
> On Thu, Feb 22, 2018 at 06:45:51PM +0100, Stefan Hellermann wrote:
>> Hi!
>>
>> My QNAP TS-209 NAS Device is crashing with the following commit, which went
>> in the kernel as commit 4904dbda41c860fd117b20f3c48adb2780eee37e
>>
>> I cannot provide a boot log, the device panics before enabling the serial
>> console.
> Hi Stefan
>
> Did you try earlyprintk? You might need to recompile your kernel to
> enable it.
Yes, I tried it, but it didn't help. I even changed qnap_ts209_init() to 
configure uart before ethernet, but it didn't help either.
>
> Looking at the code, i don't see anything obviously wrong. So i think
> i would start by looking how many times it goes through the loop in
> qnap_tsx09_find_mac_addr() with this patch reverted, and what address
> it finds the MAC address at.
>
> Then see what happens with the current crashing code. Is it failing to
> recognise the MAC address, and so keep looping around?
I think I found the failure:
The code is looping through an ext2 filesystem on a 384kB mtd partition 
(factory configuration put there by QNAP). There it looks on every 1kB 
boundary if there is a valid MAC address. The filesystem has a 1kB block 
size, so this seems to work. The MAC address is on the 37th 1kB block.

But: On the 27th block is a large file (1,5kB) without 0 bytes inside.
The code in qnap_tsx09_find_mac_addr() maps 1kB into memory (not a whole 
file or the whole 384kB) and then calls qnap_tsx09_check_mac_addr() -> 
mac_pton() -> strlen() on this memory block. as there is no 0 byte in 
the file on the 27th block, strlen runs into bad memory and the machine 
panics. The old code had no strlen().

I changed mac_pton() to use strnlen(), and now the panic is gone. I 
don't know why strlen is actually needed in mac_pton. The string is 
checked in the following loop, if there is a zero byte somewhere, the 
loop will be returned immediately. So I think the strlen() superfluous. 
Is the following patch correct?

diff --git a/lib/net_utils.c b/lib/net_utils.c
index 148fc6e99ef6..e7785cf20f59 100644
--- a/lib/net_utils.c
+++ b/lib/net_utils.c
@@ -7,10 +7,6 @@ bool mac_pton(const char *s, u8 *mac)
 ?{
 ???? int i;

-??? /* XX:XX:XX:XX:XX:XX */
-??? if (strlen(s) < 3 * ETH_ALEN - 1)
-??? ??? return false;
-
 ???? /* Don't dirty result unless string is valid MAC. */
 ???? for (i = 0; i < ETH_ALEN; i++) {
 ???? ??? if (!isxdigit(s[i * 3]) || !isxdigit(s[i * 3 + 1]))

[v2,1/1] ARM: orion5x: use mac_pton() helper

[Andrew Lunn]

> But: On the 27th block is a large file (1,5kB) without 0 bytes inside.
> The code in qnap_tsx09_find_mac_addr() maps 1kB into memory (not a whole
> file or the whole 384kB) and then calls qnap_tsx09_check_mac_addr() ->
> mac_pton() -> strlen() on this memory block. as there is no 0 byte in the
> file on the 27th block, strlen runs into bad memory and the machine panics.
> The old code had no strlen().

Yes, that sounds look a good explanation. 

> I changed mac_pton() to use strnlen(), and now the panic is gone. I don't
> know why strlen is actually needed in mac_pton. The string is checked in the
> following loop, if there is a zero byte somewhere, the loop will be returned
> immediately. So I think the strlen() superfluous. Is the following patch
> correct?

The patch has been corrupted by you email client. But otherwise, yes.

Please take a look at:

https://www.kernel.org/doc/html/v4.12/process/submitting-patches.html

It will give you hits about correctly formatting the patch. In
addition it should have:

Fixes: 4cd5773a2ae6 ("net: core: move mac_pton() to lib/net_utils.c")

before the --- line, to indicate what it is fixing.

This patch should be against
git://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git,
since it is a fix, and sent to <netdev@vger.kernel.org>.

Thanks
      Andrew

[v2,1/1] ARM: orion5x: use mac_pton() helper

[Andy Shevchenko]

On Fri, 2018-02-23 at 00:34 +0100, Andrew Lunn wrote:
> > But: On the 27th block is a large file (1,5kB) without 0 bytes
> > inside.
> > The code in qnap_tsx09_find_mac_addr() maps 1kB into memory (not a
> > whole
> > file or the whole 384kB) and then calls qnap_tsx09_check_mac_addr()
> > ->
> > mac_pton() -> strlen() on this memory block. as there is no 0 byte
> > in the
> > file on the 27th block, strlen runs into bad memory and the machine
> > panics.
> > The old code had no strlen().
> 
> Yes, that sounds look a good explanation. 
> 
> > I changed mac_pton() to use strnlen(), and now the panic is gone. I
> > don't
> > know why strlen is actually needed in mac_pton. The string is
> > checked in the
> > following loop, if there is a zero byte somewhere, the loop will be
> > returned
> > immediately. So I think the strlen() superfluous. Is the following
> > patch
> > correct?
> 
> The patch has been corrupted by you email client. But otherwise, yes.
> 
> Please take a look at:
> 
> https://www.kernel.org/doc/html/v4.12/process/submitting-patches.html
> 
> It will give you hits about correctly formatting the patch. In
> addition it should have:
> 
> Fixes: 4cd5773a2ae6 ("net: core: move mac_pton() to lib/net_utils.c")
> 
> before the --- line, to indicate what it is fixing.
> 
> This patch should be against
> git://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git,
> since it is a fix, and sent to <netdev@vger.kernel.org>.

Guys, consider this one instead:
https://patchwork.ozlabs.org/patch/851008/


-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

[v2,1/1] ARM: orion5x: use mac_pton() helper

[Andrew Lunn]

> > The patch has been corrupted by you email client. But otherwise, yes.
> > 
> > Please take a look at:
> > 
> > https://www.kernel.org/doc/html/v4.12/process/submitting-patches.html
> > 
> > It will give you hits about correctly formatting the patch. In
> > addition it should have:
> > 
> > Fixes: 4cd5773a2ae6 ("net: core: move mac_pton() to lib/net_utils.c")
> > 
> > before the --- line, to indicate what it is fixing.
> > 
> > This patch should be against
> > git://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git,
> > since it is a fix, and sent to <netdev@vger.kernel.org>.
> 
> Guys, consider this one instead:
> https://patchwork.ozlabs.org/patch/851008/

Hi Andy

Thanks for pointing this patch out.

What is the advantage of doing to the strnlen()? As Stefan says, the
code which follows will detect a short string, in that a NULL is not
in [0-9a-f], nor a : .

Thanks
	Andrew

[v2,1/1] ARM: orion5x: use mac_pton() helper

[Andy Shevchenko]

+Cc Alexey

On Fri, 2018-02-23 at 16:01 +0100, Andrew Lunn wrote:
> > > The patch has been corrupted by you email client. But otherwise,
> > > yes.
> > > 
> > > Please take a look at:
> > > 
> > > https://www.kernel.org/doc/html/v4.12/process/submitting-patches.h
> > > tml
> > > 
> > > It will give you hits about correctly formatting the patch. In
> > > addition it should have:
> > > 
> > > Fixes: 4cd5773a2ae6 ("net: core: move mac_pton() to
> > > lib/net_utils.c")
> > > 
> > > before the --- line, to indicate what it is fixing.
> > > 
> > > This patch should be against
> > > git://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git,
> > > since it is a fix, and sent to <netdev@vger.kernel.org>.
> > 
> > Guys, consider this one instead:
> > https://patchwork.ozlabs.org/patch/851008/
> 
> Hi Andy
> 
> Thanks for pointing this patch out.
> 
> What is the advantage of doing to the strnlen()? As Stefan says, the
> code which follows will detect a short string, in that a NULL is not
> in [0-9a-f], nor a : .

I'm not sure, but my understanding is that, the strchr() call in the
original code or isxdigit() in the follow up change will trash a cache a
bit. Besides that some of the users are (often?) supplying empty strings
to convert from, and in this case makes sense to bail out fast.

Alexey, can you shed a light here?

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

[v2,1/1] ARM: orion5x: use mac_pton() helper

[Andrew Lunn]

> > Hi Andy
> > 
> > Thanks for pointing this patch out.
> > 
> > What is the advantage of doing to the strnlen()? As Stefan says, the
> > code which follows will detect a short string, in that a NULL is not
> > in [0-9a-f], nor a : .
> 
> I'm not sure, but my understanding is that, the strchr() call in the
> original code or isxdigit() in the follow up change will trash a cache a
> bit. Besides that some of the users are (often?) supplying empty strings
> to convert from, and in this case makes sense to bail out fast.

Is this function being called on a hot path? In the case which is
crashing, it is during early boot, and it gets called ~ 40 times, in
quick succession. The first call to isxdigit() will need to fetch part
of the _ctype array into cache, but since the caller is only walking
memory, i hope it is still in cache for the next call to mac_pton().

	Andrew

[v2,1/1] ARM: orion5x: use mac_pton() helper

[Stefan Hellermann]

2018-02-23 16:51 GMT+01:00 Andrew Lunn <andrew@lunn.ch>:
>> > Hi Andy
>> >
>> > Thanks for pointing this patch out.
>> >
>> > What is the advantage of doing to the strnlen()? As Stefan says, the
>> > code which follows will detect a short string, in that a NULL is not
>> > in [0-9a-f], nor a : .
>>
>> I'm not sure, but my understanding is that, the strchr() call in the
>> original code or isxdigit() in the follow up change will trash a cache a
>> bit. Besides that some of the users are (often?) supplying empty strings
>> to convert from, and in this case makes sense to bail out fast.
>
> Is this function being called on a hot path? In the case which is
> crashing, it is during early boot, and it gets called ~ 40 times, in
> quick succession. The first call to isxdigit() will need to fetch part
> of the _ctype array into cache, but since the caller is only walking
> memory, i hope it is still in cache for the next call to mac_pton().
>
>         Andrew

In my case mac_pton is not called on a hot path, the slow sata disks
in the NAS dominate the boot time. For me code size matters, and on my
device it's rarely called on a zero string. It's probably slower with
the strnlen as without it. I think if there are users out there
calling it in a hot path on a zero string, they should check the zero
string themselves, this is not the common use case.

Should I send a patch to netdev, can I get some Acked-by: ?

    Stefan

[v2,1/1] ARM: orion5x: use mac_pton() helper

[Andy Shevchenko]

On Fri, 2018-02-23 at 17:36 +0100, Stefan Hellermann wrote:
> 2018-02-23 16:51 GMT+01:00 Andrew Lunn <andrew@lunn.ch>:
> > > > Hi Andy
> > > > 
> > > > Thanks for pointing this patch out.
> > > > 
> > > > What is the advantage of doing to the strnlen()? As Stefan says,
> > > > the
> > > > code which follows will detect a short string, in that a NULL is
> > > > not
> > > > in [0-9a-f], nor a : .
> > > 
> > > I'm not sure, but my understanding is that, the strchr() call in
> > > the
> > > original code or isxdigit() in the follow up change will trash a
> > > cache a
> > > bit. Besides that some of the users are (often?) supplying empty
> > > strings
> > > to convert from, and in this case makes sense to bail out fast.
> > 
> > Is this function being called on a hot path? In the case which is
> > crashing, it is during early boot, and it gets called ~ 40 times, in
> > quick succession. The first call to isxdigit() will need to fetch
> > part
> > of the _ctype array into cache, but since the caller is only walking
> > memory, i hope it is still in cache for the next call to mac_pton().
> > 
> >         Andrew
> 
> In my case mac_pton is not called on a hot path, the slow sata disks
> in the NAS dominate the boot time. For me code size matters, and on my
> device it's rarely called on a zero string. It's probably slower with
> the strnlen as without it. I think if there are users out there
> calling it in a hot path on a zero string, they should check the zero
> string themselves, this is not the common use case.
> 

> Should I send a patch to netdev, can I get some Acked-by: ?

Not from me, sorry.

Alexey would be best person to give a such.
Please, Cc him.

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

[v2,1/1] ARM: orion5x: use mac_pton() helper

[Andrew Lunn]

> Should I send a patch to netdev, can I get some Acked-by: ?

Please do. I will ack it, if it follows all the usual rules, not
corrupt, etc.

       Andrew

[v2,1/1] ARM: orion5x: use mac_pton() helper

[Alexey Dobriyan]

On Fri, Feb 23, 2018 at 05:18:48PM +0200, Andy Shevchenko wrote:
> +Cc Alexey
> 
> On Fri, 2018-02-23 at 16:01 +0100, Andrew Lunn wrote:
> > > > The patch has been corrupted by you email client. But otherwise,
> > > > yes.
> > > > 
> > > > Please take a look at:
> > > > 
> > > > https://www.kernel.org/doc/html/v4.12/process/submitting-patches.h
> > > > tml
> > > > 
> > > > It will give you hits about correctly formatting the patch. In
> > > > addition it should have:
> > > > 
> > > > Fixes: 4cd5773a2ae6 ("net: core: move mac_pton() to
> > > > lib/net_utils.c")
> > > > 
> > > > before the --- line, to indicate what it is fixing.
> > > > 
> > > > This patch should be against
> > > > git://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git,
> > > > since it is a fix, and sent to <netdev@vger.kernel.org>.
> > > 
> > > Guys, consider this one instead:
> > > https://patchwork.ozlabs.org/patch/851008/
> > 
> > Hi Andy
> > 
> > Thanks for pointing this patch out.
> > 
> > What is the advantage of doing to the strnlen()? As Stefan says, the
> > code which follows will detect a short string, in that a NULL is not
> > in [0-9a-f], nor a : .
> 
> I'm not sure, but my understanding is that, the strchr() call in the
> original code or isxdigit() in the follow up change will trash a cache a
> bit. Besides that some of the users are (often?) supplying empty strings
> to convert from, and in this case makes sense to bail out fast.
> 
> Alexey, can you shed a light here?

I went for the simplest code.
map_pton() is never on the fastpath, so code size matters more.

In this sense, conversion to mac_pton() should be done, and strnlen()
probably should not.

[PATCH] net: Allow mac_pton() to work on non-NULL terminated strings

[Stefan Hellermann]

Commit 4904dbda41c8 ("ARM: orion5x: use mac_pton() helper") crashes my
QNAP TS-209 NAS early on boot.

The boot code for the TS-209 is looping through an ext2 filesystem on a
384kB mtd partition (factory configuration put there by QNAP). There it
looks on every 1kB boundary if there is a valid MAC address. The
filesystem has a 1kB block size, so this seems to work.

On my device the MAC address is on the 37th 1kB block. But: On the 27th
block is a large file (1,5kB) without 0 bytes inside. The code in
qnap_tsx09_find_mac_addr() maps 1kB into memory (not a whole file or the
whole 384kB) and then calls qnap_tsx09_check_mac_addr() -> mac_pton() ->
strlen() on this memory block. as there is no 0 byte in the file on the
27th block, strlen() runs into bad memory and the machine panics. The old
code had no strlen().

Actually mac_pton() doesn't need to call strlen(), the following loop
catches short strings quite nicely. The strlen() seems to be an
optimization for calls to mac_pton with empty string. But this is rarely
the case and this is not a hot path. Remove it to reduce code size and
speed up calls with an not empty string.

Besides fixing the crash there is are other users interested in
this change, see https://patchwork.ozlabs.org/patch/851008/

Fixes: 4904dbda41c8 ("ARM: orion5x: use mac_pton() helper")
Signed-off-by: Stefan Hellermann <stefan@the2masters.de>
Cc: <stable@vger.kernel.org> [4.4+]
---
 lib/net_utils.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/lib/net_utils.c b/lib/net_utils.c
index af525353395d..9d38da67ee44 100644
--- a/lib/net_utils.c
+++ b/lib/net_utils.c
@@ -8,10 +8,6 @@ bool mac_pton(const char *s, u8 *mac)
 {
 	int i;
 
-	/* XX:XX:XX:XX:XX:XX */
-	if (strlen(s) < 3 * ETH_ALEN - 1)
-		return false;
-
 	/* Don't dirty result unless string is valid MAC. */
 	for (i = 0; i < ETH_ALEN; i++) {
 		if (!isxdigit(s[i * 3]) || !isxdigit(s[i * 3 + 1]))
-- 
2.11.0

[PATCH] net: Allow mac_pton() to work on non-NULL terminated strings

[Stefan Hellermann]

Commit 4904dbda41c8 ("ARM: orion5x: use mac_pton() helper") crashes my
QNAP TS-209 NAS early on boot.

The boot code for the TS-209 is looping through an ext2 filesystem on a
384kB mtd partition (factory configuration put there by QNAP). There it
looks on every 1kB boundary if there is a valid MAC address. The
filesystem has a 1kB block size, so this seems to work.

On my device the MAC address is on the 37th 1kB block. But: On the 27th
block is a large file (1,5kB) without 0 bytes inside. The code in
qnap_tsx09_find_mac_addr() maps 1kB into memory (not a whole file or the
whole 384kB) and then calls qnap_tsx09_check_mac_addr() -> mac_pton() ->
strlen() on this memory block. as there is no 0 byte in the file on the
27th block, strlen() runs into bad memory and the machine panics. The old
code had no strlen().

Actually mac_pton() doesn't need to call strlen(), the following loop
catches short strings quite nicely. The strlen() seems to be an
optimization for calls to mac_pton with empty string. But this is rarely
the case and this is not a hot path. Remove it to reduce code size and
speed up calls with an not empty string.

Besides fixing the crash there is are other users interested in
this change, see https://patchwork.ozlabs.org/patch/851008/

Fixes: 4904dbda41c8 ("ARM: orion5x: use mac_pton() helper")
Signed-off-by: Stefan Hellermann <stefan@the2masters.de>
Cc: <stable@vger.kernel.org> [4.4+]
---
 lib/net_utils.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/lib/net_utils.c b/lib/net_utils.c
index af525353395d..9d38da67ee44 100644
--- a/lib/net_utils.c
+++ b/lib/net_utils.c
@@ -8,10 +8,6 @@ bool mac_pton(const char *s, u8 *mac)
 {
 	int i;
 
-	/* XX:XX:XX:XX:XX:XX */
-	if (strlen(s) < 3 * ETH_ALEN - 1)
-		return false;
-
 	/* Don't dirty result unless string is valid MAC. */
 	for (i = 0; i < ETH_ALEN; i++) {
 		if (!isxdigit(s[i * 3]) || !isxdigit(s[i * 3 + 1]))
-- 
2.11.0

[PATCH] net: Allow mac_pton() to work on non-NULL terminated strings

[Stefan Hellermann]

Commit 4904dbda41c8 ("ARM: orion5x: use mac_pton() helper") crashes my
QNAP TS-209 NAS early on boot.

The boot code for the TS-209 is looping through an ext2 filesystem on a
384kB mtd partition (factory configuration put there by QNAP). There it
looks on every 1kB boundary if there is a valid MAC address. The
filesystem has a 1kB block size, so this seems to work.

On my device the MAC address is on the 37th 1kB block. But: On the 27th
block is a large file (1,5kB) without 0 bytes inside. The code in
qnap_tsx09_find_mac_addr() maps 1kB into memory (not a whole file or the
whole 384kB) and then calls qnap_tsx09_check_mac_addr() -> mac_pton() ->
strlen() on this memory block. as there is no 0 byte in the file on the
27th block, strlen() runs into bad memory and the machine panics. The old
code had no strlen().

Actually mac_pton() doesn't need to call strlen(), the following loop
catches short strings quite nicely. The strlen() seems to be an
optimization for calls to mac_pton with empty string. But this is rarely
the case and this is not a hot path. Remove it to reduce code size and
speed up calls with an not empty string.

Besides fixing the crash there is are other users interested in
this change, see https://patchwork.ozlabs.org/patch/851008/

Fixes: 4904dbda41c8 ("ARM: orion5x: use mac_pton() helper")
Signed-off-by: Stefan Hellermann <stefan@the2masters.de>
Cc: <stable@vger.kernel.org> [4.4+]
---
 lib/net_utils.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/lib/net_utils.c b/lib/net_utils.c
index af525353395d..9d38da67ee44 100644
--- a/lib/net_utils.c
+++ b/lib/net_utils.c
@@ -8,10 +8,6 @@ bool mac_pton(const char *s, u8 *mac)
 {
 	int i;
 
-	/* XX:XX:XX:XX:XX:XX */
-	if (strlen(s) < 3 * ETH_ALEN - 1)
-		return false;
-
 	/* Don't dirty result unless string is valid MAC. */
 	for (i = 0; i < ETH_ALEN; i++) {
 		if (!isxdigit(s[i * 3]) || !isxdigit(s[i * 3 + 1]))
-- 
2.11.0

Re: [PATCH] net: Allow mac_pton() to work on non-NULL terminated strings

[Andrew Lunn]

On Fri, Feb 23, 2018 at 09:17:48PM +0100, Stefan Hellermann wrote:
> Commit 4904dbda41c8 ("ARM: orion5x: use mac_pton() helper") crashes my
> QNAP TS-209 NAS early on boot.
> 
> The boot code for the TS-209 is looping through an ext2 filesystem on a
> 384kB mtd partition (factory configuration put there by QNAP). There it
> looks on every 1kB boundary if there is a valid MAC address. The
> filesystem has a 1kB block size, so this seems to work.
> 
> On my device the MAC address is on the 37th 1kB block. But: On the 27th
> block is a large file (1,5kB) without 0 bytes inside. The code in
> qnap_tsx09_find_mac_addr() maps 1kB into memory (not a whole file or the
> whole 384kB) and then calls qnap_tsx09_check_mac_addr() -> mac_pton() ->
> strlen() on this memory block. as there is no 0 byte in the file on the
> 27th block, strlen() runs into bad memory and the machine panics. The old
> code had no strlen().
> 
> Actually mac_pton() doesn't need to call strlen(), the following loop
> catches short strings quite nicely. The strlen() seems to be an
> optimization for calls to mac_pton with empty string. But this is rarely
> the case and this is not a hot path. Remove it to reduce code size and
> speed up calls with an not empty string.
> 
> Besides fixing the crash there is are other users interested in
> this change, see https://patchwork.ozlabs.org/patch/851008/
> 
> Fixes: 4904dbda41c8 ("ARM: orion5x: use mac_pton() helper")
> Signed-off-by: Stefan Hellermann <stefan@the2masters.de>
> Cc: <stable@vger.kernel.org> [4.4+]

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

Re: [PATCH] net: Allow mac_pton() to work on non-NULL terminated strings

[Andrew Lunn]

On Fri, Feb 23, 2018 at 09:17:48PM +0100, Stefan Hellermann wrote:
> Commit 4904dbda41c8 ("ARM: orion5x: use mac_pton() helper") crashes my
> QNAP TS-209 NAS early on boot.
> 
> The boot code for the TS-209 is looping through an ext2 filesystem on a
> 384kB mtd partition (factory configuration put there by QNAP). There it
> looks on every 1kB boundary if there is a valid MAC address. The
> filesystem has a 1kB block size, so this seems to work.
> 
> On my device the MAC address is on the 37th 1kB block. But: On the 27th
> block is a large file (1,5kB) without 0 bytes inside. The code in
> qnap_tsx09_find_mac_addr() maps 1kB into memory (not a whole file or the
> whole 384kB) and then calls qnap_tsx09_check_mac_addr() -> mac_pton() ->
> strlen() on this memory block. as there is no 0 byte in the file on the
> 27th block, strlen() runs into bad memory and the machine panics. The old
> code had no strlen().
> 
> Actually mac_pton() doesn't need to call strlen(), the following loop
> catches short strings quite nicely. The strlen() seems to be an
> optimization for calls to mac_pton with empty string. But this is rarely
> the case and this is not a hot path. Remove it to reduce code size and
> speed up calls with an not empty string.
> 
> Besides fixing the crash there is are other users interested in
> this change, see https://patchwork.ozlabs.org/patch/851008/
> 
> Fixes: 4904dbda41c8 ("ARM: orion5x: use mac_pton() helper")
> Signed-off-by: Stefan Hellermann <stefan@the2masters.de>
> Cc: <stable@vger.kernel.org> [4.4+]

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

[PATCH] net: Allow mac_pton() to work on non-NULL terminated strings

[Andrew Lunn]

On Fri, Feb 23, 2018 at 09:17:48PM +0100, Stefan Hellermann wrote:
> Commit 4904dbda41c8 ("ARM: orion5x: use mac_pton() helper") crashes my
> QNAP TS-209 NAS early on boot.
> 
> The boot code for the TS-209 is looping through an ext2 filesystem on a
> 384kB mtd partition (factory configuration put there by QNAP). There it
> looks on every 1kB boundary if there is a valid MAC address. The
> filesystem has a 1kB block size, so this seems to work.
> 
> On my device the MAC address is on the 37th 1kB block. But: On the 27th
> block is a large file (1,5kB) without 0 bytes inside. The code in
> qnap_tsx09_find_mac_addr() maps 1kB into memory (not a whole file or the
> whole 384kB) and then calls qnap_tsx09_check_mac_addr() -> mac_pton() ->
> strlen() on this memory block. as there is no 0 byte in the file on the
> 27th block, strlen() runs into bad memory and the machine panics. The old
> code had no strlen().
> 
> Actually mac_pton() doesn't need to call strlen(), the following loop
> catches short strings quite nicely. The strlen() seems to be an
> optimization for calls to mac_pton with empty string. But this is rarely
> the case and this is not a hot path. Remove it to reduce code size and
> speed up calls with an not empty string.
> 
> Besides fixing the crash there is are other users interested in
> this change, see https://patchwork.ozlabs.org/patch/851008/
> 
> Fixes: 4904dbda41c8 ("ARM: orion5x: use mac_pton() helper")
> Signed-off-by: Stefan Hellermann <stefan@the2masters.de>
> Cc: <stable@vger.kernel.org> [4.4+]

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

Re: [PATCH] net: Allow mac_pton() to work on non-NULL terminated strings

[Alexey Dobriyan]

On Fri, Feb 23, 2018 at 09:17:48PM +0100, Stefan Hellermann wrote:
> @@ -8,10 +8,6 @@ bool mac_pton(const char *s, u8 *mac)
>  {
>  	int i;
>  
> -	/* XX:XX:XX:XX:XX:XX */
> -	if (strlen(s) < 3 * ETH_ALEN - 1)
> -		return false;
> -
>  	/* Don't dirty result unless string is valid MAC. */
>  	for (i = 0; i < ETH_ALEN; i++) {
>  		if (!isxdigit(s[i * 3]) || !isxdigit(s[i * 3 + 1]))

Short string will bail in the loop, indeed.

Reviewed-by: Alexey Dobriyan <adobriyan@gmail.com>

Re: [PATCH] net: Allow mac_pton() to work on non-NULL terminated strings

[Alexey Dobriyan]

On Fri, Feb 23, 2018 at 09:17:48PM +0100, Stefan Hellermann wrote:
> @@ -8,10 +8,6 @@ bool mac_pton(const char *s, u8 *mac)
>  {
>  	int i;
>  
> -	/* XX:XX:XX:XX:XX:XX */
> -	if (strlen(s) < 3 * ETH_ALEN - 1)
> -		return false;
> -
>  	/* Don't dirty result unless string is valid MAC. */
>  	for (i = 0; i < ETH_ALEN; i++) {
>  		if (!isxdigit(s[i * 3]) || !isxdigit(s[i * 3 + 1]))

Short string will bail in the loop, indeed.

Reviewed-by: Alexey Dobriyan <adobriyan@gmail.com>

[PATCH] net: Allow mac_pton() to work on non-NULL terminated strings

[Alexey Dobriyan]

On Fri, Feb 23, 2018 at 09:17:48PM +0100, Stefan Hellermann wrote:
> @@ -8,10 +8,6 @@ bool mac_pton(const char *s, u8 *mac)
>  {
>  	int i;
>  
> -	/* XX:XX:XX:XX:XX:XX */
> -	if (strlen(s) < 3 * ETH_ALEN - 1)
> -		return false;
> -
>  	/* Don't dirty result unless string is valid MAC. */
>  	for (i = 0; i < ETH_ALEN; i++) {
>  		if (!isxdigit(s[i * 3]) || !isxdigit(s[i * 3 + 1]))

Short string will bail in the loop, indeed.

Reviewed-by: Alexey Dobriyan <adobriyan@gmail.com>

Re: [PATCH] net: Allow mac_pton() to work on non-NULL terminated strings

[Andy Shevchenko]

On Fri, 2018-02-23 at 23:41 +0300, Alexey Dobriyan wrote:
> On Fri, Feb 23, 2018 at 09:17:48PM +0100, Stefan Hellermann wrote:
> > @@ -8,10 +8,6 @@ bool mac_pton(const char *s, u8 *mac)
> >  {
> >  	int i;
> >  
> > -	/* XX:XX:XX:XX:XX:XX */
> > -	if (strlen(s) < 3 * ETH_ALEN - 1)
> > -		return false;
> > -
> >  	/* Don't dirty result unless string is valid MAC. */
> >  	for (i = 0; i < ETH_ALEN; i++) {
> >  		if (!isxdigit(s[i * 3]) || !isxdigit(s[i * 3 + 1]))
> 
> Short string will bail in the loop, indeed.
> 
> Reviewed-by: Alexey Dobriyan <adobriyan@gmail.com>

Since the author is okay with the change, I'm following:

Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [PATCH] net: Allow mac_pton() to work on non-NULL terminated strings

[Andy Shevchenko]

On Fri, 2018-02-23 at 23:41 +0300, Alexey Dobriyan wrote:
> On Fri, Feb 23, 2018 at 09:17:48PM +0100, Stefan Hellermann wrote:
> > @@ -8,10 +8,6 @@ bool mac_pton(const char *s, u8 *mac)
> >  {
> >  	int i;
> >  
> > -	/* XX:XX:XX:XX:XX:XX */
> > -	if (strlen(s) < 3 * ETH_ALEN - 1)
> > -		return false;
> > -
> >  	/* Don't dirty result unless string is valid MAC. */
> >  	for (i = 0; i < ETH_ALEN; i++) {
> >  		if (!isxdigit(s[i * 3]) || !isxdigit(s[i * 3 + 1]))
> 
> Short string will bail in the loop, indeed.
> 
> Reviewed-by: Alexey Dobriyan <adobriyan@gmail.com>

Since the author is okay with the change, I'm following:

Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

[PATCH] net: Allow mac_pton() to work on non-NULL terminated strings

[Andy Shevchenko]

On Fri, 2018-02-23 at 23:41 +0300, Alexey Dobriyan wrote:
> On Fri, Feb 23, 2018 at 09:17:48PM +0100, Stefan Hellermann wrote:
> > @@ -8,10 +8,6 @@ bool mac_pton(const char *s, u8 *mac)
> >  {
> >  	int i;
> >  
> > -	/* XX:XX:XX:XX:XX:XX */
> > -	if (strlen(s) < 3 * ETH_ALEN - 1)
> > -		return false;
> > -
> >  	/* Don't dirty result unless string is valid MAC. */
> >  	for (i = 0; i < ETH_ALEN; i++) {
> >  		if (!isxdigit(s[i * 3]) || !isxdigit(s[i * 3 + 1]))
> 
> Short string will bail in the loop, indeed.
> 
> Reviewed-by: Alexey Dobriyan <adobriyan@gmail.com>

Since the author is okay with the change, I'm following:

Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [PATCH] net: Allow mac_pton() to work on non-NULL terminated strings

[David Miller]

From: Stefan Hellermann <stefan@the2masters.de>
Date: Fri, 23 Feb 2018 21:17:48 +0100

> Commit 4904dbda41c8 ("ARM: orion5x: use mac_pton() helper") crashes my
> QNAP TS-209 NAS early on boot.
> 
> The boot code for the TS-209 is looping through an ext2 filesystem on a
> 384kB mtd partition (factory configuration put there by QNAP). There it
> looks on every 1kB boundary if there is a valid MAC address. The
> filesystem has a 1kB block size, so this seems to work.
> 
> On my device the MAC address is on the 37th 1kB block. But: On the 27th
> block is a large file (1,5kB) without 0 bytes inside. The code in
> qnap_tsx09_find_mac_addr() maps 1kB into memory (not a whole file or the
> whole 384kB) and then calls qnap_tsx09_check_mac_addr() -> mac_pton() ->
> strlen() on this memory block. as there is no 0 byte in the file on the
> 27th block, strlen() runs into bad memory and the machine panics. The old
> code had no strlen().
> 
> Actually mac_pton() doesn't need to call strlen(), the following loop
> catches short strings quite nicely. The strlen() seems to be an
> optimization for calls to mac_pton with empty string. But this is rarely
> the case and this is not a hot path. Remove it to reduce code size and
> speed up calls with an not empty string.
> 
> Besides fixing the crash there is are other users interested in
> this change, see https://patchwork.ozlabs.org/patch/851008/
> 
> Fixes: 4904dbda41c8 ("ARM: orion5x: use mac_pton() helper")
> Signed-off-by: Stefan Hellermann <stefan@the2masters.de>
> Cc: <stable@vger.kernel.org> [4.4+]

Nope, instead the Fixes: tag commit should be completely reverted.

It is wrong on several levels.

If we don't check for NULL termination, only the caller knows if it
is safe to blindly parse the buffer and whether it's length is
legitimate, and in what manner it is ok to do so.

But that's not my main beef.

The major problem is that the orion5x code passes an __iomem pointer
to mac_pton().

It is never correct to use real C pointer dereferencing on an __iomem
pointer.  __iomem pointer accesses must go through the appropriate
iomem accessor functions and interfaces, because __iomem pointers are
opaque and not real C pointers.

sparse should have emitted a ton of warnings for the orion5x code, and
furthermore it should have done so when the __iomem pointer is passed
straight into mac_pton().

It is therefore the orion5x's code business to do such non-portable
operations, and to be able to assume that the mac buffer is safe to be
non-NULL terminated.

Therefore I am reverted the above mentioned commit to fix this
problem.

Thank you.

[PATCH] net: Allow mac_pton() to work on non-NULL terminated strings

[David Miller]

From: Stefan Hellermann <stefan@the2masters.de>
Date: Fri, 23 Feb 2018 21:17:48 +0100

> Commit 4904dbda41c8 ("ARM: orion5x: use mac_pton() helper") crashes my
> QNAP TS-209 NAS early on boot.
> 
> The boot code for the TS-209 is looping through an ext2 filesystem on a
> 384kB mtd partition (factory configuration put there by QNAP). There it
> looks on every 1kB boundary if there is a valid MAC address. The
> filesystem has a 1kB block size, so this seems to work.
> 
> On my device the MAC address is on the 37th 1kB block. But: On the 27th
> block is a large file (1,5kB) without 0 bytes inside. The code in
> qnap_tsx09_find_mac_addr() maps 1kB into memory (not a whole file or the
> whole 384kB) and then calls qnap_tsx09_check_mac_addr() -> mac_pton() ->
> strlen() on this memory block. as there is no 0 byte in the file on the
> 27th block, strlen() runs into bad memory and the machine panics. The old
> code had no strlen().
> 
> Actually mac_pton() doesn't need to call strlen(), the following loop
> catches short strings quite nicely. The strlen() seems to be an
> optimization for calls to mac_pton with empty string. But this is rarely
> the case and this is not a hot path. Remove it to reduce code size and
> speed up calls with an not empty string.
> 
> Besides fixing the crash there is are other users interested in
> this change, see https://patchwork.ozlabs.org/patch/851008/
> 
> Fixes: 4904dbda41c8 ("ARM: orion5x: use mac_pton() helper")
> Signed-off-by: Stefan Hellermann <stefan@the2masters.de>
> Cc: <stable@vger.kernel.org> [4.4+]

Nope, instead the Fixes: tag commit should be completely reverted.

It is wrong on several levels.

If we don't check for NULL termination, only the caller knows if it
is safe to blindly parse the buffer and whether it's length is
legitimate, and in what manner it is ok to do so.

But that's not my main beef.

The major problem is that the orion5x code passes an __iomem pointer
to mac_pton().

It is never correct to use real C pointer dereferencing on an __iomem
pointer.  __iomem pointer accesses must go through the appropriate
iomem accessor functions and interfaces, because __iomem pointers are
opaque and not real C pointers.

sparse should have emitted a ton of warnings for the orion5x code, and
furthermore it should have done so when the __iomem pointer is passed
straight into mac_pton().

It is therefore the orion5x's code business to do such non-portable
operations, and to be able to assume that the mac buffer is safe to be
non-NULL terminated.

Therefore I am reverted the above mentioned commit to fix this
problem.

Thank you.