* [PATCH v2 1/1] ARM: orion5x: use mac_pton() helper @ 2015-10-02 14:12 Andy Shevchenko 2015-10-13 11:27 ` Detlef Vollmann 2018-02-22 17:45 ` [v2,1/1] " Stefan Hellermann 0 siblings, 2 replies; 29+ messages in thread From: Andy Shevchenko @ 2015-10-02 14:12 UTC (permalink / raw) To: linux-arm-kernel 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 ^ permalink raw reply related [flat|nested] 29+ messages in thread
* [PATCH v2 1/1] ARM: orion5x: use mac_pton() helper 2015-10-02 14:12 [PATCH v2 1/1] ARM: orion5x: use mac_pton() helper Andy Shevchenko @ 2015-10-13 11:27 ` Detlef Vollmann 2015-10-15 7:51 ` Gregory CLEMENT 2018-02-22 17:45 ` [v2,1/1] " Stefan Hellermann 1 sibling, 1 reply; 29+ messages in thread From: Detlef Vollmann @ 2015-10-13 11:27 UTC (permalink / raw) To: linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 29+ messages in thread
* [PATCH v2 1/1] ARM: orion5x: use mac_pton() helper 2015-10-13 11:27 ` Detlef Vollmann @ 2015-10-15 7:51 ` Gregory CLEMENT 0 siblings, 0 replies; 29+ messages in thread From: Gregory CLEMENT @ 2015-10-15 7:51 UTC (permalink / raw) To: linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 29+ messages in thread
* [v2,1/1] ARM: orion5x: use mac_pton() helper 2015-10-02 14:12 [PATCH v2 1/1] ARM: orion5x: use mac_pton() helper Andy Shevchenko 2015-10-13 11:27 ` Detlef Vollmann @ 2018-02-22 17:45 ` Stefan Hellermann 2018-02-22 21:42 ` Andrew Lunn 1 sibling, 1 reply; 29+ messages in thread From: Stefan Hellermann @ 2018-02-22 17:45 UTC (permalink / raw) To: linux-arm-kernel 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); > } > ^ permalink raw reply [flat|nested] 29+ messages in thread
* [v2,1/1] ARM: orion5x: use mac_pton() helper 2018-02-22 17:45 ` [v2,1/1] " Stefan Hellermann @ 2018-02-22 21:42 ` Andrew Lunn 2018-02-22 23:18 ` Stefan Hellermann 0 siblings, 1 reply; 29+ messages in thread From: Andrew Lunn @ 2018-02-22 21:42 UTC (permalink / raw) To: linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 29+ messages in thread
* [v2,1/1] ARM: orion5x: use mac_pton() helper 2018-02-22 21:42 ` Andrew Lunn @ 2018-02-22 23:18 ` Stefan Hellermann 2018-02-22 23:34 ` Andrew Lunn 0 siblings, 1 reply; 29+ messages in thread From: Stefan Hellermann @ 2018-02-22 23:18 UTC (permalink / raw) To: linux-arm-kernel 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])) ^ permalink raw reply related [flat|nested] 29+ messages in thread
* [v2,1/1] ARM: orion5x: use mac_pton() helper 2018-02-22 23:18 ` Stefan Hellermann @ 2018-02-22 23:34 ` Andrew Lunn 2018-02-23 10:09 ` Andy Shevchenko 0 siblings, 1 reply; 29+ messages in thread From: Andrew Lunn @ 2018-02-22 23:34 UTC (permalink / raw) To: linux-arm-kernel > 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 ^ permalink raw reply [flat|nested] 29+ messages in thread
* [v2,1/1] ARM: orion5x: use mac_pton() helper 2018-02-22 23:34 ` Andrew Lunn @ 2018-02-23 10:09 ` Andy Shevchenko 2018-02-23 15:01 ` Andrew Lunn 0 siblings, 1 reply; 29+ messages in thread From: Andy Shevchenko @ 2018-02-23 10:09 UTC (permalink / raw) To: linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 29+ messages in thread
* [v2,1/1] ARM: orion5x: use mac_pton() helper 2018-02-23 10:09 ` Andy Shevchenko @ 2018-02-23 15:01 ` Andrew Lunn 2018-02-23 15:18 ` Andy Shevchenko 0 siblings, 1 reply; 29+ messages in thread From: Andrew Lunn @ 2018-02-23 15:01 UTC (permalink / raw) To: linux-arm-kernel > > 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 ^ permalink raw reply [flat|nested] 29+ messages in thread
* [v2,1/1] ARM: orion5x: use mac_pton() helper 2018-02-23 15:01 ` Andrew Lunn @ 2018-02-23 15:18 ` Andy Shevchenko 2018-02-23 15:51 ` Andrew Lunn 2018-02-23 18:20 ` Alexey Dobriyan 0 siblings, 2 replies; 29+ messages in thread From: Andy Shevchenko @ 2018-02-23 15:18 UTC (permalink / raw) To: linux-arm-kernel +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 ^ permalink raw reply [flat|nested] 29+ messages in thread
* [v2,1/1] ARM: orion5x: use mac_pton() helper 2018-02-23 15:18 ` Andy Shevchenko @ 2018-02-23 15:51 ` Andrew Lunn 2018-02-23 16:36 ` Stefan Hellermann 2018-02-23 18:20 ` Alexey Dobriyan 1 sibling, 1 reply; 29+ messages in thread From: Andrew Lunn @ 2018-02-23 15:51 UTC (permalink / raw) To: linux-arm-kernel > > 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 ^ permalink raw reply [flat|nested] 29+ messages in thread
* [v2,1/1] ARM: orion5x: use mac_pton() helper 2018-02-23 15:51 ` Andrew Lunn @ 2018-02-23 16:36 ` Stefan Hellermann 2018-02-23 16:57 ` Andy Shevchenko 2018-02-23 17:23 ` Andrew Lunn 0 siblings, 2 replies; 29+ messages in thread From: Stefan Hellermann @ 2018-02-23 16:36 UTC (permalink / raw) To: linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 29+ messages in thread
* [v2,1/1] ARM: orion5x: use mac_pton() helper 2018-02-23 16:36 ` Stefan Hellermann @ 2018-02-23 16:57 ` Andy Shevchenko 2018-02-23 17:23 ` Andrew Lunn 1 sibling, 0 replies; 29+ messages in thread From: Andy Shevchenko @ 2018-02-23 16:57 UTC (permalink / raw) To: linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 29+ messages in thread
* [v2,1/1] ARM: orion5x: use mac_pton() helper 2018-02-23 16:36 ` Stefan Hellermann 2018-02-23 16:57 ` Andy Shevchenko @ 2018-02-23 17:23 ` Andrew Lunn 1 sibling, 0 replies; 29+ messages in thread From: Andrew Lunn @ 2018-02-23 17:23 UTC (permalink / raw) To: linux-arm-kernel > 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 ^ permalink raw reply [flat|nested] 29+ messages in thread
* [v2,1/1] ARM: orion5x: use mac_pton() helper 2018-02-23 15:18 ` Andy Shevchenko 2018-02-23 15:51 ` Andrew Lunn @ 2018-02-23 18:20 ` Alexey Dobriyan 2018-02-23 20:17 ` Stefan Hellermann 1 sibling, 1 reply; 29+ messages in thread From: Alexey Dobriyan @ 2018-02-23 18:20 UTC (permalink / raw) To: linux-arm-kernel 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. ^ permalink raw reply [flat|nested] 29+ messages in thread
* [PATCH] net: Allow mac_pton() to work on non-NULL terminated strings 2018-02-23 18:20 ` Alexey Dobriyan 2018-02-23 20:17 ` Stefan Hellermann @ 2018-02-23 20:17 ` Stefan Hellermann 0 siblings, 0 replies; 29+ messages in thread From: Stefan Hellermann @ 2018-02-23 20:17 UTC (permalink / raw) To: netdev, adobriyan, andriy.shevchenko, andrew, linux, jason, dv, gregory.clement, linux-arm-kernel Cc: Stefan Hellermann, [ 4 . 4+ ] 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 ^ permalink raw reply related [flat|nested] 29+ messages in thread
* [PATCH] net: Allow mac_pton() to work on non-NULL terminated strings @ 2018-02-23 20:17 ` Stefan Hellermann 0 siblings, 0 replies; 29+ messages in thread From: Stefan Hellermann @ 2018-02-23 20:17 UTC (permalink / raw) To: linux-arm-kernel 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 ^ permalink raw reply related [flat|nested] 29+ messages in thread
* [PATCH] net: Allow mac_pton() to work on non-NULL terminated strings @ 2018-02-23 20:17 ` Stefan Hellermann 0 siblings, 0 replies; 29+ messages in thread From: Stefan Hellermann @ 2018-02-23 20:17 UTC (permalink / raw) To: netdev, adobriyan, andriy.shevchenko, andrew, linux, jason, dv, gregory.clement, linux-arm-kernel Cc: Stefan Hellermann, [ 4 . 4+ ] 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 ^ permalink raw reply related [flat|nested] 29+ messages in thread
* Re: [PATCH] net: Allow mac_pton() to work on non-NULL terminated strings 2018-02-23 20:17 ` Stefan Hellermann (?) @ 2018-02-23 20:27 ` Andrew Lunn -1 siblings, 0 replies; 29+ messages in thread From: Andrew Lunn @ 2018-02-23 20:27 UTC (permalink / raw) To: Stefan Hellermann Cc: linux, jason, netdev, [ 4 . 4+ ], linux-arm-kernel, gregory.clement, andriy.shevchenko, adobriyan, dv 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 ^ permalink raw reply [flat|nested] 29+ messages in thread
* [PATCH] net: Allow mac_pton() to work on non-NULL terminated strings @ 2018-02-23 20:27 ` Andrew Lunn 0 siblings, 0 replies; 29+ messages in thread From: Andrew Lunn @ 2018-02-23 20:27 UTC (permalink / raw) To: linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH] net: Allow mac_pton() to work on non-NULL terminated strings @ 2018-02-23 20:27 ` Andrew Lunn 0 siblings, 0 replies; 29+ messages in thread From: Andrew Lunn @ 2018-02-23 20:27 UTC (permalink / raw) To: Stefan Hellermann Cc: netdev, adobriyan, andriy.shevchenko, linux, jason, dv, gregory.clement, linux-arm-kernel, [ 4 . 4+ ] 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 ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH] net: Allow mac_pton() to work on non-NULL terminated strings 2018-02-23 20:17 ` Stefan Hellermann (?) @ 2018-02-23 20:41 ` Alexey Dobriyan -1 siblings, 0 replies; 29+ messages in thread From: Alexey Dobriyan @ 2018-02-23 20:41 UTC (permalink / raw) To: Stefan Hellermann Cc: andrew, linux, jason, netdev, [ 4 . 4+ ], linux-arm-kernel, gregory.clement, andriy.shevchenko, dv 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> ^ permalink raw reply [flat|nested] 29+ messages in thread
* [PATCH] net: Allow mac_pton() to work on non-NULL terminated strings @ 2018-02-23 20:41 ` Alexey Dobriyan 0 siblings, 0 replies; 29+ messages in thread From: Alexey Dobriyan @ 2018-02-23 20:41 UTC (permalink / raw) To: linux-arm-kernel 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> ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH] net: Allow mac_pton() to work on non-NULL terminated strings @ 2018-02-23 20:41 ` Alexey Dobriyan 0 siblings, 0 replies; 29+ messages in thread From: Alexey Dobriyan @ 2018-02-23 20:41 UTC (permalink / raw) To: Stefan Hellermann Cc: netdev, andriy.shevchenko, andrew, linux, jason, dv, gregory.clement, linux-arm-kernel, [ 4 . 4+ ] 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> ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH] net: Allow mac_pton() to work on non-NULL terminated strings 2018-02-23 20:41 ` Alexey Dobriyan (?) @ 2018-02-23 20:51 ` Andy Shevchenko -1 siblings, 0 replies; 29+ messages in thread From: Andy Shevchenko @ 2018-02-23 20:51 UTC (permalink / raw) To: Alexey Dobriyan, Stefan Hellermann Cc: andrew, linux, jason, netdev, [ 4 . 4+ ], linux-arm-kernel, gregory.clement, dv 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 ^ permalink raw reply [flat|nested] 29+ messages in thread
* [PATCH] net: Allow mac_pton() to work on non-NULL terminated strings @ 2018-02-23 20:51 ` Andy Shevchenko 0 siblings, 0 replies; 29+ messages in thread From: Andy Shevchenko @ 2018-02-23 20:51 UTC (permalink / raw) To: linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH] net: Allow mac_pton() to work on non-NULL terminated strings @ 2018-02-23 20:51 ` Andy Shevchenko 0 siblings, 0 replies; 29+ messages in thread From: Andy Shevchenko @ 2018-02-23 20:51 UTC (permalink / raw) To: Alexey Dobriyan, Stefan Hellermann Cc: netdev, andrew, linux, jason, dv, gregory.clement, linux-arm-kernel, [ 4 . 4+ ] 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 ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH] net: Allow mac_pton() to work on non-NULL terminated strings 2018-02-23 20:17 ` Stefan Hellermann @ 2018-02-26 18:37 ` David Miller -1 siblings, 0 replies; 29+ messages in thread From: David Miller @ 2018-02-26 18:37 UTC (permalink / raw) To: stefan Cc: netdev, adobriyan, andriy.shevchenko, andrew, linux, jason, dv, gregory.clement, linux-arm-kernel, stable 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. ^ permalink raw reply [flat|nested] 29+ messages in thread
* [PATCH] net: Allow mac_pton() to work on non-NULL terminated strings @ 2018-02-26 18:37 ` David Miller 0 siblings, 0 replies; 29+ messages in thread From: David Miller @ 2018-02-26 18:37 UTC (permalink / raw) To: linux-arm-kernel 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. ^ permalink raw reply [flat|nested] 29+ messages in thread
end of thread, other threads:[~2018-02-26 18:37 UTC | newest] Thread overview: 29+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2015-10-02 14:12 [PATCH v2 1/1] ARM: orion5x: use mac_pton() helper Andy Shevchenko 2015-10-13 11:27 ` Detlef Vollmann 2015-10-15 7:51 ` Gregory CLEMENT 2018-02-22 17:45 ` [v2,1/1] " Stefan Hellermann 2018-02-22 21:42 ` Andrew Lunn 2018-02-22 23:18 ` Stefan Hellermann 2018-02-22 23:34 ` Andrew Lunn 2018-02-23 10:09 ` Andy Shevchenko 2018-02-23 15:01 ` Andrew Lunn 2018-02-23 15:18 ` Andy Shevchenko 2018-02-23 15:51 ` Andrew Lunn 2018-02-23 16:36 ` Stefan Hellermann 2018-02-23 16:57 ` Andy Shevchenko 2018-02-23 17:23 ` Andrew Lunn 2018-02-23 18:20 ` Alexey Dobriyan 2018-02-23 20:17 ` [PATCH] net: Allow mac_pton() to work on non-NULL terminated strings Stefan Hellermann 2018-02-23 20:17 ` Stefan Hellermann 2018-02-23 20:17 ` Stefan Hellermann 2018-02-23 20:27 ` Andrew Lunn 2018-02-23 20:27 ` Andrew Lunn 2018-02-23 20:27 ` Andrew Lunn 2018-02-23 20:41 ` Alexey Dobriyan 2018-02-23 20:41 ` Alexey Dobriyan 2018-02-23 20:41 ` Alexey Dobriyan 2018-02-23 20:51 ` Andy Shevchenko 2018-02-23 20:51 ` Andy Shevchenko 2018-02-23 20:51 ` Andy Shevchenko 2018-02-26 18:37 ` David Miller 2018-02-26 18:37 ` David Miller
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.