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: Wed, 13 Apr 2022 12:04:09 +0200 From: Joachim Wiberg <troglobit@...il.com> To: Nikolay Aleksandrov <razor@...ckwall.org>, Roopa Prabhu <roopa@...dia.com> Cc: netdev@...r.kernel.org, bridge@...ts.linux-foundation.org, "David S . Miller" <davem@...emloft.net>, Jakub Kicinski <kuba@...nel.org>, Tobias Waldekranz <tobias@...dekranz.com>, Vladimir Oltean <vladimir.oltean@....com> Subject: Re: [PATCH RFC net-next 04/13] net: bridge: netlink support for controlling BUM flooding to bridge On Tue, Apr 12, 2022 at 21:24, Nikolay Aleksandrov <razor@...ckwall.org> wrote: > On 11/04/2022 16:38, Joachim Wiberg wrote: >> 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. > 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, OK, I'll have a go at it. > 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. Ah yes, will read up on those, thanks! >> + 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. Aha, there it is, awesome! >> +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. Understood. >> @@ -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... A-firm. Thank you for the honest review. Netlink is still much of a mystery in many ways to me. Best regards /Joachim
Powered by blists - more mailing lists