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: <af16fdce-427e-c73a-b18b-d5f1c1d9d624@cogentembedded.com>
Date:   Thu, 24 May 2018 19:18:28 +0300
From:   Sergei Shtylyov <sergei.shtylyov@...entembedded.com>
To:     Vladimir Zapolskiy <vz@...ia.com>, Andrew Lunn <andrew@...n.ch>,
        Vladimir Zapolskiy <vladimir_zapolskiy@...tor.com>
Cc:     "David S. Miller" <davem@...emloft.net>, netdev@...r.kernel.org,
        linux-renesas-soc@...r.kernel.org
Subject: Re: [PATCH 1/6] ravb: remove custom .nway_reset from ethtool ops

Hello!

On 05/24/2018 05:11 PM, 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.

  You don't say that *not* grabbing the spinlock is safe... 

>>> Another note is that the change implicitly replaces phy_start_aneg()
>>> with a newer phy_restart_aneg().

   Hm, perphaps this could be a material for a separate patch? 

>>>
>>> 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.

   The -stable fixes can vary from version to version, IIUC. You could be
asked to backport your patch if Greg KH (or somebody else from the -stable
kernel maintainers) gets in trouble backporting your patch. 

> Hopefully Sergei as the driver maintainer can verify the fixes on

   I'm *not* a maintainer, just a humble reviewer! :-)

> older kernels and suggest the right kernel versions for backporting.

   This would be asking too much from me, I'm afraid...
   Still, Dave, could you please give me a couple of days to spend on
this series?

> --
> With best wishes,
> Vladimir

MBR, Sergei