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] [day] [month] [year] [list]

Date:   Mon, 25 Jan 2021 10:46:11 -0800
From:   Jakub Kicinski <kuba@...nel.org>
To:     Vladimir Oltean <olteanv@...il.com>
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 Sun, 24 Jan 2021 10:20:39 +0200 Vladimir Oltean wrote:
> 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:  
> > > +	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!