netdev - Re: Correct way to access MDIO bus

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

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <55DBB3AE.1000603@gmail.com>
Date:	Mon, 24 Aug 2015 17:15:42 -0700
From:	Florian Fainelli <f.fainelli@...il.com>
To:	Russell King - ARM Linux <linux@....linux.org.uk>,
	netdev@...r.kernel.org, David Miller <davem@...emloft.net>,
	Giuseppe CAVALLARO <peppe.cavallaro@...com>,
	Ben Hutchings <ben@...adent.org.uk>
Subject: Re: Correct way to access MDIO bus - phy.c seems buggy

On 24/08/15 17:09, Russell King - ARM Linux wrote:
> Hi,
> 
> While trying to track down instability in the FEC driver, I've come
> across this question: what is the correct way to access the MDIO bus?
> 
> Is it via:
> 
> 	bus->write()
> 
> where 'bus' is a struct mii_bus, or should it be via mdiobus_write()?
> 
> What I'm seeing in the FEC driver is two thread trying to access the
> MDIO bus simultaneously - one thread trying to do a read, and another
> trying to do a write.  The result is far from pretty with the current
> mainline code, because we can end up re-initialising a spinlock while
> it's held by the fec interrupt handler.
> 
> I think the correct answer is that mdiobus_write() should be used,
> which makes drivers/net/phy/phy.c horribly buggy, as it bypasses the
> locking at the mdiobus level by doing this:

Right the correct way is to use mdiobus_write() which takes the bus mutex.

> 
> mmd_phy_indirect()
> {
> 	bus->write(bus, addr, MII_MMD_CTRL, devad);
> 	bus->write(bus, addr, MII_MMD_DATA, prtad);
> 	bus->write(bus, addr, MII_MMD_CTRL, (devad | MII_MMD_CTRL_NOINCR));
> }
> 
> However, it's not as simple as that, because the whole set of writes
> need to be done atomically.  The mdio bus lock needs to be taken around
> the internals of phy_read_mmd_indirect() and phy_write_mmd_indirect().

Well, yes, the bus lock should be grabbed at the beginning and released
at the end of this function at the very least, good catch.

> 
> This bug can be provoked by running an ethtool command which accesses
> the phy in a tight loop on a SMP platform.  For example:
> 
> 	while :; do ethtool --show-eee eth0; done
> 
> Patch will follow tomorrow.

Good thing is it looks like you have isolated the only cases where we do
not grab the MDIO bus mutexm the rest of the code, except
phy_mmd_{read,write}_indirect() looks correct.
-- 
Florian
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html