[BUG?] bridge_id not getting set for bridges created with

[BUG?] bridge_id not getting set for bridges created with

[Tom Gundersen]

Hi,

If IFLA_ADDRESS is set when creating a bridge over netlink, e.g.

# ip link add test-bridge address b6:83:a2:b3:2f:0e type bride

the bridge id is not set:

# brctl show
bridge name    bridge id        STP enabled    interfaces
bridge1        8000.000000000000    no


It looks like this is caused by br_stp_set_bridge_id never beeing
called. There appear to be many ways to solve this, but not sure what
is the preferred route...

Cheers,

Tom

Re: [BUG?] bridge_id not getting set for bridges created with

[Tom Gundersen]

On Mon, Apr 21, 2014 at 9:33 PM, Tom Gundersen <teg@jklm.no> wrote:
> Hi,
>
> If IFLA_ADDRESS is set when creating a bridge over netlink, e.g.
>
> # ip link add test-bridge address b6:83:a2:b3:2f:0e type bride
>
> the bridge id is not set:
>
> # brctl show
> bridge name    bridge id        STP enabled    interfaces
> bridge1        8000.000000000000    no

My example was incomplete. To see the bug one should of course first
add an interface to the bridge:

# brctl addif bridge1 eth0


and observe that the bridge id remains unset:

# brctl show
bridge name    bridge id        STP enabled    interfaces
bridge1        8000.000000000000    no            eth0



If bridge1 had been created without specifying the mac address the
result is as expected:

# brctl show
bridge name    bridge id        STP enabled    interfaces
bridge1        8000.28d244547cdb    no            eth0

Cheers,

Tom

Re: [BUG?] bridge_id not getting set for bridges created with

[Toshiaki Makita]

On Mon, 2014-04-21 at 22:56 +0200, Tom Gundersen wrote:
> On Mon, Apr 21, 2014 at 9:33 PM, Tom Gundersen <teg@jklm.no> wrote:
> > Hi,
> >
> > If IFLA_ADDRESS is set when creating a bridge over netlink, e.g.
> >
> > # ip link add test-bridge address b6:83:a2:b3:2f:0e type bride
> >
> > the bridge id is not set:
> >
> > # brctl show
> > bridge name    bridge id        STP enabled    interfaces
> > bridge1        8000.000000000000    no
> 
> My example was incomplete. To see the bug one should of course first
> add an interface to the bridge:
> 
> # brctl addif bridge1 eth0
> 
> 
> and observe that the bridge id remains unset:
> 
> # brctl show
> bridge name    bridge id        STP enabled    interfaces
> bridge1        8000.000000000000    no            eth0
> 
> 
> 
> If bridge1 had been created without specifying the mac address the
> result is as expected:
> 
> # brctl show
> bridge name    bridge id        STP enabled    interfaces
> bridge1        8000.28d244547cdb    no            eth0
> 

Hi.

This seems to be a bug.
What's worse is that local fdb entry won't be created and we will not be
able to receive frames on the bridge device...

rtnl_create_link() doesn't call ndo_set_mac_address but populates
dev->dev_addr by itself. I'm not sure if it is safe to call
ndo_set_mac_address in this function (maybe affects other drivers).  I
think this can be handled in bridge codes (br_link_ops->newlink).

I'm submitting a patch to fix this.

Thanks,
Toshiaki Makita

[PATCH net] bridge: Handle IFLA_ADDRESS correctly when creating bridge device

[Toshiaki Makita]

When bridge device is created with IFLA_ADDRESS, we are not calling
br_stp_change_bridge_id(), which leads to incorrect local fdb
management and bridge id calculation, and prevents us from receiving
frames on the bridge device.

Reported-by: Tom Gundersen <teg@jklm.no>
Signed-off-by: Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp>
---
 net/bridge/br_netlink.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/net/bridge/br_netlink.c b/net/bridge/br_netlink.c
index e74b6d53..a8b664e 100644
--- a/net/bridge/br_netlink.c
+++ b/net/bridge/br_netlink.c
@@ -445,6 +445,25 @@ static int br_validate(struct nlattr *tb[], struct nlattr *data[])
 	return 0;
 }
 
+static int br_dev_newlink(struct net *src_net, struct net_device *dev,
+			  struct nlattr *tb[], struct nlattr *data[])
+{
+	int err;
+	struct net_bridge *br = netdev_priv(dev);
+
+	if (tb[IFLA_ADDRESS]) {
+		spin_lock_bh(&br->lock);
+		br_stp_change_bridge_id(br, nla_data(tb[IFLA_ADDRESS]));
+		spin_unlock_bh(&br->lock);
+	}
+
+	err = register_netdevice(dev);
+	if (err)
+		return err;
+
+	return 0;
+}
+
 static size_t br_get_link_af_size(const struct net_device *dev)
 {
 	struct net_port_vlans *pv;
@@ -473,6 +492,7 @@ struct rtnl_link_ops br_link_ops __read_mostly = {
 	.priv_size	= sizeof(struct net_bridge),
 	.setup		= br_dev_setup,
 	.validate	= br_validate,
+	.newlink	= br_dev_newlink,
 	.dellink	= br_dev_delete,
 };
 
-- 
1.8.1.2

Re: [PATCH net] bridge: Handle IFLA_ADDRESS correctly when creating bridge device

[Tom Gundersen]

On Thu, Apr 24, 2014 at 2:16 PM, Toshiaki Makita
<makita.toshiaki@lab.ntt.co.jp> wrote:
> When bridge device is created with IFLA_ADDRESS, we are not calling
> br_stp_change_bridge_id(), which leads to incorrect local fdb
> management and bridge id calculation, and prevents us from receiving
> frames on the bridge device.
>
> Reported-by: Tom Gundersen <teg@jklm.no>

Thanks. That looks correct to me (not able to test at the moment
though). Would this be appropriate for stable if it goes in?

Cheers,

Tom

> Signed-off-by: Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp>
> ---
>  net/bridge/br_netlink.c | 20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)
>
> diff --git a/net/bridge/br_netlink.c b/net/bridge/br_netlink.c
> index e74b6d53..a8b664e 100644
> --- a/net/bridge/br_netlink.c
> +++ b/net/bridge/br_netlink.c
> @@ -445,6 +445,25 @@ static int br_validate(struct nlattr *tb[], struct nlattr *data[])
>         return 0;
>  }
>
> +static int br_dev_newlink(struct net *src_net, struct net_device *dev,
> +                         struct nlattr *tb[], struct nlattr *data[])
> +{
> +       int err;
> +       struct net_bridge *br = netdev_priv(dev);
> +
> +       if (tb[IFLA_ADDRESS]) {
> +               spin_lock_bh(&br->lock);
> +               br_stp_change_bridge_id(br, nla_data(tb[IFLA_ADDRESS]));
> +               spin_unlock_bh(&br->lock);
> +       }
> +
> +       err = register_netdevice(dev);
> +       if (err)
> +               return err;
> +
> +       return 0;
> +}
> +
>  static size_t br_get_link_af_size(const struct net_device *dev)
>  {
>         struct net_port_vlans *pv;
> @@ -473,6 +492,7 @@ struct rtnl_link_ops br_link_ops __read_mostly = {
>         .priv_size      = sizeof(struct net_bridge),
>         .setup          = br_dev_setup,
>         .validate       = br_validate,
> +       .newlink        = br_dev_newlink,
>         .dellink        = br_dev_delete,
>  };
>
> --
> 1.8.1.2
>
>

Re: [PATCH net] bridge: Handle IFLA_ADDRESS correctly when creating bridge device

[Toshiaki Makita]

On Thu, 2014-04-24 at 18:04 +0200, Tom Gundersen wrote:
> On Thu, Apr 24, 2014 at 2:16 PM, Toshiaki Makita
> <makita.toshiaki@lab.ntt.co.jp> wrote:
> > When bridge device is created with IFLA_ADDRESS, we are not calling
> > br_stp_change_bridge_id(), which leads to incorrect local fdb
> > management and bridge id calculation, and prevents us from receiving
> > frames on the bridge device.
> >
> > Reported-by: Tom Gundersen <teg@jklm.no>
> 
> Thanks. That looks correct to me (not able to test at the moment
> though). Would this be appropriate for stable if it goes in?

This can apply 3.14 as is.
For 3.12 or earlier tree, this needs modification to insert the fdb
entry (or backport fdb fix patchset (684bd2e1)).
And for 3.2, another patch needs to be backported before this because
br_fdb_change_mac_address() doesn't exist.

Thanks,
Toshiaki Makita

Re: [PATCH net] bridge: Handle IFLA_ADDRESS correctly when creating bridge device

[Stephen Hemminger]

On Thu, 24 Apr 2014 21:16:27 +0900
Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp> wrote:

> +static int br_dev_newlink(struct net *src_net, struct net_device *dev,
> +			  struct nlattr *tb[], struct nlattr *data[])
> +{
> +	int err;
> +	struct net_bridge *br = netdev_priv(dev);
> +
> +	if (tb[IFLA_ADDRESS]) {
> +		spin_lock_bh(&br->lock);
> +		br_stp_change_bridge_id(br, nla_data(tb[IFLA_ADDRESS]));
> +		spin_unlock_bh(&br->lock);
> +	}
> +
> +	err = register_netdevice(dev);
> +	if (err)
> +		return err;
> +
> +	return 0;
> +}

Looks good.

Why not just do simpler tail call??
    return register_netdevice(dev);

Re: [PATCH net] bridge: Handle IFLA_ADDRESS correctly when creating bridge device

[Toshiaki Makita]

On Thu, 2014-04-24 at 11:38 -0700, Stephen Hemminger wrote:
> On Thu, 24 Apr 2014 21:16:27 +0900
> Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp> wrote:
> 
> > +static int br_dev_newlink(struct net *src_net, struct net_device *dev,
> > +			  struct nlattr *tb[], struct nlattr *data[])
> > +{
> > +	int err;
> > +	struct net_bridge *br = netdev_priv(dev);
> > +
> > +	if (tb[IFLA_ADDRESS]) {
> > +		spin_lock_bh(&br->lock);
> > +		br_stp_change_bridge_id(br, nla_data(tb[IFLA_ADDRESS]));
> > +		spin_unlock_bh(&br->lock);
> > +	}
> > +
> > +	err = register_netdevice(dev);
> > +	if (err)
> > +		return err;
> > +
> > +	return 0;
> > +}
> 
> Looks good.
> 
> Why not just do simpler tail call??
>     return register_netdevice(dev);

Yes, It's simpler.
I mimicked team_newlink()'s style, so we can also make it simpler
later :)

I'll send v2.

Thanks,
Toshiaki Makita

[PATCH net v2] bridge: Handle IFLA_ADDRESS correctly when creating bridge device

[Toshiaki Makita]

When bridge device is created with IFLA_ADDRESS, we are not calling
br_stp_change_bridge_id(), which leads to incorrect local fdb
management and bridge id calculation, and prevents us from receiving
frames on the bridge device.

Reported-by: Tom Gundersen <teg@jklm.no>
Signed-off-by: Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp>
---

v1->v2:
- Remove variable "err" and directly return register_netdevice().

 net/bridge/br_netlink.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/net/bridge/br_netlink.c b/net/bridge/br_netlink.c
index e74b6d53..e8844d9 100644
--- a/net/bridge/br_netlink.c
+++ b/net/bridge/br_netlink.c
@@ -445,6 +445,20 @@ static int br_validate(struct nlattr *tb[], struct nlattr *data[])
 	return 0;
 }
 
+static int br_dev_newlink(struct net *src_net, struct net_device *dev,
+			  struct nlattr *tb[], struct nlattr *data[])
+{
+	struct net_bridge *br = netdev_priv(dev);
+
+	if (tb[IFLA_ADDRESS]) {
+		spin_lock_bh(&br->lock);
+		br_stp_change_bridge_id(br, nla_data(tb[IFLA_ADDRESS]));
+		spin_unlock_bh(&br->lock);
+	}
+
+	return register_netdevice(dev);
+}
+
 static size_t br_get_link_af_size(const struct net_device *dev)
 {
 	struct net_port_vlans *pv;
@@ -473,6 +487,7 @@ struct rtnl_link_ops br_link_ops __read_mostly = {
 	.priv_size	= sizeof(struct net_bridge),
 	.setup		= br_dev_setup,
 	.validate	= br_validate,
+	.newlink	= br_dev_newlink,
 	.dellink	= br_dev_delete,
 };
 
-- 
1.8.1.2

Re: [PATCH net v2] bridge: Handle IFLA_ADDRESS correctly when creating bridge device

[David Miller]

From: Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp>
Date: Fri, 25 Apr 2014 17:01:18 +0900

> When bridge device is created with IFLA_ADDRESS, we are not calling
> br_stp_change_bridge_id(), which leads to incorrect local fdb
> management and bridge id calculation, and prevents us from receiving
> frames on the bridge device.
> 
> Reported-by: Tom Gundersen <teg@jklm.no>
> Signed-off-by: Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp>

Applied and queued up for -stable.

Unsubscribe me in this Forum

[Amidu Sila]

Gentlemen,
Please unsubscribe me in the forum.
Regards
Amidu