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: <fb553ce5-c533-44a2-a134-fbb552f247bb@kili.mountain> Date: Tue, 30 May 2023 17:40:11 +0300 From: Dan Carpenter <dan.carpenter@...aro.org> To: Andrew Lunn <andrew@...n.ch> Cc: Jakub Kicinski <kuba@...nel.org>, Russell King <linux@...linux.org.uk>, 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 02:39:53PM +0200, Andrew Lunn wrote: > On Mon, May 29, 2023 at 09:58:02PM -0700, Jakub Kicinski wrote: > > On Fri, 26 May 2023 14:45:54 +0300 Dan Carpenter wrote: > > > The "val" variable is used to store error codes from phy_read() so > > > it needs to be signed for the error handling to work as expected. > > > > > > Fixes: 014068dcb5b1 ("net: phy: genphy_loopback: add link speed configuration") > > > Signed-off-by: Dan Carpenter <dan.carpenter@...aro.org> > > > > Is it going to be obvious to PHY-savvy folks that the val passed to > > phy_read_poll_timeout() must be an int? Is it a very common pattern? > > My outsider intuition is that since regs are 16b, u16 is reasonable, > > and more people may make the same mistake. > > It is common to get this wrong in general with PHY drivers. Dan > regularly posts fixes like this soon after a PHY driver patch it > merged. I really wish we could somehow get the compiler to warn when > the result from phy_read() is stored into a unsigned type. It would > save Dan a lot of work. I don't see these as much as I used. It's maybe once per month. I'm not sure why, maybe kbuild emails everyone before I see it? GCC will warn about this with -Wtype-limits. Clang will also trigger a warning. The Smatch check for this had a bug where it only warned about if (x < 0) { if x was u32 or larger. I fixed that bug which is why I was looking at this code. I will push the fix for that in a couple days. regards, dan carpenter
Powered by blists - more mailing lists