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: <20210122205216.7f1e05f6@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com>
Date:   Fri, 22 Jan 2021 20:52:16 -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 Thu, 21 Jan 2021 18:01:28 +0200 Vladimir Oltean wrote:
> +/* Since the dsa/tagging sysfs device attribute is per master, the assumption
> + * is that all DSA switches within a tree share the same tagger, otherwise
> + * they would have formed disjoint trees (different "dsa,member" values).
> + */
> +int dsa_tree_change_tag_proto(struct dsa_switch_tree *dst,
> +			      struct net_device *master,
> +			      const struct dsa_device_ops *tag_ops,
> +			      const struct dsa_device_ops *old_tag_ops)
> +{
> +	struct dsa_notifier_tag_proto_info info;
> +	struct dsa_port *dp;
> +	int err;
> +
> +	/* 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.

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

> +		if (!dsa_is_user_port(dp->ds, dp->index))
> +			continue;
> +
> +		if (dp->slave->flags & IFF_UP)
> +			return -EBUSY;
> +	}
> +
> +	mutex_lock(&dst->tagger_lock);
> +
> +	info.tag_ops = old_tag_ops;
> +	err = dsa_tree_notify(dst, DSA_NOTIFIER_TAG_PROTO_DEL, &info);
> +	if (err)
> +		return err;
> +
> +	info.tag_ops = tag_ops;
> +	err = dsa_tree_notify(dst, DSA_NOTIFIER_TAG_PROTO_SET, &info);
> +	if (err)
> +		goto out_unwind_tagger;
> +
> +	mutex_unlock(&dst->tagger_lock);
> +
> +	return 0;
> +
> +out_unwind_tagger:
> +	info.tag_ops = old_tag_ops;
> +	dsa_tree_notify(dst, DSA_NOTIFIER_TAG_PROTO_SET, &info);
> +	mutex_unlock(&dst->tagger_lock);
> +	return err;
> +}