Re: [Bridge] [PATCH net-next v5 2/2] bridge: vlan: signal if anything changed on vlan add

[Toshiaki Makita]

On 2017/10/26 22:41, Nikolay Aleksandrov wrote:
> Before this patch there was no way to tell if the vlan add operation
> actually changed anything, thus we would always generate a notification
> on adds. Let's make the notifications more precise and generate them
> only if anything changed, so use the new bool parameter to signal that the
> vlan was updated. We cannot return an error because there are valid use
> cases that will be broken (e.g. overlapping range add) and also we can't
> risk masking errors due to calls into drivers for vlan add which can
> potentially return anything.
> 
> Signed-off-by: Nikolay Aleksandrov 
> ---
> v5: fix br_vlan_add return (v1 leftover) spotted by Toshiaki Makita
> v4: set changed always to false in the non-vlan config case
> v3: fix non-vlan config functions reported by kbuild bot
> v2: pass changed down to vlan add functions instead of using a specific
> error that needs to be masked
> 
>  net/bridge/br_netlink.c |  9 --
>  net/bridge/br_private.h | 14 ++---
>  net/bridge/br_vlan.c| 76 
> +++--
>  3 files changed, 71 insertions(+), 28 deletions(-)
> 
> diff --git a/net/bridge/br_netlink.c b/net/bridge/br_netlink.c
> index d0290ede9342..e732403669c6 100644
> --- a/net/bridge/br_netlink.c
> +++ b/net/bridge/br_netlink.c
> @@ -508,6 +508,7 @@ int br_getlink(struct sk_buff *skb, u32 pid, u32 seq,
>  static int br_vlan_info(struct net_bridge *br, struct net_bridge_port *p,
>   int cmd, struct bridge_vlan_info *vinfo, bool *changed)
>  {
> + bool curr_change;
>   int err = 0;

Just a question.
Why are you defining another variable here?
Is it impossible to pass "changed" down to [br|nbp]_vlan_add() like
other functions you modified in patch 1/2?

-- 
Toshiaki Makita

Re: [Bridge] Linux Bridge Static FDB move

[Stephen Hemminger]

If you have a moving MAC then you have a network that is flapping,
duplicate MAC, or worse a network loop. All of these are signs of a broken
L2 network. The bridge can't fix these

On Oct 26, 2017 19:33,  wrote:

> Thank you for the reply.
>
> Viraj
> 
> From: Toshiaki Makita 
> Sent: Thursday, October 26, 2017 4:13 AM
> To: Raiyani, Viraj; bridge@lists.linux-foundation.org
> Subject: Re: [Bridge] Linux Bridge Static FDB move
>
> On 2017/10/26 1:47, viraj.raiy...@dell.com wrote:
> > Hi Everyone,
> >
> >
> > Is it possible to program the FDB entry in Linux Bridge which is static,
> > non-local and doesn't move to new interface when the same source MAC
> > packet comes on a different interface in the same bridge  ?
>
> AFAIK no.
> Bridge supports static fdb entries, but br_fdb_update() updates their
> dst even for static entries.
>
> >
> >
> > I tried programming the MAC as permanent that prevents the moving of MAC
> > to a new interface in the same bridge, however it treats the MAC as
> > local and doesn't do the forwarding ?
>
> Yes, local entries will deliver frames to the bridge device itself.
>
> --
> Toshiaki Makita
>
>

Re: [Bridge] Linux Bridge Static FDB move

[Viraj.Raiyani]

Thank you for the reply.

Viraj 

From: Toshiaki Makita 
Sent: Thursday, October 26, 2017 4:13 AM
To: Raiyani, Viraj; bridge@lists.linux-foundation.org
Subject: Re: [Bridge] Linux Bridge Static FDB move

On 2017/10/26 1:47, viraj.raiy...@dell.com wrote:
> Hi Everyone,
>
>
> Is it possible to program the FDB entry in Linux Bridge which is static,
> non-local and doesn't move to new interface when the same source MAC
> packet comes on a different interface in the same bridge  ?

AFAIK no.
Bridge supports static fdb entries, but br_fdb_update() updates their
dst even for static entries.

>
>
> I tried programming the MAC as permanent that prevents the moving of MAC
> to a new interface in the same bridge, however it treats the MAC as
> local and doesn't do the forwarding ?

Yes, local entries will deliver frames to the bridge device itself.

--
Toshiaki Makita

[Bridge] [PATCH net-next v5 2/2] bridge: vlan: signal if anything changed on vlan add

[Nikolay Aleksandrov]

Before this patch there was no way to tell if the vlan add operation
actually changed anything, thus we would always generate a notification
on adds. Let's make the notifications more precise and generate them
only if anything changed, so use the new bool parameter to signal that the
vlan was updated. We cannot return an error because there are valid use
cases that will be broken (e.g. overlapping range add) and also we can't
risk masking errors due to calls into drivers for vlan add which can
potentially return anything.

Signed-off-by: Nikolay Aleksandrov 
---
v5: fix br_vlan_add return (v1 leftover) spotted by Toshiaki Makita
v4: set changed always to false in the non-vlan config case
v3: fix non-vlan config functions reported by kbuild bot
v2: pass changed down to vlan add functions instead of using a specific
error that needs to be masked

 net/bridge/br_netlink.c |  9 --
 net/bridge/br_private.h | 14 ++---
 net/bridge/br_vlan.c| 76 +++--
 3 files changed, 71 insertions(+), 28 deletions(-)

diff --git a/net/bridge/br_netlink.c b/net/bridge/br_netlink.c
index d0290ede9342..e732403669c6 100644
--- a/net/bridge/br_netlink.c
+++ b/net/bridge/br_netlink.c
@@ -508,6 +508,7 @@ int br_getlink(struct sk_buff *skb, u32 pid, u32 seq,
 static int br_vlan_info(struct net_bridge *br, struct net_bridge_port *p,
int cmd, struct bridge_vlan_info *vinfo, bool *changed)
 {
+   bool curr_change;
int err = 0;
 
switch (cmd) {
@@ -516,12 +517,14 @@ static int br_vlan_info(struct net_bridge *br, struct 
net_bridge_port *p,
/* if the MASTER flag is set this will act on the global
 * per-VLAN entry as well
 */
-   err = nbp_vlan_add(p, vinfo->vid, vinfo->flags);
+   err = nbp_vlan_add(p, vinfo->vid, vinfo->flags,
+  _change);
} else {
vinfo->flags |= BRIDGE_VLAN_INFO_BRENTRY;
-   err = br_vlan_add(br, vinfo->vid, vinfo->flags);
+   err = br_vlan_add(br, vinfo->vid, vinfo->flags,
+ _change);
}
-   if (!err)
+   if (curr_change)
*changed = true;
break;
 
diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
index fa0039f44818..860e4afaf71a 100644
--- a/net/bridge/br_private.h
+++ b/net/bridge/br_private.h
@@ -803,7 +803,8 @@ struct sk_buff *br_handle_vlan(struct net_bridge *br,
   const struct net_bridge_port *port,
   struct net_bridge_vlan_group *vg,
   struct sk_buff *skb);
-int br_vlan_add(struct net_bridge *br, u16 vid, u16 flags);
+int br_vlan_add(struct net_bridge *br, u16 vid, u16 flags,
+   bool *changed);
 int br_vlan_delete(struct net_bridge *br, u16 vid);
 void br_vlan_flush(struct net_bridge *br);
 struct net_bridge_vlan *br_vlan_find(struct net_bridge_vlan_group *vg, u16 
vid);
@@ -816,7 +817,8 @@ int br_vlan_set_stats(struct net_bridge *br, unsigned long 
val);
 int br_vlan_init(struct net_bridge *br);
 int br_vlan_set_default_pvid(struct net_bridge *br, unsigned long val);
 int __br_vlan_set_default_pvid(struct net_bridge *br, u16 pvid);
-int nbp_vlan_add(struct net_bridge_port *port, u16 vid, u16 flags);
+int nbp_vlan_add(struct net_bridge_port *port, u16 vid, u16 flags,
+bool *changed);
 int nbp_vlan_delete(struct net_bridge_port *port, u16 vid);
 void nbp_vlan_flush(struct net_bridge_port *port);
 int nbp_vlan_init(struct net_bridge_port *port);
@@ -903,8 +905,10 @@ static inline struct sk_buff *br_handle_vlan(struct 
net_bridge *br,
return skb;
 }
 
-static inline int br_vlan_add(struct net_bridge *br, u16 vid, u16 flags)
+static inline int br_vlan_add(struct net_bridge *br, u16 vid, u16 flags,
+ bool *changed)
 {
+   *changed = false;
return -EOPNOTSUPP;
 }
 
@@ -926,8 +930,10 @@ static inline int br_vlan_init(struct net_bridge *br)
return 0;
 }
 
-static inline int nbp_vlan_add(struct net_bridge_port *port, u16 vid, u16 
flags)
+static inline int nbp_vlan_add(struct net_bridge_port *port, u16 vid, u16 
flags,
+  bool *changed)
 {
+   *changed = false;
return -EOPNOTSUPP;
 }
 
diff --git a/net/bridge/br_vlan.c b/net/bridge/br_vlan.c
index 233a30040c91..c79a5ed794e5 100644
--- a/net/bridge/br_vlan.c
+++ b/net/bridge/br_vlan.c
@@ -32,27 +32,34 @@ static struct net_bridge_vlan *br_vlan_lookup(struct 
rhashtable *tbl, u16 vid)
return rhashtable_lookup_fast(tbl, , br_vlan_rht_params);
 }
 
-static void __vlan_add_pvid(struct net_bridge_vlan_group *vg, u16 vid)
+static bool __vlan_add_pvid(struct net_bridge_vlan_group 

[Bridge] [PATCH net-next v5 1/2] bridge: netlink: make setlink/dellink notifications more accurate

[Nikolay Aleksandrov]

Before this patch we had cases that either sent notifications when there
were in fact no changes (e.g. non-existent vlan delete) or didn't send
notifications when there were changes (e.g. vlan add range with an error in
the middle, port flags change + vlan update error). This patch sends down
a boolean to the functions setlink/dellink use and if there is even a
single configuration change (port flag, vlan add/del, port state) then
we always send a notification. This is all done to keep backwards
compatibility with the opportunistic vlan delete, where one could
specify a vlan range that has missing vlans inside and still everything
in that range will be cleared, this is mostly used to clear the whole
vlan config with a single call, i.e. range 1-4094.

Signed-off-by: Nikolay Aleksandrov 
Acked-by: Stephen Hemminger 
---
v5: no changes, added Stephen's ack

 net/bridge/br_netlink.c| 44 +-
 net/bridge/br_netlink_tunnel.c | 14 +-
 net/bridge/br_private_tunnel.h |  3 ++-
 3 files changed, 37 insertions(+), 24 deletions(-)

diff --git a/net/bridge/br_netlink.c b/net/bridge/br_netlink.c
index fb61b6c79235..d0290ede9342 100644
--- a/net/bridge/br_netlink.c
+++ b/net/bridge/br_netlink.c
@@ -506,7 +506,7 @@ int br_getlink(struct sk_buff *skb, u32 pid, u32 seq,
 }
 
 static int br_vlan_info(struct net_bridge *br, struct net_bridge_port *p,
-   int cmd, struct bridge_vlan_info *vinfo)
+   int cmd, struct bridge_vlan_info *vinfo, bool *changed)
 {
int err = 0;
 
@@ -517,21 +517,24 @@ static int br_vlan_info(struct net_bridge *br, struct 
net_bridge_port *p,
 * per-VLAN entry as well
 */
err = nbp_vlan_add(p, vinfo->vid, vinfo->flags);
-   if (err)
-   break;
} else {
vinfo->flags |= BRIDGE_VLAN_INFO_BRENTRY;
err = br_vlan_add(br, vinfo->vid, vinfo->flags);
}
+   if (!err)
+   *changed = true;
break;
 
case RTM_DELLINK:
if (p) {
-   nbp_vlan_delete(p, vinfo->vid);
-   if (vinfo->flags & BRIDGE_VLAN_INFO_MASTER)
-   br_vlan_delete(p->br, vinfo->vid);
-   } else {
-   br_vlan_delete(br, vinfo->vid);
+   if (!nbp_vlan_delete(p, vinfo->vid))
+   *changed = true;
+
+   if ((vinfo->flags & BRIDGE_VLAN_INFO_MASTER) &&
+   !br_vlan_delete(p->br, vinfo->vid))
+   *changed = true;
+   } else if (!br_vlan_delete(br, vinfo->vid)) {
+   *changed = true;
}
break;
}
@@ -542,7 +545,8 @@ static int br_vlan_info(struct net_bridge *br, struct 
net_bridge_port *p,
 static int br_process_vlan_info(struct net_bridge *br,
struct net_bridge_port *p, int cmd,
struct bridge_vlan_info *vinfo_curr,
-   struct bridge_vlan_info **vinfo_last)
+   struct bridge_vlan_info **vinfo_last,
+   bool *changed)
 {
if (!vinfo_curr->vid || vinfo_curr->vid >= VLAN_VID_MASK)
return -EINVAL;
@@ -572,7 +576,7 @@ static int br_process_vlan_info(struct net_bridge *br,
   sizeof(struct bridge_vlan_info));
for (v = (*vinfo_last)->vid; v <= vinfo_curr->vid; v++) {
tmp_vinfo.vid = v;
-   err = br_vlan_info(br, p, cmd, _vinfo);
+   err = br_vlan_info(br, p, cmd, _vinfo, changed);
if (err)
break;
}
@@ -581,13 +585,13 @@ static int br_process_vlan_info(struct net_bridge *br,
return err;
}
 
-   return br_vlan_info(br, p, cmd, vinfo_curr);
+   return br_vlan_info(br, p, cmd, vinfo_curr, changed);
 }
 
 static int br_afspec(struct net_bridge *br,
 struct net_bridge_port *p,
 struct nlattr *af_spec,
-int cmd)
+int cmd, bool *changed)
 {
struct bridge_vlan_info *vinfo_curr = NULL;
struct bridge_vlan_info *vinfo_last = NULL;
@@ -607,7 +611,8 @@ static int br_afspec(struct net_bridge *br,
return err;
err = br_process_vlan_tunnel_info(br, p, cmd,
  _curr,
- _last);
+ _last,
+ 

[Bridge] [PATCH net-next v5 0/2] bridge: make setlink/dellink notifications more accurate

[Nikolay Aleksandrov]

Hi,
Before this set the bridge would generate a notification on vlan add or del
even if they didn't actually do any changes, which confuses listeners and
is generally not preferred. We could also lose notifications on actual
changes if one adds a range of vlans and there's an error in the middle.
The problem with just breaking and returning an error is that we could
break existing user-space scripts which rely on the vlan delete to clear
all existing entries in the specified range and ignore the non-existing
errors (typically used to clear the current vlan config).
So in order to make the notifications more accurate while keeping backwards
compatibility we add a boolean that tracks if anything actually changed
during the config calls.

The vlan add is more difficult to fix because it always returns 0 even if
nothing changed, but we cannot use a specific error because the drivers
can return anything and we may mask it, also we'd need to update all places
that directly return the add result, thus to signal that a vlan was created
or updated and in order not to break overlapping vlan range add we pass
down the new boolean that tracks changes to the add functions to check
if anything was actually updated.

v5: fix br_vlan_add return (v1 leftover) spotted by Toshiaki Makita
v4: set changed always to false in the non-vlan config case and retested
v3: rebased to latest net-next and fixed non-vlan config functions reported
by kbuild test bot
v2: pass changed down to vlan add instead of masking errors

Thanks,
 Nik

Nikolay Aleksandrov (2):
  bridge: netlink: make setlink/dellink notifications more accurate
  bridge: vlan: signal if anything changed on vlan add

 net/bridge/br_netlink.c| 51 +---
 net/bridge/br_netlink_tunnel.c | 14 +---
 net/bridge/br_private.h| 14 +---
 net/bridge/br_private_tunnel.h |  3 +-
 net/bridge/br_vlan.c   | 76 ++
 5 files changed, 107 insertions(+), 51 deletions(-)

-- 
2.1.4

Re: [Bridge] Linux Bridge Static FDB move

[Toshiaki Makita]

On 2017/10/26 1:47, viraj.raiy...@dell.com wrote:
> Hi Everyone,
> 
> 
> Is it possible to program the FDB entry in Linux Bridge which is static,
> non-local and doesn't move to new interface when the same source MAC
> packet comes on a different interface in the same bridge  ?

AFAIK no.
Bridge supports static fdb entries, but br_fdb_update() updates their
dst even for static entries.

> 
> 
> I tried programming the MAC as permanent that prevents the moving of MAC
> to a new interface in the same bridge, however it treats the MAC as
> local and doesn't do the forwarding ?

Yes, local entries will deliver frames to the bridge device itself.

-- 
Toshiaki Makita

Re: [Bridge] [PATCH net-next v4 2/2] bridge: vlan: signal if anything changed on vlan add

[Nikolay Aleksandrov]

On 26/10/17 14:02, Nikolay Aleksandrov wrote:
> On 26/10/17 13:16, Toshiaki Makita wrote:
>> On 2017/10/26 7:52, Nikolay Aleksandrov wrote:
>> ...
>>> @@ -559,6 +574,7 @@ int br_vlan_add(struct net_bridge *br, u16 vid, u16 
>>> flags)
>>>  
>>> ASSERT_RTNL();
>>>  
>>> +   *changed = false;
>>> vg = br_vlan_group(br);
>>> vlan = br_vlan_find(vg, vid);
>>> if (vlan) {
>>> @@ -576,9 +592,12 @@ int br_vlan_add(struct net_bridge *br, u16 vid, u16 
>>> flags)
>>> refcount_inc(>refcnt);
>>> vlan->flags |= BRIDGE_VLAN_INFO_BRENTRY;
>>> vg->num_vlans++;
>>> +   *changed = true;
>>> }
>>> -   __vlan_add_flags(vlan, flags);
>>> -   return 0;
>>> +   if (__vlan_add_flags(vlan, flags))
>>> +   *changed = true;
>>> +
>>> +   return ret;
>>
>> "ret" isn't always initialized here, is it?
>>
>>
>> Toshiaki Makita
> 
> Oh, good catch! Right you are, weird that there was no warning even with W=1 
> as
> I always check that before sending a set.
> 
> Thanks,
>  Nik
> 

Unfortunately that was a leftover from v0 of this set where I always 
initialized ret.
Will fix and send v5, thanks again.

Re: [Bridge] [PATCH net-next v4 2/2] bridge: vlan: signal if anything changed on vlan add

[Nikolay Aleksandrov]

On 26/10/17 13:16, Toshiaki Makita wrote:
> On 2017/10/26 7:52, Nikolay Aleksandrov wrote:
> ...
>> @@ -559,6 +574,7 @@ int br_vlan_add(struct net_bridge *br, u16 vid, u16 
>> flags)
>>  
>>  ASSERT_RTNL();
>>  
>> +*changed = false;
>>  vg = br_vlan_group(br);
>>  vlan = br_vlan_find(vg, vid);
>>  if (vlan) {
>> @@ -576,9 +592,12 @@ int br_vlan_add(struct net_bridge *br, u16 vid, u16 
>> flags)
>>  refcount_inc(>refcnt);
>>  vlan->flags |= BRIDGE_VLAN_INFO_BRENTRY;
>>  vg->num_vlans++;
>> +*changed = true;
>>  }
>> -__vlan_add_flags(vlan, flags);
>> -return 0;
>> +if (__vlan_add_flags(vlan, flags))
>> +*changed = true;
>> +
>> +return ret;
> 
> "ret" isn't always initialized here, is it?
> 
> 
> Toshiaki Makita

Oh, good catch! Right you are, weird that there was no warning even with W=1 as
I always check that before sending a set.

Thanks,
 Nik

Re: [Bridge] [PATCH net-next v4 2/2] bridge: vlan: signal if anything changed on vlan add

[Toshiaki Makita]

On 2017/10/26 7:52, Nikolay Aleksandrov wrote:
...
> @@ -559,6 +574,7 @@ int br_vlan_add(struct net_bridge *br, u16 vid, u16 flags)
>  
>   ASSERT_RTNL();
>  
> + *changed = false;
>   vg = br_vlan_group(br);
>   vlan = br_vlan_find(vg, vid);
>   if (vlan) {
> @@ -576,9 +592,12 @@ int br_vlan_add(struct net_bridge *br, u16 vid, u16 
> flags)
>   refcount_inc(>refcnt);
>   vlan->flags |= BRIDGE_VLAN_INFO_BRENTRY;
>   vg->num_vlans++;
> + *changed = true;
>   }
> - __vlan_add_flags(vlan, flags);
> - return 0;
> + if (__vlan_add_flags(vlan, flags))
> + *changed = true;
> +
> + return ret;

"ret" isn't always initialized here, is it?


Toshiaki Makita

Re: [Bridge] [PATCH net-next v3 1/2] bridge: netlink: make setlink/dellink notifications more accurate

[Nikolay Aleksandrov]

On 26/10/17 11:17, Stephen Hemminger wrote:
> On Thu, 26 Oct 2017 00:59:41 +0300
> Nikolay Aleksandrov  wrote:
> 
>> Before this patch we had cases that either sent notifications when there
>> were in fact no changes (e.g. non-existent vlan delete) or didn't send
>> notifications when there were changes (e.g. vlan add range with an error in
>> the middle, port flags change + vlan update error). This patch sends down
>> a boolean to the functions setlink/dellink use and if there is even a
>> single configuration change (port flag, vlan add/del, port state) then
>> we always send a notification. This is all done to keep backwards
>> compatibility with the opportunistic vlan delete, where one could
>> specify a vlan range that has missing vlans inside and still everything
>> in that range will be cleared, this is mostly used to clear the whole
>> vlan config with a single call, i.e. range 1-4094.
>>
>> Signed-off-by: Nikolay Aleksandrov 
> 
> This looks correct.
> As a general note, generating a notice with no change should be ok for a 
> correctly
> written application. But missing a notification would cause synchronization 
> problems.
> 
> Acked-by: Stephen Hemminger 
> 

Stephen, thank you for the review. Could you please send the ack to
v4 of this set ? Since it's already sent, I can't add it myself.

Thanks,
 Nik

Re: [Bridge] [PATCH net-next v3 1/2] bridge: netlink: make setlink/dellink notifications more accurate

[Stephen Hemminger]

On Thu, 26 Oct 2017 00:59:41 +0300
Nikolay Aleksandrov  wrote:

> Before this patch we had cases that either sent notifications when there
> were in fact no changes (e.g. non-existent vlan delete) or didn't send
> notifications when there were changes (e.g. vlan add range with an error in
> the middle, port flags change + vlan update error). This patch sends down
> a boolean to the functions setlink/dellink use and if there is even a
> single configuration change (port flag, vlan add/del, port state) then
> we always send a notification. This is all done to keep backwards
> compatibility with the opportunistic vlan delete, where one could
> specify a vlan range that has missing vlans inside and still everything
> in that range will be cleared, this is mostly used to clear the whole
> vlan config with a single call, i.e. range 1-4094.
> 
> Signed-off-by: Nikolay Aleksandrov 

This looks correct.
As a general note, generating a notice with no change should be ok for a 
correctly
written application. But missing a notification would cause synchronization 
problems.

Acked-by: Stephen Hemminger 

[Bridge] Linux Bridge Static FDB move

[Viraj.Raiyani]

Hi Everyone,


Is it possible to program the FDB entry in Linux Bridge which is static, 
non-local and doesn't move to new interface when the same source MAC packet 
comes on a different interface in the same bridge  ?


I tried programming the MAC as permanent that prevents the moving of MAC to a 
new interface in the same bridge, however it treats the MAC as local and 
doesn't do the forwarding ?


Thanks,

Viraj