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]

Message-ID: <898155b2-08d7-1977-d0c9-bbb1ae99c0bb@nvidia.com>
Date:   Thu, 9 Dec 2021 18:57:23 +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 18:23, Nikolay Aleksandrov wrote:
> On 09/12/2021 18:08, Nikolay Aleksandrov wrote:
>> 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
> 
> Actually they'll have to be exported together always... that's unfortunate. I get
> why you need the extra netlink logic, I think we'll have to keep it.
So one relatively simple way to drop most of the logic is to have BRPORT_HORIZON_GROUP's
attribute get set after ISOLATED so it always overwrites it, which is similar to the current
situation but if you set only ISOLATED later it will function as intended (i.e. drop the logic
for horizon_group when using the old attr). Still will have to disallow ISOLATED = 0/HORIZON_GROUP >0
and ISOLATED=1/HORIZON_GROUP=0 though as these don't make sense.

e.g.:
if (tb[IFLA_BRPORT_ISOLATED])
	p->horizon_group = !!nla_get_u8(tb[IFLA_BRPORT_ISOLATED]);
if (tb[IFLA_BRPORT_HORIZON_GROUP])
	p->horizon_group = nla_get_u32(tb[IFLA_BRPORT_HORIZON_GROUP]);



This will be also in line with how they're exported (ISOLATED = 1 and HORIZON_GROUP >0).