mirror of
https://github.com/torvalds/linux.git
synced 2026-03-08 04:44:45 +01:00
bfd264fbbb
There is a crash when unbinding the sja1105 driver under special
circumstances:
Unable to handle kernel NULL pointer dereference at virtual address 0000000000000030
Call trace:
phylink_run_resolve_and_disable+0x10/0x90
sja1105_static_config_reload+0xc0/0x410
sja1105_vlan_filtering+0x100/0x140
dsa_port_vlan_filtering+0x13c/0x368
dsa_port_reset_vlan_filtering.isra.0+0xe8/0x198
dsa_port_bridge_leave+0x130/0x248
dsa_user_changeupper.part.0+0x74/0x158
dsa_user_netdevice_event+0x50c/0xa50
notifier_call_chain+0x78/0x148
raw_notifier_call_chain+0x20/0x38
call_netdevice_notifiers_info+0x58/0xa8
__netdev_upper_dev_unlink+0xac/0x220
netdev_upper_dev_unlink+0x38/0x70
del_nbp+0x1a4/0x320
br_del_if+0x3c/0xd8
br_device_event+0xf8/0x2d8
notifier_call_chain+0x78/0x148
raw_notifier_call_chain+0x20/0x38
call_netdevice_notifiers_info+0x58/0xa8
unregister_netdevice_many_notify+0x314/0x848
unregister_netdevice_queue+0xe8/0xf8
dsa_user_destroy+0x50/0xa8
dsa_port_teardown+0x80/0x98
dsa_switch_teardown_ports+0x4c/0xb8
dsa_switch_deinit+0x94/0xb8
dsa_switch_put_tree+0x2c/0xc0
dsa_unregister_switch+0x38/0x60
sja1105_remove+0x24/0x40
spi_remove+0x38/0x60
device_remove+0x54/0x90
device_release_driver_internal+0x1d4/0x230
device_driver_detach+0x20/0x38
unbind_store+0xbc/0xc8
---[ end trace 0000000000000000 ]---
which requires an explanation.
When a port offloads a bridge, the switch must be reset to change
the VLAN awareness state (the SJA1105_VLAN_FILTERING reason for
sja1105_static_config_reload()). When the port leaves a VLAN-aware
bridge, it must also be reset for the same reason: it is returning
to operation as a VLAN-unaware standalone port.
sja1105_static_config_reload() triggers the phylink link replay helpers.
Because sja1105 is a switch, it has multiple user ports. During unbind,
ports are torn down one by one in dsa_switch_teardown_ports() ->
dsa_port_teardown() -> dsa_user_destroy().
The crash happens when the first user port is not part of the VLAN-aware
bridge, but any other user port is.
Tearing down the first user port causes phylink_destroy() to be called
on dp->pl, and this pointer to be set to NULL. Then, when the second
user port is torn down, this was offloading a VLAN-aware bridge port, so
indirectly it will trigger sja1105_static_config_reload().
The latter function iterates using dsa_switch_for_each_available_port(),
and unconditionally dereferences dp->pl, including for the
aforementioned torn down previous port, and passes that to phylink.
This is where the NULL pointer is coming from.
There are multiple levels at which this could be avoided:
- add an "if (dp->pl)" in sja1105_static_config_reload()
- make the phylink replay helpers NULL-tolerant
- mark ports as DSA_PORT_TYPE_UNUSED after dsa_port_phylink_destroy()
has run, such that subsequent dsa_switch_for_each_available_port()
iterations skip them
- disconnect the entire switch at once from switchdev and
NETDEV_CHANGEUPPER events while unbinding, not just port by port,
likely using a "ds->unbinding = true" mechanism or similar
however options 3 and 4 are quite heavy and might have side effects.
Although 2 allows to keep the driver simpler, the phylink API it not
NULL-tolerant in general and is not responsible for the NULL pointer
(this is something done by dsa_port_phylink_destroy()). So I went
with 1.
Functionally speaking, skipping the replay helpers for ports without
a phylink instance is fine, because that only happens during driver
removal (an operation which cannot be cancelled). The ports are not
required to work (although they probably still will - untested
assumption - as long as we don't overwrite the last port speed with
SJA1105_SPEED_AUTO).
Fixes:
|
||
|---|---|---|
| .. | ||
| accel | ||
| accessibility | ||
| acpi | ||
| amba | ||
| android | ||
| ata | ||
| atm | ||
| auxdisplay | ||
| base | ||
| bcma | ||
| block | ||
| bluetooth | ||
| bus | ||
| cache | ||
| cdrom | ||
| cdx | ||
| char | ||
| clk | ||
| clocksource | ||
| comedi | ||
| connector | ||
| counter | ||
| cpufreq | ||
| cpuidle | ||
| crypto | ||
| cxl | ||
| dax | ||
| dca | ||
| devfreq | ||
| dibs | ||
| dio | ||
| dma | ||
| dma-buf | ||
| dpll | ||
| edac | ||
| eisa | ||
| extcon | ||
| firewire | ||
| firmware | ||
| fpga | ||
| fsi | ||
| fwctl | ||
| gnss | ||
| gpib | ||
| gpio | ||
| gpu | ||
| greybus | ||
| hid | ||
| hsi | ||
| hte | ||
| hv | ||
| hwmon | ||
| hwspinlock | ||
| hwtracing | ||
| i2c | ||
| i3c | ||
| idle | ||
| iio | ||
| infiniband | ||
| input | ||
| interconnect | ||
| iommu | ||
| ipack | ||
| irqchip | ||
| isdn | ||
| leds | ||
| macintosh | ||
| mailbox | ||
| mcb | ||
| md | ||
| media | ||
| memory | ||
| memstick | ||
| message | ||
| mfd | ||
| misc | ||
| mmc | ||
| most | ||
| mtd | ||
| mux | ||
| net | ||
| nfc | ||
| ntb | ||
| nubus | ||
| nvdimm | ||
| nvme | ||
| nvmem | ||
| of | ||
| opp | ||
| parisc | ||
| parport | ||
| pci | ||
| pcmcia | ||
| peci | ||
| perf | ||
| phy | ||
| pinctrl | ||
| platform | ||
| pmdomain | ||
| pnp | ||
| power | ||
| powercap | ||
| pps | ||
| ps3 | ||
| ptp | ||
| pwm | ||
| rapidio | ||
| ras | ||
| regulator | ||
| remoteproc | ||
| resctrl | ||
| reset | ||
| rpmsg | ||
| rtc | ||
| s390 | ||
| sbus | ||
| scsi | ||
| sh | ||
| siox | ||
| slimbus | ||
| soc | ||
| soundwire | ||
| spi | ||
| spmi | ||
| ssb | ||
| staging | ||
| target | ||
| tc | ||
| tee | ||
| thermal | ||
| thunderbolt | ||
| tty | ||
| ufs | ||
| uio | ||
| usb | ||
| vdpa | ||
| vfio | ||
| vhost | ||
| video | ||
| virt | ||
| virtio | ||
| w1 | ||
| watchdog | ||
| xen | ||
| zorro | ||
| Kconfig | ||
| Makefile | ||