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]

Message-ID: <20180418130718.GA16044@troglobit>
Date:   Wed, 18 Apr 2018 15:07:19 +0200
From:   Joachim Nilsson <troglobit@...il.com>
To:     Nikolay Aleksandrov <nikolay@...ulusnetworks.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 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 ...

> > - 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.

> 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