netdev - Re: [PATCH v5 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: <9c5f179b-056b-f513-6500-eb36f9f8df0e@gmail.com>
Date:   Wed, 20 Jan 2021 19:53:41 -0800
From:   Florian Fainelli <f.fainelli@...il.com>
To:     Vladimir Oltean <olteanv@...il.com>,
        "David S . Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>, netdev@...r.kernel.org
Cc:     Andrew Lunn <andrew@...n.ch>,
        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 v5 net-next 07/10] net: dsa: allow changing the tag
 protocol via the "tagging" device attribute



On 1/20/2021 6:36 PM, Vladimir Oltean wrote:
> From: Vladimir Oltean <vladimir.oltean@....com>
> 
> Currently DSA exposes the following sysfs:
> $ cat /sys/class/net/eno2/dsa/tagging
> ocelot
> 
> which is a read-only device attribute, introduced in the kernel as
> commit 98cdb4807123 ("net: dsa: Expose tagging protocol to user-space"),
> and used by libpcap since its commit 993db3800d7d ("Add support for DSA
> link-layer types").
> 
> It would be nice if we could extend this device attribute by making it
> writable:
> $ echo ocelot-8021q > /sys/class/net/eno2/dsa/tagging
> 
> This is useful with DSA switches that can make use of more than one
> tagging protocol. It may be useful in dsa_loop in the future too, to
> perform offline testing of various taggers, or for changing between dsa
> and edsa on Marvell switches, if that is desirable.
> 
> In terms of implementation, drivers can now move their tagging protocol
> configuration outside of .setup/.teardown, and into .set_tag_protocol
> and .del_tag_protocol. The calling order is:
> 
> .setup -> [.set_tag_protocol -> .del_tag_protocol]+ -> .teardown
> 
> There was one more contract between the DSA framework and drivers, which
> is that if a CPU port needs to account for the tagger overhead in its
> MTU configuration, it must do that privately. Which means it needs the
> information about what tagger it uses before we call its MTU
> configuration function. That promise is still held.
> 
> Writing to the tagging sysfs will first tear down the tagging protocol
> for all switches in the tree attached to that DSA master, then will
> attempt setup with the new tagger.
> 
> Writing will fail quickly with -EOPNOTSUPP for drivers that don't
> support .set_tag_protocol, since that is checked during the deletion
> phase. It is assumed that all switches within the same DSA tree use the
> same driver, and therefore either all have .set_tag_protocol implemented,
> or none do.
> 
> Signed-off-by: Vladimir Oltean <vladimir.oltean@....com>
> ---

We talked about it over IRC and I like the approach you have taken, few
comments below:

[snip]

> +static int dsa_switch_tag_proto_del(struct dsa_switch *ds,
> +				    struct dsa_notifier_tag_proto_info *info)
> +{
> +	int err = 0, port;
> +
> +	for (port = 0; port < ds->num_ports; port++) {
> +		if (!dsa_switch_tag_proto_match(ds, port, info))
> +			continue;
> +
> +		/* Check early if we can replace it, so we don't delete it
> +		 * for nothing and leave the switch dangling.
> +		 */
> +		if (!ds->ops->set_tag_protocol) {
> +			err = -EOPNOTSUPP;
> +			break;
> +		}

This can be moved out of the loop.

> +
> +		/* The delete method is optional, just the setter
> +		 * is mandatory
> +		 */
> +		if (ds->ops->del_tag_protocol)
> +			ds->ops->del_tag_protocol(ds, port,
> +						  info->tag_ops->proto);
> +	}
> +
> +	return err;
> +}
> +
> +static int dsa_switch_tag_proto_set(struct dsa_switch *ds,
> +				    struct dsa_notifier_tag_proto_info *info)
> +{
> +	bool proto_changed = false;
> +	int port, err;
> +
> +	for (port = 0; port < ds->num_ports; port++) {
> +		struct dsa_port *cpu_dp = dsa_to_port(ds, port);
> +
> +		if (!dsa_switch_tag_proto_match(ds, port, info))
> +			continue;
> +
> +		err = ds->ops->set_tag_protocol(ds, cpu_dp->index,
> +						info->tag_ops->proto);
> +		if (err)
> +			return err;

Don't you need to test for ds->ops->set_tag_protocol to be implemented
before calling it? Similar comment to earlier, can we do an early check
for the operation being supported outside of the loop?
-- 
Florian