From 1e843b57f9da7892392f3bf549572042a94828c0 Mon Sep 17 00:00:00 2001 From: Vladimir Oltean Date: Mon, 8 Aug 2022 15:51:27 +0300 Subject: [PATCH] net: dsa: felix: suppress non-changes to the tagging protocol The way in which dsa_tree_change_tag_proto() works is that when dsa_tree_notify() fails, it doesn't know whether the operation failed mid way in a multi-switch tree, or it failed for a single-switch tree. So even though drivers need to fail cleanly in ds->ops->change_tag_protocol(), DSA will still call dsa_tree_notify() again, to restore the old tag protocol for potential switches in the tree where the change did succeeed (before failing for others). This means for the felix driver that if we report an error in felix_change_tag_protocol(), we'll get another call where proto_ops == old_proto_ops. If we proceed to act upon that, we may do unexpected things. For example, we will call dsa_tag_8021q_register() twice in a row, without any dsa_tag_8021q_unregister() in between. Then we will actually call dsa_tag_8021q_unregister() via old_proto_ops->teardown, which (if it manages to run at all, after walking through corrupted data structures) will leave the ports inoperational anyway. The bug can be readily reproduced if we force an error while in tag_8021q mode; this crashes the kernel. echo ocelot-8021q > /sys/class/net/eno2/dsa/tagging echo edsa > /sys/class/net/eno2/dsa/tagging # -EPROTONOSUPPORT Unable to handle kernel NULL pointer dereference at virtual address 0000000000000014 Call trace: vcap_entry_get+0x24/0x124 ocelot_vcap_filter_del+0x198/0x270 felix_tag_8021q_vlan_del+0xd4/0x21c dsa_switch_tag_8021q_vlan_del+0x168/0x2cc dsa_switch_event+0x68/0x1170 dsa_tree_notify+0x14/0x34 dsa_port_tag_8021q_vlan_del+0x84/0x110 dsa_tag_8021q_unregister+0x15c/0x1c0 felix_tag_8021q_teardown+0x16c/0x180 felix_change_tag_protocol+0x1bc/0x230 dsa_switch_event+0x14c/0x1170 dsa_tree_change_tag_proto+0x118/0x1c0 Fixes: e3882de250d2 ("net: dsa: felix: reimplement tagging protocol change with function pointers") Signed-off-by: Vladimir Oltean Reviewed-by: Florian Fainelli Link: https://lore.kernel.org/r/20220808125127.3344094-1-vladimir.oltean@nxp.com Signed-off-by: Jakub Kicinski --- drivers/net/dsa/ocelot/felix.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/drivers/net/dsa/ocelot/felix.c b/drivers/net/dsa/ocelot/felix.c index 859196898a7d0..aadb0bd7c24f1 100644 --- a/drivers/net/dsa/ocelot/felix.c +++ b/drivers/net/dsa/ocelot/felix.c @@ -610,6 +610,9 @@ static int felix_change_tag_protocol(struct dsa_switch *ds, old_proto_ops = felix->tag_proto_ops; + if (proto_ops == old_proto_ops) + return 0; + err = proto_ops->setup(ds); if (err) goto setup_failed; -- 2.39.5