linux-kernel - Re: [PATCH net v2] net: phy: mscc: fix deadlock in vsc85xx_default

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: <20181123175811.vjwv5krdrqdyq4ri@qschulz>
Date:   Fri, 23 Nov 2018 18:58:11 +0100
From:   Quentin Schulz <quentin.schulz@...tlin.com>
To:     Andrew Lunn <andrew@...n.ch>
Cc:     davem@...emloft.net, f.fainelli@...il.com,
        allan.nielsen@...rochip.com, linux-kernel@...r.kernel.org,
        netdev@...r.kernel.org, thomas.petazzoni@...tlin.com,
        alexandre.belloni@...tlin.com
Subject: Re: [PATCH net v2] net: phy: mscc: fix deadlock in
 vsc85xx_default_config

Hi Andrew,

On Fri, Nov 23, 2018 at 04:08:06PM +0100, Andrew Lunn wrote:
> On Fri, Nov 23, 2018 at 09:16:36AM +0100, Quentin Schulz wrote:
> > The vsc85xx_default_config function called in the vsc85xx_config_init
> > function which is used by VSC8530, VSC8531, VSC8540 and VSC8541 PHYs
> > mistakenly calls phy_read and phy_write in-between phy_select_page and
> > phy_restore_page.
> > 
> > phy_select_page and phy_restore_page actually take and release the MDIO
> > bus lock and phy_write and phy_read take and release the lock to write
> > or read to a PHY register.
> > 
> > Let's fix this deadlock by using phy_modify_paged which handles
> > correctly a read followed by a write in a non-standard page.
> > 
> > Fixes: 6a0bfbbe20b0 ("net: phy: mscc: migrate to phy_select/restore_page functions")
> > 
> > Signed-off-by: Quentin Schulz <quentin.schulz@...tlin.com>
> > ---
> > 
> > v2:
> >  - use phy_modify_paged instead of
> >  phy_select_page -> __phy_read -> __phy_write -> phy_restore_page
> > 
> >  drivers/net/phy/mscc.c | 13 ++++++-------
> >  1 file changed, 6 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/net/phy/mscc.c b/drivers/net/phy/mscc.c
> > index 62269e578718..4dcf7ad06259 100644
> > --- a/drivers/net/phy/mscc.c
> > +++ b/drivers/net/phy/mscc.c
> > @@ -810,17 +810,16 @@ static int vsc85xx_default_config(struct phy_device *phydev)
> >  
> >  	phydev->mdix_ctrl = ETH_TP_MDI_AUTO;
> >  	mutex_lock(&phydev->lock);
> > -	rc = phy_select_page(phydev, MSCC_PHY_PAGE_EXTENDED_2);
> > +
> > +	reg_val = RGMII_RX_CLK_DELAY_1_1_NS << RGMII_RX_CLK_DELAY_POS;
> > +
> > +	rc = phy_modify_paged(phydev, MSCC_PHY_PAGE_EXTENDED_2,
> > +			      MSCC_PHY_RGMII_CNTL, RGMII_RX_CLK_DELAY_MASK,
> > +			      reg_val);
> >  	if (rc < 0)
> >  		goto out_unlock;
> 
> Hi Quentin
> 
> Isn't this goto now pointless. You are not jumping over anything.
> 

Grmbl episode 2 :)

That's what you get from adding a line, taking the ones you're replacing
as reference and then remove the lines you've to replace without
checking what's left.

Sorry for the noise, I'll boot up my brain next time.

Thanks,
Quentin

Download attachment "signature.asc" of type "application/pgp-signature" (834 bytes)