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: <20161123144636.GK14947@lunn.ch>
Date:   Wed, 23 Nov 2016 15:46:36 +0100
From:   Andrew Lunn <andrew@...n.ch>
To:     "Allan W. Nielsen" <allan.nielsen@...rosemi.com>
Cc:     Florian Fainelli <f.fainelli@...il.com>, 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

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

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.

	  Andrew