Re: [PATCH v6 net-next 07/10] net: dsa: allow changing the tag protocol via the "tagging" device attribute
From: Vladimir Oltean <olteanv@gmail.com>
Date: 2021-01-24 08:21:47
On Fri, Jan 22, 2021 at 08:52:16PM -0800, Jakub Kicinski wrote:
On Thu, 21 Jan 2021 18:01:28 +0200 Vladimir Oltean wrote:quoted
+ /* At the moment we don't allow changing the tag protocol under + * traffic. May revisit in the future. + */ + if (master->flags & IFF_UP) + return -EBUSY;But you're not holding rtnl_lock at this point, this check is advisory at best.
Yes, I should hold the rtnl_mutex.
quoted
+ list_for_each_entry(dp, &dst->ports, list) {What protects this iteration? All sysfs guarantees you is that struct net_device *master itself will not disappear. Could you explain the locking expectations a bit?
The dsa_group sysfs is removed in:
dsa_unregister_switch
-> mutex_lock(&dsa2_mutex)
-> dsa_switch_remove
-> dsa_tree_teardown
-> dsa_tree_teardown_master
-> dsa_master_teardown
-> sysfs_remove_group
There are 2 points here:
1. sysfs_remove_group actually waits for a concurrent tagging_store()
call to finish (at least it does when I put an msleep(10000) inside
tagging_store).
2. After the sysfs_remove_group, dsa_tree_change_tag_proto should never
be called again.
Next comes:
-> dsa_tree_teardown
-> dsa_tree_teardown_switches
-> dsa_port_teardown
-> dsa_slave_destroy
After this, all DSA net devices are unregistered and freed.
Next comes:
-> dsa_switch_remove
-> dsa_switch_release_ports
-> mutex_unlock(&dsa2_mutex)
where the dst->ports list is finally freed.
So there is no chance that the dst->ports list is modified concurrently
with tagging_store.