ARM: orion5x: use mac_pton() helper

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

Signed-off-by Andy Shevchenko <[email protected]>

Cc Detlef Vollmann <[email protected]>

Cc Gregory CLEMENT <[email protected]>

---
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(-)

Quoted 5 lines · Andy Shevchenko

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

Signed-off-by Andy Shevchenko <[email protected]>

Cc Detlef Vollmann <[email protected]>

Cc Gregory CLEMENT <[email protected]>

Quoted 8 lines

---
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

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

Signed-off-by Andy Shevchenko <[email protected]>

Cc Detlef Vollmann <[email protected]>

Cc Gregory CLEMENT <[email protected]>

---
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(-)

arch/arm/mach-orion5x/Kconfig +3 -0

index 08d2be2..66f1c95 100644
@@ -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.

arch/arm/mach-orion5x/dns323-setup.c +3 -50

index f267e58..bc279a8 100644
@@ -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);

arch/arm/mach-orion5x/tsx09-common.c +4 -45

index 24b2959..d42e006 100644
@@ -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);
  		}
 

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.

Quoted 5 lines

Hi!

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

Quoted 2 lines

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?

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?

Quoted 11 lines

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.

Quoted 9 lines

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 <[email protected]>.

Quoted 11 lines

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")

Quoted 5 lines

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 <[email protected]>.

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

Quoted 14 lines

Quoted 14 lines

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")

Quoted 8 lines

Quoted 5 lines

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 <[email protected]>.

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 : .

Quoted 4 lines

Hi Andy

Thanks for pointing this patch out.

Quoted 3 lines

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.

Quoted 4 lines

Quoted 4 lines

Hi Andy

Thanks for pointing this patch out.

Quoted 8 lines

Quoted 3 lines

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

2018-02-23 16:51 GMT+01:00 Andrew Lunn <[email protected]>:

Quoted 18 lines

Quoted 10 lines

Quoted 9 lines

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 : .

Quoted 7 lines

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.

Quoted 8 lines

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: ?

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

+Cc Alexey

Quoted 23 lines · Andrew Lunn

Quoted 14 lines

Quoted 14 lines

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")

Quoted 8 lines

Quoted 5 lines

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 <[email protected]>.

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

Quoted 7 lines

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?

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 <[email protected]>

Cc <[email protected]> [4.4+]

lib/net_utils.c bool mac_pton(const char *s, u8 *mac) · line 13

Quoted 11 lines · Stefan Hellermann

lib/net_utils.c +0 -4

@@ -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 <[email protected]>

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 <[email protected]>

Cc <[email protected]> [4.4+]