diff mbox

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

Message ID 1398341787.4171.16.camel@ubuntu-vm-makita
State Superseded, archived
Delegated to: David Miller
Headers show

Commit Message

Toshiaki Makita April 24, 2014, 12:16 p.m. UTC
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(+)

Comments

Tom Gundersen April 24, 2014, 4:04 p.m. UTC | #1
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
>
>
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Stephen Hemminger April 24, 2014, 6:38 p.m. UTC | #2
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);
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Toshiaki Makita April 25, 2014, 7:54 a.m. UTC | #3
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

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Toshiaki Makita April 25, 2014, 8:18 a.m. UTC | #4
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

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

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,
 };