netdev - Re: [PATCH] bridge: extend BR_ISOLATE to full split-horizon

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]

Date:   Thu, 9 Dec 2021 18:08:03 +0200
From:   Nikolay Aleksandrov <nikolay@...dia.com>
To:     Jakub Kicinski <kuba@...nel.org>,
        David Lamparter <equinox@...c24.net>
CC:     <netdev@...r.kernel.org>, Alexandra Winter <wintera@...ux.ibm.com>
Subject: Re: [PATCH] bridge: extend BR_ISOLATE to full split-horizon

On 09/12/2021 17:42, Jakub Kicinski wrote:
> On Thu,  9 Dec 2021 13:14:32 +0100 David Lamparter wrote:
>> Split-horizon essentially just means being able to create multiple
>> groups of isolated ports that are isolated within the group, but not
>> with respect to each other.
>>
>> The intent is very different, while isolation is a policy feature,
>> split-horizon is intended to provide functional "multiple member ports
>> are treated as one for loop avoidance."  But it boils down to the same
>> thing in the end.
>>
>> Signed-off-by: David Lamparter <equinox@...c24.net>
>> Cc: Nikolay Aleksandrov <nikolay@...dia.com>
>> Cc: Alexandra Winter <wintera@...ux.ibm.com>
> 
> Does not apply to net-next, you'll need to repost even if the code is
> good. Please put [PATCH net-next] in the subject.
> 

Hi,
For some reason this patch didn't make it to my inbox.. Anyway I was
able to see it now online, a few comments (sorry can't do them inline due
to missing mbox patch):
- please drop the sysfs part, we're not extending sysfs anymore
- split the bridge change from the driver
- drop the /* BR_ISOLATED - previously BIT(16) */ comment
- [IFLA_BRPORT_HORIZON_GROUP] = NLA_POLICY_MIN(NLA_S32, 0), why not just { .type = NLA_U32 } ?
- just forbid having both set (tb[IFLA_BRPORT_ISOLATED] && tb[IFLA_BRPORT_HORIZON_GROUP])
  user-space should use just one of the two, if isolated is set then it overwrites any older
  IFLA_BRPORT_HORIZON_GROUP settings, that should simplify things considerably

Why the limitation (UAPI limited to positive signed int. (recommended ifindex namespace)) ?
You have the full unsigned space available, user-space can use it as it sees fit.
You can just remove the comment about recommended ifindex.

Also please extend the port isolation self-test with a test for a different horizon group.

Thanks,
 Nik