Joachim Wiberg
2022-Apr-11 13:38 UTC
[Bridge] [PATCH RFC net-next 04/13] net: bridge: netlink support for controlling BUM flooding to bridge
This patch adds netlink support for controlling the new broadcast, unicast, and multicast flooding flags to the bridge itself. The messy part is in br_setport(), which re-indents a large block of code for the port settings. To reduce code duplication a few new variables have been added; new_flags and dev. The latter is used for the recently renamed br_switchdev_set_dev_flag(), which can now be used by underlying switching fabric drivers as another source of information when controlling flooding of unknown BUM traffic to the CPU port. Signed-off-by: Joachim Wiberg <troglobit at gmail.com> --- net/bridge/br_netlink.c | 160 ++++++++++++++++++++++++++++++---------- 1 file changed, 123 insertions(+), 37 deletions(-) diff --git a/net/bridge/br_netlink.c b/net/bridge/br_netlink.c index 8f4297287b32..68bbf703b31a 100644 --- a/net/bridge/br_netlink.c +++ b/net/bridge/br_netlink.c @@ -225,13 +225,29 @@ static inline size_t br_nlmsg_size(struct net_device *dev, u32 filter_mask) + nla_total_size(4); /* IFLA_BRPORT_BACKUP_PORT */ } -static int br_port_fill_attrs(struct sk_buff *skb, +static int br_port_fill_attrs(struct sk_buff *skb, const struct net_bridge *br, const struct net_bridge_port *p) { - u8 mode = !!(p->flags & BR_HAIRPIN_MODE); struct net_bridge_port *backup_p; u64 timerval; + u8 mode; + if (!p) { + if (!br) + return -EINVAL; + + if (nla_put_u8(skb, IFLA_BRPORT_UNICAST_FLOOD, + br_opt_get(br, BROPT_UNICAST_FLOOD)) || + nla_put_u8(skb, IFLA_BRPORT_MCAST_FLOOD, + br_opt_get(br, BROPT_MCAST_FLOOD)) || + nla_put_u8(skb, IFLA_BRPORT_BCAST_FLOOD, + br_opt_get(br, BROPT_BCAST_FLOOD))) + return -EMSGSIZE; + + return 0; + } + + mode = !!(p->flags & BR_HAIRPIN_MODE); if (nla_put_u8(skb, IFLA_BRPORT_STATE, p->state) || nla_put_u16(skb, IFLA_BRPORT_PRIORITY, p->priority) || nla_put_u32(skb, IFLA_BRPORT_COST, p->path_cost) || @@ -475,11 +491,11 @@ static int br_fill_ifinfo(struct sk_buff *skb, nla_put_u32(skb, IFLA_LINK, dev_get_iflink(dev)))) goto nla_put_failure; - if (event == RTM_NEWLINK && port) { + if (event == RTM_NEWLINK) { struct nlattr *nest; nest = nla_nest_start(skb, IFLA_PROTINFO); - if (nest == NULL || br_port_fill_attrs(skb, port) < 0) + if (!nest || br_port_fill_attrs(skb, br, port) < 0) goto nla_put_failure; nla_nest_end(skb, nest); } @@ -911,43 +927,113 @@ static void br_set_port_flag(struct net_bridge_port *p, struct nlattr *tb[], p->flags &= ~mask; } +/* Map bridge options to brport flags */ +static unsigned long br_boolopt_map_flags(struct br_boolopt_multi *bm) +{ + unsigned long bitmap = bm->optmask; + unsigned long bitmask = 0; + int opt_id; + + for_each_set_bit(opt_id, &bitmap, BR_BOOLOPT_MAX) { + if (!(bm->optval & BIT(opt_id))) + continue; + + switch (opt_id) { + case BROPT_UNICAST_FLOOD: + bitmask |= BR_FLOOD; + break; + case BROPT_MCAST_FLOOD: + bitmask |= BR_MCAST_FLOOD; + break; + case BROPT_BCAST_FLOOD: + bitmask |= BR_BCAST_FLOOD; + break; + } + } + + return bitmask; +} + +static void br_set_bropt(struct net_bridge *br, struct nlattr *tb[], + int attrtype, enum net_bridge_opts opt) +{ + if (!tb[attrtype]) + return; + + br_opt_toggle(br, opt, !!nla_get_u8(tb[attrtype])); +} + +#define BROPT_MASK (BROPT_UNICAST_FLOOD | BROPT_MCAST_FLOOD | BROPT_MCAST_FLOOD) + /* Process bridge protocol info on port */ -static int br_setport(struct net_bridge_port *p, struct nlattr *tb[], - struct netlink_ext_ack *extack) +static int br_setport(struct net_bridge *br, struct net_bridge_port *p, + struct nlattr *tb[], struct netlink_ext_ack *extack) { - unsigned long old_flags, changed_mask; + unsigned long old_flags, new_flags, changed_mask; + struct br_boolopt_multi old_opts = { + .optmask = BROPT_MASK + }; bool br_vlan_tunnel_old; + struct net_device *dev; int err; - old_flags = p->flags; - br_vlan_tunnel_old = (old_flags & BR_VLAN_TUNNEL) ? true : false; - - br_set_port_flag(p, tb, IFLA_BRPORT_MODE, BR_HAIRPIN_MODE); - br_set_port_flag(p, tb, IFLA_BRPORT_GUARD, BR_BPDU_GUARD); - br_set_port_flag(p, tb, IFLA_BRPORT_FAST_LEAVE, - BR_MULTICAST_FAST_LEAVE); - br_set_port_flag(p, tb, IFLA_BRPORT_PROTECT, BR_ROOT_BLOCK); - br_set_port_flag(p, tb, IFLA_BRPORT_LEARNING, BR_LEARNING); - br_set_port_flag(p, tb, IFLA_BRPORT_UNICAST_FLOOD, BR_FLOOD); - br_set_port_flag(p, tb, IFLA_BRPORT_MCAST_FLOOD, BR_MCAST_FLOOD); - br_set_port_flag(p, tb, IFLA_BRPORT_MCAST_TO_UCAST, - BR_MULTICAST_TO_UNICAST); - br_set_port_flag(p, tb, IFLA_BRPORT_BCAST_FLOOD, BR_BCAST_FLOOD); - br_set_port_flag(p, tb, IFLA_BRPORT_PROXYARP, BR_PROXYARP); - br_set_port_flag(p, tb, IFLA_BRPORT_PROXYARP_WIFI, BR_PROXYARP_WIFI); - br_set_port_flag(p, tb, IFLA_BRPORT_VLAN_TUNNEL, BR_VLAN_TUNNEL); - br_set_port_flag(p, tb, IFLA_BRPORT_NEIGH_SUPPRESS, BR_NEIGH_SUPPRESS); - br_set_port_flag(p, tb, IFLA_BRPORT_ISOLATED, BR_ISOLATED); - br_set_port_flag(p, tb, IFLA_BRPORT_LOCKED, BR_PORT_LOCKED); - - changed_mask = old_flags ^ p->flags; - - err = br_switchdev_set_dev_flag(p->dev, p->flags, changed_mask, extack); + if (p) { + old_flags = p->flags; + br_vlan_tunnel_old = (old_flags & BR_VLAN_TUNNEL) ? true : false; + + br_set_port_flag(p, tb, IFLA_BRPORT_MODE, BR_HAIRPIN_MODE); + br_set_port_flag(p, tb, IFLA_BRPORT_GUARD, BR_BPDU_GUARD); + br_set_port_flag(p, tb, IFLA_BRPORT_FAST_LEAVE, + BR_MULTICAST_FAST_LEAVE); + br_set_port_flag(p, tb, IFLA_BRPORT_PROTECT, BR_ROOT_BLOCK); + br_set_port_flag(p, tb, IFLA_BRPORT_LEARNING, BR_LEARNING); + br_set_port_flag(p, tb, IFLA_BRPORT_UNICAST_FLOOD, BR_FLOOD); + br_set_port_flag(p, tb, IFLA_BRPORT_MCAST_FLOOD, BR_MCAST_FLOOD); + br_set_port_flag(p, tb, IFLA_BRPORT_MCAST_TO_UCAST, + BR_MULTICAST_TO_UNICAST); + br_set_port_flag(p, tb, IFLA_BRPORT_BCAST_FLOOD, BR_BCAST_FLOOD); + br_set_port_flag(p, tb, IFLA_BRPORT_PROXYARP, BR_PROXYARP); + br_set_port_flag(p, tb, IFLA_BRPORT_PROXYARP_WIFI, BR_PROXYARP_WIFI); + br_set_port_flag(p, tb, IFLA_BRPORT_VLAN_TUNNEL, BR_VLAN_TUNNEL); + br_set_port_flag(p, tb, IFLA_BRPORT_NEIGH_SUPPRESS, BR_NEIGH_SUPPRESS); + br_set_port_flag(p, tb, IFLA_BRPORT_ISOLATED, BR_ISOLATED); + br_set_port_flag(p, tb, IFLA_BRPORT_LOCKED, BR_PORT_LOCKED); + + new_flags = p->flags; + dev = p->dev; + } else { + struct br_boolopt_multi opts = { + .optmask = BROPT_MASK + }; + + br_boolopt_multi_get(br, &old_opts); + old_flags = br_boolopt_map_flags(&old_opts); + + br_set_bropt(br, tb, IFLA_BRPORT_UNICAST_FLOOD, BROPT_UNICAST_FLOOD); + br_set_bropt(br, tb, IFLA_BRPORT_MCAST_FLOOD, BROPT_MCAST_FLOOD); + br_set_bropt(br, tb, IFLA_BRPORT_BCAST_FLOOD, BROPT_BCAST_FLOOD); + + br_boolopt_multi_get(br, &opts); + new_flags = br_boolopt_map_flags(&opts); + dev = br->dev; + } + + changed_mask = old_flags ^ new_flags; + + err = br_switchdev_set_dev_flag(dev, new_flags, changed_mask, extack); if (err) { - p->flags = old_flags; + if (!p) + br_boolopt_multi_toggle(br, &old_opts, extack); + else + p->flags = old_flags; + return err; } + /* Skip the rest for the bridge itself, for now */ + if (!p) + return 0; + if (br_vlan_tunnel_old && !(p->flags & BR_VLAN_TUNNEL)) nbp_vlan_tunnel_info_flush(p); @@ -1048,7 +1134,7 @@ int br_setlink(struct net_device *dev, struct nlmsghdr *nlh, u16 flags, if (!p && !afspec) return -EINVAL; - if (p && protinfo) { + if (protinfo) { if (protinfo->nla_type & NLA_F_NESTED) { err = nla_parse_nested_deprecated(tb, IFLA_BRPORT_MAX, protinfo, @@ -1058,9 +1144,9 @@ int br_setlink(struct net_device *dev, struct nlmsghdr *nlh, u16 flags, return err; spin_lock_bh(&br->lock); - err = br_setport(p, tb, extack); + err = br_setport(br, p, tb, extack); spin_unlock_bh(&br->lock); - } else { + } else if (p) { /* Binary compatibility with old RSTP */ if (nla_len(protinfo) < sizeof(u8)) return -EINVAL; @@ -1153,7 +1239,7 @@ static int br_port_slave_changelink(struct net_device *brdev, return 0; spin_lock_bh(&br->lock); - ret = br_setport(br_port_get_rtnl(dev), data, extack); + ret = br_setport(br, br_port_get_rtnl(dev), data, extack); spin_unlock_bh(&br->lock); return ret; @@ -1163,7 +1249,7 @@ static int br_port_fill_slave_info(struct sk_buff *skb, const struct net_device *brdev, const struct net_device *dev) { - return br_port_fill_attrs(skb, br_port_get_rtnl(dev)); + return br_port_fill_attrs(skb, NULL, br_port_get_rtnl(dev)); } static size_t br_port_get_slave_size(const struct net_device *brdev, -- 2.25.1
Nikolay Aleksandrov
2022-Apr-12 18:24 UTC
[Bridge] [PATCH RFC net-next 04/13] net: bridge: netlink support for controlling BUM flooding to bridge
On 11/04/2022 16:38, Joachim Wiberg wrote:> This patch adds netlink support for controlling the new broadcast, > unicast, and multicast flooding flags to the bridge itself. > > The messy part is in br_setport(), which re-indents a large block of > code for the port settings. To reduce code duplication a few new > variables have been added; new_flags and dev. The latter is used for > the recently renamed br_switchdev_set_dev_flag(), which can now be used > by underlying switching fabric drivers as another source of information > when controlling flooding of unknown BUM traffic to the CPU port. > > Signed-off-by: Joachim Wiberg <troglobit at gmail.com> > ---Absolutely not. This is just wrong on a few levels and way too hacky. Please separate the bridge handling altogether and make it clean. No need to integrate it with the port handling, also I think you've missed a few things about bool options, more below... For boolopts examples you can check BR_BOOLOPT_NO_LL_LEARN, BR_BOOLOPT_MCAST_VLAN_SNOOPING and BR_BOOLOPT_MST_ENABLE.> net/bridge/br_netlink.c | 160 ++++++++++++++++++++++++++++++---------- > 1 file changed, 123 insertions(+), 37 deletions(-) > > diff --git a/net/bridge/br_netlink.c b/net/bridge/br_netlink.c > index 8f4297287b32..68bbf703b31a 100644 > --- a/net/bridge/br_netlink.c > +++ b/net/bridge/br_netlink.c > @@ -225,13 +225,29 @@ static inline size_t br_nlmsg_size(struct net_device *dev, u32 filter_mask) > + nla_total_size(4); /* IFLA_BRPORT_BACKUP_PORT */ > } > > -static int br_port_fill_attrs(struct sk_buff *skb, > +static int br_port_fill_attrs(struct sk_buff *skb, const struct net_bridge *br, > const struct net_bridge_port *p) > { > - u8 mode = !!(p->flags & BR_HAIRPIN_MODE); > struct net_bridge_port *backup_p; > u64 timerval; > + u8 mode; > > + if (!p) { > + if (!br) > + return -EINVAL; > + > + if (nla_put_u8(skb, IFLA_BRPORT_UNICAST_FLOOD, > + br_opt_get(br, BROPT_UNICAST_FLOOD)) || > + nla_put_u8(skb, IFLA_BRPORT_MCAST_FLOOD, > + br_opt_get(br, BROPT_MCAST_FLOOD)) || > + nla_put_u8(skb, IFLA_BRPORT_BCAST_FLOOD, > + br_opt_get(br, BROPT_BCAST_FLOOD))) > + return -EMSGSIZE;No. Bool opts are already exposed through IFLA_BR_MULTI_BOOLOPT.> + > + return 0; > + } > + > + mode = !!(p->flags & BR_HAIRPIN_MODE); > if (nla_put_u8(skb, IFLA_BRPORT_STATE, p->state) || > nla_put_u16(skb, IFLA_BRPORT_PRIORITY, p->priority) || > nla_put_u32(skb, IFLA_BRPORT_COST, p->path_cost) || > @@ -475,11 +491,11 @@ static int br_fill_ifinfo(struct sk_buff *skb, > nla_put_u32(skb, IFLA_LINK, dev_get_iflink(dev)))) > goto nla_put_failure; > > - if (event == RTM_NEWLINK && port) { > + if (event == RTM_NEWLINK) { > struct nlattr *nest; > > nest = nla_nest_start(skb, IFLA_PROTINFO); > - if (nest == NULL || br_port_fill_attrs(skb, port) < 0) > + if (!nest || br_port_fill_attrs(skb, br, port) < 0) > goto nla_put_failure; > nla_nest_end(skb, nest); > } > @@ -911,43 +927,113 @@ static void br_set_port_flag(struct net_bridge_port *p, struct nlattr *tb[], > p->flags &= ~mask; > } > > +/* Map bridge options to brport flags */ > +static unsigned long br_boolopt_map_flags(struct br_boolopt_multi *bm) > +{ > + unsigned long bitmap = bm->optmask; > + unsigned long bitmask = 0; > + int opt_id; > + > + for_each_set_bit(opt_id, &bitmap, BR_BOOLOPT_MAX) { > + if (!(bm->optval & BIT(opt_id))) > + continue; > + > + switch (opt_id) { > + case BROPT_UNICAST_FLOOD: > + bitmask |= BR_FLOOD; > + break; > + case BROPT_MCAST_FLOOD: > + bitmask |= BR_MCAST_FLOOD; > + break; > + case BROPT_BCAST_FLOOD: > + bitmask |= BR_BCAST_FLOOD; > + break; > + } > + } > + > + return bitmask; > +} > + > +static void br_set_bropt(struct net_bridge *br, struct nlattr *tb[], > + int attrtype, enum net_bridge_opts opt) > +{ > + if (!tb[attrtype]) > + return; > + > + br_opt_toggle(br, opt, !!nla_get_u8(tb[attrtype])); > +}These must be controlled via the boolopt api and attributes, not through additional nl attributes.> + > +#define BROPT_MASK (BROPT_UNICAST_FLOOD | BROPT_MCAST_FLOOD | BROPT_MCAST_FLOOD) > + > /* Process bridge protocol info on port */ > -static int br_setport(struct net_bridge_port *p, struct nlattr *tb[], > - struct netlink_ext_ack *extack) > +static int br_setport(struct net_bridge *br, struct net_bridge_port *p, > + struct nlattr *tb[], struct netlink_ext_ack *extack) > { > - unsigned long old_flags, changed_mask; > + unsigned long old_flags, new_flags, changed_mask; > + struct br_boolopt_multi old_opts = { > + .optmask = BROPT_MASK > + }; > bool br_vlan_tunnel_old; > + struct net_device *dev; > int err; > > - old_flags = p->flags; > - br_vlan_tunnel_old = (old_flags & BR_VLAN_TUNNEL) ? true : false; > - > - br_set_port_flag(p, tb, IFLA_BRPORT_MODE, BR_HAIRPIN_MODE); > - br_set_port_flag(p, tb, IFLA_BRPORT_GUARD, BR_BPDU_GUARD); > - br_set_port_flag(p, tb, IFLA_BRPORT_FAST_LEAVE, > - BR_MULTICAST_FAST_LEAVE); > - br_set_port_flag(p, tb, IFLA_BRPORT_PROTECT, BR_ROOT_BLOCK); > - br_set_port_flag(p, tb, IFLA_BRPORT_LEARNING, BR_LEARNING); > - br_set_port_flag(p, tb, IFLA_BRPORT_UNICAST_FLOOD, BR_FLOOD); > - br_set_port_flag(p, tb, IFLA_BRPORT_MCAST_FLOOD, BR_MCAST_FLOOD); > - br_set_port_flag(p, tb, IFLA_BRPORT_MCAST_TO_UCAST, > - BR_MULTICAST_TO_UNICAST); > - br_set_port_flag(p, tb, IFLA_BRPORT_BCAST_FLOOD, BR_BCAST_FLOOD); > - br_set_port_flag(p, tb, IFLA_BRPORT_PROXYARP, BR_PROXYARP); > - br_set_port_flag(p, tb, IFLA_BRPORT_PROXYARP_WIFI, BR_PROXYARP_WIFI); > - br_set_port_flag(p, tb, IFLA_BRPORT_VLAN_TUNNEL, BR_VLAN_TUNNEL); > - br_set_port_flag(p, tb, IFLA_BRPORT_NEIGH_SUPPRESS, BR_NEIGH_SUPPRESS); > - br_set_port_flag(p, tb, IFLA_BRPORT_ISOLATED, BR_ISOLATED); > - br_set_port_flag(p, tb, IFLA_BRPORT_LOCKED, BR_PORT_LOCKED); > - > - changed_mask = old_flags ^ p->flags; > - > - err = br_switchdev_set_dev_flag(p->dev, p->flags, changed_mask, extack); > + if (p) { > + old_flags = p->flags; > + br_vlan_tunnel_old = (old_flags & BR_VLAN_TUNNEL) ? true : false; > + > + br_set_port_flag(p, tb, IFLA_BRPORT_MODE, BR_HAIRPIN_MODE); > + br_set_port_flag(p, tb, IFLA_BRPORT_GUARD, BR_BPDU_GUARD); > + br_set_port_flag(p, tb, IFLA_BRPORT_FAST_LEAVE, > + BR_MULTICAST_FAST_LEAVE); > + br_set_port_flag(p, tb, IFLA_BRPORT_PROTECT, BR_ROOT_BLOCK); > + br_set_port_flag(p, tb, IFLA_BRPORT_LEARNING, BR_LEARNING); > + br_set_port_flag(p, tb, IFLA_BRPORT_UNICAST_FLOOD, BR_FLOOD); > + br_set_port_flag(p, tb, IFLA_BRPORT_MCAST_FLOOD, BR_MCAST_FLOOD); > + br_set_port_flag(p, tb, IFLA_BRPORT_MCAST_TO_UCAST, > + BR_MULTICAST_TO_UNICAST); > + br_set_port_flag(p, tb, IFLA_BRPORT_BCAST_FLOOD, BR_BCAST_FLOOD); > + br_set_port_flag(p, tb, IFLA_BRPORT_PROXYARP, BR_PROXYARP); > + br_set_port_flag(p, tb, IFLA_BRPORT_PROXYARP_WIFI, BR_PROXYARP_WIFI); > + br_set_port_flag(p, tb, IFLA_BRPORT_VLAN_TUNNEL, BR_VLAN_TUNNEL); > + br_set_port_flag(p, tb, IFLA_BRPORT_NEIGH_SUPPRESS, BR_NEIGH_SUPPRESS); > + br_set_port_flag(p, tb, IFLA_BRPORT_ISOLATED, BR_ISOLATED); > + br_set_port_flag(p, tb, IFLA_BRPORT_LOCKED, BR_PORT_LOCKED); > + > + new_flags = p->flags; > + dev = p->dev; > + } else { > + struct br_boolopt_multi opts = { > + .optmask = BROPT_MASK > + }; > + > + br_boolopt_multi_get(br, &old_opts); > + old_flags = br_boolopt_map_flags(&old_opts); > + > + br_set_bropt(br, tb, IFLA_BRPORT_UNICAST_FLOOD, BROPT_UNICAST_FLOOD); > + br_set_bropt(br, tb, IFLA_BRPORT_MCAST_FLOOD, BROPT_MCAST_FLOOD); > + br_set_bropt(br, tb, IFLA_BRPORT_BCAST_FLOOD, BROPT_BCAST_FLOOD); > + > + br_boolopt_multi_get(br, &opts); > + new_flags = br_boolopt_map_flags(&opts); > + dev = br->dev; > + } > + > + changed_mask = old_flags ^ new_flags; > + > + err = br_switchdev_set_dev_flag(dev, new_flags, changed_mask, extack); > if (err) { > - p->flags = old_flags; > + if (!p) > + br_boolopt_multi_toggle(br, &old_opts, extack); > + else > + p->flags = old_flags; > + > return err; > } > > + /* Skip the rest for the bridge itself, for now */ > + if (!p) > + return 0; > + > if (br_vlan_tunnel_old && !(p->flags & BR_VLAN_TUNNEL)) > nbp_vlan_tunnel_info_flush(p); > > @@ -1048,7 +1134,7 @@ int br_setlink(struct net_device *dev, struct nlmsghdr *nlh, u16 flags, > if (!p && !afspec) > return -EINVAL; > > - if (p && protinfo) { > + if (protinfo) { > if (protinfo->nla_type & NLA_F_NESTED) { > err = nla_parse_nested_deprecated(tb, IFLA_BRPORT_MAX, > protinfo, > @@ -1058,9 +1144,9 @@ int br_setlink(struct net_device *dev, struct nlmsghdr *nlh, u16 flags, > return err; > > spin_lock_bh(&br->lock); > - err = br_setport(p, tb, extack); > + err = br_setport(br, p, tb, extack);setport is for *port* only...> spin_unlock_bh(&br->lock); > - } else { > + } else if (p) { > /* Binary compatibility with old RSTP */ > if (nla_len(protinfo) < sizeof(u8)) > return -EINVAL; > @@ -1153,7 +1239,7 @@ static int br_port_slave_changelink(struct net_device *brdev, > return 0; > > spin_lock_bh(&br->lock); > - ret = br_setport(br_port_get_rtnl(dev), data, extack); > + ret = br_setport(br, br_port_get_rtnl(dev), data, extack); > spin_unlock_bh(&br->lock); > > return ret; > @@ -1163,7 +1249,7 @@ static int br_port_fill_slave_info(struct sk_buff *skb, > const struct net_device *brdev, > const struct net_device *dev) > { > - return br_port_fill_attrs(skb, br_port_get_rtnl(dev)); > + return br_port_fill_attrs(skb, NULL, br_port_get_rtnl(dev)); > } > > static size_t br_port_get_slave_size(const struct net_device *brdev,