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
| ||
|
Date: Mon, 23 May 2022 23:08:35 +0000 From: Vladimir Oltean <vladimir.oltean@....com> To: Florian Fainelli <f.fainelli@...il.com> CC: Vladimir Oltean <olteanv@...il.com>, "netdev@...r.kernel.org" <netdev@...r.kernel.org>, 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" <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> Subject: Re: [RFC PATCH net-next 10/12] net: dsa: allow the DSA master to be seen and changed through rtnetlink On Mon, May 23, 2022 at 11:41:45AM -0700, Florian Fainelli wrote: > > +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. TBH I don't have a very strong opinion here. IFLA_LINK does not mean the same thing for all virtual netdevices, it means one thing for vlan/macvlan where it describes an upper/lower relationship and another for veth where it describes the pair, and yet another for DSA where it describes the host port. What seems to be universally loved about IFLA_LINK is that the notation "eth0@...1" used by iproute2 is cute, it lets loose users' imagination. I did ask here whether it would be good to introduce a more specific attribute, no response though. https://lore.kernel.org/netdev/20210411170939.cxmva5vdcpqu4bmi@skbuf/ If the IFLA_LINK meaning is namespaced per netdev kind, I suppose we could reuse that just fine to change the DSA master. In any case I wouldn't want to make the debate of the century out of this. > > + 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? The idea was to avoid code movement if we ever add other netlink attributes other than IFLA_DSA_MASTER. But not sure whether to optimize for that. > [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? Here we print if "fail_on_err" was false. We avoid failing on errors when we are in an error rollback code path. This is also the reason why I did not set extack, presumably because it may have been set before by ds->ops->port_change_master. Printing to the console shows all errors along the path, setting the extack shows only the first, or last, error. > > + > > + 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. Here we could populate the extack. > [snip] > > > + if (!ds->ops->port_change_master) > > + return -EOPNOTSUPP; > > This could be provided over extactk since it is not even supposed to be > happening. What do you mean it's not supposed to be happening? This is the only place where we have a NULL check for ds->ops->port_change_master. I didn't add an extack here because I didn't think there's much to say beside the usual strerror(EOPNOTSUPP) = "Operation not supported". I may add an extack saying "Driver does not support changing DSA master" or some sort of message like that.
Powered by blists - more mailing lists