| 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: <77EF4405DD4BB54AACCE7DB593DF6A9A9E2BFE@SJEXCHMB14.corp.ad.broadcom.com>
Date: Wed, 19 Aug 2015 09:34:30 +0000
From: Premkumar Jonnala <pjonnala@...adcom.com>
To: Scott Feldman <sfeldma@...il.com>
CC: "netdev@...r.kernel.org" <netdev@...r.kernel.org>
Subject: RE: [PATCH] bridge: Enable configuration of ageing interval for
bridges and switch devices.
Hello Scott,
Thank you for the diff and comments. Please see my comments inline.
> -----Original Message-----
> From: Scott Feldman [mailto:sfeldma@...il.com]
> Sent: Tuesday, August 18, 2015 12:48 PM
> To: Premkumar Jonnala
> Cc: netdev@...r.kernel.org
> Subject: Re: [PATCH] bridge: Enable configuration of ageing interval for bridges
> and switch devices.
>
>
>
> On Fri, 14 Aug 2015, Premkumar Jonnala wrote:
>
> > Bridge devices have ageing interval used to age out MAC addresses
> > from FDB. This ageing interval was not configuratble.
> >
> > Enable netlink based configuration of ageing interval for bridges and
> > switch devices. The ageing interval changes the timer used to purge
> > inactive FDB entries in bridges. The ageing interval config is
> > propagated to switch devices, so that platform or hardware based
> > ageing works according to configuration.
> >
> > Signed-off-by: Premkumar Jonnala <pjonnala@...adcom.com>
>
> Hi Premkumar,
>
> I agree with Roopa that we should use existing IFLA_BR_AGEING_TIME.
What is the motivation for using 'ip link' command to configure bridge attributes? IMHO,
bridge command is better suited for that.
>
> I hope you don't mind if share a patch for setting bridge-level attributes
> down to the switch hardware ports. It uses the switchdev_port_attr_set()
> call on the bridge interface itself when any IFLA_BR_xxx attribute is set
> via netlink. switchdev_port_attr_set() will do a recursive walk of all
> lower devs from the bridge, stopping at each to set the attribute. I
> added one small tweak to switchdev_port_attr_set() to allow skipping over
> ports that return -EOPNOTSUPP. The advantage to using
> switchdev_port_attr_set() is it automatically knows how to find the leaf
> ports in a stacked driver setup, and it uses the prepare/commit
> transaction model to make sure all ports can support new attr setting
> before committing setting to hardware.
>
> I gave an example using rocker to set IFLA_BR_AGEING_TIME. It's stubbed
> out, but you get the idea. Other IFLA_BR_xxx attrs can be added to this
> model.
>
> Does this look like something that would work? If so, would you mind
> running with this patch and maybe even extending it for other IFLA_BR_xxx
> attrs?
Sure, let me go with this patch. Couple of comments inline.
>
> Again, I hope I'm not stepping on toes here by rewriting your patch, but
> it was the best way to communicate the idea of how to use the switchdev
> frame work for these bridge attrs.
>
> -scott
>
>
>
>
> diff --git a/drivers/net/ethernet/rocker/rocker.c
> b/drivers/net/ethernet/rocker/rocker.c
> index 0cc9e8f..830f8e6 100644
> --- a/drivers/net/ethernet/rocker/rocker.c
> +++ b/drivers/net/ethernet/rocker/rocker.c
> @@ -4680,6 +4680,24 @@ static int rocker_port_brport_flags_set(struct
> rocker_port *rocker_port,
> return err;
> }
>
> +static int rocker_port_bridge_set(struct rocker_port *rocker_port,
> + enum switchdev_trans trans,
> + struct switchdev_attr_bridge *bridge)
> +{
> + switch (bridge->attr) {
> + case IFLA_BR_AGEING_TIME:
> + {
> + u32 ageing_time = bridge->val;
> + /* XXX push ageing_time down to device */
> + }
> + break;
> + default:
> + return -EOPNOTSUPP;
> + }
> +
> + return 0;
> +}
> +
> static int rocker_port_attr_set(struct net_device *dev,
> struct switchdev_attr *attr)
> {
> @@ -4707,6 +4725,10 @@ static int rocker_port_attr_set(struct net_device
> *dev,
> err = rocker_port_brport_flags_set(rocker_port, attr->trans,
> attr->u.brport_flags);
> break;
> + case SWITCHDEV_ATTR_BRIDGE:
> + err = rocker_port_bridge_set(rocker_port, attr->trans,
> + &attr->u.bridge);
> + break;
> default:
> err = -EOPNOTSUPP;
> break;
> diff --git a/include/net/switchdev.h b/include/net/switchdev.h
> index 319baab..22a6dbe 100644
> --- a/include/net/switchdev.h
> +++ b/include/net/switchdev.h
> @@ -15,6 +15,7 @@
> #include <linux/notifier.h>
>
> #define SWITCHDEV_F_NO_RECURSE BIT(0)
> +#define SWITCHDEV_F_SKIP_EOPNOTSUPP BIT(1)
>
> enum switchdev_trans {
> SWITCHDEV_TRANS_NONE,
> @@ -28,6 +29,7 @@ enum switchdev_attr_id {
> SWITCHDEV_ATTR_PORT_PARENT_ID,
> SWITCHDEV_ATTR_PORT_STP_STATE,
> SWITCHDEV_ATTR_PORT_BRIDGE_FLAGS,
> + SWITCHDEV_ATTR_BRIDGE,
> };
>
> struct switchdev_attr {
> @@ -38,6 +40,10 @@ struct switchdev_attr {
> struct netdev_phys_item_id ppid; /* PORT_PARENT_ID
> */
> u8 stp_state; /* PORT_STP_STATE
> */
> unsigned long brport_flags; /*
> PORT_BRIDGE_FLAGS */
> + struct switchdev_attr_bridge { /* BRIDGE */
> + enum ifla_br attr;
> + u32 val;
> + } bridge;
> } u;
> };
>
> diff --git a/include/uapi/linux/if_link.h b/include/uapi/linux/if_link.h
> index ff659f0..0630053 100644
> --- a/include/uapi/linux/if_link.h
> +++ b/include/uapi/linux/if_link.h
> @@ -222,7 +222,7 @@ enum in6_addr_gen_mode {
>
> /* Bridge section */
>
> -enum {
> +enum ifla_br {
> IFLA_BR_UNSPEC,
> IFLA_BR_FORWARD_DELAY,
> IFLA_BR_HELLO_TIME,
> diff --git a/net/bridge/br_netlink.c b/net/bridge/br_netlink.c
> index 0f2408f..01401ea 100644
> --- a/net/bridge/br_netlink.c
> +++ b/net/bridge/br_netlink.c
> @@ -759,9 +759,9 @@ static int br_changelink(struct net_device *brdev, struct
> nlattr *tb[],
> }
>
> if (data[IFLA_BR_AGEING_TIME]) {
> - u32 ageing_time = nla_get_u32(data[IFLA_BR_AGEING_TIME]);
Should we do some range checking here to ensure that the value is within a certain range.
IEEE 802.1d recommends that the ageing time be between 10 sec and 1 million seconds.
> -
> - br->ageing_time = clock_t_to_jiffies(ageing_time);
> + err = br_set_ageing_time(br,
> nla_get_u32(data[IFLA_BR_AGEING_TIME]));
> + if (err)
> + return err;
> }
>
> if (data[IFLA_BR_STP_STATE]) {
> diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
> index 3d95647..9807a6f 100644
> --- a/net/bridge/br_private.h
> +++ b/net/bridge/br_private.h
> @@ -807,6 +807,7 @@ void __br_set_forward_delay(struct net_bridge *br,
> unsigned long t);
> int br_set_forward_delay(struct net_bridge *br, unsigned long x);
> int br_set_hello_time(struct net_bridge *br, unsigned long x);
> int br_set_max_age(struct net_bridge *br, unsigned long x);
> +int br_set_ageing_time(struct net_bridge *br, u32 ageing_time);
>
>
> /* br_stp_if.c */
> diff --git a/net/bridge/br_stp.c b/net/bridge/br_stp.c
> index ed74ffa..5de27bc 100644
> --- a/net/bridge/br_stp.c
> +++ b/net/bridge/br_stp.c
> @@ -566,6 +566,25 @@ int br_set_max_age(struct net_bridge *br, unsigned
> long val)
>
> }
>
> +int br_set_ageing_time(struct net_bridge *br, u32 ageing_time)
> +{
> + struct switchdev_attr attr = {
> + .id = SWITCHDEV_ATTR_BRIDGE,
> + .flags = SWITCHDEV_F_SKIP_EOPNOTSUPP,
> + .u.bridge.attr = IFLA_BR_AGEING_TIME,
> + .u.bridge.val = ageing_time,
> + };
> + int err;
> +
> + err = switchdev_port_attr_set(br->dev, &attr);
> + if (err)
> + return err;
> +
> + br->ageing_time = clock_t_to_jiffies(ageing_time);
Should we restart the timer here the new time takes effect?
> +
> + return 0;
> +}
> +
> void __br_set_forward_delay(struct net_bridge *br, unsigned long t)
> {
> br->bridge_forward_delay = t;
> diff --git a/net/switchdev/switchdev.c b/net/switchdev/switchdev.c
> index 16c1c43..b990301 100644
> --- a/net/switchdev/switchdev.c
> +++ b/net/switchdev/switchdev.c
> @@ -73,7 +73,7 @@ static int __switchdev_port_attr_set(struct net_device
> *dev,
> return ops->switchdev_port_attr_set(dev, attr);
>
> if (attr->flags & SWITCHDEV_F_NO_RECURSE)
> - return err;
> + goto done;
>
> /* Switch device port(s) may be stacked under
> * bond/team/vlan dev, so recurse down to set attr on
> @@ -82,10 +82,17 @@ static int __switchdev_port_attr_set(struct net_device
> *dev,
>
> netdev_for_each_lower_dev(dev, lower_dev, iter) {
> err = __switchdev_port_attr_set(lower_dev, attr);
> + if (err == -EOPNOTSUPP &&
> + attr->flags & SWITCHDEV_F_SKIP_EOPNOTSUPP)
> + continue;
> if (err)
> break;
> }
>
> +done:
> + if (err == -EOPNOTSUPP && attr->flags &
> SWITCHDEV_F_SKIP_EOPNOTSUPP)
> + err = 0;
> +
> return err;
> }
-Prem
--
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