[PATCH net 0/2] net: dsa: mt7530: fix two crashes on driver unbind

[PATCH net 0/2] net: dsa: mt7530: fix two crashes on driver unbind

[Aleksei Sviridkin]

Unbinding the MT7530 driver from an MT7531 dereferences NULL in
regulator_disable(). On a Netcraze NC-1012 (MT7981B + MT7531, 6.18.44):

  # echo mdio-bus:1f > /sys/bus/mdio_bus/drivers/mt7530-mdio/unbind

oopses there, and the build it was found on sets CONFIG_PANIC_ON_OOPS, so
the board goes down with it. Fix that and the same command gets as far as
mt7530_remove_common(), which disposes interrupt descriptors a PHY still
holds; the switch's own regmap-irq thread then faults in handle_nested_irq()
a fraction of a second later. rmmod reaches both, since mdio_module_driver()
calls .remove on module exit.

Patch 1 is the regulator one. mt7530_probe() requests the core and io
supplies only for ID_MT7530 and mt7530_setup() enables them under the same
test, but mt7530_remove() disables them unconditionally, so on an MT7621 or
an MT7531 both pointers are still NULL from kzalloc. It reaches the MDIO
front end only.

Patch 2 is the interrupt one, and it reaches further. mt7530_remove_common()
disposes the per-PHY interrupt mappings while phylib still has handlers
installed on them; phylib only frees those in phy_disconnect(), which
dsa_unregister_switch() reaches. That helper is called from both front ends,
so it also covers the MMIO parts - MT7988, EN7581, AN7583 and EN7528 - which
have no regulators and never meet the first defect at all.

The order is not arbitrary. On an MT7531 the regulator fault happens in the
first thing mt7530_remove() does with the switch, so execution never reaches
the interrupt defect. The second only became visible once the first was
fixed, which is also how both came to be found on one board.

Found and verified there. Without patch 1 the unbind panics in
regulator_disable(); with patch 1 alone the panic moves on to
handle_nested_irq(); with both, two unbind/bind cycles run back to back -
each unbind removes the switch from the driver directory and takes lan1-lan4
with it, each bind brings them back and the two cabled ports relink at
1Gbps/full, uptime does not reset and pstore gains no new record. The kernel
under test was identified by the
sha256 of its ELF notes section, read from /sys/kernel/notes on the running
board and computed in advance from the flashed image.

What hardware could not answer here. There is no MT7530 or MT7621 part on
this bench, so the ID_MT7530 branch that patch 1 adds was checked by reading
the generated code rather than by running it, and no MMIO part was available
to exercise patch 2 on that front end either. One unrelated WARN remains
across the unbind, from sysfs_remove_link() under dsa_user_destroy(); it is a
separate DSA teardown-ordering defect and is not addressed here.

Aleksei Sviridkin (2):
  net: dsa: mt7530: fix NULL dereference on unbind of MT7531 and MT7621
  net: dsa: mt7530: unregister the switch before freeing its MDIO IRQs

 drivers/net/dsa/mt7530-mdio.c | 18 ++++++++++--------
 drivers/net/dsa/mt7530.c      |  4 ++--
 2 files changed, 12 insertions(+), 10 deletions(-)

-- 
2.53.0

[PATCH net 1/2] net: dsa: mt7530: fix NULL dereference on unbind of MT7531 and MT7621

[Aleksei Sviridkin]

The core and io supplies are only requested for ID_MT7530: both the
devm_regulator_get() in probe and the regulator_enable() in
mt7530_setup() are guarded by the switch id, but mt7530_remove()
disables them unconditionally. On an MT7621 or an MT7531 both pointers
are NULL, so rmmod or a sysfs unbind calls regulator_disable() on NULL.

Fixes: ddda1ac116c8 ("net: dsa: mt7530: support the 7530 switch on the Mediatek MT7621 SoC")
Signed-off-by: Aleksei Sviridkin <f@lex.la>
Assisted-by: LLM
---
Found by accident on a Netcraze NC-1012 (MT7981B + MT7531, 6.18.44) while
looking for a way to tear a DSA port down at runtime:

  # echo mdio-bus:1f > /sys/bus/mdio_bus/drivers/mt7530-mdio/unbind
  Unable to handle kernel access to user memory outside uaccess routines
    at virtual address 0000000000000078
  pc : regulator_disable+0x14/0x48
  lr : mt7530_remove+0x1c/0x80
  x0 : 0000000000000000
  Call trace:
   regulator_disable+0x14/0x48 (P)
   mt7530_remove+0x1c/0x80
   mdio_remove+0x20/0x40
   device_release_driver_internal+0x1cc/0x220
   unbind_store+0xac/0xb0
  Kernel panic - not syncing: Oops: Fatal exception

The oops itself is a process-context oops that kills the writing task. It
became a panic and a reboot because OpenWrt's generic kernel config sets
CONFIG_PANIC_ON_OOPS=y and this target does not override it, not because of
anything local to this bench. With CONFIG_REGULATOR=n the stub
regulator_disable() returns 0 and nothing is dereferenced at all -
NET_DSA_MT7530 neither selects nor depends on REGULATOR - so the severity is
config-dependent, and the commit message states the mechanism rather than an
outcome.

x0 is the regulator pointer and regulator_disable() reads regulator->rdev
straight away, so the NULL comes from the field never being assigned rather
than from an error pointer: with CONFIG_REGULATOR=y devm_regulator_get()
hands back a valid pointer or an ERR_PTR, and on anything but ID_MT7530 it
is never called at all.

The same shape applies to MT7621, which mt7530_of_match also binds. The MMIO
driver is unaffected: it makes no regulator calls at all, though it does still
carry the include.

The id test is used rather than a NULL check because the driver already
says "these supplies belong to ID_MT7530" that way in the other two
places it matters: the devm_regulator_get() pair in mt7530_probe() and the
regulator_set_voltage()/regulator_enable() pair in mt7530_setup(). A NULL
check would be a third spelling of the same condition.

Tested on the board above. Without the patch the unbind panics as shown; both
pointers come out of kzalloc and are never assigned on an MT7531, so the fault
is structural rather than timing-dependent. With this patch plus the unrelated
teardown fix described below, the same unbind runs to completion: mdio-bus:1f
leaves /sys/bus/mdio_bus/drivers/mt7530-mdio/, lan1-lan4 disappear, the kernel
prints "DSA: tree 0 torn down", and uptime does not reset. The kernel under
test was identified by the sha256 of its ELF notes section, read from
/sys/kernel/notes on the running board and computed in advance from the image
that was flashed.

dmesg is not silent across that unbind. It gains one WARN - a single cut
here/WARNING/end trace block - from sysfs_remove_link() under
dsa_user_destroy() reaching an already-removed netdev directory: "kernfs: can
not remove 'phydev', no directory". That is a DSA teardown-ordering defect
rather than a regulator one, and in this run it fired on the first unbind
after boot. dmesg is where it shows: pstore gained no new record across the
run, but pstore records only oopses and panics and could not have caught a
WARN.

That run needed one unrelated fix on top, deliberately kept out of this patch:
mt7530_remove_common() frees the MDIO IRQs before dsa_unregister_switch()
hands them back, and the board dies in handle_nested_irq() a few hundred
milliseconds later. With this patch alone, the panic moves from
regulator_disable+0x14 to that second defect, and mt7530_remove is reached at
+0x24/0x90 rather than +0x1c/0x80 - which is the point: the NULL dereference
is gone and execution now gets past it. That second defect is being handled
separately.

Not tested: the ID_MT7530 branch, which must still disable both rails. There
is no MT7530 or MT7621 hardware here, so the disassembly stands in for it -
before the change mt7530_remove() falls from the priv NULL check straight into
ldr x0, [x19, #40] / bl regulator_disable; after it, ldr w0, [x19, #72]
(priv->id) / cbz w0 gates both calls, and ID_MT7530 is 0. Built with W=1, no
warnings; checkpatch --strict clean.

 drivers/net/dsa/mt7530-mdio.c | 18 ++++++++++--------
 1 file changed, 10 insertions(+), 8 deletions(-)

diff --git a/drivers/net/dsa/mt7530-mdio.c b/drivers/net/dsa/mt7530-mdio.c
index 784dd58a7158..de42f70afcfa 100644
--- a/drivers/net/dsa/mt7530-mdio.c
+++ b/drivers/net/dsa/mt7530-mdio.c
@@ -227,15 +227,17 @@ mt7530_remove(struct mdio_device *mdiodev)
 	if (!priv)
 		return;
 
-	ret = regulator_disable(priv->core_pwr);
-	if (ret < 0)
-		dev_err(priv->dev,
-			"Failed to disable core power: %d\n", ret);
+	if (priv->id == ID_MT7530) {
+		ret = regulator_disable(priv->core_pwr);
+		if (ret < 0)
+			dev_err(priv->dev,
+				"Failed to disable core power: %d\n", ret);
 
-	ret = regulator_disable(priv->io_pwr);
-	if (ret < 0)
-		dev_err(priv->dev, "Failed to disable io pwr: %d\n",
-			ret);
+		ret = regulator_disable(priv->io_pwr);
+		if (ret < 0)
+			dev_err(priv->dev, "Failed to disable io pwr: %d\n",
+				ret);
+	}
 
 	mt7530_remove_common(priv);
 
-- 
2.53.0

[PATCH net 2/2] net: dsa: mt7530: unregister the switch before freeing its MDIO IRQs

[Aleksei Sviridkin]

mt7530_remove_common() disposes the per-PHY interrupt mappings first and
unregisters the switch second, but phylib only frees those interrupts
inside dsa_unregister_switch(). Unbinding the driver therefore frees
descriptors that are still in use, and the switch's own regmap-irq thread
takes a nested interrupt on one that is already gone.

Fixes: ba751e28d442 ("net: dsa: mt7530: add interrupt support")
Signed-off-by: Aleksei Sviridkin <f@lex.la>
Assisted-by: LLM
---
Found on a Netcraze NC-1012 (MT7981B + MT7531, 6.18.44) directly behind the
regulator fix in patch 1: with that one applied the unbind stops faulting in
mt7530_remove() and reaches the teardown, where the kernel says what is wrong
in words before it dies.

  # echo mdio-bus:1f > /sys/bus/mdio_bus/drivers/mt7530-mdio/unbind
  remove_proc_entry: removing non-empty directory 'irq/81', leaking at least
    'mt7530-0:02'
  WARNING: CPU: 0 PID: 4629 at remove_proc_entry+0x1d0/0x1f0
  ...
   mt7530_remove_common+0x1c/0x30
   mt7530_remove+0x24/0x90
   mdio_remove+0x20/0x40
   unbind_store+0xac/0xb0

  Unable to handle kernel read from unreadable memory at virtual address
    00000000000000ac
  pc : handle_nested_irq+0x28/0x168
  Kernel panic - not syncing: Oops: Fatal exception

The WARN comes from unregister_irq_proc() under irq_free_descs(), fired for a
mapping that a PHY still holds. The captured record shows one, for
mt7530-0:02, and already carries the W taint bit, so at least one earlier WARN
fell outside the ramoops window. 294 ms later the switch's own regmap-irq
thread - PID 627, Comm irq/53-mt7530 - takes a nested interrupt for a mapping
that is already gone: irq_find_mapping() returns 0, irq_to_desc() returns NULL
and handle_nested_irq() locks desc->lock without checking, which is the read
at +0xac in the trace. Both timestamps are from the same ramoops record.

Reach is wider than the board that found it. mt7530_remove_common() is called
from both front ends - mt7530-mdio.c and mt7530-mmio.c - so it covers the MMIO
parts as well, which have no regulators at all and never meet the defect patch
1 fixes.

What decides whether a given switch is hit is not the irq_domain but whether
the PHY interrupts are mapped on it. Either mt7530_setup_mdio_irq() created
those mappings, which it only does when the devicetree has no mdio node under
the switch, or OF created them from per-PHY interrupts properties when it has
one. A switch with an irq_domain and neither is left alone: irq_find_mapping()
returns 0 for every port and irq_dispose_mapping(0) returns at once. The
teardown is guarded on the domain alone, so it walks that loop either way.

Tested on the board above, with both patches applied. Two unbind/bind cycles
back to back: each unbind removed mdio-bus:1f from the driver directory and
took lan1-lan4 with it, each bind brought them back, and the two cabled ports
relinked at 1Gbps/full. uptime went from 167 to 183 across both cycles without
resetting, and
pstore gained no new record. dmesg carries one unrelated WARN, from
sysfs_remove_link() under dsa_user_destroy() - a separate DSA
teardown-ordering defect, handled on its own - and it fired once, on the first
unbind, not on the second.

Not tested: any MMIO part - there is no MT7988, EN7581, AN7583 or EN7528
hardware here. The object file was checked instead:
after the change mt7530_remove_common() calls dsa_unregister_switch() first
and only then tests priv->irq_domain and calls mt7530_free_mdio_irq().
Built with W=1, no warnings; checkpatch --strict clean.

 drivers/net/dsa/mt7530.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
index 3e61eb3c2b1e..90fd04665ebf 100644
--- a/drivers/net/dsa/mt7530.c
+++ b/drivers/net/dsa/mt7530.c
@@ -3593,11 +3593,11 @@ EXPORT_SYMBOL_GPL(mt7530_probe_common);
 void
 mt7530_remove_common(struct mt7530_priv *priv)
 {
+	dsa_unregister_switch(priv->ds);
+
 	if (priv->irq_domain)
 		mt7530_free_mdio_irq(priv);
 
-	dsa_unregister_switch(priv->ds);
-
 	mutex_destroy(&priv->reg_mutex);
 }
 EXPORT_SYMBOL_GPL(mt7530_remove_common);
-- 
2.53.0

Re: [PATCH net 2/2] net: dsa: mt7530: unregister the switch before freeing its MDIO IRQs

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider:

Critical: 0 · High: 1 · Medium: 0 (1) · Low: 0

- [High] Incomplete fix: mt7530_remove_common()
  (drivers/net/dsa/mt7530.c:3593-3602) still calls…

Pre-existing issues:
- [Medium] Power-sequencing ordering on the MDIO front end:
  mt7530_remove() in drivers/net/dsa/mt7530-mdio.c disables…

--

net: dsa: mt7530: unregister the switch before freeing its MDIO IRQs

This moves dsa_unregister_switch() ahead of the per-PHY interrupt
disposal in mt7530_remove_common(), so that phylib frees the nested PHY
interrupts before their mappings are destroyed. The reported symptom is a
remove_proc_entry() warning from irq_free_descs() followed by a NULL
dereference in handle_nested_irq().

> diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
> index 3e61eb3c2b1e2..90fd04665ebf1 100644
> --- a/drivers/net/dsa/mt7530.c
> +++ b/drivers/net/dsa/mt7530.c
> @@ -3593,11 +3593,11 @@ EXPORT_SYMBOL_GPL(mt7530_probe_common);
>  void
>  mt7530_remove_common(struct mt7530_priv *priv)
>  {
> +	dsa_unregister_switch(priv->ds);
> +
>  	if (priv->irq_domain)
>  		mt7530_free_mdio_irq(priv);

[Severity: High]
Does this reorder actually close the handle_nested_irq() fault described
in the changelog, or does it only close the phylib half of it?

The parent threaded interrupt is owned by devres.  mt7530_setup_irq()
registers the chip with devm_regmap_add_irq_chip_fwnode() and keeps only
the domain pointer:

drivers/net/dsa/mt7530.c:mt7530_setup_irq() {
	ret = devm_regmap_add_irq_chip_fwnode(dev, dev_fwnode(dev),
					      priv->regmap, irq,
					      IRQF_ONESHOT,
					      0, chip,
					      &irq_data);
	...
	priv->irq_domain = regmap_irq_get_domain(irq_data);
}

so free_irq() on the parent and irq_domain_remove() happen only from the
devres release, devm_regmap_irq_chip_release() -> regmap_del_irq_chip(),
which runs after mt7530_remove() has returned.  Nothing in
mt7530_remove_common() disables, frees or synchronizes against that
parent IRQ, so the regmap-irq thread is still live while
mt7530_free_mdio_irq() destroys the mappings.

On the thread side there is no zero check on the lookup:

drivers/base/regmap/regmap-irq.c:regmap_irq_thread() {
	...
		data->status_buf[i] &= ~data->mask_buf[i];
	...
			handle_nested_irq(irq_find_mapping(data->domain, i));
}

mask_buf is read there without data->lock, so the thread can carry a
pre-mask snapshot past the filter.  Can the following still happen with
this patch applied, on a part with a PHY_LC bit latched?

  regmap_irq_thread()			mt7530_remove_common()
    read_irq_data(), PHY0_LC set
    passes the mask_buf filter
					dsa_unregister_switch()
					  phy_disconnect()
					    phy_free_interrupt()
					      free_irq(nested virq)
					mt7530_free_mdio_irq()
					  irq_dispose_mapping()
    irq_find_mapping() -> 0
    handle_nested_irq(0)

kernel/irq/chip.c:handle_nested_irq() has no NULL check:

	struct irq_desc *desc = irq_to_desc(irq);
	...
	scoped_guard(raw_spinlock_irq, &desc->lock) {

which is the read at handle_nested_irq+0xac quoted in the changelog.

Is the driver-side disposal loop needed at all?  regmap_del_irq_chip()
already walks the domain and disposes every mapping, and it does so in
the safe order:

drivers/base/regmap/regmap-irq.c:regmap_del_irq_chip() {
	free_irq(irq, d);

	/* Dispose all virtual irq from irq domain before removing it */
	...
}

Would dropping mt7530_free_mdio_irq() from the remove path (leaving the
disposal to regmap_del_irq_chip(), which runs after the parent IRQ is
freed) be a better fix than moving it after dsa_unregister_switch()?

> -	dsa_unregister_switch(priv->ds);
> -
>  	mutex_destroy(&priv->reg_mutex);
>  }
>  EXPORT_SYMBOL_GPL(mt7530_remove_common);

[Severity: Medium]
This is a pre-existing ordering problem and not something this patch
introduces, but it interacts with the new placement of
dsa_unregister_switch(), so it may be worth mentioning.

On the MDIO front end the supplies are dropped before any of this runs:

drivers/net/dsa/mt7530-mdio.c:mt7530_remove() {
	if (priv->id == ID_MT7530) {
		ret = regulator_disable(priv->core_pwr);
		...
		ret = regulator_disable(priv->io_pwr);
		...
	}

	mt7530_remove_common(priv);
}

After this patch the whole register-level teardown reached from
dsa_unregister_switch() - internal PHY power-down, port disable writes,
and the regmap-irq mask sync that clears the PHY_LC bits through
mt7530_irq_mask_sync() - is the first thing mt7530_remove_common() does,
i.e. entirely after core_pwr and io_pwr were released on ID_MT7530.

Can those writes still reach the chip?  MDIO writes are unacknowledged,
and regmap-irq updates its mask_buf shadow regardless of what
handle_mask_sync() returns:

drivers/net/dsa/mt7530.c:mt7530_irq_mask_sync() {
	ret = regmap_update_bits(priv->regmap, MT7530_SYS_INT_EN,
				 mask_buf_def, ~mask_buf);
	...
}

so MT7530_SYS_INT_EN can be left armed and MT7530_SYS_INT_STS latched
while software believes they are masked.  MT7530_TOP_SIG_CTRL's
TOP_SIG_CTRL_NORMAL bit, set in mt7530_setup_irq(), likewise has no
teardown counterpart.  Should the regulator_disable() calls move after
mt7530_remove_common() instead?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260914202421.2737079-1-f%40lex.la