[PATCH net] bridge: Fix problems around fdb entries pointing to the bridge device

[PATCH net] bridge: Fix problems around fdb entries pointing to the bridge device

[Toshiaki Makita]

Adding fdb entries pointing to the bridge device uses fdb_insert(),
which lacks various checks and does not respect added_by_user flag.

As a result, some inconsistent behavior can happen:
* Adding temporary entries succeeds but results in permanent entries.
* Same goes for "dynamic" and "use".
* Changing mac address of the bridge device causes deletion of
  user-added entries.
* Replacing existing entries looks successful from userspace but actually
  not, regardless of NLM_F_EXCL flag.

Use the same logic as other entries and fix them.

Fixes: 3741873b4f73 ("bridge: allow adding of fdb entries pointing to the bridge device")
Signed-off-by: Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp>
---
 net/bridge/br_fdb.c | 52 +++++++++++++++++++++++++++-------------------------
 1 file changed, 27 insertions(+), 25 deletions(-)

diff --git a/net/bridge/br_fdb.c b/net/bridge/br_fdb.c
index c18080a..cd620fa 100644
--- a/net/bridge/br_fdb.c
+++ b/net/bridge/br_fdb.c
@@ -267,7 +267,7 @@ void br_fdb_change_mac_address(struct net_bridge *br, const u8 *newaddr)
 
 	/* If old entry was unassociated with any port, then delete it. */
 	f = __br_fdb_get(br, br->dev->dev_addr, 0);
-	if (f && f->is_local && !f->dst)
+	if (f && f->is_local && !f->dst && !f->added_by_user)
 		fdb_delete_local(br, NULL, f);
 
 	fdb_insert(br, NULL, newaddr, 0);
@@ -282,7 +282,7 @@ void br_fdb_change_mac_address(struct net_bridge *br, const u8 *newaddr)
 		if (!br_vlan_should_use(v))
 			continue;
 		f = __br_fdb_get(br, br->dev->dev_addr, v->vid);
-		if (f && f->is_local && !f->dst)
+		if (f && f->is_local && !f->dst && !f->added_by_user)
 			fdb_delete_local(br, NULL, f);
 		fdb_insert(br, NULL, newaddr, v->vid);
 	}
@@ -764,20 +764,25 @@ out:
 }
 
 /* Update (create or replace) forwarding database entry */
-static int fdb_add_entry(struct net_bridge_port *source, const __u8 *addr,
-			 __u16 state, __u16 flags, __u16 vid)
+static int fdb_add_entry(struct net_bridge *br, struct net_bridge_port *source,
+			 const __u8 *addr, __u16 state, __u16 flags, __u16 vid)
 {
-	struct net_bridge *br = source->br;
 	struct hlist_head *head = &br->hash[br_mac_hash(addr, vid)];
 	struct net_bridge_fdb_entry *fdb;
 	bool modified = false;
 
 	/* If the port cannot learn allow only local and static entries */
-	if (!(state & NUD_PERMANENT) && !(state & NUD_NOARP) &&
+	if (source && !(state & NUD_PERMANENT) && !(state & NUD_NOARP) &&
 	    !(source->state == BR_STATE_LEARNING ||
 	      source->state == BR_STATE_FORWARDING))
 		return -EPERM;
 
+	if (!source && !(state & NUD_PERMANENT)) {
+		pr_info("bridge: RTM_NEWNEIGH %s without NUD_PERMANENT\n",
+			br->dev->name);
+		return -EINVAL;
+	}
+
 	fdb = fdb_find(head, addr, vid);
 	if (fdb == NULL) {
 		if (!(flags & NLM_F_CREATE))
@@ -832,22 +837,28 @@ static int fdb_add_entry(struct net_bridge_port *source, const __u8 *addr,
 	return 0;
 }
 
-static int __br_fdb_add(struct ndmsg *ndm, struct net_bridge_port *p,
-	       const unsigned char *addr, u16 nlh_flags, u16 vid)
+static int __br_fdb_add(struct ndmsg *ndm, struct net_bridge *br,
+			struct net_bridge_port *p, const unsigned char *addr,
+			u16 nlh_flags, u16 vid)
 {
 	int err = 0;
 
 	if (ndm->ndm_flags & NTF_USE) {
+		if (!p) {
+			pr_info("bridge: RTM_NEWNEIGH %s with NTF_USE is not supported\n",
+				br->dev->name);
+			return -EINVAL;
+		}
 		local_bh_disable();
 		rcu_read_lock();
-		br_fdb_update(p->br, p, addr, vid, true);
+		br_fdb_update(br, p, addr, vid, true);
 		rcu_read_unlock();
 		local_bh_enable();
 	} else {
-		spin_lock_bh(&p->br->hash_lock);
-		err = fdb_add_entry(p, addr, ndm->ndm_state,
+		spin_lock_bh(&br->hash_lock);
+		err = fdb_add_entry(br, p, addr, ndm->ndm_state,
 				    nlh_flags, vid);
-		spin_unlock_bh(&p->br->hash_lock);
+		spin_unlock_bh(&br->hash_lock);
 	}
 
 	return err;
@@ -884,6 +895,7 @@ int br_fdb_add(struct ndmsg *ndm, struct nlattr *tb[],
 				dev->name);
 			return -EINVAL;
 		}
+		br = p->br;
 		vg = nbp_vlan_group(p);
 	}
 
@@ -895,15 +907,9 @@ int br_fdb_add(struct ndmsg *ndm, struct nlattr *tb[],
 		}
 
 		/* VID was specified, so use it. */
-		if (dev->priv_flags & IFF_EBRIDGE)
-			err = br_fdb_insert(br, NULL, addr, vid);
-		else
-			err = __br_fdb_add(ndm, p, addr, nlh_flags, vid);
+		err = __br_fdb_add(ndm, br, p, addr, nlh_flags, vid);
 	} else {
-		if (dev->priv_flags & IFF_EBRIDGE)
-			err = br_fdb_insert(br, NULL, addr, 0);
-		else
-			err = __br_fdb_add(ndm, p, addr, nlh_flags, 0);
+		err = __br_fdb_add(ndm, br, p, addr, nlh_flags, 0);
 		if (err || !vg || !vg->num_vlans)
 			goto out;
 
@@ -914,11 +920,7 @@ int br_fdb_add(struct ndmsg *ndm, struct nlattr *tb[],
 		list_for_each_entry(v, &vg->vlan_list, vlist) {
 			if (!br_vlan_should_use(v))
 				continue;
-			if (dev->priv_flags & IFF_EBRIDGE)
-				err = br_fdb_insert(br, NULL, addr, v->vid);
-			else
-				err = __br_fdb_add(ndm, p, addr, nlh_flags,
-						   v->vid);
+			err = __br_fdb_add(ndm, br, p, addr, nlh_flags, v->vid);
 			if (err)
 				goto out;
 		}
-- 
1.8.3.1

[Bridge] [PATCH net] bridge: Fix problems around fdb entries pointing to the bridge device

[Toshiaki Makita]

Adding fdb entries pointing to the bridge device uses fdb_insert(),
which lacks various checks and does not respect added_by_user flag.

As a result, some inconsistent behavior can happen:
* Adding temporary entries succeeds but results in permanent entries.
* Same goes for "dynamic" and "use".
* Changing mac address of the bridge device causes deletion of
  user-added entries.
* Replacing existing entries looks successful from userspace but actually
  not, regardless of NLM_F_EXCL flag.

Use the same logic as other entries and fix them.

Fixes: 3741873b4f73 ("bridge: allow adding of fdb entries pointing to the bridge device")
Signed-off-by: Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp>
---
 net/bridge/br_fdb.c | 52 +++++++++++++++++++++++++++-------------------------
 1 file changed, 27 insertions(+), 25 deletions(-)

diff --git a/net/bridge/br_fdb.c b/net/bridge/br_fdb.c
index c18080a..cd620fa 100644
--- a/net/bridge/br_fdb.c
+++ b/net/bridge/br_fdb.c
@@ -267,7 +267,7 @@ void br_fdb_change_mac_address(struct net_bridge *br, const u8 *newaddr)
 
 	/* If old entry was unassociated with any port, then delete it. */
 	f = __br_fdb_get(br, br->dev->dev_addr, 0);
-	if (f && f->is_local && !f->dst)
+	if (f && f->is_local && !f->dst && !f->added_by_user)
 		fdb_delete_local(br, NULL, f);
 
 	fdb_insert(br, NULL, newaddr, 0);
@@ -282,7 +282,7 @@ void br_fdb_change_mac_address(struct net_bridge *br, const u8 *newaddr)
 		if (!br_vlan_should_use(v))
 			continue;
 		f = __br_fdb_get(br, br->dev->dev_addr, v->vid);
-		if (f && f->is_local && !f->dst)
+		if (f && f->is_local && !f->dst && !f->added_by_user)
 			fdb_delete_local(br, NULL, f);
 		fdb_insert(br, NULL, newaddr, v->vid);
 	}
@@ -764,20 +764,25 @@ out:
 }
 
 /* Update (create or replace) forwarding database entry */
-static int fdb_add_entry(struct net_bridge_port *source, const __u8 *addr,
-			 __u16 state, __u16 flags, __u16 vid)
+static int fdb_add_entry(struct net_bridge *br, struct net_bridge_port *source,
+			 const __u8 *addr, __u16 state, __u16 flags, __u16 vid)
 {
-	struct net_bridge *br = source->br;
 	struct hlist_head *head = &br->hash[br_mac_hash(addr, vid)];
 	struct net_bridge_fdb_entry *fdb;
 	bool modified = false;
 
 	/* If the port cannot learn allow only local and static entries */
-	if (!(state & NUD_PERMANENT) && !(state & NUD_NOARP) &&
+	if (source && !(state & NUD_PERMANENT) && !(state & NUD_NOARP) &&
 	    !(source->state == BR_STATE_LEARNING ||
 	      source->state == BR_STATE_FORWARDING))
 		return -EPERM;
 
+	if (!source && !(state & NUD_PERMANENT)) {
+		pr_info("bridge: RTM_NEWNEIGH %s without NUD_PERMANENT\n",
+			br->dev->name);
+		return -EINVAL;
+	}
+
 	fdb = fdb_find(head, addr, vid);
 	if (fdb == NULL) {
 		if (!(flags & NLM_F_CREATE))
@@ -832,22 +837,28 @@ static int fdb_add_entry(struct net_bridge_port *source, const __u8 *addr,
 	return 0;
 }
 
-static int __br_fdb_add(struct ndmsg *ndm, struct net_bridge_port *p,
-	       const unsigned char *addr, u16 nlh_flags, u16 vid)
+static int __br_fdb_add(struct ndmsg *ndm, struct net_bridge *br,
+			struct net_bridge_port *p, const unsigned char *addr,
+			u16 nlh_flags, u16 vid)
 {
 	int err = 0;
 
 	if (ndm->ndm_flags & NTF_USE) {
+		if (!p) {
+			pr_info("bridge: RTM_NEWNEIGH %s with NTF_USE is not supported\n",
+				br->dev->name);
+			return -EINVAL;
+		}
 		local_bh_disable();
 		rcu_read_lock();
-		br_fdb_update(p->br, p, addr, vid, true);
+		br_fdb_update(br, p, addr, vid, true);
 		rcu_read_unlock();
 		local_bh_enable();
 	} else {
-		spin_lock_bh(&p->br->hash_lock);
-		err = fdb_add_entry(p, addr, ndm->ndm_state,
+		spin_lock_bh(&br->hash_lock);
+		err = fdb_add_entry(br, p, addr, ndm->ndm_state,
 				    nlh_flags, vid);
-		spin_unlock_bh(&p->br->hash_lock);
+		spin_unlock_bh(&br->hash_lock);
 	}
 
 	return err;
@@ -884,6 +895,7 @@ int br_fdb_add(struct ndmsg *ndm, struct nlattr *tb[],
 				dev->name);
 			return -EINVAL;
 		}
+		br = p->br;
 		vg = nbp_vlan_group(p);
 	}
 
@@ -895,15 +907,9 @@ int br_fdb_add(struct ndmsg *ndm, struct nlattr *tb[],
 		}
 
 		/* VID was specified, so use it. */
-		if (dev->priv_flags & IFF_EBRIDGE)
-			err = br_fdb_insert(br, NULL, addr, vid);
-		else
-			err = __br_fdb_add(ndm, p, addr, nlh_flags, vid);
+		err = __br_fdb_add(ndm, br, p, addr, nlh_flags, vid);
 	} else {
-		if (dev->priv_flags & IFF_EBRIDGE)
-			err = br_fdb_insert(br, NULL, addr, 0);
-		else
-			err = __br_fdb_add(ndm, p, addr, nlh_flags, 0);
+		err = __br_fdb_add(ndm, br, p, addr, nlh_flags, 0);
 		if (err || !vg || !vg->num_vlans)
 			goto out;
 
@@ -914,11 +920,7 @@ int br_fdb_add(struct ndmsg *ndm, struct nlattr *tb[],
 		list_for_each_entry(v, &vg->vlan_list, vlist) {
 			if (!br_vlan_should_use(v))
 				continue;
-			if (dev->priv_flags & IFF_EBRIDGE)
-				err = br_fdb_insert(br, NULL, addr, v->vid);
-			else
-				err = __br_fdb_add(ndm, p, addr, nlh_flags,
-						   v->vid);
+			err = __br_fdb_add(ndm, br, p, addr, nlh_flags, v->vid);
 			if (err)
 				goto out;
 		}
-- 
1.8.3.1

Re: [PATCH net] bridge: Fix problems around fdb entries pointing to the bridge device

[Roopa Prabhu via Bridge]

On 8/3/16, 7:11 PM, Toshiaki Makita wrote:
> Adding fdb entries pointing to the bridge device uses fdb_insert(),
> which lacks various checks and does not respect added_by_user flag.
>
> As a result, some inconsistent behavior can happen:
> * Adding temporary entries succeeds but results in permanent entries.

IIRC this is not specific to fdb entries on the bridge device. all temp
fdb entries via iproute2 result in permanent entries. thats why 'dynamic'
was added.
> * Same goes for "dynamic" and "use".
This patch seems to not allow dynamic and use entries
on the bridge device. I don't see a strong use-case to
allow them, but any reason you want to add the restriction now ?
> * Changing mac address of the bridge device causes deletion of
>   user-added entries.
unless I am missing something, this does not seem to be related to the
external fdb entry on the bridge device.

> * Replacing existing entries looks successful from userspace but actually
>   not, regardless of NLM_F_EXCL flag.
curious about this one. I will try it, but if you have an example that
will help.
>
> Use the same logic as other entries and fix them.
>
> Fixes: 3741873b4f73 ("bridge: allow adding of fdb entries pointing to the bridge device")
> Signed-off-by: Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp>
> ---
no objections to the patch.
Good to see both paths being unified by making __br_fdb_add work
for both cases.

>  net/bridge/br_fdb.c | 52 +++++++++++++++++++++++++++-------------------------
>  1 file changed, 27 insertions(+), 25 deletions(-)
>
> diff --git a/net/bridge/br_fdb.c b/net/bridge/br_fdb.c
> index c18080a..cd620fa 100644
> --- a/net/bridge/br_fdb.c
> +++ b/net/bridge/br_fdb.c
> @@ -267,7 +267,7 @@ void br_fdb_change_mac_address(struct net_bridge *br, const u8 *newaddr)
>  
>  	/* If old entry was unassociated with any port, then delete it. */
>  	f = __br_fdb_get(br, br->dev->dev_addr, 0);
> -	if (f && f->is_local && !f->dst)
> +	if (f && f->is_local && !f->dst && !f->added_by_user)
>  		fdb_delete_local(br, NULL, f);
>  
>  	fdb_insert(br, NULL, newaddr, 0);
> @@ -282,7 +282,7 @@ void br_fdb_change_mac_address(struct net_bridge *br, const u8 *newaddr)
>  		if (!br_vlan_should_use(v))
>  			continue;
>  		f = __br_fdb_get(br, br->dev->dev_addr, v->vid);
> -		if (f && f->is_local && !f->dst)
> +		if (f && f->is_local && !f->dst && !f->added_by_user)
>  			fdb_delete_local(br, NULL, f);
>  		fdb_insert(br, NULL, newaddr, v->vid);
>  	}
> @@ -764,20 +764,25 @@ out:
>  }
>  
>  /* Update (create or replace) forwarding database entry */
> -static int fdb_add_entry(struct net_bridge_port *source, const __u8 *addr,
> -			 __u16 state, __u16 flags, __u16 vid)
> +static int fdb_add_entry(struct net_bridge *br, struct net_bridge_port *source,
> +			 const __u8 *addr, __u16 state, __u16 flags, __u16 vid)
>  {
> -	struct net_bridge *br = source->br;
>  	struct hlist_head *head = &br->hash[br_mac_hash(addr, vid)];
>  	struct net_bridge_fdb_entry *fdb;
>  	bool modified = false;
>  
>  	/* If the port cannot learn allow only local and static entries */
> -	if (!(state & NUD_PERMANENT) && !(state & NUD_NOARP) &&
> +	if (source && !(state & NUD_PERMANENT) && !(state & NUD_NOARP) &&
>  	    !(source->state == BR_STATE_LEARNING ||
>  	      source->state == BR_STATE_FORWARDING))
>  		return -EPERM;
>  
> +	if (!source && !(state & NUD_PERMANENT)) {
> +		pr_info("bridge: RTM_NEWNEIGH %s without NUD_PERMANENT\n",
> +			br->dev->name);
> +		return -EINVAL;
> +	}
> +
>  	fdb = fdb_find(head, addr, vid);
>  	if (fdb == NULL) {
>  		if (!(flags & NLM_F_CREATE))
> @@ -832,22 +837,28 @@ static int fdb_add_entry(struct net_bridge_port *source, const __u8 *addr,
>  	return 0;
>  }
>  
> -static int __br_fdb_add(struct ndmsg *ndm, struct net_bridge_port *p,
> -	       const unsigned char *addr, u16 nlh_flags, u16 vid)
> +static int __br_fdb_add(struct ndmsg *ndm, struct net_bridge *br,
> +			struct net_bridge_port *p, const unsigned char *addr,
> +			u16 nlh_flags, u16 vid)
>  {
>  	int err = 0;
>  
>  	if (ndm->ndm_flags & NTF_USE) {
> +		if (!p) {
> +			pr_info("bridge: RTM_NEWNEIGH %s with NTF_USE is not supported\n",
> +				br->dev->name);
> +			return -EINVAL;
> +		}
>  		local_bh_disable();
>  		rcu_read_lock();
> -		br_fdb_update(p->br, p, addr, vid, true);
> +		br_fdb_update(br, p, addr, vid, true);
>  		rcu_read_unlock();
>  		local_bh_enable();
>  	} else {
> -		spin_lock_bh(&p->br->hash_lock);
> -		err = fdb_add_entry(p, addr, ndm->ndm_state,
> +		spin_lock_bh(&br->hash_lock);
> +		err = fdb_add_entry(br, p, addr, ndm->ndm_state,
>  				    nlh_flags, vid);
> -		spin_unlock_bh(&p->br->hash_lock);
> +		spin_unlock_bh(&br->hash_lock);
>  	}
>  
>  	return err;
> @@ -884,6 +895,7 @@ int br_fdb_add(struct ndmsg *ndm, struct nlattr *tb[],
>  				dev->name);
>  			return -EINVAL;
>  		}
> +		br = p->br;
>  		vg = nbp_vlan_group(p);
>  	}
>  
> @@ -895,15 +907,9 @@ int br_fdb_add(struct ndmsg *ndm, struct nlattr *tb[],
>  		}
>  
>  		/* VID was specified, so use it. */
> -		if (dev->priv_flags & IFF_EBRIDGE)
> -			err = br_fdb_insert(br, NULL, addr, vid);
> -		else
> -			err = __br_fdb_add(ndm, p, addr, nlh_flags, vid);
> +		err = __br_fdb_add(ndm, br, p, addr, nlh_flags, vid);
>  	} else {
> -		if (dev->priv_flags & IFF_EBRIDGE)
> -			err = br_fdb_insert(br, NULL, addr, 0);
> -		else
> -			err = __br_fdb_add(ndm, p, addr, nlh_flags, 0);
> +		err = __br_fdb_add(ndm, br, p, addr, nlh_flags, 0);
>  		if (err || !vg || !vg->num_vlans)
>  			goto out;
>  
> @@ -914,11 +920,7 @@ int br_fdb_add(struct ndmsg *ndm, struct nlattr *tb[],
>  		list_for_each_entry(v, &vg->vlan_list, vlist) {
>  			if (!br_vlan_should_use(v))
>  				continue;
> -			if (dev->priv_flags & IFF_EBRIDGE)
> -				err = br_fdb_insert(br, NULL, addr, v->vid);
> -			else
> -				err = __br_fdb_add(ndm, p, addr, nlh_flags,
> -						   v->vid);
> +			err = __br_fdb_add(ndm, br, p, addr, nlh_flags, v->vid);
>  			if (err)
>  				goto out;
>  		}

Re: [Bridge] [PATCH net] bridge: Fix problems around fdb entries pointing to the bridge device

[Roopa Prabhu]

On 8/3/16, 7:11 PM, Toshiaki Makita wrote:
> Adding fdb entries pointing to the bridge device uses fdb_insert(),
> which lacks various checks and does not respect added_by_user flag.
>
> As a result, some inconsistent behavior can happen:
> * Adding temporary entries succeeds but results in permanent entries.

IIRC this is not specific to fdb entries on the bridge device. all temp
fdb entries via iproute2 result in permanent entries. thats why 'dynamic'
was added.
> * Same goes for "dynamic" and "use".
This patch seems to not allow dynamic and use entries
on the bridge device. I don't see a strong use-case to
allow them, but any reason you want to add the restriction now ?
> * Changing mac address of the bridge device causes deletion of
>   user-added entries.
unless I am missing something, this does not seem to be related to the
external fdb entry on the bridge device.

> * Replacing existing entries looks successful from userspace but actually
>   not, regardless of NLM_F_EXCL flag.
curious about this one. I will try it, but if you have an example that
will help.
>
> Use the same logic as other entries and fix them.
>
> Fixes: 3741873b4f73 ("bridge: allow adding of fdb entries pointing to the bridge device")
> Signed-off-by: Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp>
> ---
no objections to the patch.
Good to see both paths being unified by making __br_fdb_add work
for both cases.

>  net/bridge/br_fdb.c | 52 +++++++++++++++++++++++++++-------------------------
>  1 file changed, 27 insertions(+), 25 deletions(-)
>
> diff --git a/net/bridge/br_fdb.c b/net/bridge/br_fdb.c
> index c18080a..cd620fa 100644
> --- a/net/bridge/br_fdb.c
> +++ b/net/bridge/br_fdb.c
> @@ -267,7 +267,7 @@ void br_fdb_change_mac_address(struct net_bridge *br, const u8 *newaddr)
>  
>  	/* If old entry was unassociated with any port, then delete it. */
>  	f = __br_fdb_get(br, br->dev->dev_addr, 0);
> -	if (f && f->is_local && !f->dst)
> +	if (f && f->is_local && !f->dst && !f->added_by_user)
>  		fdb_delete_local(br, NULL, f);
>  
>  	fdb_insert(br, NULL, newaddr, 0);
> @@ -282,7 +282,7 @@ void br_fdb_change_mac_address(struct net_bridge *br, const u8 *newaddr)
>  		if (!br_vlan_should_use(v))
>  			continue;
>  		f = __br_fdb_get(br, br->dev->dev_addr, v->vid);
> -		if (f && f->is_local && !f->dst)
> +		if (f && f->is_local && !f->dst && !f->added_by_user)
>  			fdb_delete_local(br, NULL, f);
>  		fdb_insert(br, NULL, newaddr, v->vid);
>  	}
> @@ -764,20 +764,25 @@ out:
>  }
>  
>  /* Update (create or replace) forwarding database entry */
> -static int fdb_add_entry(struct net_bridge_port *source, const __u8 *addr,
> -			 __u16 state, __u16 flags, __u16 vid)
> +static int fdb_add_entry(struct net_bridge *br, struct net_bridge_port *source,
> +			 const __u8 *addr, __u16 state, __u16 flags, __u16 vid)
>  {
> -	struct net_bridge *br = source->br;
>  	struct hlist_head *head = &br->hash[br_mac_hash(addr, vid)];
>  	struct net_bridge_fdb_entry *fdb;
>  	bool modified = false;
>  
>  	/* If the port cannot learn allow only local and static entries */
> -	if (!(state & NUD_PERMANENT) && !(state & NUD_NOARP) &&
> +	if (source && !(state & NUD_PERMANENT) && !(state & NUD_NOARP) &&
>  	    !(source->state == BR_STATE_LEARNING ||
>  	      source->state == BR_STATE_FORWARDING))
>  		return -EPERM;
>  
> +	if (!source && !(state & NUD_PERMANENT)) {
> +		pr_info("bridge: RTM_NEWNEIGH %s without NUD_PERMANENT\n",
> +			br->dev->name);
> +		return -EINVAL;
> +	}
> +
>  	fdb = fdb_find(head, addr, vid);
>  	if (fdb == NULL) {
>  		if (!(flags & NLM_F_CREATE))
> @@ -832,22 +837,28 @@ static int fdb_add_entry(struct net_bridge_port *source, const __u8 *addr,
>  	return 0;
>  }
>  
> -static int __br_fdb_add(struct ndmsg *ndm, struct net_bridge_port *p,
> -	       const unsigned char *addr, u16 nlh_flags, u16 vid)
> +static int __br_fdb_add(struct ndmsg *ndm, struct net_bridge *br,
> +			struct net_bridge_port *p, const unsigned char *addr,
> +			u16 nlh_flags, u16 vid)
>  {
>  	int err = 0;
>  
>  	if (ndm->ndm_flags & NTF_USE) {
> +		if (!p) {
> +			pr_info("bridge: RTM_NEWNEIGH %s with NTF_USE is not supported\n",
> +				br->dev->name);
> +			return -EINVAL;
> +		}
>  		local_bh_disable();
>  		rcu_read_lock();
> -		br_fdb_update(p->br, p, addr, vid, true);
> +		br_fdb_update(br, p, addr, vid, true);
>  		rcu_read_unlock();
>  		local_bh_enable();
>  	} else {
> -		spin_lock_bh(&p->br->hash_lock);
> -		err = fdb_add_entry(p, addr, ndm->ndm_state,
> +		spin_lock_bh(&br->hash_lock);
> +		err = fdb_add_entry(br, p, addr, ndm->ndm_state,
>  				    nlh_flags, vid);
> -		spin_unlock_bh(&p->br->hash_lock);
> +		spin_unlock_bh(&br->hash_lock);
>  	}
>  
>  	return err;
> @@ -884,6 +895,7 @@ int br_fdb_add(struct ndmsg *ndm, struct nlattr *tb[],
>  				dev->name);
>  			return -EINVAL;
>  		}
> +		br = p->br;
>  		vg = nbp_vlan_group(p);
>  	}
>  
> @@ -895,15 +907,9 @@ int br_fdb_add(struct ndmsg *ndm, struct nlattr *tb[],
>  		}
>  
>  		/* VID was specified, so use it. */
> -		if (dev->priv_flags & IFF_EBRIDGE)
> -			err = br_fdb_insert(br, NULL, addr, vid);
> -		else
> -			err = __br_fdb_add(ndm, p, addr, nlh_flags, vid);
> +		err = __br_fdb_add(ndm, br, p, addr, nlh_flags, vid);
>  	} else {
> -		if (dev->priv_flags & IFF_EBRIDGE)
> -			err = br_fdb_insert(br, NULL, addr, 0);
> -		else
> -			err = __br_fdb_add(ndm, p, addr, nlh_flags, 0);
> +		err = __br_fdb_add(ndm, br, p, addr, nlh_flags, 0);
>  		if (err || !vg || !vg->num_vlans)
>  			goto out;
>  
> @@ -914,11 +920,7 @@ int br_fdb_add(struct ndmsg *ndm, struct nlattr *tb[],
>  		list_for_each_entry(v, &vg->vlan_list, vlist) {
>  			if (!br_vlan_should_use(v))
>  				continue;
> -			if (dev->priv_flags & IFF_EBRIDGE)
> -				err = br_fdb_insert(br, NULL, addr, v->vid);
> -			else
> -				err = __br_fdb_add(ndm, p, addr, nlh_flags,
> -						   v->vid);
> +			err = __br_fdb_add(ndm, br, p, addr, nlh_flags, v->vid);
>  			if (err)
>  				goto out;
>  		}

Re: [PATCH net] bridge: Fix problems around fdb entries pointing to the bridge device

[Toshiaki Makita]

On 2016/08/04 16:24, Roopa Prabhu wrote:
> On 8/3/16, 7:11 PM, Toshiaki Makita wrote:
>> Adding fdb entries pointing to the bridge device uses fdb_insert(),
>> which lacks various checks and does not respect added_by_user flag.
>>
>> As a result, some inconsistent behavior can happen:
>> * Adding temporary entries succeeds but results in permanent entries.
> 
> IIRC this is not specific to fdb entries on the bridge device. all temp
> fdb entries via iproute2 result in permanent entries. thats why 'dynamic'
> was added.

Sorry for confusing you, I meant "temp" (i.e., "static") of bridge fdb
command.
"temp", "dynamic" and "use" should not result in "permanent".

>> * Same goes for "dynamic" and "use".
> This patch seems to not allow dynamic and use entries
> on the bridge device. I don't see a strong use-case to
> allow them, but any reason you want to add the restriction now ?

Because dynamic fdb entries pointing the bridge device cannot be
created. So it is prohibited. I cannot find other appropriate behavior
about this.
Or are you suggesting local entries with aging supported or something
like that?

>> * Changing mac address of the bridge device causes deletion of
>>   user-added entries.
> unless I am missing something, this does not seem to be related to the
> external fdb entry on the bridge device.

Yes this is related to manually-added fdb entries on the bridge device.
When manual addition of fdb pointing the bridge device was introduced,
we should have set added_by_user on adding the entry and modify
br_fdb_change_mac_address() to respect the flag, but both were not done.

> 
>> * Replacing existing entries looks successful from userspace but actually
>>   not, regardless of NLM_F_EXCL flag.
> curious about this one. I will try it, but if you have an example that
> will help.

Before:
# bridge fdb add 12:34:56:78:90:ab dev enp3s0 master
# bridge fdb add 12:34:56:78:90:ab dev br0; echo $?
0
# bridge fdb show
...
12:34:56:78:90:ab dev enp3s0 master br0 permanent

# bridge fdb replace 12:34:56:78:90:ab dev br0; echo $?
0
# bridge fdb show
...
12:34:56:78:90:ab dev enp3s0 master br0 permanent

After:
# bridge fdb add 12:34:56:78:90:ab dev enp3s0 master
# bridge fdb add 12:34:56:78:90:ab dev br0; echo $?
RTNETLINK answers: File exists
255

# bridge fdb replace 12:34:56:78:90:ab dev br0; echo $?
0
# bridge fdb show
...
12:34:56:78:90:ab dev br0 master br0 permanent

-- 
Toshiaki Makita

Re: [Bridge] [PATCH net] bridge: Fix problems around fdb entries pointing to the bridge device

[Toshiaki Makita]

On 2016/08/04 16:24, Roopa Prabhu wrote:
> On 8/3/16, 7:11 PM, Toshiaki Makita wrote:
>> Adding fdb entries pointing to the bridge device uses fdb_insert(),
>> which lacks various checks and does not respect added_by_user flag.
>>
>> As a result, some inconsistent behavior can happen:
>> * Adding temporary entries succeeds but results in permanent entries.
> 
> IIRC this is not specific to fdb entries on the bridge device. all temp
> fdb entries via iproute2 result in permanent entries. thats why 'dynamic'
> was added.

Sorry for confusing you, I meant "temp" (i.e., "static") of bridge fdb
command.
"temp", "dynamic" and "use" should not result in "permanent".

>> * Same goes for "dynamic" and "use".
> This patch seems to not allow dynamic and use entries
> on the bridge device. I don't see a strong use-case to
> allow them, but any reason you want to add the restriction now ?

Because dynamic fdb entries pointing the bridge device cannot be
created. So it is prohibited. I cannot find other appropriate behavior
about this.
Or are you suggesting local entries with aging supported or something
like that?

>> * Changing mac address of the bridge device causes deletion of
>>   user-added entries.
> unless I am missing something, this does not seem to be related to the
> external fdb entry on the bridge device.

Yes this is related to manually-added fdb entries on the bridge device.
When manual addition of fdb pointing the bridge device was introduced,
we should have set added_by_user on adding the entry and modify
br_fdb_change_mac_address() to respect the flag, but both were not done.

> 
>> * Replacing existing entries looks successful from userspace but actually
>>   not, regardless of NLM_F_EXCL flag.
> curious about this one. I will try it, but if you have an example that
> will help.

Before:
# bridge fdb add 12:34:56:78:90:ab dev enp3s0 master
# bridge fdb add 12:34:56:78:90:ab dev br0; echo $?
0
# bridge fdb show
...
12:34:56:78:90:ab dev enp3s0 master br0 permanent

# bridge fdb replace 12:34:56:78:90:ab dev br0; echo $?
0
# bridge fdb show
...
12:34:56:78:90:ab dev enp3s0 master br0 permanent

After:
# bridge fdb add 12:34:56:78:90:ab dev enp3s0 master
# bridge fdb add 12:34:56:78:90:ab dev br0; echo $?
RTNETLINK answers: File exists
255

# bridge fdb replace 12:34:56:78:90:ab dev br0; echo $?
0
# bridge fdb show
...
12:34:56:78:90:ab dev br0 master br0 permanent

-- 
Toshiaki Makita

Re: [PATCH net] bridge: Fix problems around fdb entries pointing to the bridge device

[Toshiaki Makita]

On 2016/08/04 17:15, Toshiaki Makita wrote:
> On 2016/08/04 16:24, Roopa Prabhu wrote:
>> On 8/3/16, 7:11 PM, Toshiaki Makita wrote:
>>> Adding fdb entries pointing to the bridge device uses fdb_insert(),
>>> which lacks various checks and does not respect added_by_user flag.
>>>
>>> As a result, some inconsistent behavior can happen:
>>> * Adding temporary entries succeeds but results in permanent entries.
>>
>> IIRC this is not specific to fdb entries on the bridge device. all temp
>> fdb entries via iproute2 result in permanent entries. thats why 'dynamic'
>> was added.
> 
> Sorry for confusing you, I meant "temp" (i.e., "static") of bridge fdb
> command.
> "temp", "dynamic" and "use" should not result in "permanent".

I probably misread you in my previous reply so the previous answer looks
weird...

What I should have said is:
Other temp fdb entries via iproute2 result in static (== temp), not
permanent.
"dynamic" and "use" should be meant for dynamic entries, not static (temp).

-- 
Toshiaki Makita

Re: [Bridge] [PATCH net] bridge: Fix problems around fdb entries pointing to the bridge device

[Toshiaki Makita]

On 2016/08/04 17:15, Toshiaki Makita wrote:
> On 2016/08/04 16:24, Roopa Prabhu wrote:
>> On 8/3/16, 7:11 PM, Toshiaki Makita wrote:
>>> Adding fdb entries pointing to the bridge device uses fdb_insert(),
>>> which lacks various checks and does not respect added_by_user flag.
>>>
>>> As a result, some inconsistent behavior can happen:
>>> * Adding temporary entries succeeds but results in permanent entries.
>>
>> IIRC this is not specific to fdb entries on the bridge device. all temp
>> fdb entries via iproute2 result in permanent entries. thats why 'dynamic'
>> was added.
> 
> Sorry for confusing you, I meant "temp" (i.e., "static") of bridge fdb
> command.
> "temp", "dynamic" and "use" should not result in "permanent".

I probably misread you in my previous reply so the previous answer looks
weird...

What I should have said is:
Other temp fdb entries via iproute2 result in static (== temp), not
permanent.
"dynamic" and "use" should be meant for dynamic entries, not static (temp).

-- 
Toshiaki Makita

Re: [PATCH net] bridge: Fix problems around fdb entries pointing to the bridge device

[Roopa Prabhu]

On 8/4/16, 1:15 AM, Toshiaki Makita wrote:
> On 2016/08/04 16:24, Roopa Prabhu wrote:
>> On 8/3/16, 7:11 PM, Toshiaki Makita wrote:
>>> Adding fdb entries pointing to the bridge device uses fdb_insert(),
>>> which lacks various checks and does not respect added_by_user flag.
>>>
>>> As a result, some inconsistent behavior can happen:
>>> * Adding temporary entries succeeds but results in permanent entries.
>> IIRC this is not specific to fdb entries on the bridge device. all temp
>> fdb entries via iproute2 result in permanent entries. thats why 'dynamic'
>> was added.
> Sorry for confusing you, I meant "temp" (i.e., "static") of bridge fdb
> command.
> "temp", "dynamic" and "use" should not result in "permanent".
>
>>> * Same goes for "dynamic" and "use".
>> This patch seems to not allow dynamic and use entries
>> on the bridge device. I don't see a strong use-case to
>> allow them, but any reason you want to add the restriction now ?
> Because dynamic fdb entries pointing the bridge device cannot be
> created. So it is prohibited. I cannot find other appropriate behavior
> about this.
> Or are you suggesting local entries with aging supported or something
> like that?
no, i am ok with prohibiting it, just was wondering if this is necessary.

>
>>> * Changing mac address of the bridge device causes deletion of
>>>   user-added entries.
>> unless I am missing something, this does not seem to be related to the
>> external fdb entry on the bridge device.
> Yes this is related to manually-added fdb entries on the bridge device.
> When manual addition of fdb pointing the bridge device was introduced,
> we should have set added_by_user on adding the entry and modify
> br_fdb_change_mac_address() to respect the flag, but both were not done.
>
>>> * Replacing existing entries looks successful from userspace but actually
>>>   not, regardless of NLM_F_EXCL flag.
>> curious about this one. I will try it, but if you have an example that
>> will help.
> Before:
> # bridge fdb add 12:34:56:78:90:ab dev enp3s0 master
> # bridge fdb add 12:34:56:78:90:ab dev br0; echo $?
> 0
> # bridge fdb show
> ...
> 12:34:56:78:90:ab dev enp3s0 master br0 permanent
>
> # bridge fdb replace 12:34:56:78:90:ab dev br0; echo $?
> 0
> # bridge fdb show
> ...
> 12:34:56:78:90:ab dev enp3s0 master br0 permanent
>
> After:
> # bridge fdb add 12:34:56:78:90:ab dev enp3s0 master
> # bridge fdb add 12:34:56:78:90:ab dev br0; echo $?
> RTNETLINK answers: File exists
> 255
>
> # bridge fdb replace 12:34:56:78:90:ab dev br0; echo $?
> 0
> # bridge fdb show
> ...
> 12:34:56:78:90:ab dev br0 master br0 permanent
>
ok, thanks for the example.

Acked-by: Roopa Prabhu <roopa@cumulusnetworks.com>

Re: [Bridge] [PATCH net] bridge: Fix problems around fdb entries pointing to the bridge device

[Roopa Prabhu]

On 8/4/16, 1:15 AM, Toshiaki Makita wrote:
> On 2016/08/04 16:24, Roopa Prabhu wrote:
>> On 8/3/16, 7:11 PM, Toshiaki Makita wrote:
>>> Adding fdb entries pointing to the bridge device uses fdb_insert(),
>>> which lacks various checks and does not respect added_by_user flag.
>>>
>>> As a result, some inconsistent behavior can happen:
>>> * Adding temporary entries succeeds but results in permanent entries.
>> IIRC this is not specific to fdb entries on the bridge device. all temp
>> fdb entries via iproute2 result in permanent entries. thats why 'dynamic'
>> was added.
> Sorry for confusing you, I meant "temp" (i.e., "static") of bridge fdb
> command.
> "temp", "dynamic" and "use" should not result in "permanent".
>
>>> * Same goes for "dynamic" and "use".
>> This patch seems to not allow dynamic and use entries
>> on the bridge device. I don't see a strong use-case to
>> allow them, but any reason you want to add the restriction now ?
> Because dynamic fdb entries pointing the bridge device cannot be
> created. So it is prohibited. I cannot find other appropriate behavior
> about this.
> Or are you suggesting local entries with aging supported or something
> like that?
no, i am ok with prohibiting it, just was wondering if this is necessary.

>
>>> * Changing mac address of the bridge device causes deletion of
>>>   user-added entries.
>> unless I am missing something, this does not seem to be related to the
>> external fdb entry on the bridge device.
> Yes this is related to manually-added fdb entries on the bridge device.
> When manual addition of fdb pointing the bridge device was introduced,
> we should have set added_by_user on adding the entry and modify
> br_fdb_change_mac_address() to respect the flag, but both were not done.
>
>>> * Replacing existing entries looks successful from userspace but actually
>>>   not, regardless of NLM_F_EXCL flag.
>> curious about this one. I will try it, but if you have an example that
>> will help.
> Before:
> # bridge fdb add 12:34:56:78:90:ab dev enp3s0 master
> # bridge fdb add 12:34:56:78:90:ab dev br0; echo $?
> 0
> # bridge fdb show
> ...
> 12:34:56:78:90:ab dev enp3s0 master br0 permanent
>
> # bridge fdb replace 12:34:56:78:90:ab dev br0; echo $?
> 0
> # bridge fdb show
> ...
> 12:34:56:78:90:ab dev enp3s0 master br0 permanent
>
> After:
> # bridge fdb add 12:34:56:78:90:ab dev enp3s0 master
> # bridge fdb add 12:34:56:78:90:ab dev br0; echo $?
> RTNETLINK answers: File exists
> 255
>
> # bridge fdb replace 12:34:56:78:90:ab dev br0; echo $?
> 0
> # bridge fdb show
> ...
> 12:34:56:78:90:ab dev br0 master br0 permanent
>
ok, thanks for the example.

Acked-by: Roopa Prabhu <roopa@cumulusnetworks.com>

Re: [PATCH net] bridge: Fix problems around fdb entries pointing to the bridge device

[Stephen Hemminger]

On Thu, 04 Aug 2016 11:27:25 -0700
Roopa Prabhu <roopa@cumulusnetworks.com> wrote:

> On 8/4/16, 1:15 AM, Toshiaki Makita wrote:
> > On 2016/08/04 16:24, Roopa Prabhu wrote:  
> >> On 8/3/16, 7:11 PM, Toshiaki Makita wrote:  
> >>> Adding fdb entries pointing to the bridge device uses fdb_insert(),
> >>> which lacks various checks and does not respect added_by_user flag.
> >>>
> >>> As a result, some inconsistent behavior can happen:
> >>> * Adding temporary entries succeeds but results in permanent entries.  
> >> IIRC this is not specific to fdb entries on the bridge device. all temp
> >> fdb entries via iproute2 result in permanent entries. thats why 'dynamic'
> >> was added.  
> > Sorry for confusing you, I meant "temp" (i.e., "static") of bridge fdb
> > command.
> > "temp", "dynamic" and "use" should not result in "permanent".
> >  
> >>> * Same goes for "dynamic" and "use".  
> >> This patch seems to not allow dynamic and use entries
> >> on the bridge device. I don't see a strong use-case to
> >> allow them, but any reason you want to add the restriction now ?  
> > Because dynamic fdb entries pointing the bridge device cannot be
> > created. So it is prohibited. I cannot find other appropriate behavior
> > about this.
> > Or are you suggesting local entries with aging supported or something
> > like that?  
> no, i am ok with prohibiting it, just was wondering if this is necessary.
> 
> >  
> >>> * Changing mac address of the bridge device causes deletion of
> >>>   user-added entries.  
> >> unless I am missing something, this does not seem to be related to the
> >> external fdb entry on the bridge device.  
> > Yes this is related to manually-added fdb entries on the bridge device.
> > When manual addition of fdb pointing the bridge device was introduced,
> > we should have set added_by_user on adding the entry and modify
> > br_fdb_change_mac_address() to respect the flag, but both were not done.
> >  
> >>> * Replacing existing entries looks successful from userspace but actually
> >>>   not, regardless of NLM_F_EXCL flag.  
> >> curious about this one. I will try it, but if you have an example that
> >> will help.  
> > Before:
> > # bridge fdb add 12:34:56:78:90:ab dev enp3s0 master
> > # bridge fdb add 12:34:56:78:90:ab dev br0; echo $?
> > 0
> > # bridge fdb show
> > ...
> > 12:34:56:78:90:ab dev enp3s0 master br0 permanent
> >
> > # bridge fdb replace 12:34:56:78:90:ab dev br0; echo $?
> > 0
> > # bridge fdb show
> > ...
> > 12:34:56:78:90:ab dev enp3s0 master br0 permanent
> >
> > After:
> > # bridge fdb add 12:34:56:78:90:ab dev enp3s0 master
> > # bridge fdb add 12:34:56:78:90:ab dev br0; echo $?
> > RTNETLINK answers: File exists
> > 255
> >
> > # bridge fdb replace 12:34:56:78:90:ab dev br0; echo $?
> > 0
> > # bridge fdb show
> > ...
> > 12:34:56:78:90:ab dev br0 master br0 permanent
> >  
> ok, thanks for the example.
> 
> Acked-by: Roopa Prabhu <roopa@cumulusnetworks.com>

It should be possible to manually add fdb entries with any combination
of valid flags. That is it should be possible to add temporary as well as permanent
entries. There are people that use this facility to do long and short ageing temporary
entries.

Re: [Bridge] [PATCH net] bridge: Fix problems around fdb entries pointing to the bridge device

[Stephen Hemminger]

On Thu, 04 Aug 2016 11:27:25 -0700
Roopa Prabhu <roopa@cumulusnetworks.com> wrote:

> On 8/4/16, 1:15 AM, Toshiaki Makita wrote:
> > On 2016/08/04 16:24, Roopa Prabhu wrote:  
> >> On 8/3/16, 7:11 PM, Toshiaki Makita wrote:  
> >>> Adding fdb entries pointing to the bridge device uses fdb_insert(),
> >>> which lacks various checks and does not respect added_by_user flag.
> >>>
> >>> As a result, some inconsistent behavior can happen:
> >>> * Adding temporary entries succeeds but results in permanent entries.  
> >> IIRC this is not specific to fdb entries on the bridge device. all temp
> >> fdb entries via iproute2 result in permanent entries. thats why 'dynamic'
> >> was added.  
> > Sorry for confusing you, I meant "temp" (i.e., "static") of bridge fdb
> > command.
> > "temp", "dynamic" and "use" should not result in "permanent".
> >  
> >>> * Same goes for "dynamic" and "use".  
> >> This patch seems to not allow dynamic and use entries
> >> on the bridge device. I don't see a strong use-case to
> >> allow them, but any reason you want to add the restriction now ?  
> > Because dynamic fdb entries pointing the bridge device cannot be
> > created. So it is prohibited. I cannot find other appropriate behavior
> > about this.
> > Or are you suggesting local entries with aging supported or something
> > like that?  
> no, i am ok with prohibiting it, just was wondering if this is necessary.
> 
> >  
> >>> * Changing mac address of the bridge device causes deletion of
> >>>   user-added entries.  
> >> unless I am missing something, this does not seem to be related to the
> >> external fdb entry on the bridge device.  
> > Yes this is related to manually-added fdb entries on the bridge device.
> > When manual addition of fdb pointing the bridge device was introduced,
> > we should have set added_by_user on adding the entry and modify
> > br_fdb_change_mac_address() to respect the flag, but both were not done.
> >  
> >>> * Replacing existing entries looks successful from userspace but actually
> >>>   not, regardless of NLM_F_EXCL flag.  
> >> curious about this one. I will try it, but if you have an example that
> >> will help.  
> > Before:
> > # bridge fdb add 12:34:56:78:90:ab dev enp3s0 master
> > # bridge fdb add 12:34:56:78:90:ab dev br0; echo $?
> > 0
> > # bridge fdb show
> > ...
> > 12:34:56:78:90:ab dev enp3s0 master br0 permanent
> >
> > # bridge fdb replace 12:34:56:78:90:ab dev br0; echo $?
> > 0
> > # bridge fdb show
> > ...
> > 12:34:56:78:90:ab dev enp3s0 master br0 permanent
> >
> > After:
> > # bridge fdb add 12:34:56:78:90:ab dev enp3s0 master
> > # bridge fdb add 12:34:56:78:90:ab dev br0; echo $?
> > RTNETLINK answers: File exists
> > 255
> >
> > # bridge fdb replace 12:34:56:78:90:ab dev br0; echo $?
> > 0
> > # bridge fdb show
> > ...
> > 12:34:56:78:90:ab dev br0 master br0 permanent
> >  
> ok, thanks for the example.
> 
> Acked-by: Roopa Prabhu <roopa@cumulusnetworks.com>

It should be possible to manually add fdb entries with any combination
of valid flags. That is it should be possible to add temporary as well as permanent
entries. There are people that use this facility to do long and short ageing temporary
entries.

Re: [PATCH net] bridge: Fix problems around fdb entries pointing to the bridge device

[Toshiaki Makita]

On 16/08/06 (土) 7:26, Stephen Hemminger wrote:
> On Thu, 04 Aug 2016 11:27:25 -0700
> Roopa Prabhu <roopa@cumulusnetworks.com> wrote:
>
>> On 8/4/16, 1:15 AM, Toshiaki Makita wrote:
>>> On 2016/08/04 16:24, Roopa Prabhu wrote:
>>>> On 8/3/16, 7:11 PM, Toshiaki Makita wrote:
>>>>> Adding fdb entries pointing to the bridge device uses fdb_insert(),
>>>>> which lacks various checks and does not respect added_by_user flag.
>>>>>
>>>>> As a result, some inconsistent behavior can happen:
>>>>> * Adding temporary entries succeeds but results in permanent entries.
>>>> IIRC this is not specific to fdb entries on the bridge device. all temp
>>>> fdb entries via iproute2 result in permanent entries. thats why 'dynamic'
>>>> was added.
>>> Sorry for confusing you, I meant "temp" (i.e., "static") of bridge fdb
>>> command.
>>> "temp", "dynamic" and "use" should not result in "permanent".
>>>
>>>>> * Same goes for "dynamic" and "use".
>>>> This patch seems to not allow dynamic and use entries
>>>> on the bridge device. I don't see a strong use-case to
>>>> allow them, but any reason you want to add the restriction now ?
>>> Because dynamic fdb entries pointing the bridge device cannot be
>>> created. So it is prohibited. I cannot find other appropriate behavior
>>> about this.
>>> Or are you suggesting local entries with aging supported or something
>>> like that?
>> no, i am ok with prohibiting it, just was wondering if this is necessary.
>>
>>>
>>>>> * Changing mac address of the bridge device causes deletion of
>>>>>   user-added entries.
>>>> unless I am missing something, this does not seem to be related to the
>>>> external fdb entry on the bridge device.
>>> Yes this is related to manually-added fdb entries on the bridge device.
>>> When manual addition of fdb pointing the bridge device was introduced,
>>> we should have set added_by_user on adding the entry and modify
>>> br_fdb_change_mac_address() to respect the flag, but both were not done.
>>>
>>>>> * Replacing existing entries looks successful from userspace but actually
>>>>>   not, regardless of NLM_F_EXCL flag.
>>>> curious about this one. I will try it, but if you have an example that
>>>> will help.
>>> Before:
>>> # bridge fdb add 12:34:56:78:90:ab dev enp3s0 master
>>> # bridge fdb add 12:34:56:78:90:ab dev br0; echo $?
>>> 0
>>> # bridge fdb show
>>> ...
>>> 12:34:56:78:90:ab dev enp3s0 master br0 permanent
>>>
>>> # bridge fdb replace 12:34:56:78:90:ab dev br0; echo $?
>>> 0
>>> # bridge fdb show
>>> ...
>>> 12:34:56:78:90:ab dev enp3s0 master br0 permanent
>>>
>>> After:
>>> # bridge fdb add 12:34:56:78:90:ab dev enp3s0 master
>>> # bridge fdb add 12:34:56:78:90:ab dev br0; echo $?
>>> RTNETLINK answers: File exists
>>> 255
>>>
>>> # bridge fdb replace 12:34:56:78:90:ab dev br0; echo $?
>>> 0
>>> # bridge fdb show
>>> ...
>>> 12:34:56:78:90:ab dev br0 master br0 permanent
>>>
>> ok, thanks for the example.
>>
>> Acked-by: Roopa Prabhu <roopa@cumulusnetworks.com>
>
> It should be possible to manually add fdb entries with any combination
> of valid flags. That is it should be possible to add temporary as well as permanent
> entries. There are people that use this facility to do long and short ageing temporary
> entries.

I see. But allowing dynamic entries means that bridge effectively 
supports local entries with aging supported, and bridge has not have 
such a feature so far: Bridge currently accepts addition of dynamic 
entries, but any addition of fdb entries pointing to the bridge device 
results in permanent entries. Bridge is just pretending support of 
dynamic local entries for now.
What I'm fixing here is just a user API problem, so I'm not thinking we 
should allow dynamic entries here. I guess we can remove the restriction 
in -next in the future.

Thanks,
Toshiaki Makita

Re: [Bridge] [PATCH net] bridge: Fix problems around fdb entries pointing to the bridge device

[Toshiaki Makita]

On 16/08/06 (土) 7:26, Stephen Hemminger wrote:
> On Thu, 04 Aug 2016 11:27:25 -0700
> Roopa Prabhu <roopa@cumulusnetworks.com> wrote:
>
>> On 8/4/16, 1:15 AM, Toshiaki Makita wrote:
>>> On 2016/08/04 16:24, Roopa Prabhu wrote:
>>>> On 8/3/16, 7:11 PM, Toshiaki Makita wrote:
>>>>> Adding fdb entries pointing to the bridge device uses fdb_insert(),
>>>>> which lacks various checks and does not respect added_by_user flag.
>>>>>
>>>>> As a result, some inconsistent behavior can happen:
>>>>> * Adding temporary entries succeeds but results in permanent entries.
>>>> IIRC this is not specific to fdb entries on the bridge device. all temp
>>>> fdb entries via iproute2 result in permanent entries. thats why 'dynamic'
>>>> was added.
>>> Sorry for confusing you, I meant "temp" (i.e., "static") of bridge fdb
>>> command.
>>> "temp", "dynamic" and "use" should not result in "permanent".
>>>
>>>>> * Same goes for "dynamic" and "use".
>>>> This patch seems to not allow dynamic and use entries
>>>> on the bridge device. I don't see a strong use-case to
>>>> allow them, but any reason you want to add the restriction now ?
>>> Because dynamic fdb entries pointing the bridge device cannot be
>>> created. So it is prohibited. I cannot find other appropriate behavior
>>> about this.
>>> Or are you suggesting local entries with aging supported or something
>>> like that?
>> no, i am ok with prohibiting it, just was wondering if this is necessary.
>>
>>>
>>>>> * Changing mac address of the bridge device causes deletion of
>>>>>   user-added entries.
>>>> unless I am missing something, this does not seem to be related to the
>>>> external fdb entry on the bridge device.
>>> Yes this is related to manually-added fdb entries on the bridge device.
>>> When manual addition of fdb pointing the bridge device was introduced,
>>> we should have set added_by_user on adding the entry and modify
>>> br_fdb_change_mac_address() to respect the flag, but both were not done.
>>>
>>>>> * Replacing existing entries looks successful from userspace but actually
>>>>>   not, regardless of NLM_F_EXCL flag.
>>>> curious about this one. I will try it, but if you have an example that
>>>> will help.
>>> Before:
>>> # bridge fdb add 12:34:56:78:90:ab dev enp3s0 master
>>> # bridge fdb add 12:34:56:78:90:ab dev br0; echo $?
>>> 0
>>> # bridge fdb show
>>> ...
>>> 12:34:56:78:90:ab dev enp3s0 master br0 permanent
>>>
>>> # bridge fdb replace 12:34:56:78:90:ab dev br0; echo $?
>>> 0
>>> # bridge fdb show
>>> ...
>>> 12:34:56:78:90:ab dev enp3s0 master br0 permanent
>>>
>>> After:
>>> # bridge fdb add 12:34:56:78:90:ab dev enp3s0 master
>>> # bridge fdb add 12:34:56:78:90:ab dev br0; echo $?
>>> RTNETLINK answers: File exists
>>> 255
>>>
>>> # bridge fdb replace 12:34:56:78:90:ab dev br0; echo $?
>>> 0
>>> # bridge fdb show
>>> ...
>>> 12:34:56:78:90:ab dev br0 master br0 permanent
>>>
>> ok, thanks for the example.
>>
>> Acked-by: Roopa Prabhu <roopa@cumulusnetworks.com>
>
> It should be possible to manually add fdb entries with any combination
> of valid flags. That is it should be possible to add temporary as well as permanent
> entries. There are people that use this facility to do long and short ageing temporary
> entries.

I see. But allowing dynamic entries means that bridge effectively 
supports local entries with aging supported, and bridge has not have 
such a feature so far: Bridge currently accepts addition of dynamic 
entries, but any addition of fdb entries pointing to the bridge device 
results in permanent entries. Bridge is just pretending support of 
dynamic local entries for now.
What I'm fixing here is just a user API problem, so I'm not thinking we 
should allow dynamic entries here. I guess we can remove the restriction 
in -next in the future.

Thanks,
Toshiaki Makita

Re: [PATCH net] bridge: Fix problems around fdb entries pointing to the bridge device

[David Miller]

From: Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp>
Date: Thu,  4 Aug 2016 11:11:19 +0900

> Adding fdb entries pointing to the bridge device uses fdb_insert(),
> which lacks various checks and does not respect added_by_user flag.
> 
> As a result, some inconsistent behavior can happen:
> * Adding temporary entries succeeds but results in permanent entries.
> * Same goes for "dynamic" and "use".
> * Changing mac address of the bridge device causes deletion of
>   user-added entries.
> * Replacing existing entries looks successful from userspace but actually
>   not, regardless of NLM_F_EXCL flag.
> 
> Use the same logic as other entries and fix them.
> 
> Fixes: 3741873b4f73 ("bridge: allow adding of fdb entries pointing to the bridge device")
> Signed-off-by: Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp>

Applied, thanks.

Re: [Bridge] [PATCH net] bridge: Fix problems around fdb entries pointing to the bridge device

[David Miller]

From: Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp>
Date: Thu,  4 Aug 2016 11:11:19 +0900

> Adding fdb entries pointing to the bridge device uses fdb_insert(),
> which lacks various checks and does not respect added_by_user flag.
> 
> As a result, some inconsistent behavior can happen:
> * Adding temporary entries succeeds but results in permanent entries.
> * Same goes for "dynamic" and "use".
> * Changing mac address of the bridge device causes deletion of
>   user-added entries.
> * Replacing existing entries looks successful from userspace but actually
>   not, regardless of NLM_F_EXCL flag.
> 
> Use the same logic as other entries and fix them.
> 
> Fixes: 3741873b4f73 ("bridge: allow adding of fdb entries pointing to the bridge device")
> Signed-off-by: Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp>

Applied, thanks.