| 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
| ||
|
Message-ID: <d5d485e3-bddf-f052-2f46-f306f53f3d34@gmail.com>
Date: Mon, 23 May 2022 11:41:45 -0700
From: Florian Fainelli <f.fainelli@...il.com>
To: Vladimir Oltean <olteanv@...il.com>, netdev@...r.kernel.org
Cc: Jakub Kicinski <kuba@...nel.org>,
Vivien Didelot <vivien.didelot@...il.com>,
Andrew Lunn <andrew@...n.ch>,
Tobias Waldekranz <tobias@...dekranz.com>,
Marek Behún <kabel@...nel.org>,
Ansuel Smith <ansuelsmth@...il.com>,
DENG Qingfang <dqfext@...il.com>,
Alvin Šipraga <alsi@...g-olufsen.dk>,
Claudiu Manoil <claudiu.manoil@....com>,
Alexandre Belloni <alexandre.belloni@...tlin.com>,
UNGLinuxDriver@...rochip.com,
Colin Foster <colin.foster@...advantage.com>,
Linus Walleij <linus.walleij@...aro.org>,
Luiz Angelo Daros de Luca <luizluca@...il.com>,
Roopa Prabhu <roopa@...dia.com>,
Nikolay Aleksandrov <razor@...ckwall.org>,
Frank Wunderlich <frank-w@...lic-files.de>,
Vladimir Oltean <vladimir.oltean@....com>
Subject: Re: [RFC PATCH net-next 10/12] net: dsa: allow the DSA master to be
seen and changed through rtnetlink
On 5/23/22 03:42, Vladimir Oltean wrote:
> From: Vladimir Oltean <vladimir.oltean@....com>
>
> Some DSA switches have multiple CPU ports, which can be used to improve
> CPU termination throughput, but DSA, through dsa_tree_setup_cpu_ports(),
> sets up only the first one, leading to suboptimal use of hardware.
>
> The desire is to not change the default configuration but to permit the
> user to create a dynamic mapping between individual user ports and the
> CPU port that they are served by, configurable through rtnetlink. It is
> also intended to permit load balancing between CPU ports, and in that
> case, the foreseen model is for the DSA master to be a bonding interface
> whose lowers are the physical DSA masters.
>
> To that end, we create a struct rtnl_link_ops for DSA user ports with
> the "dsa" kind. We expose the IFLA_DSA_MASTER link attribute that
> contains the ifindex of the newly desired DSA master.
>
> Signed-off-by: Vladimir Oltean <vladimir.oltean@....com>
> ---
[snip]
> +
> +static int dsa_changelink(struct net_device *dev, struct nlattr *tb[],
> + struct nlattr *data[],
> + struct netlink_ext_ack *extack)
> +{
> + int err;
> +
> + if (!data)
> + return 0;
> +
> + if (data[IFLA_DSA_MASTER]) {
We could add a comment to explain that IFLA_LINK is "reserved" for
standard usage of associating the DSA device with a different upper
type, like VLAN, bridge master etc.
> + u32 ifindex = nla_get_u32(data[IFLA_DSA_MASTER]);
> + struct net_device *master;
> +
> + master = __dev_get_by_index(dev_net(dev), ifindex);
> + if (!master)
> + return -EINVAL;
> +
> + err = dsa_slave_change_master(dev, master, extack);
> + if (err)
> + return err;
> + }
I would be tempted to reduce the indentation here because we are almost
guaranteed to add code in that conditional section?
[snip]
>
> +static int dsa_port_assign_master(struct dsa_port *dp,
> + struct net_device *master,
> + struct netlink_ext_ack *extack,
> + bool fail_on_err)
> +{
> + struct dsa_switch *ds = dp->ds;
> + int port = dp->index, err;
> +
> + err = ds->ops->port_change_master(ds, port, master, extack);
> + if (err && !fail_on_err)
> + dev_err(ds->dev, "port %d failed to assign master %s: %pe\n",
> + port, master->name, ERR_PTR(err));
Should not that go over extack instead?
> +
> + if (err && fail_on_err)
> + return err;
> +
> + dp->cpu_dp = master->dsa_ptr;
> +
> + return 0;
> +}
> +
> +/* Change the dp->cpu_dp affinity for a user port. Note that both cross-chip
> + * notifiers and drivers have implicit assumptions about user-to-CPU-port
> + * mappings, so we unfortunately cannot delay the deletion of the objects
> + * (switchdev, standalone addresses, standalone VLANs) on the old CPU port
> + * until the new CPU port has been set up. So we need to completely tear down
> + * the old CPU port before changing it, and restore it on errors during the
> + * bringup of the new one.
> + */
> +int dsa_port_change_master(struct dsa_port *dp, struct net_device *master,
> + struct netlink_ext_ack *extack)
> +{
> + struct net_device *bridge_dev = dsa_port_bridge_dev_get(dp);
> + struct net_device *old_master = dsa_port_to_master(dp);
> + struct net_device *dev = dp->slave;
> + struct dsa_switch *ds = dp->ds;
> + int port = dp->index;
> + bool vlan_filtering;
> + int err, tmp;
> +
> + /* Bridges may hold host FDB, MDB and VLAN objects. These need to be
> + * migrated, so dynamically unoffload and later reoffload the bridge
> + * port.
> + */
> + if (bridge_dev) {
> + dsa_port_pre_bridge_leave(dp, bridge_dev);
> + dsa_port_bridge_leave(dp, bridge_dev);
> + }
> +
> + /* The port might still be VLAN filtering even if it's no longer
> + * under a bridge, either due to ds->vlan_filtering_is_global or
> + * ds->needs_standalone_vlan_filtering. In turn this means VLANs
> + * on the CPU port.
> + */
> + vlan_filtering = dsa_port_is_vlan_filtering(dp);
> + if (vlan_filtering) {
> + err = dsa_slave_manage_vlan_filtering(dev, false);
> + if (err) {
> + dev_err(ds->dev,
> + "port %d failed to remove standalone VLANs: %pe\n",
> + port, ERR_PTR(err));
Likewise, should not that be via extack? And likewise for pretty much
any message down below.
[snip]
> + if (!ds->ops->port_change_master)
> + return -EOPNOTSUPP;
This could be provided over extactk since it is not even supposed to be
happening.
--
Florian
Powered by blists - more mailing lists