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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <25151.1540504752@famine>
Date:   Thu, 25 Oct 2018 14:59:12 -0700
From:   Jay Vosburgh <jay.vosburgh@...onical.com>
To:     Chas Williams <3chas3@...il.com>
cc:     davem@...emloft.net, netdev@...r.kernel.org, vfalico@...il.com,
        andy@...yhouse.net, jiri@...nulli.us, kuznet@....inr.ac.ru,
        yoshfuji@...ux-ipv6.org
Subject: Re: [PATCH net-next] net/ipv6: Block IPv6 addrconf on team ports

Chas Williams <3chas3@...il.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@...il.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@...onical.com