From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jay Vosburgh Subject: Re: [PATCH net-next] net/ipv6: Block IPv6 addrconf on team ports Date: Thu, 25 Oct 2018 14:59:12 -0700 Message-ID: <25151.1540504752@famine> References: <20181025210227.25544-1-3chas3@gmail.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 8BIT Cc: davem@davemloft.net, netdev@vger.kernel.org, vfalico@gmail.com, andy@greyhouse.net, jiri@resnulli.us, kuznet@ms2.inr.ac.ru, yoshfuji@linux-ipv6.org To: Chas Williams <3chas3@gmail.com> Return-path: Received: from youngberry.canonical.com ([91.189.89.112]:46657 "EHLO youngberry.canonical.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1725941AbeJZGdr (ORCPT ); Fri, 26 Oct 2018 02:33:47 -0400 In-reply-to: <20181025210227.25544-1-3chas3@gmail.com> Content-ID: <25150.1540504752.1@famine> Sender: netdev-owner@vger.kernel.org List-ID: Chas Williams <3chas3@gmail.com> wrote: >netif_is_lag_port should be used to identify link aggregation ports. >For this to work, we need to reorganize the bonding and team drivers >so that the necessary flags are set before dev_open is called. > >commit 31e77c93e432 ("sched/fair: Update blocked load when newly idle") >made this decision originally based on the IFF_SLAVE flag which isn't >used by the team driver. Note, we do need to retain the IFF_SLAVE >check for the eql driver. Is 31e77c93e432 the correct commit reference? I don't see anything in there about IFF_SLAVE or bonding; it's a patch to the process scheduler. And, as Jiri said, the subject doesn't mention bonding. >Signed-off-by: Chas Williams <3chas3@gmail.com> >--- > drivers/net/bonding/bond_main.c | 4 ++-- > drivers/net/team/team.c | 7 +++++-- > net/ipv6/addrconf.c | 2 +- > 3 files changed, 8 insertions(+), 5 deletions(-) > >diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c >index ffa37adb7681..5cdad164332b 100644 >--- a/drivers/net/bonding/bond_main.c >+++ b/drivers/net/bonding/bond_main.c >@@ -1536,6 +1536,7 @@ int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev, > > /* set slave flag before open to prevent IPv6 addrconf */ > slave_dev->flags |= IFF_SLAVE; >+ slave_dev->priv_flags |= IFF_BONDING; > > /* open the slave since the application closed it */ > res = dev_open(slave_dev); >@@ -1544,7 +1545,6 @@ int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev, > goto err_restore_mac; > } > >- slave_dev->priv_flags |= IFF_BONDING; > /* initialize slave stats */ > dev_get_stats(new_slave->dev, &new_slave->slave_stats); > >@@ -1804,10 +1804,10 @@ int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev, > slave_disable_netpoll(new_slave); > > err_close: >- slave_dev->priv_flags &= ~IFF_BONDING; > dev_close(slave_dev); > > err_restore_mac: >+ slave_dev->priv_flags &= ~IFF_BONDING; > slave_dev->flags &= ~IFF_SLAVE; > if (!bond->params.fail_over_mac || > BOND_MODE(bond) != BOND_MODE_ACTIVEBACKUP) { >diff --git a/drivers/net/team/team.c b/drivers/net/team/team.c >index db633ae9f784..8fc7d57e9f6d 100644 >--- a/drivers/net/team/team.c >+++ b/drivers/net/team/team.c >@@ -1128,14 +1128,12 @@ static int team_upper_dev_link(struct team *team, struct team_port *port, > &lag_upper_info, extack); > if (err) > return err; >- port->dev->priv_flags |= IFF_TEAM_PORT; > return 0; > } > > static void team_upper_dev_unlink(struct team *team, struct team_port *port) > { > netdev_upper_dev_unlink(port->dev, team->dev); >- port->dev->priv_flags &= ~IFF_TEAM_PORT; > } > > static void __team_port_change_port_added(struct team_port *port, bool linkup); >@@ -1214,6 +1212,9 @@ static int team_port_add(struct team *team, struct net_device *port_dev, > goto err_port_enter; > } > >+ /* set slave flag before open to prevent IPv6 addrconf */ >+ port->dev->priv_flags |= IFF_TEAM_PORT; >+ > err = dev_open(port_dev); > if (err) { > netdev_dbg(dev, "Device %s opening failed\n", >@@ -1292,6 +1293,7 @@ static int team_port_add(struct team *team, struct net_device *port_dev, > dev_close(port_dev); > > err_dev_open: >+ port->dev->priv_flags &= ~IFF_TEAM_PORT; > team_port_leave(team, port); > team_port_set_orig_dev_addr(port); > >@@ -1328,6 +1330,7 @@ static int team_port_del(struct team *team, struct net_device *port_dev) > dev_uc_unsync(port_dev, dev); > dev_mc_unsync(port_dev, dev); > dev_close(port_dev); >+ port->dev->priv_flags &= ~IFF_TEAM_PORT; > team_port_leave(team, port); > > __team_option_inst_mark_removed_port(team, port); >diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c >index 45b84dd5c4eb..121f863022ed 100644 >--- a/net/ipv6/addrconf.c >+++ b/net/ipv6/addrconf.c >@@ -3482,7 +3482,7 @@ static int addrconf_notify(struct notifier_block *this, unsigned long event, > > case NETDEV_UP: > case NETDEV_CHANGE: >- if (dev->flags & IFF_SLAVE) >+ if (netif_is_lag_port(dev) || dev->flags & IFF_SLAVE) Note that netvsc_vf_join() also uses IFF_SLAVE in order skip IPv6 addrconf for netvsc devices; I don't believe its usage will pass netif_is_lag_port(). It looks like the above will work, but your commit message mentions eql as the reason for retaining the IFF_SLAVE test, and eql isn't the only user of IFF_SLAVE in this manner. -J > break; > > if (idev && idev->cnf.disable_ipv6) >-- >2.14.4 > --- -Jay Vosburgh, jay.vosburgh@canonical.com