* [PATCH net] net: dsa: felix: suppress non-changes to the tagging protocol
@ 2022-08-08 12:51 Vladimir Oltean
2022-08-08 20:49 ` Florian Fainelli
2022-08-09 19:20 ` patchwork-bot+netdevbpf
0 siblings, 2 replies; 3+ messages in thread
From: Vladimir Oltean @ 2022-08-08 12:51 UTC (permalink / raw
To: netdev
Cc: Andrew Lunn, Vivien Didelot, Florian Fainelli, Claudiu Manoil,
Alexandre Belloni, UNGLinuxDriver, David S. Miller, Eric Dumazet,
Jakub Kicinski, Paolo Abeni
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: 7a29d220f4c0 ("net: dsa: felix: reimplement tagging protocol change with function pointers")
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
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 b8ef3f333520..9aa47752369a 100644
--- a/drivers/net/dsa/ocelot/felix.c
+++ b/drivers/net/dsa/ocelot/felix.c
@@ -683,6 +683,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.34.1
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH net] net: dsa: felix: suppress non-changes to the tagging protocol
2022-08-08 12:51 [PATCH net] net: dsa: felix: suppress non-changes to the tagging protocol Vladimir Oltean
@ 2022-08-08 20:49 ` Florian Fainelli
2022-08-09 19:20 ` patchwork-bot+netdevbpf
1 sibling, 0 replies; 3+ messages in thread
From: Florian Fainelli @ 2022-08-08 20:49 UTC (permalink / raw
To: Vladimir Oltean, netdev
Cc: Andrew Lunn, Vivien Didelot, Claudiu Manoil, Alexandre Belloni,
UNGLinuxDriver, David S. Miller, Eric Dumazet, Jakub Kicinski,
Paolo Abeni
On 8/8/22 05:51, Vladimir Oltean wrote:
> 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: 7a29d220f4c0 ("net: dsa: felix: reimplement tagging protocol change with function pointers")
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
--
Florian
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH net] net: dsa: felix: suppress non-changes to the tagging protocol
2022-08-08 12:51 [PATCH net] net: dsa: felix: suppress non-changes to the tagging protocol Vladimir Oltean
2022-08-08 20:49 ` Florian Fainelli
@ 2022-08-09 19:20 ` patchwork-bot+netdevbpf
1 sibling, 0 replies; 3+ messages in thread
From: patchwork-bot+netdevbpf @ 2022-08-09 19:20 UTC (permalink / raw
To: Vladimir Oltean
Cc: netdev, andrew, vivien.didelot, f.fainelli, claudiu.manoil,
alexandre.belloni, UNGLinuxDriver, davem, edumazet, kuba, pabeni
Hello:
This patch was applied to netdev/net.git (master)
by Jakub Kicinski <kuba@kernel.org>:
On Mon, 8 Aug 2022 15:51:27 +0300 you wrote:
> 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).
>
> [...]
Here is the summary with links:
- [net] net: dsa: felix: suppress non-changes to the tagging protocol
https://git.kernel.org/netdev/net/c/4c46bb49460e
You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2022-08-09 19:21 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2022-08-08 12:51 [PATCH net] net: dsa: felix: suppress non-changes to the tagging protocol Vladimir Oltean
2022-08-08 20:49 ` Florian Fainelli
2022-08-09 19:20 ` patchwork-bot+netdevbpf
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).