Skip to content

Commit

Permalink
Merge branch 'net-unsync-addresses-from-ports'
Browse files Browse the repository at this point in the history
From: Benjamin Poirier <[email protected]>
To: [email protected]
Cc: Jay Vosburgh <[email protected]>,
	Veaceslav Falico <[email protected]>,
	Andy Gospodarek <[email protected]>,
	"David S. Miller" <[email protected]>,
	Eric Dumazet <[email protected]>,
	Jakub Kicinski <[email protected]>, Paolo Abeni <[email protected]>,
	Jiri Pirko <[email protected]>, Shuah Khan <[email protected]>,
	Jonathan Toppins <[email protected]>,
	[email protected]
Subject: [PATCH net v3 0/4] Unsync addresses from ports when stopping aggregated devices
Date: Wed,  7 Sep 2022 16:56:38 +0900	[thread overview]
Message-ID: <[email protected]> (raw)

This series fixes similar problems in the bonding and team drivers.

Because of missing dev_{uc,mc}_unsync() calls, addresses added to
underlying devices may be leftover after the aggregated device is deleted.
Add the missing calls and a few related tests.

v2:
* fix selftest installation, see patch 3

v3:
* Split lacpdu_multicast changes to their own patch, #1
* In ndo_{add,del}_slave methods, only perform address list changes when
  the aggregated device is up (patches 2 & 3)
* Add selftest function related to the above change (patch 4)
====================

Acked-by: Jay Vosburgh <[email protected]>
Signed-off-by: David S. Miller <[email protected]>
  • Loading branch information
davem330 committed Sep 16, 2022
2 parents 21be1ad + bbb774d commit 34d2d33
Show file tree
Hide file tree
Showing 14 changed files with 297 additions and 32 deletions.
1 change: 1 addition & 0 deletions MAINTAINERS
Original file line number Diff line number Diff line change
Expand Up @@ -19948,6 +19948,7 @@ S: Supported
F: drivers/net/team/
F: include/linux/if_team.h
F: include/uapi/linux/if_team.h
F: tools/testing/selftests/net/team/

TECHNOLOGIC SYSTEMS TS-5500 PLATFORM SUPPORT
M: "Savoir-faire Linux Inc." <[email protected]>
Expand Down
5 changes: 3 additions & 2 deletions drivers/net/bonding/bond_3ad.c
Original file line number Diff line number Diff line change
Expand Up @@ -88,8 +88,9 @@ static const u8 null_mac_addr[ETH_ALEN + 2] __long_aligned = {
static const u16 ad_ticks_per_sec = 1000 / AD_TIMER_INTERVAL;
static const int ad_delta_in_ticks = (AD_TIMER_INTERVAL * HZ) / 1000;

static const u8 lacpdu_mcast_addr[ETH_ALEN + 2] __long_aligned =
MULTICAST_LACPDU_ADDR;
const u8 lacpdu_mcast_addr[ETH_ALEN + 2] __long_aligned = {
0x01, 0x80, 0xC2, 0x00, 0x00, 0x02
};

/* ================= main 802.3ad protocol functions ================== */
static int ad_lacpdu_send(struct port *port);
Expand Down
57 changes: 36 additions & 21 deletions drivers/net/bonding/bond_main.c
Original file line number Diff line number Diff line change
Expand Up @@ -865,12 +865,8 @@ static void bond_hw_addr_flush(struct net_device *bond_dev,
dev_uc_unsync(slave_dev, bond_dev);
dev_mc_unsync(slave_dev, bond_dev);

if (BOND_MODE(bond) == BOND_MODE_8023AD) {
/* del lacpdu mc addr from mc list */
u8 lacpdu_multicast[ETH_ALEN] = MULTICAST_LACPDU_ADDR;

dev_mc_del(slave_dev, lacpdu_multicast);
}
if (BOND_MODE(bond) == BOND_MODE_8023AD)
dev_mc_del(slave_dev, lacpdu_mcast_addr);
}

/*--------------------------- Active slave change ---------------------------*/
Expand All @@ -890,7 +886,8 @@ static void bond_hw_addr_swap(struct bonding *bond, struct slave *new_active,
if (bond->dev->flags & IFF_ALLMULTI)
dev_set_allmulti(old_active->dev, -1);

bond_hw_addr_flush(bond->dev, old_active->dev);
if (bond->dev->flags & IFF_UP)
bond_hw_addr_flush(bond->dev, old_active->dev);
}

if (new_active) {
Expand All @@ -901,10 +898,12 @@ static void bond_hw_addr_swap(struct bonding *bond, struct slave *new_active,
if (bond->dev->flags & IFF_ALLMULTI)
dev_set_allmulti(new_active->dev, 1);

netif_addr_lock_bh(bond->dev);
dev_uc_sync(new_active->dev, bond->dev);
dev_mc_sync(new_active->dev, bond->dev);
netif_addr_unlock_bh(bond->dev);
if (bond->dev->flags & IFF_UP) {
netif_addr_lock_bh(bond->dev);
dev_uc_sync(new_active->dev, bond->dev);
dev_mc_sync(new_active->dev, bond->dev);
netif_addr_unlock_bh(bond->dev);
}
}
}

Expand Down Expand Up @@ -2166,16 +2165,14 @@ int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev,
}
}

netif_addr_lock_bh(bond_dev);
dev_mc_sync_multiple(slave_dev, bond_dev);
dev_uc_sync_multiple(slave_dev, bond_dev);
netif_addr_unlock_bh(bond_dev);
if (bond_dev->flags & IFF_UP) {
netif_addr_lock_bh(bond_dev);
dev_mc_sync_multiple(slave_dev, bond_dev);
dev_uc_sync_multiple(slave_dev, bond_dev);
netif_addr_unlock_bh(bond_dev);

if (BOND_MODE(bond) == BOND_MODE_8023AD) {
/* add lacpdu mc addr to mc list */
u8 lacpdu_multicast[ETH_ALEN] = MULTICAST_LACPDU_ADDR;

dev_mc_add(slave_dev, lacpdu_multicast);
if (BOND_MODE(bond) == BOND_MODE_8023AD)
dev_mc_add(slave_dev, lacpdu_mcast_addr);
}
}

Expand Down Expand Up @@ -2447,7 +2444,8 @@ static int __bond_release_one(struct net_device *bond_dev,
if (old_flags & IFF_ALLMULTI)
dev_set_allmulti(slave_dev, -1);

bond_hw_addr_flush(bond_dev, slave_dev);
if (old_flags & IFF_UP)
bond_hw_addr_flush(bond_dev, slave_dev);
}

slave_disable_netpoll(slave);
Expand Down Expand Up @@ -4221,6 +4219,9 @@ static int bond_open(struct net_device *bond_dev)
/* register to receive LACPDUs */
bond->recv_probe = bond_3ad_lacpdu_recv;
bond_3ad_initiate_agg_selection(bond, 1);

bond_for_each_slave(bond, slave, iter)
dev_mc_add(slave->dev, lacpdu_mcast_addr);
}

if (bond_mode_can_use_xmit_hash(bond))
Expand All @@ -4232,13 +4233,27 @@ static int bond_open(struct net_device *bond_dev)
static int bond_close(struct net_device *bond_dev)
{
struct bonding *bond = netdev_priv(bond_dev);
struct slave *slave;

bond_work_cancel_all(bond);
bond->send_peer_notif = 0;
if (bond_is_lb(bond))
bond_alb_deinitialize(bond);
bond->recv_probe = NULL;

if (bond_uses_primary(bond)) {
rcu_read_lock();
slave = rcu_dereference(bond->curr_active_slave);
if (slave)
bond_hw_addr_flush(bond_dev, slave->dev);
rcu_read_unlock();
} else {
struct list_head *iter;

bond_for_each_slave(bond, slave, iter)
bond_hw_addr_flush(bond_dev, slave->dev);
}

return 0;
}

Expand Down
24 changes: 18 additions & 6 deletions drivers/net/team/team.c
Original file line number Diff line number Diff line change
Expand Up @@ -1275,10 +1275,12 @@ static int team_port_add(struct team *team, struct net_device *port_dev,
}
}

netif_addr_lock_bh(dev);
dev_uc_sync_multiple(port_dev, dev);
dev_mc_sync_multiple(port_dev, dev);
netif_addr_unlock_bh(dev);
if (dev->flags & IFF_UP) {
netif_addr_lock_bh(dev);
dev_uc_sync_multiple(port_dev, dev);
dev_mc_sync_multiple(port_dev, dev);
netif_addr_unlock_bh(dev);
}

port->index = -1;
list_add_tail_rcu(&port->list, &team->port_list);
Expand Down Expand Up @@ -1349,8 +1351,10 @@ static int team_port_del(struct team *team, struct net_device *port_dev)
netdev_rx_handler_unregister(port_dev);
team_port_disable_netpoll(port);
vlan_vids_del_by_dev(port_dev, dev);
dev_uc_unsync(port_dev, dev);
dev_mc_unsync(port_dev, dev);
if (dev->flags & IFF_UP) {
dev_uc_unsync(port_dev, dev);
dev_mc_unsync(port_dev, dev);
}
dev_close(port_dev);
team_port_leave(team, port);

Expand Down Expand Up @@ -1700,6 +1704,14 @@ static int team_open(struct net_device *dev)

static int team_close(struct net_device *dev)
{
struct team *team = netdev_priv(dev);
struct team_port *port;

list_for_each_entry(port, &team->port_list, list) {
dev_uc_unsync(port->dev, dev);
dev_mc_unsync(port->dev, dev);
}

return 0;
}

Expand Down
2 changes: 0 additions & 2 deletions include/net/bond_3ad.h
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,6 @@
#define PKT_TYPE_LACPDU cpu_to_be16(ETH_P_SLOW)
#define AD_TIMER_INTERVAL 100 /*msec*/

#define MULTICAST_LACPDU_ADDR {0x01, 0x80, 0xC2, 0x00, 0x00, 0x02}

#define AD_LACP_SLOW 0
#define AD_LACP_FAST 1

Expand Down
3 changes: 3 additions & 0 deletions include/net/bonding.h
Original file line number Diff line number Diff line change
Expand Up @@ -786,6 +786,9 @@ extern struct rtnl_link_ops bond_link_ops;
/* exported from bond_sysfs_slave.c */
extern const struct sysfs_ops slave_sysfs_ops;

/* exported from bond_3ad.c */
extern const u8 lacpdu_mcast_addr[];

static inline netdev_tx_t bond_tx_drop(struct net_device *dev, struct sk_buff *skb)
{
dev_core_stats_tx_dropped_inc(dev);
Expand Down
1 change: 1 addition & 0 deletions tools/testing/selftests/Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ TARGETS += damon
TARGETS += drivers/dma-buf
TARGETS += drivers/s390x/uvdevice
TARGETS += drivers/net/bonding
TARGETS += drivers/net/team
TARGETS += efivarfs
TARGETS += exec
TARGETS += filesystems
Expand Down
5 changes: 4 additions & 1 deletion tools/testing/selftests/drivers/net/bonding/Makefile
Original file line number Diff line number Diff line change
@@ -1,6 +1,9 @@
# SPDX-License-Identifier: GPL-2.0
# Makefile for net selftests

TEST_PROGS := bond-break-lacpdu-tx.sh
TEST_PROGS := bond-break-lacpdu-tx.sh \
dev_addr_lists.sh

TEST_FILES := lag_lib.sh

include ../../../lib.mk
1 change: 1 addition & 0 deletions tools/testing/selftests/drivers/net/bonding/config
Original file line number Diff line number Diff line change
@@ -1 +1,2 @@
CONFIG_BONDING=y
CONFIG_MACVLAN=y
109 changes: 109 additions & 0 deletions tools/testing/selftests/drivers/net/bonding/dev_addr_lists.sh
Original file line number Diff line number Diff line change
@@ -0,0 +1,109 @@
#!/bin/bash
# SPDX-License-Identifier: GPL-2.0
#
# Test bond device handling of addr lists (dev->uc, mc)
#

ALL_TESTS="
bond_cleanup_mode1
bond_cleanup_mode4
bond_listen_lacpdu_multicast_case_down
bond_listen_lacpdu_multicast_case_up
"

REQUIRE_MZ=no
NUM_NETIFS=0
lib_dir=$(dirname "$0")
source "$lib_dir"/../../../net/forwarding/lib.sh

source "$lib_dir"/lag_lib.sh


destroy()
{
local ifnames=(dummy1 dummy2 bond1 mv0)
local ifname

for ifname in "${ifnames[@]}"; do
ip link del "$ifname" &>/dev/null
done
}

cleanup()
{
pre_cleanup

destroy
}


# bond driver control paths vary between modes that have a primary slave
# (bond_uses_primary()) and others. Test both kinds of modes.

bond_cleanup_mode1()
{
RET=0

test_LAG_cleanup "bonding" "active-backup"
}

bond_cleanup_mode4() {
RET=0

test_LAG_cleanup "bonding" "802.3ad"
}

bond_listen_lacpdu_multicast()
{
# Initial state of bond device, up | down
local init_state=$1
local lacpdu_mc="01:80:c2:00:00:02"

ip link add dummy1 type dummy
ip link add bond1 "$init_state" type bond mode 802.3ad
ip link set dev dummy1 master bond1
if [ "$init_state" = "down" ]; then
ip link set dev bond1 up
fi

grep_bridge_fdb "$lacpdu_mc" bridge fdb show brport dummy1 >/dev/null
check_err $? "LACPDU multicast address not present on slave (1)"

ip link set dev bond1 down

not grep_bridge_fdb "$lacpdu_mc" bridge fdb show brport dummy1 >/dev/null
check_err $? "LACPDU multicast address still present on slave"

ip link set dev bond1 up

grep_bridge_fdb "$lacpdu_mc" bridge fdb show brport dummy1 >/dev/null
check_err $? "LACPDU multicast address not present on slave (2)"

cleanup

log_test "bonding LACPDU multicast address to slave (from bond $init_state)"
}

# The LACPDU mc addr is added by different paths depending on the initial state
# of the bond when enslaving a device. Test both cases.

bond_listen_lacpdu_multicast_case_down()
{
RET=0

bond_listen_lacpdu_multicast "down"
}

bond_listen_lacpdu_multicast_case_up()
{
RET=0

bond_listen_lacpdu_multicast "up"
}


trap cleanup EXIT

tests_run

exit "$EXIT_STATUS"
61 changes: 61 additions & 0 deletions tools/testing/selftests/drivers/net/bonding/lag_lib.sh
Original file line number Diff line number Diff line change
@@ -0,0 +1,61 @@
#!/bin/bash
# SPDX-License-Identifier: GPL-2.0

# Test that a link aggregation device (bonding, team) removes the hardware
# addresses that it adds on its underlying devices.
test_LAG_cleanup()
{
local driver=$1
local mode=$2
local ucaddr="02:00:00:12:34:56"
local addr6="fe80::78:9abc/64"
local mcaddr="33:33:ff:78:9a:bc"
local name

ip link add dummy1 type dummy
ip link add dummy2 type dummy
if [ "$driver" = "bonding" ]; then
name="bond1"
ip link add "$name" up type bond mode "$mode"
ip link set dev dummy1 master "$name"
ip link set dev dummy2 master "$name"
elif [ "$driver" = "team" ]; then
name="team0"
teamd -d -c '
{
"device": "'"$name"'",
"runner": {
"name": "'"$mode"'"
},
"ports": {
"dummy1":
{},
"dummy2":
{}
}
}
'
ip link set dev "$name" up
else
check_err 1
log_test test_LAG_cleanup ": unknown driver \"$driver\""
return
fi

# Used to test dev->uc handling
ip link add mv0 link "$name" up address "$ucaddr" type macvlan
# Used to test dev->mc handling
ip address add "$addr6" dev "$name"
ip link set dev "$name" down
ip link del "$name"

not grep_bridge_fdb "$ucaddr" bridge fdb show >/dev/null
check_err $? "macvlan unicast address still present on a slave"

not grep_bridge_fdb "$mcaddr" bridge fdb show >/dev/null
check_err $? "IPv6 solicited-node multicast mac address still present on a slave"

cleanup

log_test "$driver cleanup mode $mode"
}
6 changes: 6 additions & 0 deletions tools/testing/selftests/drivers/net/team/Makefile
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
# SPDX-License-Identifier: GPL-2.0
# Makefile for net selftests

TEST_PROGS := dev_addr_lists.sh

include ../../../lib.mk
Loading

0 comments on commit 34d2d33

Please sign in to comment.