netdev - Re: [PATCH 1/6] ravb: remove custom .nway

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <2bf96562-e402-d797-31e6-ba7e262e5637@mleia.com>
Date:   Thu, 24 May 2018 17:11:07 +0300
From:   Vladimir Zapolskiy <vz@...ia.com>
To:     Andrew Lunn <andrew@...n.ch>,
        Vladimir Zapolskiy <vladimir_zapolskiy@...tor.com>
Cc:     "David S. Miller" <davem@...emloft.net>,
        Sergei Shtylyov <sergei.shtylyov@...entembedded.com>,
        netdev@...r.kernel.org, linux-renesas-soc@...r.kernel.org
Subject: Re: [PATCH 1/6] ravb: remove custom .nway_reset from ethtool ops

On 05/24/2018 04:22 PM, Andrew Lunn wrote:
> On Thu, May 24, 2018 at 02:11:53PM +0300, Vladimir Zapolskiy wrote:
>> The change fixes a sleep in atomic context issue, which can be
>> always triggered by running 'ethtool -r' command, because
>> phy_start_aneg() protects phydev fields by a mutex.
>>
>> Another note is that the change implicitly replaces phy_start_aneg()
>> with a newer phy_restart_aneg().
>>
>> Signed-off-by: Vladimir Zapolskiy <vladimir_zapolskiy@...tor.com>
>> ---
>>  drivers/net/ethernet/renesas/ravb_main.c | 17 +----------------
>>  1 file changed, 1 insertion(+), 16 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
>> index 68f122140966..4a043eb0e2aa 100644
>> --- a/drivers/net/ethernet/renesas/ravb_main.c
>> +++ b/drivers/net/ethernet/renesas/ravb_main.c
>> @@ -1150,21 +1150,6 @@ static int ravb_set_link_ksettings(struct net_device *ndev,
>>  	return error;
>>  }
>>  
>> -static int ravb_nway_reset(struct net_device *ndev)
>> -{
>> -	struct ravb_private *priv = netdev_priv(ndev);
>> -	int error = -ENODEV;
>> -	unsigned long flags;
>> -
>> -	if (ndev->phydev) {
>> -		spin_lock_irqsave(&priv->lock, flags);
>> -		error = phy_start_aneg(ndev->phydev);
>> -		spin_unlock_irqrestore(&priv->lock, flags);
>> -	}
> 
> Eck! phylib assumes thread context and takes a mutex while calling
> into the PHY driver.
> 
> It would be good to add some sort of fixes: tag. Maybe for the commit
> that added the generic nway_reset? That would at least cover some
> stable kernels.
> 
> Reviewed-by: Andrew Lunn <andrew@...n.ch>
> 

Hi Andrew, thank you for review.

generally it makes sense to add Fixes tag, but as I said in
the commit message the problem is present before reused phy_ethtool_*()
functions were added to the kernel, so some kind of juggling with
the proper kernel version would be required in assumption that
the fixes are backported as an unmodified changes.

Hopefully Sergei as the driver maintainer can verify the fixes on
older kernels and suggest the right kernel versions for backporting.

--
With best wishes,
Vladimir