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