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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20210124082039.gkgxskot7oxseub7@skbuf>
Date:   Sun, 24 Jan 2021 10:20:39 +0200
From:   Vladimir Oltean <olteanv@...il.com>
To:     Jakub Kicinski <kuba@...nel.org>
Cc:     "David S . Miller" <davem@...emloft.net>, netdev@...r.kernel.org,
        Andrew Lunn <andrew@...n.ch>,
        Florian Fainelli <f.fainelli@...il.com>,
        Vivien Didelot <vivien.didelot@...il.com>,
        Richard Cochran <richardcochran@...il.com>,
        Claudiu Manoil <claudiu.manoil@....com>,
        Alexandru Marginean <alexandru.marginean@....com>,
        Alexandre Belloni <alexandre.belloni@...tlin.com>,
        Xiaoliang Yang <xiaoliang.yang_1@....com>,
        Hongbo Wang <hongbo.wang@....com>,
        Vladimir Oltean <vladimir.oltean@....com>,
        Po Liu <po.liu@....com>, Yangbo Lu <yangbo.lu@....com>,
        Maxim Kochetkov <fido_max@...ox.ru>,
        Eldar Gasanov <eldargasanov2@...il.com>,
        Andrey L <al@...omtech.com>,
        Tobias Waldekranz <tobias@...dekranz.com>,
        UNGLinuxDriver@...rochip.com
Subject: Re: [PATCH v6 net-next 07/10] net: dsa: allow changing the tag
 protocol via the "tagging" device attribute

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:
> > +	/* 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.

> > +	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.