netdev - Re: [PATCH net-next 0/2] net: phy: add generic ndo_do_ioctl handler phy_do

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: <c1f7d8ce-2bc2-5141-9f28-a659d2af4e10@gmail.com>
Date:   Sun, 19 Jan 2020 19:19:19 +0100
From:   Heiner Kallweit <hkallweit1@...il.com>
To:     Andrew Lunn <andrew@...n.ch>
Cc:     Florian Fainelli <f.fainelli@...il.com>,
        David Miller <davem@...emloft.net>,
        Realtek linux nic maintainers <nic_swsd@...ltek.com>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>
Subject: Re: [PATCH net-next 0/2] net: phy: add generic ndo_do_ioctl handler
 phy_do_ioctl

On 19.01.2020 18:51, Andrew Lunn wrote:
> Hi Heiner
> 
>> Almost all drivers have the running check. I found five that don't:
>>
>> *ag71xx, fec_mpc52xx*
>> They don't have the running check but should, because the PHY is
>> attached in ndo_open only.
> 
> So long an ndo_close() sets the phydev pointer to NULL, it should be
> safe. But do the drivers do this?
> 
Setting ndev->phydev to NULL is done by phy_detach(), typically
called from phy_disconnect(). Both drivers do this.
In mpc52xx_fec_close() there might be a small chance for a race
because the chip is stopped before calling phy_diconnect().
Stopping the chip may make the MDIO bus unaccessible.

Actually I was wondering why we need to check at all whether
net_device is running. Typically the PHY is connected / disconnected
in ndo_open / ndo_close, and if both are implemented sanely then
checking for ndev->phydev should be sufficient.

Also it seems we don't consider situations like runtime PM yet.
Then the MDIO bus may not be accessible, but ndev is running
and PHY is attached. Maybe we should add a check for ndev being
present? Because typically netif_device_detach() is called
when (runtime-)suspending.

>> *agere, faraday, rdc*
>> They don't have the running check and attach the PHY in probe.
>>
>> So yes, we could add a second helper w/o the running check, even if
>> it's just for three drivers. There may be more in the future.
>>
>>> Do you plan to convert any more MAC drivers?
>>>
>> Not yet ;) Question would be whether one patch would be sufficient
>> or whether we need one patch per driver that needs to be ACKed by
>> the respective maintainer.
> 
> For this sort of mechanical change, i would do one patch for all
> without running, and another with running. If any driver needs more
> than a mechanical change, then do a patch per driver, and get the
> maintainer involved.
> 
>     Andrew
> 

Heiner