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;
+}