| 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
| ||
|
Message-ID: <F95AC9340317A84688A5F0DF0246F3F201524E04@szxeml504-mbs.china.huawei.com>
Date: Fri, 6 Jan 2017 02:29:52 +0000
From: maowenan <maowenan@...wei.com>
To: Florian Fainelli <f.fainelli@...il.com>,
David Laight <David.Laight@...LAB.COM>,
"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
Dingtianhong <dingtianhong@...wei.com>,
"weiyongjun (A)" <weiyongjun1@...wei.com>
Subject: RE: [PATCH] net:phy fix driver reference count error when attach
and detach phy device
> -----Original Message-----
> From: Florian Fainelli [mailto:f.fainelli@...il.com]
> Sent: Tuesday, December 13, 2016 12:33 AM
> To: maowenan; David Laight; netdev@...r.kernel.org; Dingtianhong;
> weiyongjun (A)
> Subject: Re: [PATCH] net:phy fix driver reference count error when attach and
> detach phy device
>
> On 12/12/2016 12:49 AM, maowenan wrote:
> >
> >
> > On 2016/12/5 16:47, maowenan wrote:
> >>
> >>
> >> On 2016/12/2 17:45, David Laight wrote:
> >>> From: Mao Wenan
> >>>> Sent: 30 November 2016 10:23
> >>>> The nic in my board use the phy dev from marvell, and the system
> >>>> will load the marvell phy driver automatically, but when I remove
> >>>> the phy drivers, the system immediately panic:
> >>>> Call trace:
> >>>> [ 2582.834493] [<ffff800000715384>] phy_state_machine+0x3c/0x438 [
> >>>> 2582.851754] [<ffff8000000db3b8>] process_one_work+0x150/0x428 [
> >>>> 2582.868188] [<ffff8000000db7d4>] worker_thread+0x144/0x4b0 [
> >>>> 2582.883882] [<ffff8000000e1d0c>] kthread+0xfc/0x110
> >>>>
> >>>> there should be proper reference counting in place to avoid that.
> >>>> I found that phy_attach_direct() forgets to add phy device driver
> >>>> reference count, and phy_detach() forgets to subtract reference count.
> >>>> This patch is to fix this bug, after that panic is disappeared when
> >>>> remove marvell.ko
> >>>>
> >>>> Signed-off-by: Mao Wenan <maowenan@...wei.com>
> >>>> ---
> >>>> drivers/net/phy/phy_device.c | 7 +++++++
> >>>> 1 file changed, 7 insertions(+)
> >>>>
> >>>> diff --git a/drivers/net/phy/phy_device.c
> >>>> b/drivers/net/phy/phy_device.c index 1a4bf8a..a7ec7c2 100644
> >>>> --- a/drivers/net/phy/phy_device.c
> >>>> +++ b/drivers/net/phy/phy_device.c
> >>>> @@ -866,6 +866,11 @@ 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;
> >>>> + }
> >>>
> >>> If this is the phy code, what stops the phy driver being unloaded
> >>> before the try_module_get() obtains a reference.
> >>> If it isn't the phy driver then there ought to be a reference count
> >>> obtained when the phy driver is located (by whatever decides which phy
> driver to use).
> >>> Even if that code later releases its reference (it probably
> >>> shouldn't on success) then you can't fail to get an extra reference here.
> >>
> >> [Mao Wenan]Yes, this is phy code, in function phy_attach_direct(),
> drivers/net/phy/phy_device.c.
> >> when one NIC driver to do probe behavior, it will attach one matched
> >> phy driver. phy_attach_direct() is to obtain phy driver reference and
> >> bind phy driver, if try_module_get() execute on success, the
> >> reference count is added; if failed, the driver can't be attached to this NIC,
> and it can't added the phy driver reference count. So before try_module_get
> obtains a reference, phy driver can't can't be bound to this NIC.
> >> when the phy driver is attached to NIC, the reference count is added,
> >> if someone remove phy driver directly, it will be failed because reference
> count is not equal to 0.
> >>
> >> An example of call trace when there is NIC driver to attch one phy driver:
> >> hns_nic_dev_probe->hns_nic_try_get_ae->hns_nic_init_phy->of_phy_conne
> >> ct->phy_connect_direct->phy_attach_direct
> >>
> >> Consider the steps of phy driver(marvell.ko) added and removed, and NIC
> driver(hns_enet_drv.ko) added and removed:
> >> 1)insmod marvell ref=0
> >> 2)insmod hns_enet_drv ref=1
> >> 3)rmmod marvell (should not on success, ref=1)
> >> 4)rmmod hns_enet_drv ref=0
> >> 5)rmmod marvell (should on success, because ref=0)
> >>
> >> if we don't add the reference count in phy_attach_direct(the second
> >> step ref=0), so the third step rmmod marvell will be panic, because there is
> one user remain use marvell driver and phy_stat_machine use the NULL drv
> pointer.
> >>
> >>>
> >>>> +
> >>>> get_device(d);
> >>>>
> >>>> /* Assume that if there is no driver, that it doesn't @@ -921,6
> >>>> +926,7 @@ int phy_attach_direct(struct net_device *dev, struct
> >>>> phy_device *phydev,
> >>>>
> >>>> error:
> >>>> put_device(d);
> >>>> + module_put(d->driver->owner);
> >>>
> >>> Are those two in the wrong order ?
> >>>
> >>>> module_put(bus->owner);
> >>>> return err;
> >>>> }
> >>>> @@ -998,6 +1004,7 @@ void phy_detach(struct phy_device *phydev)
> >>>> bus = phydev->mdio.bus;
> >>>>
> >>>> put_device(&phydev->mdio.dev);
> >>>> + module_put(phydev->mdio.dev.driver->owner);
> >>>> module_put(bus->owner);
> >>>
> >>> Where is this code called from?
> >>> You can't call it from the phy driver because the driver can be
> >>> unloaded as soon as the last reference is removed.
> >>> At that point the code memory is freed.
> >>
> >> [Mao Wenan] it is called by NIC when it is removed, which aims to
> >> disconnect one bound phy driver. If this phy driver is not used for this NIC,
> reference count should be subtracted, and phy driver can be removed if there is
> no user.
> >> hns_nic_dev_remove->phy_disconnect->phy_detach
> >>
> >>
> >>
> >>>
> >>>> }
> >>>> EXPORT_SYMBOL(phy_detach);
> >>>> --
> >>>> 2.7.0
> >>>>
> >>>
> >>>
> >>> .
> >>>
> >
> > @Florian Fainelli, what's your comments about this patch?
>
> I am trying to reproduce what you are seeing, but at first glance is looks like an
> appropriate solution to me. Do you mind giving me a couple more days?
>
> Thanks!
> --
> Florian
Hi Florian,
Do you have any update about this patch?
Thank you!
Powered by blists - more mailing lists