linux-kernel - Re: [PATCH NET v3 1/2] net: phy: Add phy loopback support in net phy framework

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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <20170624135004.GK4875@lunn.ch>
Date:   Sat, 24 Jun 2017 15:50:04 +0200
From:   Andrew Lunn <andrew@...n.ch>
To:     Yunsheng Lin <linyunsheng@...wei.com>
Cc:     f.fainelli@...il.com, liguozhu@...ilicon.com, linuxarm@...wei.com,
        linux-kernel@...r.kernel.org, netdev@...r.kernel.org,
        davem@...emloft.net, tremyfr@...il.com
Subject: Re: [PATCH NET v3 1/2] net: phy: Add phy loopback support in net phy
 framework

> phy_resume call phydrv->resume without take mutex.
> if phy driver implement resume function, for example marvell_resume, then
> 
> static int marvell_resume(struct phy_device *phydev)
> {
> 	int err;
> 
> 	/* Resume the fiber mode first */
> 	if (!(phydev->supported & SUPPORTED_FIBRE)) {
> 		err = phy_write(phydev, MII_MARVELL_PHY_PAGE, MII_M1111_FIBER);
> 		if (err < 0)
> 			goto error;
> 
> 		/* With the page set, use the generic resume */
> 		err = genphy_resume(phydev);
> 		if (err < 0)
> 			goto error;
> 
> 		/* Then, the copper link */
> 		err = phy_write(phydev, MII_MARVELL_PHY_PAGE, MII_M1111_COPPER);
> 		if (err < 0)
> 			goto error;
> 	}
> 
> the code above executes without holding a mutex expect genphy_resume
> 
> 	/* With the page set, use the generic resume */
> 	return genphy_resume(phydev);
> 
> error:
> 	phy_write(phydev, MII_MARVELL_PHY_PAGE, MII_M1111_COPPER);
> 	return err;
> }
> 
> So I think the correct way to hold a lock is in phy_* function, not in genphy_*,
> and current genphy_resume and phy_resume is broken in this way.

Suspend and resume are a bit odd. As you found out, everywhere else,
the core takes the mutex before calling into the driver function.

The general rule of thumb, is that many driver writers don't
understand locking. So it is better to do the locking in the core,
which is generally written by people who do understand locking. And
core code gets used more, so bugs tend to be found.

     Andrew