[PATCH net-next] net/ipv6: Block IPv6 addrconf on team ports

[PATCH net-next] net/ipv6: Block IPv6 addrconf on team ports

[Chas Williams]

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.

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)
 			break;
 
 		if (idev && idev->cnf.disable_ipv6)
-- 
2.14.4

Re: [PATCH net-next] net/ipv6: Block IPv6 addrconf on team ports

[Jiri Pirko]

Thu, Oct 25, 2018 at 11:02:27PM CEST, 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.
>
>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 +-

Subject talks about "team" yet you modify bond and team. Confusing..

Re: [PATCH net-next] net/ipv6: Block IPv6 addrconf on team ports

[Chas Williams]

On 10/25/2018 05:10 PM, Jiri Pirko wrote:
> Thu, Oct 25, 2018 at 11:02:27PM CEST, 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.
>>
>> 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 +-
> 
> Subject talks about "team" yet you modify bond and team. Confusing..

The subject discusses what I want to do. The body of the message
covers how I had to do it. The behavior of bonding with respect to
addrconf isn't changed but netif_is_lag_port is picky about the
flags it wants to see from bonding.  So some bonding changes are
necessary.

Re: [PATCH net-next] net/ipv6: Block IPv6 addrconf on team ports

[Jay Vosburgh]

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

Re: [PATCH net-next] net/ipv6: Block IPv6 addrconf on team ports

[Chas Williams]

On 10/25/2018 05:59 PM, Jay Vosburgh wrote:
> 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.

No, that's wrong.  It should be c2edacf80e155.

> 	And, as Jiri said, the subject doesn't mention bonding.

The behavior of bonding wasn't changed.  The intent of the patch
is to add team slaves to the interfaces that don't get automatic
IPv6 addresses.  The body discusses why bonding had to change as
well.

I was under the impression that the subject needs to kept short.
If there a better way to phrase what I want to do?

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

Re: [PATCH net-next] net/ipv6: Block IPv6 addrconf on team ports

[Jay Vosburgh]

Chas Williams <3chas3@gmail.com> wrote:

>On 10/25/2018 05:59 PM, Jay Vosburgh wrote:
>> 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.
>
>No, that's wrong.  It should be c2edacf80e155.
>
>> 	And, as Jiri said, the subject doesn't mention bonding.
>
>The behavior of bonding wasn't changed.  The intent of the patch
>is to add team slaves to the interfaces that don't get automatic
>IPv6 addresses.  The body discusses why bonding had to change as
>well.

	Sure, but the bonding code has changed, and the current
presentation makes it harder for reviewers to follow (or perhaps even
notice).

>I was under the impression that the subject needs to kept short.
>If there a better way to phrase what I want to do?

	I'd suggest splitting this into three patches: A first patch
that adds the new IPv6 functionality, then one patch each for team and
bonding to take advantage of that new functionality.  Each of the three
would then be very straightforward, change just one thing, and should be
clearer all around.

	-J

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

Re: [PATCH net-next] net/ipv6: Block IPv6 addrconf on team ports

[Jiri Pirko]

Fri, Oct 26, 2018 at 12:40:31AM CEST, jay.vosburgh@canonical.com wrote:
>Chas Williams <3chas3@gmail.com> wrote:
>
>>On 10/25/2018 05:59 PM, Jay Vosburgh wrote:
>>> 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.
>>
>>No, that's wrong.  It should be c2edacf80e155.
>>
>>> 	And, as Jiri said, the subject doesn't mention bonding.
>>
>>The behavior of bonding wasn't changed.  The intent of the patch
>>is to add team slaves to the interfaces that don't get automatic
>>IPv6 addresses.  The body discusses why bonding had to change as
>>well.
>
>	Sure, but the bonding code has changed, and the current
>presentation makes it harder for reviewers to follow (or perhaps even
>notice).
>
>>I was under the impression that the subject needs to kept short.
>>If there a better way to phrase what I want to do?
>
>	I'd suggest splitting this into three patches: A first patch
>that adds the new IPv6 functionality, then one patch each for team and
>bonding to take advantage of that new functionality.  Each of the three
>would then be very straightforward, change just one thing, and should be
>clearer all around.

+1