netdev - Re: [RFC PATCH] net: bridge: multicast querier per VLAN support

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:   Wed, 18 Apr 2018 16:14:26 +0300
From:   Nikolay Aleksandrov <nikolay@...ulusnetworks.com>
To:     Joachim Nilsson <troglobit@...il.com>
Cc:     netdev@...r.kernel.org,
        Stephen Hemminger <stephen@...workplumber.org>,
        roopa <roopa@...ulusnetworks.com>
Subject: Re: [RFC PATCH] net: bridge: multicast querier per VLAN support

On 18/04/18 16:07, Joachim Nilsson wrote:
> On Wed, Apr 18, 2018 at 03:31:57PM +0300, Nikolay Aleksandrov wrote:
>> On 18/04/18 15:07, Joachim Nilsson wrote:
>>> - First of all, is this patch useful to anyone
>> Obviously to us as it's based on our patch. :-)
>> We actually recently discussed what will be needed to make it acceptable to upstream.
> 
> Great! :)
> 
>>> - The current br_multicast.c is very complex.  The support for both IPv4
>>>    and IPv6 is a no-brainer, but it also has #ifdef VLAN_FILTERING and
>>>    'br->vlan_enabled' ... this has likely been discussed before, but if
>>>    we could remove those code paths I believe what's left would be quite
>>>    a bit easier to read and maintain.
>> br->vlan_enabled has a wrapper that can be used without ifdefs, as does br_vlan_find()
>> so in short - you can remove the ifdefs and use the wrappers,  they'll degrade to always
>> false/null when vlans are disabled.
> 
> Thanks, I'll have a look at that and prepare an RFC v2!
> 
>>> - Many per-bridge specific multicast sysfs settings may need to have a
>>>    corresponding per-VLAN setting, e.g. snooping, query_interval, etc.
>>>    How should we go about that? (For status reporting I have a proposal)
>> We'll have to add more to the per-vlan context, but yes it has to happen.
>> It will be only netlink interface for config/retrieval, no sysfs.
> 
> Some settings are possible to do with sysfs, like multicast_query_interval
> and ...

We want to avoid sysfs in general, all of networking config and stats
are moving to netlink. It is better controlled and structured for such
changes, also provides nice interfaces for automatic  type checks etc.

Also (but a minor reason) there is no tree/entity in sysfs for the vlans
where to add this. It will either have to be a file which does some
format string hack (like us currently) or will need to add new tree for
them which I'd really like to avoid for the bridge.

> 
>>> - Dito per-port specific multicast sysfs settings, e.g. multicast_router
>> I'm not sure I follow this one, there is per-port mcast router config now ?
> 
> Sorry no, I meant we may want to add more per-VLAN settings when we get
> this base patch merged.  Like router ports, we may want to be able to
> set them per VLAN.

Sure, that can be done easily via netlink. br_afspec() can decode any
additional per-vlan attributes and can be fairly easily extended.
Also after my vlan rhastable change, we have per-vlan context even today
(e.g. per-vlan stats use it) so we'll just extend that.

> 
>> Thanks for the effort, I see that you have done some of the required cleanups
>> for this to be upstreamable, but as you've noted above we need to make it
>> complete (with the per-vlan contexts and all).
> 
> There's definitely more work to be done.  Agreeing on a base set of changes
> to start with is maybe the most important, as well as making it complete.>
>> I will review this patch in detail later and come back if there's anything.
> 
> Thank you so much for the quick feedback so far! :)
> 
> Cheers
>  /Joachim
>