All of lore.kernel.org
 help / color / mirror / Atom feed
* [BUG?] bridge_id not getting set for bridges created with
@ 2014-04-21 19:33 ` Tom Gundersen
  0 siblings, 0 replies; 22+ messages in thread
From: Tom Gundersen @ 2014-04-21 19:33 UTC (permalink / raw)
  To: netdev, bridge; +Cc: C. R. Oldham

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

^ permalink raw reply	[flat|nested] 22+ messages in thread

* [Bridge] [BUG?] bridge_id not getting set for bridges created with
@ 2014-04-21 19:33 ` Tom Gundersen
  0 siblings, 0 replies; 22+ messages in thread
From: Tom Gundersen @ 2014-04-21 19:33 UTC (permalink / raw)
  To: netdev, bridge; +Cc: C. R. Oldham

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

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [BUG?] bridge_id not getting set for bridges created with
  2014-04-21 19:33 ` [Bridge] " Tom Gundersen
@ 2014-04-21 20:56   ` Tom Gundersen
  -1 siblings, 0 replies; 22+ messages in thread
From: Tom Gundersen @ 2014-04-21 20:56 UTC (permalink / raw)
  To: netdev, bridge; +Cc: C. R. Oldham

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

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [Bridge] [BUG?] bridge_id not getting set for bridges created with
@ 2014-04-21 20:56   ` Tom Gundersen
  0 siblings, 0 replies; 22+ messages in thread
From: Tom Gundersen @ 2014-04-21 20:56 UTC (permalink / raw)
  To: netdev, bridge; +Cc: C. R. Oldham

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

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [BUG?] bridge_id not getting set for bridges created with
  2014-04-21 20:56   ` [Bridge] " Tom Gundersen
@ 2014-04-24 12:06     ` Toshiaki Makita
  -1 siblings, 0 replies; 22+ messages in thread
From: Toshiaki Makita @ 2014-04-24 12:06 UTC (permalink / raw)
  To: Tom Gundersen; +Cc: netdev, bridge, C. R. Oldham

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

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [Bridge] [BUG?] bridge_id not getting set for bridges created with
@ 2014-04-24 12:06     ` Toshiaki Makita
  0 siblings, 0 replies; 22+ messages in thread
From: Toshiaki Makita @ 2014-04-24 12:06 UTC (permalink / raw)
  To: Tom Gundersen; +Cc: netdev, bridge, C. R. Oldham

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


^ permalink raw reply	[flat|nested] 22+ messages in thread

* [PATCH net] bridge: Handle IFLA_ADDRESS correctly when creating bridge device
  2014-04-24 12:06     ` [Bridge] " Toshiaki Makita
@ 2014-04-24 12:16       ` Toshiaki Makita
  -1 siblings, 0 replies; 22+ messages in thread
From: Toshiaki Makita @ 2014-04-24 12:16 UTC (permalink / raw)
  To: David S. Miller, Stephen Hemminger
  Cc: Tom Gundersen, netdev, bridge, C. R. Oldham

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

^ permalink raw reply related	[flat|nested] 22+ messages in thread

* [Bridge] [PATCH net] bridge: Handle IFLA_ADDRESS correctly when creating bridge device
@ 2014-04-24 12:16       ` Toshiaki Makita
  0 siblings, 0 replies; 22+ messages in thread
From: Toshiaki Makita @ 2014-04-24 12:16 UTC (permalink / raw)
  To: David S. Miller, Stephen Hemminger
  Cc: Tom Gundersen, netdev, bridge, C. R. Oldham

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



^ permalink raw reply related	[flat|nested] 22+ messages in thread

* Unsubscribe me in this Forum
  2014-04-24 12:06     ` [Bridge] " Toshiaki Makita
@ 2014-04-24 12:30       ` Amidu Sila
  -1 siblings, 0 replies; 22+ messages in thread
From: Amidu Sila @ 2014-04-24 12:30 UTC (permalink / raw)
  To: netdev, bridge

Gentlemen,
Please unsubscribe me in the forum.
Regards
Amidu

^ permalink raw reply	[flat|nested] 22+ messages in thread

* [Bridge] Unsubscribe me in this Forum
@ 2014-04-24 12:30       ` Amidu Sila
  0 siblings, 0 replies; 22+ messages in thread
From: Amidu Sila @ 2014-04-24 12:30 UTC (permalink / raw)
  To: netdev, bridge

Gentlemen,
Please unsubscribe me in the forum.
Regards
Amidu

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH net] bridge: Handle IFLA_ADDRESS correctly when creating bridge device
  2014-04-24 12:16       ` [Bridge] " Toshiaki Makita
@ 2014-04-24 16:04         ` Tom Gundersen
  -1 siblings, 0 replies; 22+ messages in thread
From: Tom Gundersen @ 2014-04-24 16:04 UTC (permalink / raw)
  To: Toshiaki Makita
  Cc: David S. Miller, Stephen Hemminger, netdev, bridge, C. R. Oldham

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

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [Bridge] [PATCH net] bridge: Handle IFLA_ADDRESS correctly when creating bridge device
@ 2014-04-24 16:04         ` Tom Gundersen
  0 siblings, 0 replies; 22+ messages in thread
From: Tom Gundersen @ 2014-04-24 16:04 UTC (permalink / raw)
  To: Toshiaki Makita
  Cc: Stephen Hemminger, netdev, bridge, David S. Miller, C. R. Oldham

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

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH net] bridge: Handle IFLA_ADDRESS correctly when creating bridge device
  2014-04-24 12:16       ` [Bridge] " Toshiaki Makita
@ 2014-04-24 18:38         ` Stephen Hemminger
  -1 siblings, 0 replies; 22+ messages in thread
From: Stephen Hemminger @ 2014-04-24 18:38 UTC (permalink / raw)
  To: Toshiaki Makita
  Cc: Tom Gundersen, netdev, bridge, David S. Miller, C. R. Oldham

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

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [Bridge] [PATCH net] bridge: Handle IFLA_ADDRESS correctly when creating bridge device
@ 2014-04-24 18:38         ` Stephen Hemminger
  0 siblings, 0 replies; 22+ messages in thread
From: Stephen Hemminger @ 2014-04-24 18:38 UTC (permalink / raw)
  To: Toshiaki Makita
  Cc: Tom Gundersen, netdev, bridge, David S. Miller, C. R. Oldham

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

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH net] bridge: Handle IFLA_ADDRESS correctly when creating bridge device
  2014-04-24 18:38         ` [Bridge] " Stephen Hemminger
@ 2014-04-25  7:54           ` Toshiaki Makita
  -1 siblings, 0 replies; 22+ messages in thread
From: Toshiaki Makita @ 2014-04-25  7:54 UTC (permalink / raw)
  To: Stephen Hemminger
  Cc: Tom Gundersen, netdev, bridge, David S. Miller, C. R. Oldham

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

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [Bridge] [PATCH net] bridge: Handle IFLA_ADDRESS correctly when creating bridge device
@ 2014-04-25  7:54           ` Toshiaki Makita
  0 siblings, 0 replies; 22+ messages in thread
From: Toshiaki Makita @ 2014-04-25  7:54 UTC (permalink / raw)
  To: Stephen Hemminger
  Cc: Tom Gundersen, netdev, bridge, David S. Miller, C. R. Oldham

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


^ permalink raw reply	[flat|nested] 22+ messages in thread

* [PATCH net v2] bridge: Handle IFLA_ADDRESS correctly when creating bridge device
  2014-04-25  7:54           ` [Bridge] " Toshiaki Makita
@ 2014-04-25  8:01             ` Toshiaki Makita
  -1 siblings, 0 replies; 22+ messages in thread
From: Toshiaki Makita @ 2014-04-25  8:01 UTC (permalink / raw)
  To: David S. Miller, Stephen Hemminger
  Cc: Tom Gundersen, netdev, bridge, C. R. Oldham

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

^ permalink raw reply related	[flat|nested] 22+ messages in thread

* [Bridge] [PATCH net v2] bridge: Handle IFLA_ADDRESS correctly when creating bridge device
@ 2014-04-25  8:01             ` Toshiaki Makita
  0 siblings, 0 replies; 22+ messages in thread
From: Toshiaki Makita @ 2014-04-25  8:01 UTC (permalink / raw)
  To: David S. Miller, Stephen Hemminger
  Cc: Tom Gundersen, netdev, bridge, C. R. Oldham

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


^ permalink raw reply related	[flat|nested] 22+ messages in thread

* Re: [PATCH net] bridge: Handle IFLA_ADDRESS correctly when creating bridge device
  2014-04-24 16:04         ` [Bridge] " Tom Gundersen
@ 2014-04-25  8:18           ` Toshiaki Makita
  -1 siblings, 0 replies; 22+ messages in thread
From: Toshiaki Makita @ 2014-04-25  8:18 UTC (permalink / raw)
  To: Tom Gundersen
  Cc: Stephen Hemminger, netdev, bridge, David S. Miller, C. R. Oldham

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

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [Bridge] [PATCH net] bridge: Handle IFLA_ADDRESS correctly when creating bridge device
@ 2014-04-25  8:18           ` Toshiaki Makita
  0 siblings, 0 replies; 22+ messages in thread
From: Toshiaki Makita @ 2014-04-25  8:18 UTC (permalink / raw)
  To: Tom Gundersen
  Cc: Stephen Hemminger, netdev, bridge, David S. Miller, C. R. Oldham

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


^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH net v2] bridge: Handle IFLA_ADDRESS correctly when creating bridge device
  2014-04-25  8:01             ` [Bridge] " Toshiaki Makita
@ 2014-04-27 23:54               ` David Miller
  -1 siblings, 0 replies; 22+ messages in thread
From: David Miller @ 2014-04-27 23:54 UTC (permalink / raw)
  To: makita.toshiaki; +Cc: stephen, netdev, teg, bridge, cr

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.

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [Bridge] [PATCH net v2] bridge: Handle IFLA_ADDRESS correctly when creating bridge device
@ 2014-04-27 23:54               ` David Miller
  0 siblings, 0 replies; 22+ messages in thread
From: David Miller @ 2014-04-27 23:54 UTC (permalink / raw)
  To: makita.toshiaki; +Cc: stephen, netdev, teg, bridge, cr

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.

^ permalink raw reply	[flat|nested] 22+ messages in thread

end of thread, other threads:[~2014-04-27 23:54 UTC | newest]

Thread overview: 22+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-04-21 19:33 [BUG?] bridge_id not getting set for bridges created with Tom Gundersen
2014-04-21 19:33 ` [Bridge] " Tom Gundersen
2014-04-21 20:56 ` Tom Gundersen
2014-04-21 20:56   ` [Bridge] " Tom Gundersen
2014-04-24 12:06   ` Toshiaki Makita
2014-04-24 12:06     ` [Bridge] " Toshiaki Makita
2014-04-24 12:16     ` [PATCH net] bridge: Handle IFLA_ADDRESS correctly when creating bridge device Toshiaki Makita
2014-04-24 12:16       ` [Bridge] " Toshiaki Makita
2014-04-24 16:04       ` Tom Gundersen
2014-04-24 16:04         ` [Bridge] " Tom Gundersen
2014-04-25  8:18         ` Toshiaki Makita
2014-04-25  8:18           ` [Bridge] " Toshiaki Makita
2014-04-24 18:38       ` Stephen Hemminger
2014-04-24 18:38         ` [Bridge] " Stephen Hemminger
2014-04-25  7:54         ` Toshiaki Makita
2014-04-25  7:54           ` [Bridge] " Toshiaki Makita
2014-04-25  8:01           ` [PATCH net v2] " Toshiaki Makita
2014-04-25  8:01             ` [Bridge] " Toshiaki Makita
2014-04-27 23:54             ` David Miller
2014-04-27 23:54               ` [Bridge] " David Miller
2014-04-24 12:30     ` Unsubscribe me in this Forum Amidu Sila
2014-04-24 12:30       ` [Bridge] " Amidu Sila

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.