netdev - Re: [PATCH net-next 4/5] net: phy: bcm7xxx: Add support for downshift/Wirespeed

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: <2cdf4aad-ed42-2065-f13e-dfea3069fef3@gmail.com>
Date:   Wed, 23 Nov 2016 10:16:00 -0800
From:   Florian Fainelli <f.fainelli@...il.com>
To:     Andrew Lunn <andrew@...n.ch>,
        "Allan W. Nielsen" <allan.nielsen@...rosemi.com>
Cc:     netdev@...r.kernel.org, davem@...emloft.net,
        bcm-kernel-feedback-list@...adcom.com,
        raju.lakkaraju@...rosemi.com, vivien.didelot@...oirfairelinux.com
Subject: Re: [PATCH net-next 4/5] net: phy: bcm7xxx: Add support for
 downshift/Wirespeed

On 11/23/2016 06:46 AM, Andrew Lunn wrote:
>>>> Maybe we should think about this locking a bit. It is normal for the
>>>> lock to be held when using ops in the phy driver structure. The
>>>> exception is suspend/resume. Maybe we should also take the lock before
>>>> calling the phydev->drv->get_tunable() and phydev->drv->set_tunable()?
>>>
>>> Yes, that certainly seems like a good approach to me, let me cook a
>>> patch doing that.
>>
>> Just for my understanding (such that I will not make the same mistake again)...
>>
>> Why is it that phy functions such as get_wol needs to take the phy_lock and
>> others like get_tunable does not.
>>
>> I do understand the arguments on why the lock should be held by the caller of
>> get_tunable, but I do not understand why the same argument does not apply for
>> get_wol.
> 
> Hi Allan
> 
> phy_ethtool_get_wol and friends probably should take the
> phy_lock. This inconsistency is probably leading to locking
> bugs. e.g. at803x_set_wol() does a read-modify-write, and does not
> take the lock.
> 
> There is no comment in the patch adding phy_ethtool_set_wol() to say
> why the lock is not taken, and a quick look at the code does not
> suggest a reason why it could not be taken/released by
> phy_ethtool_set_wol().

Yes, this should happen. I don't see how we cannot have two user-space
processes not racing with each other here for instance, see
mv643xx_eth_get_wol and cpsw_get_wol.

> 
> I think it would be a good idea to change this.
> 
> phy_suspend()/phy_resume() might have good reasons to avoid the lock,
> i've no idea how it is supposed to work. Is there a danger something
> else is holding the lock and has already been suspended? I guess not,
> otherwise there is little hope suspend would work at all.

phy_suspend() and phy_resume() usually get called after phy_disconnect()
or phy_stop() have been invoked, and even then this is during the
Ethernet driver's suspend resume/resume path, so there is no room for
concurrency to occur (user space is quiesced, and the PHY state machine
is stopped/halted), but still, if we were to change the calling context
it would be a good idea to acquire phydev->lock.
-- 
Florian