netdev - Re: [PATCH RFC net-next 04/13] net: bridge: netlink support for controlling BUM flooding to bridge

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <87h76x9u5i.fsf@gmail.com>
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