Re: [PATCH v6 net-next 07/10] net: dsa: allow changing the tag protocol via the "tagging" device attribute

On Sun, 24 Jan 2021 10:20:39 +0200 Vladimir Oltean wrote:

On Fri, Jan 22, 2021 at 08:52:16PM -0800, Jakub Kicinski wrote:

quoted

On Thu, 21 Jan 2021 18:01:28 +0200 Vladimir Oltean wrote:  

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.

Sounds good, thanks!