| 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: <E4CD12F19ABA0C4D8729E087A761DC3505D842BA@ORSMSX101.amr.corp.intel.com>
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