[PATCH net v3 0/4] net: phy: make PHY driver unbind safe against attach and use

[Aleksei Sviridkin]

Unbinding a PHY driver through sysfs while its MAC brings the port up
can oops. On a Keenetic KN-1012 (MT7981, mtk_eth_soc), an unbind of the
wan PHY driver racing "ip link set wan up" faulted on the first attempt,
with no delay added anywhere. The fault was inside the PHY driver's
config_init: the driver read phydev->drv after phy_remove() had cleared
it. Patch 3 has the trace.

Serialising attach against unbind is not enough on its own. With only
that, an unbind that loses the race waits for the attach and then
removes the driver from a PHY that is now attached. On the board the
consumer then faulted one step later: in phylink_bringup_phy() right
after the attach returned, or in _phy_state_machine() at the next
ifdown. A DSA port gets there without any race, because DSA keeps its
PHYs attached from switch setup to teardown. Patch 4 makes the unbind
wait until the consumer has detached.

1: refuse a second attach of a PHY that is already attached, before
   taking anything. That -EBUSY path used to run phy_detach() on the
   first consumer's attachment. Found in review of patch 2, whose
   module accounting it unbalanced.
2: put the module reference phy_attach_direct() took, not whatever
   driver is bound at detach time. Found by reading. On the KN-1012 the
   sequence that would leak faults earlier, before the detach reaches
   the put.
3: a per-PHY mutex and a "bound" flag. An attach is either done with
   the driver before phy_remove() starts tearing it down, or is refused
   with -EAGAIN, or, after the unbind has finished, gets the generic
   driver as today.
4: phy_remove() waits for phy_detach() when the PHY is attached,
   except when the PHY device itself is being deleted.

1, 2 and 3 stand on their own. Without 4, an unbind that loses the race
to an attach still leaves that consumer with a PHY whose driver is
gone. 3 only guarantees the attach itself does not run into a driver
being removed.

On the lock inversion (Paolo): the PHY's device lock is the obvious lock
for this, but attach may run under rtnl, and rtnl is taken under that
device lock on bind and unbind in at least two places. For a PHY with an
SFP cage, phy_probe() and phy_remove() go through sfp_bus_add_upstream()
and sfp_bus_del_upstream(), which take rtnl. For a PHY LED on the netdev
trigger, led_classdev_register() and led_classdev_unregister() register
and unregister a netdevice notifier, which takes rtnl.

Removing the inversion would at least mean moving the SFP registration
out of probe and remove, and that is not net material. The SFP
upstream ops write netdev state that rtnl protects. Attach does not
always run under rtnl (DSA connects its ports before taking it), so
the registration cannot follow the attach either. Nothing under the
new mutex takes rtnl. On the generic-driver path, device_bind_driver()
can take a supplier's device lock for sync_state, as it already does
today.

Vladimir, this is where patch 4 falls short of what you asked for in
[1]: that unbinding a PHY driver should not "explode ... even in
uncontrolled situations where the netdev isn't carefully disconnected
from the PHY first". What I looked at, and what each costs:

- A, phy_remove() waits for the detach (patch 4, kept): the unbind
  blocks, uninterruptibly, until the consumer detaches. For DSA that
  means until the switch is torn down, even with the port down, since
  DSA stays connected.
- B, stop at patch 3: the oops moves one frame up, into
  phylink_bringup_phy() or _phy_state_machine(). Patch 4 has the
  phylink trace.
- C, let the caller hold the lock across attach and bringup: widens
  the series to phylink and still leaves every use after bringup.
- A with a killable wait: ->remove cannot fail, so after a kill it
  could only go on removing the driver, back into the oops.
- suppress_bind_attrs on PHY drivers: removes the sysfs unbind your
  use case relies on.
- A managed device link MAC -> PHY: the unbind would take the whole
  MAC or switch down with it.

A is a compromise. What it does not achieve: an unbind of a PHY in use
hangs instead of failing, and on DSA it hangs until switch teardown,
even with the port down.

On the KN-1012, lan4 is a DSA port on an EN8811H. Without the series,
with lan4 down, unbinding air_en8811h returned at once. Bringing lan4
up after that gave a WARN in phy_start() and no oops within 10 s.
Unbinding the switch instead faulted in phy_free_interrupt() from
phy_disconnect(). With the series (an earlier revision with the same
attach and wait code, before the device-deletion change), the same
unbind blocks with lan4 up or down, and returns when the switch is
unbound.

Vladimir, should patch 4 go separately, to net-next or as an RFC, with
1 to 3 going to net now?

Patch 4 has other costs too. The hung-task detector, when enabled,
reports the blocked unbind after its timeout. System suspend and
reboot wait on that PHY's device lock meanwhile. device_shutdown()
takes it and does not detach PHYs, so a reboot behind a blocked unbind
hangs. sysfs shows the driver link gone while ->remove is still
waiting, because the driver core removes it first.

Deleting the PHY device does not wait and keeps today's behaviour, for
example mdiobus_unregister() from a MAC driver's remove. Some MAC
drivers never detach their PHY and would otherwise hang in their own
removal. Andrew asked in 2020 whether an unbind could be blocked while
the interface is up [2]. Florian Fainelli answered that nothing bad
happens, which the traces here no longer bear out.

This series does not close one more window. phy_attach_direct() still
stores the generic driver in d->driver and calls device_bind_driver()
without the device lock the driver core expects. A real driver binding
through the driver core at the same moment can still collide with it.

I tested on the KN-1012 with OpenWrt's 6.18 kernel and a backport of
the series, on top of the board's pending phylib/phylink patches for
its late PHY and test-only mt7530 fixes. Patches 2 to 4 ran as an
earlier three-patch revision without patch 1 and otherwise identical
in code, with PROVE_LOCKING:
- the wan race, 450 iterations: no oops. In 447 the unbind was still
  pending after the attach and returned at ifdown.
- no lock-order report across the unbind/bind cycles. The controller
  unbind runs also show warnings from mtk_remove() stopping and
  disconnecting its netdevs without rtnl (the refcount underflow in
  mtk_stop() comes from stopping netdevs that were already down), and
  DSA teardown a kernfs WARN. Both predate this series.
- the device-deletion branch of patch 4, through a test module calling
  phy_device_remove() on the attached EN8811H, since unbinding the
  switch or the Ethernet controller here detaches the PHY first. The
  call returned, and it released an unbind already waiting.
- the -EAGAIN refusal of patch 3 was hit once, on an earlier revision
  with the same attach code: mtk_open() got -11 and "ip link set wan
  up" failed with "Resource temporarily unavailable".

Patch 1 ran on this revision, without PROVE_LOCKING. A test module
attaching lan1 to the EN8811H that lan4 holds got -EBUSY. lan4 kept
its PHY, and the air_en8811h refcount was unchanged. Without the
series the same call left lan4 with no PHY. The same wan race without
the series faulted on the first try.

[1] https://lore.kernel.org/netdev/20260311203410.rio7m6nuf72hs5p6@skbuf/
[2] https://lore.kernel.org/netdev/20200917131545.GL3526428@xxxxxxx/

v2: https://lore.kernel.org/netdev/20260919015340.499675-1-f@xxxxxx/
- Retargeted to net, and the race closed with locking rather than a
  NULL test at attach entry, as asked in review.
- The module reference fix is its own patch.
- New patch 1: a second attach no longer detaches the first consumer.
- An unbind of an attached PHY now waits for the detach.

Aleksei Sviridkin (4):
  net: phy: refuse a second attach before touching the PHY
  net: phy: put the driver module the attach took
  net: phy: serialise attach and detach with PHY driver bind and unbind
  net: phy: make an unbind wait for the attached consumer to detach

 drivers/net/phy/phy_device.c | 95 ++++++++++++++++++++++++++++++------
 include/linux/phy.h          | 12 +++++
 2 files changed, 92 insertions(+), 15 deletions(-)

-- 
2.53.0