On Mon, Jun 05, 2017 at 09:56:18AM -0700, Florian Fainelli wrote: > On 06/05/2017 05:15 AM, Levin, Alexander (Sasha Levin) wrote: > > On Sun, Jun 04, 2017 at 10:17:49AM -0700, Florian Fainelli wrote: > >> Hi Alex, > >> > >> On 06/04/2017 01:12 AM, Levin, Alexander (Sasha Levin) wrote: > >>> From: Mao Wenan <maowenan@xxxxxxxxxx> > >>> > >>> [ Upstream commit cafe8df8b9bc9aa3dffa827c1a6757c6cd36f657 ] > >>> > >>> 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: > >>> > >>> [ 43.361162] BUG: unable to handle kernel NULL pointer dereference at 0000000000000140 > >>> [ 43.361162] IP: phy_state_machine+0x32/0x490 > >>> [ 43.361162] PGD 59dc067 > >>> [ 43.361162] PUD 0 > >>> [ 43.361162] > >>> [ 43.361162] Oops: 0000 [#1] SMP > >>> [ 43.361162] Modules linked in: dsa_loop [last unloaded: broadcom] > >>> [ 43.361162] CPU: 0 PID: 1299 Comm: kworker/0:3 Not tainted 4.10.0-rc5+ #415 > >>> [ 43.361162] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), > >>> BIOS Ubuntu-1.8.2-1ubuntu2 04/01/2014 > >>> [ 43.361162] Workqueue: events_power_efficient phy_state_machine > >>> [ 43.361162] task: ffff880006782b80 task.stack: ffffc90000184000 > >>> [ 43.361162] RIP: 0010:phy_state_machine+0x32/0x490 > >>> [ 43.361162] RSP: 0018:ffffc90000187e18 EFLAGS: 00000246 > >>> [ 43.361162] RAX: 0000000000000000 RBX: ffff8800059e53c0 RCX: > >>> ffff880006a15c60 > >>> [ 43.361162] RDX: ffff880006782b80 RSI: 0000000000000000 RDI: > >>> ffff8800059e5428 > >>> [ 43.361162] RBP: ffffc90000187e48 R08: ffff880006a15c40 R09: > >>> 0000000000000000 > >>> [ 43.361162] R10: 0000000000000000 R11: 0000000000000000 R12: > >>> ffff8800059e5428 > >>> [ 43.361162] R13: ffff8800059e5000 R14: 0000000000000000 R15: > >>> ffff880006a15c40 > >>> [ 43.361162] FS: 0000000000000000(0000) GS:ffff880006a00000(0000) > >>> knlGS:0000000000000000 > >>> [ 43.361162] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 > >>> [ 43.361162] CR2: 0000000000000140 CR3: 0000000005979000 CR4: > >>> 00000000000006f0 > >>> [ 43.361162] Call Trace: > >>> [ 43.361162] process_one_work+0x1b4/0x3e0 > >>> [ 43.361162] worker_thread+0x43/0x4d0 > >>> [ 43.361162] ? __schedule+0x17f/0x4e0 > >>> [ 43.361162] kthread+0xf7/0x130 > >>> [ 43.361162] ? process_one_work+0x3e0/0x3e0 > >>> [ 43.361162] ? kthread_create_on_node+0x40/0x40 > >>> [ 43.361162] ret_from_fork+0x29/0x40 > >>> [ 43.361162] Code: 56 41 55 41 54 4c 8d 67 68 53 4c 8d af 40 fc ff ff > >>> 48 89 fb 4c 89 e7 48 83 ec 08 e8 c9 9d 27 00 48 8b 83 60 ff ff ff 44 8b > >>> 73 98 <48> 8b 90 40 01 00 00 44 89 f0 48 85 d2 74 08 4c 89 ef ff d2 8b > >>> > >>> Keep references on the PHY driver module right before we are going to > >>> utilize it in phy_attach_direct(), and conversely when we don't use it > >>> anymore in phy_detach(). > >>> > >>> Signed-off-by: Mao Wenan <maowenan@xxxxxxxxxx> > >>> [florian: rebase, rework commit message] > >>> Signed-off-by: Florian Fainelli <f.fainelli@xxxxxxxxx> > >>> Signed-off-by: David S. Miller <davem@xxxxxxxxxxxxx> > >>> > >>> Signed-off-by: Sasha Levin <alexander.levin@xxxxxxxxxxx> > >> > >> This commit alone will cause problems, you will also need to pick this > >> one on top of it: > >> > >> 6d9f66ac7fec2a6ccd649e5909806dfe36f1fc25 ("net: phy: Fix PHY module > >> checks and NULL deref in phy_attach_direct()") > > > > Should I also be grabbing a7dac9f9c1 > > ("phy: fix error case of phy_led_triggers_(un)register")? > > > > It says it fixes a commit that's not in -stable, but it looks like it's > > still relevant even without that commit. > > No, you don't have to pick this one, it does indeed fix something that > was only introduced in 4.10 and newer. Hm, can you ack this conflict resolution of applying 6d9f66ac7fe on top of 4.9.30 (in particular, the code in phy_attach_direct()): diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c index 686bca0ba820..a8082d3d70e3 100644 --- a/drivers/net/phy/phy_device.c +++ b/drivers/net/phy/phy_device.c @@ -860,6 +860,7 @@ int phy_attach_direct(struct net_device *dev, struct phy_device *phydev, struct module *ndev_owner = dev->dev.parent->driver->owner; struct mii_bus *bus = phydev->mdio.bus; struct device *d = &phydev->mdio.dev; + bool using_genphy = false; int err; /* For Ethernet device drivers that register their own MDIO bus, we @@ -872,11 +873,6 @@ int phy_attach_direct(struct net_device *dev, struct phy_device *phydev, return -EIO; } - if (!try_module_get(d->driver->owner)) { - dev_err(&dev->dev, "failed to get the device driver module\n"); - return -EIO; - } - get_device(d); /* Assume that if there is no driver, that it doesn't @@ -890,12 +886,22 @@ int phy_attach_direct(struct net_device *dev, struct phy_device *phydev, d->driver = &genphy_driver[GENPHY_DRV_1G].mdiodrv.driver; + using_genphy = true; + } + + if (!try_module_get(d->driver->owner)) { + dev_err(&dev->dev, "failed to get the device driver module\n"); + err = -EIO; + goto error_put_device; + } + + if (using_genphy) { err = d->driver->probe(d); if (err >= 0) err = device_bind_driver(d); if (err) - goto error; + goto error_module_put; } if (phydev->attached_dev) { @@ -931,8 +937,12 @@ int phy_attach_direct(struct net_device *dev, struct phy_device *phydev, return err; error: - put_device(d); + return err; + +error_module_put: module_put(d->driver->owner); +error_put_device: + put_device(d); if (ndev_owner != bus->owner) module_put(bus->owner); return err; @@ -993,6 +1003,8 @@ void phy_detach(struct phy_device *phydev) phydev->attached_dev = NULL; phy_suspend(phydev); + module_put(phydev->mdio.dev.driver->owner); + /* If the device had no specific driver before (i.e. - it * was using the generic driver), we unbind the device * from the generic driver so that there's a chance a @@ -1013,7 +1025,6 @@ void phy_detach(struct phy_device *phydev) bus = phydev->mdio.bus; put_device(&phydev->mdio.dev); - module_put(phydev->mdio.dev.driver->owner); if (ndev_owner != bus->owner) module_put(bus->owner); } -- Thanks, Sasha