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

2018-04-18 Thread Nikolay Aleksandrov 
On April 18, 2018 6:54:07 PM GMT+03:00, Stephen Hemminger 
 wrote:
>On Wed, 18 Apr 2018 16:14:26 +0300
>Nikolay Aleksandrov  wrote:
>
>> 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.
>
>In general, all bridge attributes need to show in netlink and sysfs.
>Sysfs is easier for scripting from languages.

True, but vlans and per-vlan settings have never been exposed via sysfs, only 
through netlink.
I'd like to avoid adding a directory with potentially 4k multiplied by the attr 
number for each vlan entries.

There is already vlan config infrastructure via netlink.

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

2018-04-18 Thread Stephen Hemminger 
On Wed, 18 Apr 2018 16:14:26 +0300
Nikolay Aleksandrov  wrote:

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

In general, all bridge attributes need to show in netlink and sysfs.
Sysfs is easier for scripting from languages.

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

2018-04-18 Thread Joachim Nilsson 
On Wed, Apr 18, 2018 at 04:14:26PM +0300, Nikolay Aleksandrov wrote:
> 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.

Aha, didn't know that. Thanks! :)

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

Yup, I did some ugly sysfs patches to read queriers per VLAN like that, just
for some basic feedback.  Really awful, although easy to debug because of it
being a simple file ... (I guess I'll have to make friends withe Netlink.)

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

Interesting, this I'll have to look at in more detail!

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

2018-04-18 Thread Nikolay Aleksandrov 
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
>

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

2018-04-18 Thread Joachim Nilsson 
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

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

2018-04-18 Thread Nikolay Aleksandrov 

On 18/04/18 15:07, Joachim Nilsson wrote:

This RFC patch¹ is an attempt to add multicast querier per VLAN support
to a VLAN aware bridge.  I'm posting it as RFC for now since non-VLAN
aware bridges are not handled, and one of my questions is if that is
complexity we need to continue supporting?

 From what I understand, multicast join/report already support per VLAN
operation, and the MDB as well support filtering per VLAN, but queries
are currently limited to per-port operation on VLAN-aware bridges.

The naive² approach of this patch relocates query timers from the bridge
to operate per VLAN, on timer expiry we send queries to all bridge ports
in the same VLAN.  Tagged port members have tagged VLAN queries.

Unlike the original patch¹, which uses a sysfs entry to set the querier
address of each VLAN, this use the IP address of the VLAN interface when
initiating a per VLAN query.  A version of inet_select_addr() is used
for this, called inet_select_dev_addr(), not included in this patch.

Open questions/TODO:

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


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


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


- 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 ?
Take a look at br_multicast_set_port_router().


- The MLD support has been kept in sync with the rest but is completely
   untested.  In particular I suspect the wrong source IP will be used.

¹) Initially based on a patch by Cumulus Networks

http://repo3.cumulusnetworks.com/repo/pool/cumulus/l/linux/linux-source-4.1_4.1.33-1+cl3u11_all.deb


I knew this looked familiar when I glanced through it :)


²) This patch is currently limited to work only on bridges with VLAN
enabled.  Care has been taken to support MLD snooping, but it is
completely untested.

Thank you for reading this far!

Signed-off-by: Joachim Nilsson 


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

I will review this patch in detail later and come back if there's anything.

Cheers,
 Nik


---
  net/bridge/br_device.c|   2 +-
  net/bridge/br_input.c |   2 +-
  net/bridge/br_multicast.c | 456 --
  net/bridge/br_private.h   |  38 +++-
  net/bridge/br_stp.c   |   5 +-
  net/bridge/br_vlan.c  |   3 +
  6 files changed, 327 insertions(+), 179 deletions(-)

diff --git a/net/bridge/br_device.c b/net/bridge/br_device.c
index 02f9f8aab047..ba35485032d8 100644
--- a/net/bridge/br_device.c
+++ b/net/bridge/br_device.c
@@ -98,7 +98,7 @@ netdev_tx_t br_dev_xmit(struct sk_buff *skb, struct 
net_device *dev)
  
  		mdst = br_mdb_get(br, skb, vid);

if ((mdst || BR_INPUT_SKB_CB_MROUTERS_ONLY(skb)) &&
-   br_multicast_querier_exists(br, eth_hdr(skb)))
+   br_multicast_querier_exists(br, vid, eth_hdr(skb)))
br_multicast_flood(mdst, skb, false, true);
else
br_flood(br, skb, BR_PKT_MULTICAST, false, true);
diff --git a/net/bridge/br_input.c b/net/bridge/br_input.c
index 56bb9189c374..13d48489e0e1 100644
--- a/net/bridge/br_input.c
+++ b/net/bridge/br_input.c
@@ -137,7 +137,7 @@ int br_handle_frame_finish(struct net *net, struct sock 
*sk, struct sk_buff *skb
mdst = br_mdb_get(br, skb, vid);
if ((mdst && mdst->addr.proto == htons(ETH_P_ALL)) ||
((mdst || BR_INPUT_SKB_CB_MROUTERS_ONLY(skb)) &&
-br_multicast_querier_exists(br, eth_hdr(skb {
+br_multicast_querier_exists(br, vid, eth_hdr(skb {
if ((mdst && mdst->host_joined) ||
br_multicast_is_router(br)) {
local_rcv = true;
diff --git a/net/bridge/br_multicast.c

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

2018-04-18 Thread Joachim Nilsson 
This RFC patch¹ is an attempt to add multicast querier per VLAN support
to a VLAN aware bridge.  I'm posting it as RFC for now since non-VLAN
aware bridges are not handled, and one of my questions is if that is
complexity we need to continue supporting?

>From what I understand, multicast join/report already support per VLAN
operation, and the MDB as well support filtering per VLAN, but queries
are currently limited to per-port operation on VLAN-aware bridges.

The naive² approach of this patch relocates query timers from the bridge
to operate per VLAN, on timer expiry we send queries to all bridge ports
in the same VLAN.  Tagged port members have tagged VLAN queries.

Unlike the original patch¹, which uses a sysfs entry to set the querier
address of each VLAN, this use the IP address of the VLAN interface when
initiating a per VLAN query.  A version of inet_select_addr() is used
for this, called inet_select_dev_addr(), not included in this patch.

Open questions/TODO:

- First of all, is this patch useful to anyone?
- 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.
- 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)
- Dito per-port specific multicast sysfs settings, e.g. multicast_router
- The MLD support has been kept in sync with the rest but is completely
  untested.  In particular I suspect the wrong source IP will be used.

¹) Initially based on a patch by Cumulus Networks
   
http://repo3.cumulusnetworks.com/repo/pool/cumulus/l/linux/linux-source-4.1_4.1.33-1+cl3u11_all.deb
²) This patch is currently limited to work only on bridges with VLAN
   enabled.  Care has been taken to support MLD snooping, but it is
   completely untested.

Thank you for reading this far!

Signed-off-by: Joachim Nilsson 
---
 net/bridge/br_device.c|   2 +-
 net/bridge/br_input.c |   2 +-
 net/bridge/br_multicast.c | 456 --
 net/bridge/br_private.h   |  38 +++-
 net/bridge/br_stp.c   |   5 +-
 net/bridge/br_vlan.c  |   3 +
 6 files changed, 327 insertions(+), 179 deletions(-)

diff --git a/net/bridge/br_device.c b/net/bridge/br_device.c
index 02f9f8aab047..ba35485032d8 100644
--- a/net/bridge/br_device.c
+++ b/net/bridge/br_device.c
@@ -98,7 +98,7 @@ netdev_tx_t br_dev_xmit(struct sk_buff *skb, struct 
net_device *dev)
 
mdst = br_mdb_get(br, skb, vid);
if ((mdst || BR_INPUT_SKB_CB_MROUTERS_ONLY(skb)) &&
-   br_multicast_querier_exists(br, eth_hdr(skb)))
+   br_multicast_querier_exists(br, vid, eth_hdr(skb)))
br_multicast_flood(mdst, skb, false, true);
else
br_flood(br, skb, BR_PKT_MULTICAST, false, true);
diff --git a/net/bridge/br_input.c b/net/bridge/br_input.c
index 56bb9189c374..13d48489e0e1 100644
--- a/net/bridge/br_input.c
+++ b/net/bridge/br_input.c
@@ -137,7 +137,7 @@ int br_handle_frame_finish(struct net *net, struct sock 
*sk, struct sk_buff *skb
mdst = br_mdb_get(br, skb, vid);
if ((mdst && mdst->addr.proto == htons(ETH_P_ALL)) ||
((mdst || BR_INPUT_SKB_CB_MROUTERS_ONLY(skb)) &&
-br_multicast_querier_exists(br, eth_hdr(skb {
+br_multicast_querier_exists(br, vid, eth_hdr(skb {
if ((mdst && mdst->host_joined) ||
br_multicast_is_router(br)) {
local_rcv = true;
diff --git a/net/bridge/br_multicast.c b/net/bridge/br_multicast.c
index 277ecd077dc4..72e47d500972 100644
--- a/net/bridge/br_multicast.c
+++ b/net/bridge/br_multicast.c
@@ -13,6 +13,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -37,7 +38,7 @@
 
 #include "br_private.h"
 
-static void br_multicast_start_querier(struct net_bridge *br,
+static void br_multicast_start_querier(struct net_bridge_vlan *vlan,
   struct bridge_mcast_own_query *query);
 static void br_multicast_add_router(struct net_bridge *br,
struct net_bridge_port *port);
@@ -46,13 +47,14 @@ static void br_ip4_multicast_leave_group(struct net_bridge 
*br,
 __be32 group,
 __u16 vid,
 const unsigned char *src);
-
+static void br_ip4_multicast_query_expired(struct timer_list *t);
 static void __del_port_router(struct net_bridge_port *p);
 #if IS_ENABLED(CONFIG_IPV6)