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 22:29:24 -0800 From: Scott Feldman <sfeldma@...il.com> To: "Arad, Ronen" <ronen.arad@...el.com> Cc: Roopa Prabhu <roopa@...ulusnetworks.com>, Netdev <netdev@...r.kernel.org>, 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 On Fri, Dec 5, 2014 at 5:04 PM, Arad, Ronen <ronen.arad@...el.com> wrote: > I have another case of propagation which is not covered by the proposed patch. > A recent patch introduced default_pvid attribute for a bridge (so far supported only via sysfs and not via netlink). > When a port joins a bridge, it inherits a PVID from the default_pvid of the bridge. > The bridge driver propagates that to the newly created net_bridge_port. This is done in br_vlan.c: > > int nbp_vlan_init(struct net_bridge_port *p) > { > int rc = 0; > > if (p->br->default_pvid) { > rc = nbp_vlan_add(p, p->br->default_pvid, > BRIDGE_VLAN_INFO_PVID | > BRIDGE_VLAN_INFO_UNTAGGED); > } > > return rc; > } > > When L2 switching is offloaded to the HW, this PVID setting need to be propagated. Agreed, it would be nice to have it propagated down, but there is a non-ideal work-around. If you set default_pvid=0 to turn off PVID, then the switch port driver can pick some internal VLAN ID just for HW purposes in matching untagged pkts. It's non-ideal because the switch port driver needs to reserve a block of VLAN IDs for internal usage or use some other matching mechanism to keep untagged pkts within this bridge. Better to have default_pvid value propagated down. But, default_pvid is a per-bridge property, not a per-bridge-port property. RTM_SETLINK/RTM_GETLINK for PF_BRIDGE does have AFSPEC for per-bridge and PROTINFO for per-bridge-port, so it seems PVID needs to be part of AFSPEC. >However, it does not come via ndo_bridge_setlink. The proposed propagation at br_setlink or an up level one at rtnetlink are not capable of handling this case. > One possible way for handling that is to replace the call to nbp_vlan_add with a call to a new function let's say > int br_propagate_vlan_add(struct net_bridge_port *port, u16 vid, u16 flags) > This function will compose a netlink message with VLAN filtering information (i.e. AF_SPEC with VLAN_INFO) and call br_setlink - leveraging the offload support proposed by Roopa. > > If this is an acceptable course of action, I could work on such patch. > > >> -----Original Message----- >> From: netdev-owner@...r.kernel.org [mailto:netdev- >> owner@...r.kernel.org] On Behalf Of Arad, Ronen >> Sent: Friday, December 05, 2014 3:21 PM >> To: Roopa Prabhu; Scott Feldman; Netdev >> Cc: Jirí 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; 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 >> >> >> >> > -----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 >> {.n + +% lzwm b 맲 r zw u ^n r z h & G h ( 階 ݢj" m z ޖ >> f h ~ m -- 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