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 PHC | |
Open Source and information security mailing list archives
| ||
|
Date: Fri, 5 Dec 2014 23:21:26 +0000 From: "Arad, Ronen" <ronen.arad@...el.com> To: Roopa Prabhu <roopa@...ulusnetworks.com>, Scott Feldman <sfeldma@...il.com>, Netdev <netdev@...r.kernel.org> CC: Jirí Pírko <jiri@...nulli.us>, Jamal Hadi Salim <jhs@...atatu.com>, Benjamin LaHaise <bcrl@...ck.org>, Thomas Graf <tgraf@...g.ch>, john fastabend <john.fastabend@...il.com>, "stephen@...workplumber.org" <stephen@...workplumber.org>, John Linville <linville@...driver.com>, "nhorman@...driver.com" <nhorman@...driver.com>, Nicolas Dichtel <nicolas.dichtel@...nd.com>, "vyasevic@...hat.com" <vyasevic@...hat.com>, Florian Fainelli <f.fainelli@...il.com>, "buytenh@...tstofly.org" <buytenh@...tstofly.org>, Aviad Raveh <aviadr@...lanox.com>, "David S. Miller" <davem@...emloft.net>, "shm@...ulusnetworks.com" <shm@...ulusnetworks.com>, Andy Gospodarek <gospo@...ulusnetworks.com> Subject: RE: [PATCH 2/3] bridge: offload bridge port attributes to switch asic if feature flag set > -----Original Message----- > From: netdev-owner@...r.kernel.org [mailto:netdev- > owner@...r.kernel.org] On Behalf Of Roopa Prabhu > Sent: Thursday, December 04, 2014 11:02 PM > To: Scott Feldman > Cc: Jiří Pírko; Jamal Hadi Salim; Benjamin LaHaise; Thomas Graf; john > fastabend; stephen@...workplumber.org; John Linville; > nhorman@...driver.com; Nicolas Dichtel; vyasevic@...hat.com; Florian > Fainelli; buytenh@...tstofly.org; Aviad Raveh; Netdev; David S. Miller; > shm@...ulusnetworks.com; Andy Gospodarek > Subject: Re: [PATCH 2/3] bridge: offload bridge port attributes to switch asic > if feature flag set > > On 12/4/14, 10:41 PM, Scott Feldman wrote: > > On Thu, Dec 4, 2014 at 6:26 PM, <roopa@...ulusnetworks.com> wrote: > >> From: Roopa Prabhu <roopa@...ulusnetworks.com> > >> > >> This allows offloading to switch asic without having the user to set > >> any flag. And this is done in the bridge driver to rollback kernel > >> settings on hw offload failure if required in the future. > >> > >> With this, it also makes sure a notification goes out only after the > >> attributes are set both in the kernel and hw. > > I like this approach as it streamlines the steps for the user in > > setting port flags. There is one case for FLOODING where you'll have > > to turn off flooding for both, and then turn on flooding in hw. You > > don't want flooding turned on on kernel and hw. > ok, maybe using the higher bits as in > https://patchwork.ozlabs.org/patch/413211/ > > might help with that. Let me think some more. > > > >> --- > >> net/bridge/br_netlink.c | 27 ++++++++++++++++++++++++++- > >> 1 file changed, 26 insertions(+), 1 deletion(-) > >> > >> diff --git a/net/bridge/br_netlink.c b/net/bridge/br_netlink.c index > >> 9f5eb55..ce173f0 100644 > >> --- a/net/bridge/br_netlink.c > >> +++ b/net/bridge/br_netlink.c > >> @@ -407,9 +407,21 @@ int br_setlink(struct net_device *dev, struct > nlmsghdr *nlh) > >> afspec, RTM_SETLINK); > >> } > >> > >> + if ((dev->features & NETIF_F_HW_SWITCH_OFFLOAD) && > >> + dev->netdev_ops->ndo_bridge_setlink) { > >> + int ret = dev->netdev_ops->ndo_bridge_setlink(dev, > >> + nlh); > > I think you want to up-level this to net/core/rtnetlink.c because > > you're only enabling the feature for one instance of a driver that > > implements ndo_bridge_setlink: the bridge driver. If another driver > > was MASTER and implemented ndo_bridge_setlink, you'd want same check > > to push setting down to SELF port driver. > > yeah, i thought about that. But i moved it here so that rollback would be > easier. There is a need for propagating setlink/dellink requests down multiple levels. The use-case I have in mind is a bridge at the top, team/bond in the middle, and port devices at the bottom. A setlink for VLAN filtering attributes would come with MASTER flag set, and either port or bond/team netdev. How would this be handled? The propagation rules between bridge and enslaved port device could be different from those between bond/team and enslaved devices. The current bridge driver does not propagate VLAN filtering from bridge to its ports as each port could have different configuration. In a case of a bond/team all members need to have the same configuration such that the a bond/team would be indistinguishable from a simple port. Therefore rtnetlink.c might not have the knowledge for propagation across multiple levels. It seems that each device which implements ndo_bridge_setlink/ndo_bridge_dellink and could have master role, need to take care of propagation to its slaves. > > > >> + if (ret && ret != -EOPNOTSUPP) { > >> + /* XXX Fix this in the future to rollback > >> + * kernel settings and return error > >> + */ > > The future is now. Let's fix this now for the rollback case (again up > > in rtnetlink.c). So then a general question comes to mind: for these > > dual target sets, is it best to try HW first and then SW, or the other > > way around? Either way, on failure on second you need to rollback > > first. And, on failure, you need to know rollback value for first, so > > you have to do a getlink on first before attempting set. > yep, exactly, I went through the same thought process yesterday when i was > trying to implement rollback. > > > >> + br_warn(p->br, "error offloading bridge attributes " > >> + "on port %u(%s)\n", (unsigned int) p->port_no, > >> + p->dev->name); > >> + } > >> + } > >> + > >> if (err == 0) > >> br_ifinfo_notify(RTM_NEWLINK, p); > >> - > >> out: > >> return err; > >> } > >> @@ -433,6 +445,19 @@ int br_dellink(struct net_device *dev, struct > nlmsghdr *nlh) > >> err = br_afspec((struct net_bridge *)netdev_priv(dev), p, > >> afspec, RTM_DELLINK); > >> > >> + if (dev->features & NETIF_F_HW_SWITCH_OFFLOAD > >> + && dev->netdev_ops->ndo_bridge_setlink) { > >> + int ret = dev->netdev_ops->ndo_bridge_dellink(dev, nlh); > >> + if (ret && ret != -EOPNOTSUPP) { > >> + /* XXX Fix this in the future to rollback > >> + * kernel settings and return error > >> + */ > >> + br_warn(p->br, "error offloading bridge attributes " > >> + "on port %u(%s)\n", (unsigned int) p->port_no, > >> + p->dev->name); > >> + } > >> + } > >> + > > Same comments as setlink above. > > > >> return err; > >> } > >> static int br_validate(struct nlattr *tb[], struct nlattr *data[]) > >> -- > >> 1.7.10.4 > >> > > -- > To unsubscribe from this list: send the line "unsubscribe netdev" in the body > of a message to majordomo@...r.kernel.org More majordomo info at > http://vger.kernel.org/majordomo-info.html
Powered by blists - more mailing lists