netdev - Re: [PATCH net-next 2/3] net: dsa: Allow default tag protocol to be overridden from DT

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]

Date:   Sun, 28 Mar 2021 17:52:43 +0200
From:   Andrew Lunn <andrew@...n.ch>
To:     Tobias Waldekranz <tobias@...dekranz.com>
Cc:     davem@...emloft.net, kuba@...nel.org, vivien.didelot@...il.com,
        f.fainelli@...il.com, olteanv@...il.com, netdev@...r.kernel.org,
        robh+dt@...nel.org, devicetree@...r.kernel.org
Subject: Re: [PATCH net-next 2/3] net: dsa: Allow default tag protocol to be
 overridden from DT

> +static int dsa_switch_setup_tag_protocol(struct dsa_switch *ds)
> +{
> +	const struct dsa_device_ops *tag_ops = ds->dst->tag_ops;
> +	struct dsa_switch_tree *dst = ds->dst;
> +	int port, err;
> +
> +	if (tag_ops->proto == dst->default_proto)
> +		return 0;
> +
> +	if (!ds->ops->change_tag_protocol) {
> +		dev_err(ds->dev, "Tag protocol cannot be modified\n");
> +		return -EINVAL;
> +	}
> +
> +	for (port = 0; port < ds->num_ports; port++) {
> +		if (!(dsa_is_dsa_port(ds, port) || dsa_is_cpu_port(ds, port)))
> +			continue;

dsa_is_dsa_port() is interesting. Do we care about the tagging
protocol on DSA ports? We never see that traffic?

> +
> +		err = ds->ops->change_tag_protocol(ds, port, tag_ops->proto);
> +		if (err) {
> +			dev_err(ds->dev, "Tag protocol \"%s\" is not supported\n",
> +				tag_ops->name);
> +			return err;
> +		}
> +	}
> +
> +	return 0;
> +}
> +

> -static int dsa_port_parse_cpu(struct dsa_port *dp, struct net_device *master)
> +static int dsa_port_parse_cpu(struct dsa_port *dp, struct net_device *master,
> +			      const char *user_protocol)
>  {
>  	struct dsa_switch *ds = dp->ds;
>  	struct dsa_switch_tree *dst = ds->dst;
> -	enum dsa_tag_protocol tag_protocol;
> +	const struct dsa_device_ops *tag_ops;
> +	enum dsa_tag_protocol default_proto;
> +
> +	/* Find out which protocol the switch would prefer. */
> +	default_proto = dsa_get_tag_protocol(dp, master);
> +	if (dst->default_proto) {
> +		if (dst->default_proto != default_proto) {
> +			dev_err(ds->dev,
> +				"A DSA switch tree can have only one tagging protovol\n");
> +			return -EINVAL;
> +		}
> +	} else {
> +		dst->default_proto = default_proto;
> +	}
> +
> +	/* See if the user wants to override that preference. */
> +	if (user_protocol && ds->ops->change_tag_protocol) {
> +		tag_ops = dsa_find_tagger_by_name(user_protocol);
> +	} else {
> +		if (user_protocol)
> +			dev_warn(ds->dev,
> +				 "Tag protocol cannot be modified, using default\n");

I would probably error out here. I don't think it is a good idea to
ignore what DT says. We also potentially have forward compatibility
problems. Somebody cut/pastes a DT fragment including an invalid
override. But the driver does not support it, so it just gives this
warning and keeps going. Sometime in the future, change support is
added, it then becomes a real error, and the driver stops probing.

       Andrew