Re: [PATCH net] net: phy: Fix lack of reference count on PHY driver

[Florian Fainelli <f.fainelli@gmail.com>]

On 02/01/2017 02:51 AM, Russell King - ARM Linux wrote:
> On Wed, Feb 01, 2017 at 10:22:08AM +0000, Russell King - ARM Linux wrote:
>> On Tue, Jan 31, 2017 at 06:46:43PM -0800, Florian Fainelli wrote:
>>> From: Mao Wenan <maowenan@huawei.com>
>>>
>>> There is currently no reference count being held on the PHY driver,
>>> which makes it possible to remove the PHY driver module while the PHY
>>> state machine is running and polling the PHY. This could cause crashes
>>> similar to this one to show up:
>>
>> Does this really solve the problem?  What if you use sysfs to unbind the
>> driver but without removing the module?
> 
> I think that's a problem, and the patch is just solving a symptom of
> it.

You are right, but there is still a fundamental problem IMHO in that you
should not be able to rmmod a PHY driver as long as a network device is
attached to the PHY, and if the PHY driver is attached from several
different network devices, they should all have a way to prevent a PHY
driver rmmod, each of them incrementing the driver refcount, which is
what the patche from Maowan does here.


> 
> If a phy driver is unbound from a device, phy_remove() will be called.
> This will set the state to PHY_DOWN (under the mutex) before calling
> the driver's remove function (if any), and finally setting phydev->drv
> to NULL.
> 
> If phy_state_machine() is called after that point, then:
> 
> void phy_state_machine(struct work_struct *work)
> {
> ...
>         if (phydev->drv->link_change_notify)
>                 phydev->drv->link_change_notify(phydev);
> 
> which happens unconditionally, causes a NULL pointer dereference, which
> is probably the same NULL pointer dereference given in Mao Wenan's patch
> description.

Yep, that's exactly the location, but then after fixing that, we can
still crash in other locations, e.g: if we bring down an interface that
was attached to the PHY we would now crash in phy_suspend ->
phy_ethtool_get_wol

All of that can be fixed, and actually should be fixed, but it still
feels like we should have an easier way to prevent the driver removal IMHO.

> 
> It looks to me as if that's the only case where this can happen, so maybe
> the above needs to be:
> 
>         if (phydev->drv && phydev->drv->link_change_notify)
>                 phydev->drv->link_change_notify(phydev);
> 
> Also, I'd suggest making sure that the workqueue is flushed in
> phy_remove() after setting phydev->drv to NULL to ensure that the
> workqueue isn't running while the phy driver is being unbound, which
> should also make module removal safe(r).  I haven't fully analysed
> that though.

That is reasonable to do as well, thanks!
-- 
Florian