lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
Open Source and information security mailing list archives
| ||
|
Message-ID: <ZHZqZyCJGZjraJ6P@shell.armlinux.org.uk> Date: Tue, 30 May 2023 22:28:07 +0100 From: "Russell King (Oracle)" <linux@...linux.org.uk> To: Andrew Lunn <andrew@...n.ch> Cc: Jakub Kicinski <kuba@...nel.org>, Dan Carpenter <dan.carpenter@...aro.org>, Oleksij Rempel <linux@...pel-privat.de>, Heiner Kallweit <hkallweit1@...il.com>, "David S. Miller" <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>, Paolo Abeni <pabeni@...hat.com>, netdev@...r.kernel.org, kernel-janitors@...r.kernel.org Subject: Re: [PATCH net] net: phy: fix a signedness bug in genphy_loopback() On Tue, May 30, 2023 at 10:09:24PM +0100, Russell King (Oracle) wrote: > Having thought about this, the best I can come up with is this, which > I think gives us everything we want without needing BUILD_BUG_ONs: > > #define phy_read_poll_timeout(phydev, regnum, val, cond, sleep_us, \ > timeout_us, sleep_before_read) \ > ({ \ > int __ret, __val; > __ret = read_poll_timeout(__val = phy_read, val, __val < 0 || (cond), \ > sleep_us, timeout_us, sleep_before_read, phydev, regnum); \ > if (__val < 0) \ > __ret = __val; \ > if (__ret) \ > phydev_err(phydev, "%s failed: %d\n", __func__, __ret); \ > __ret; \ > }) > > This looks rather horrid, but what it essentially does is: > > (val) = op(args); \ > if (cond) \ > break; \ > > expands to: > > (val) = __val = phy_read(args); > if (__val < 0 || (cond)) > break; > > As phy_read() returns an int, there is no cast or loss assigning it > to __val, since that is also an int. The conversion from int to > something else happens at the same point it always has. ... and actually produces nicer code on 32-bit ARM: Old (with the u16 val changed to an int val): 2f8: ebfffffe bl 0 <mdiobus_read> 2fc: e7e03150 ubfx r3, r0, #2, #1 extract bit 2 into r3 300: e1a04000 mov r4, r0 save return value 304: e2002004 and r2, r0, #4 extract bit 2 again 308: e1933fa0 orrs r3, r3, r0, lsr #31 grab sign bit 30c: 1a00000d bne 348 <genphy_loopback+0xd8> breaks out of loop if r3 is nonzero ... rest of loop ... ... 348: e3520000 cmp r2, #0 34c: 0a00000b beq 380 <genphy_loopback+0x110> basically tests whether bit 2 was zero, and jumps if it was. Basically (cond) is false. 350: e3540000 cmp r4, #0 354: a3a04000 movge r4, #0 358: ba00000a blt 388 <genphy_loopback+0x118> tests whether a phy_read returned an error and jumps if it did. r4 is basically __ret. ... 380: e3540000 cmp r4, #0 384: a3e0406d mvnge r4, #109 ; 0x6d if r4 (__ret) was >= 0, sets an error code (-ETIMEDOUT). 388: e1a03004 mov r3, r4 ... dev_err() bit. The new generated code is: 2f8: ebfffffe bl 0 <mdiobus_read> 2f8: R_ARM_CALL mdiobus_read 2fc: e2504000 subs r4, r0, #0 __val assignment 300: ba000014 blt 358 <genphy_loopback+0xe8> if <0, go direct to dev_err code 304: e3140004 tst r4, #4 cond test within loop 308: 1a00000d bne 344 <genphy_loopback+0xd4> ... rest of loop ... 344: e6ff4074 uxth r4, r4 cast to 16-bit uint 348: e3140004 tst r4, #4 test 34c: 13a04000 movne r4, #0 __ret is zero if bit set 350: 1a000007 bne 374 <genphy_loopback+0x104> basically returns 354: e3e0406d mvn r4, #109 ; 0x6d ... otherwise sets __ret to -ETIMEDOUT ... dev_err() code Is there a reason why it was written (cond) || val < 0 rather than val < 0 || (cond) ? Note that the order of these tests makes no difference in this situation, but I'm wondering whether it was intentional? -- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
Powered by blists - more mailing lists