[PATCH net] bonding: Prevent IPv6 link local address on enslaved devices

[PATCH net] bonding: Prevent IPv6 link local address on enslaved devices

[Karl Heiss]

Upstream commit 1f718f0f4f97 ("bonding: populate neighbour's private on
enslave") undoes the fix provided by commit c2edacf80e15 ("bonding / ipv6: no
addrconf for slaves separately from master") by effectively setting
the slave flag after the slave has been opened.  If the slave comes up quickly
enough, it will go through the IPv6 addrconf before the slave flag has been
set and will get a link local IPv6 address.

Set IFF_SLAVE before dev_open() and clear it after dev_close() to ensure that
addrconf knows to ignore on state change.

Fixes: 1f718f0f4f97 ("bonding: populate neighbour's private on enslave")

Signed-off-by: Karl Heiss <kheiss@gmail.com>
---
 drivers/net/bonding/bond_main.c |    8 ++++++--
 1 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index 9e0f8a7..200358e 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -1207,7 +1207,6 @@ static int bond_master_upper_dev_link(struct net_device *bond_dev,
 	err = netdev_master_upper_dev_link_private(slave_dev, bond_dev, slave);
 	if (err)
 		return err;
-	slave_dev->flags |= IFF_SLAVE;
 	rtmsg_ifinfo(RTM_NEWLINK, slave_dev, IFF_SLAVE, GFP_KERNEL);
 	return 0;
 }
@@ -1216,7 +1215,6 @@ static void bond_upper_dev_unlink(struct net_device *bond_dev,
 				  struct net_device *slave_dev)
 {
 	netdev_upper_dev_unlink(slave_dev, bond_dev);
-	slave_dev->flags &= ~IFF_SLAVE;
 	rtmsg_ifinfo(RTM_NEWLINK, slave_dev, IFF_SLAVE, GFP_KERNEL);
 }
 
@@ -1465,6 +1463,9 @@ 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;
+
 	/* open the slave since the application closed it */
 	res = dev_open(slave_dev);
 	if (res) {
@@ -1725,6 +1726,7 @@ err_close:
 	dev_close(slave_dev);
 
 err_restore_mac:
+	slave_dev->flags &= ~IFF_SLAVE;
 	if (!bond->params.fail_over_mac ||
 	    BOND_MODE(bond) != BOND_MODE_ACTIVEBACKUP) {
 		/* XXX TODO - fom follow mode needs to change master's
@@ -1906,6 +1908,8 @@ static int __bond_release_one(struct net_device *bond_dev,
 	/* close slave before restoring its mac address */
 	dev_close(slave_dev);
 
+	slave_dev->flags &= ~IFF_SLAVE;
+
 	if (bond->params.fail_over_mac != BOND_FOM_ACTIVE ||
 	    BOND_MODE(bond) != BOND_MODE_ACTIVEBACKUP) {
 		/* restore original ("permanent") mac address */
-- 
1.7.1

Re: [PATCH net] bonding: Prevent IPv6 link local address on enslaved devices

[Jay Vosburgh]

Karl Heiss <kheiss@gmail.com> wrote:

>Upstream commit 1f718f0f4f97 ("bonding: populate neighbour's private on
>enslave") undoes the fix provided by commit c2edacf80e15 ("bonding / ipv6: no
>addrconf for slaves separately from master") by effectively setting
>the slave flag after the slave has been opened.  If the slave comes up quickly
>enough, it will go through the IPv6 addrconf before the slave flag has been
>set and will get a link local IPv6 address.
>
>Set IFF_SLAVE before dev_open() and clear it after dev_close() to ensure that
>addrconf knows to ignore on state change.

	I think prepending "During bonding enslavement and removal
processing," (or the equivalent) makes the above sentence a bit clearer
as to what's going on.

>Fixes: 1f718f0f4f97 ("bonding: populate neighbour's private on enslave")
>
>Signed-off-by: Karl Heiss <kheiss@gmail.com>
>---
> drivers/net/bonding/bond_main.c |    8 ++++++--
> 1 files changed, 6 insertions(+), 2 deletions(-)
>
>diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
>index 9e0f8a7..200358e 100644
>--- a/drivers/net/bonding/bond_main.c
>+++ b/drivers/net/bonding/bond_main.c
>@@ -1207,7 +1207,6 @@ static int bond_master_upper_dev_link(struct net_device *bond_dev,
> 	err = netdev_master_upper_dev_link_private(slave_dev, bond_dev, slave);
> 	if (err)
> 		return err;
>-	slave_dev->flags |= IFF_SLAVE;
> 	rtmsg_ifinfo(RTM_NEWLINK, slave_dev, IFF_SLAVE, GFP_KERNEL);
> 	return 0;
> }
>@@ -1216,7 +1215,6 @@ static void bond_upper_dev_unlink(struct net_device *bond_dev,
> 				  struct net_device *slave_dev)
> {
> 	netdev_upper_dev_unlink(slave_dev, bond_dev);
>-	slave_dev->flags &= ~IFF_SLAVE;
> 	rtmsg_ifinfo(RTM_NEWLINK, slave_dev, IFF_SLAVE, GFP_KERNEL);
> }

	Will this change cause issues for user space monitoring of the
RTM_NEWLINKs, as now the message will have IFF_SLAVE in the flags for
both the "link" and "unlink" cases?  How would link be distinguished
from unlink?

	Since the unlink happens only in __bond_release_one or in the
case of a failure within bond_enslave, does clearing the flag in
bond_upper_dev_unlink cause any actual issues?

	-J

>@@ -1465,6 +1463,9 @@ 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;
>+
> 	/* open the slave since the application closed it */
> 	res = dev_open(slave_dev);
> 	if (res) {
>@@ -1725,6 +1726,7 @@ err_close:
> 	dev_close(slave_dev);
> 
> err_restore_mac:
>+	slave_dev->flags &= ~IFF_SLAVE;
> 	if (!bond->params.fail_over_mac ||
> 	    BOND_MODE(bond) != BOND_MODE_ACTIVEBACKUP) {
> 		/* XXX TODO - fom follow mode needs to change master's
>@@ -1906,6 +1908,8 @@ static int __bond_release_one(struct net_device *bond_dev,
> 	/* close slave before restoring its mac address */
> 	dev_close(slave_dev);
> 
>+	slave_dev->flags &= ~IFF_SLAVE;
>+
> 	if (bond->params.fail_over_mac != BOND_FOM_ACTIVE ||
> 	    BOND_MODE(bond) != BOND_MODE_ACTIVEBACKUP) {
> 		/* restore original ("permanent") mac address */
>-- 
>1.7.1
>

---
	-Jay Vosburgh, jay.vosburgh@canonical.com

Re: [PATCH net] bonding: Prevent IPv6 link local address on enslaved devices

[Karl Heiss]

On Fri, Jan 8, 2016 at 2:56 PM, Jay Vosburgh <jay.vosburgh@canonical.com> wrote:
> Karl Heiss <kheiss@gmail.com> wrote:
>
>>Upstream commit 1f718f0f4f97 ("bonding: populate neighbour's private on
>>enslave") undoes the fix provided by commit c2edacf80e15 ("bonding / ipv6: no
>>addrconf for slaves separately from master") by effectively setting
>>the slave flag after the slave has been opened.  If the slave comes up quickly
>>enough, it will go through the IPv6 addrconf before the slave flag has been
>>set and will get a link local IPv6 address.
>>
>>Set IFF_SLAVE before dev_open() and clear it after dev_close() to ensure that
>>addrconf knows to ignore on state change.
>
>         I think prepending "During bonding enslavement and removal
> processing," (or the equivalent) makes the above sentence a bit clearer
> as to what's going on.
>

Agreed

>>Fixes: 1f718f0f4f97 ("bonding: populate neighbour's private on enslave")
>>
>>Signed-off-by: Karl Heiss <kheiss@gmail.com>
>>---
>> drivers/net/bonding/bond_main.c |    8 ++++++--
>> 1 files changed, 6 insertions(+), 2 deletions(-)
>>
>>diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
>>index 9e0f8a7..200358e 100644
>>--- a/drivers/net/bonding/bond_main.c
>>+++ b/drivers/net/bonding/bond_main.c
>>@@ -1207,7 +1207,6 @@ static int bond_master_upper_dev_link(struct net_device *bond_dev,
>>       err = netdev_master_upper_dev_link_private(slave_dev, bond_dev, slave);
>>       if (err)
>>               return err;
>>-      slave_dev->flags |= IFF_SLAVE;
>>       rtmsg_ifinfo(RTM_NEWLINK, slave_dev, IFF_SLAVE, GFP_KERNEL);
>>       return 0;
>> }
>>@@ -1216,7 +1215,6 @@ static void bond_upper_dev_unlink(struct net_device *bond_dev,
>>                                 struct net_device *slave_dev)
>> {
>>       netdev_upper_dev_unlink(slave_dev, bond_dev);
>>-      slave_dev->flags &= ~IFF_SLAVE;
>>       rtmsg_ifinfo(RTM_NEWLINK, slave_dev, IFF_SLAVE, GFP_KERNEL);
>> }
>
>         Will this change cause issues for user space monitoring of the
> RTM_NEWLINKs, as now the message will have IFF_SLAVE in the flags for
> both the "link" and "unlink" cases?  How would link be distinguished
> from unlink?
>
>         Since the unlink happens only in __bond_release_one or in the
> case of a failure within bond_enslave, does clearing the flag in
> bond_upper_dev_unlink cause any actual issues?
>
>         -J
>

Oops.  You are correct that the RTM_NEWLINK would appear to be identical to
the link case.  I had originally done this to prevent any NETDEV_CHANGE events
from causing the link local address and subsequent neighbor advertisements just
as the device is unlinked.  However, the bond_upper_dev_unlink() changes were a
result of speculation, not actual observation.

If we feel that we are safe from any NETDEV_CHANGE events and/or the
consequences during unlink, I am fine with leaving the bond_upper_dev_unlink()
code as-is.

Karl

(P.S.: Sorry for the resend, Jay)

>>@@ -1465,6 +1463,9 @@ 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;
>>+
>>       /* open the slave since the application closed it */
>>       res = dev_open(slave_dev);
>>       if (res) {
>>@@ -1725,6 +1726,7 @@ err_close:
>>       dev_close(slave_dev);
>>
>> err_restore_mac:
>>+      slave_dev->flags &= ~IFF_SLAVE;
>>       if (!bond->params.fail_over_mac ||
>>           BOND_MODE(bond) != BOND_MODE_ACTIVEBACKUP) {
>>               /* XXX TODO - fom follow mode needs to change master's
>>@@ -1906,6 +1908,8 @@ static int __bond_release_one(struct net_device *bond_dev,
>>       /* close slave before restoring its mac address */
>>       dev_close(slave_dev);
>>
>>+      slave_dev->flags &= ~IFF_SLAVE;
>>+
>>       if (bond->params.fail_over_mac != BOND_FOM_ACTIVE ||
>>           BOND_MODE(bond) != BOND_MODE_ACTIVEBACKUP) {
>>               /* restore original ("permanent") mac address */
>>--
>>1.7.1
>>
>
> ---
>         -Jay Vosburgh, jay.vosburgh@canonical.com

Re: [PATCH net] bonding: Prevent IPv6 link local address on enslaved devices

[Jay Vosburgh]

Karl Heiss <kheiss@gmail.com> wrote:

>On Fri, Jan 8, 2016 at 2:56 PM, Jay Vosburgh <jay.vosburgh@canonical.com> wrote:
>> Karl Heiss <kheiss@gmail.com> wrote:
[...]
>>>@@ -1216,7 +1215,6 @@ static void bond_upper_dev_unlink(struct net_device *bond_dev,
>>>                                 struct net_device *slave_dev)
>>> {
>>>       netdev_upper_dev_unlink(slave_dev, bond_dev);
>>>-      slave_dev->flags &= ~IFF_SLAVE;
>>>       rtmsg_ifinfo(RTM_NEWLINK, slave_dev, IFF_SLAVE, GFP_KERNEL);
>>> }
>>
>>         Will this change cause issues for user space monitoring of the
>> RTM_NEWLINKs, as now the message will have IFF_SLAVE in the flags for
>> both the "link" and "unlink" cases?  How would link be distinguished
>> from unlink?
>>
>>         Since the unlink happens only in __bond_release_one or in the
>> case of a failure within bond_enslave, does clearing the flag in
>> bond_upper_dev_unlink cause any actual issues?
>>
>>         -J
>>
>
>Oops.  You are correct that the RTM_NEWLINK would appear to be identical to
>the link case.  I had originally done this to prevent any NETDEV_CHANGE events
>from causing the link local address and subsequent neighbor advertisements just
>as the device is unlinked.  However, the bond_upper_dev_unlink() changes were a
>result of speculation, not actual observation.
>
>If we feel that we are safe from any NETDEV_CHANGE events and/or the
>consequences during unlink, I am fine with leaving the bond_upper_dev_unlink()
>code as-is.

	I looked briefly, and I don't see a source of NETDEV_CHANGE
notifiers between the bond_upper_dev_unlink and dev_close calls in
__bond_release_one.  Note that dev_set_promiscuity / allmulti do end up
in __dev_notify_flags, but it excludes NETDEV_CHANGE for PROMISC and
ALLMULTI, so I think that's not an issue.

	-J

---
	-Jay Vosburgh, jay.vosburgh@canonical.com