diff mbox series

bonding: 3ad: fix a crash in agg_device_up()

Message ID 20210602124448.49828-1-zhudi21@huawei.com (mailing list archive)
State Superseded
Delegated to: Netdev Maintainers
Headers show
Series bonding: 3ad: fix a crash in agg_device_up() | expand

Checks

Context Check Description
netdev/cover_letter success Link
netdev/fixes_present success Link
netdev/patch_count success Link
netdev/tree_selection success Guessed tree name to be net-next
netdev/subject_prefix warning Target tree name not specified in the subject
netdev/cc_maintainers warning 1 maintainers not CCed: andy@greyhouse.net
netdev/source_inline success Was 0 now: 0
netdev/verify_signedoff success Link
netdev/module_param success Was 0 now: 0
netdev/build_32bit success Errors and warnings before: 0 this patch: 0
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/verify_fixes success Link
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 19 lines checked
netdev/build_allmodconfig_warn success Errors and warnings before: 0 this patch: 0
netdev/header_inline success Link

Commit Message

zhudi (J) June 2, 2021, 12:44 p.m. UTC 
From: Di Zhu <zhudi21@huawei.com>

When doing the test of restarting the network card, the system is
broken because the port->slave is null pointer in agg_device_up().
After in-depth investigation, we found the real cause: in
bond_3ad_unbind_slave()  if there are no active ports in the
aggregator to be deleted, the ad_clear_agg() will be called to
set "aggregator->lag_ports = NULL", but the ports originally
belonging to the aggregator are still linked together.

Before bond_3ad_unbind_slave():
	aggregator4->lag_ports = port1->port2->port3
After bond_3ad_unbind_slave():
	aggregator4->lag_ports = NULL
	port1->port2->port3

After the port2 is deleted, the port is still  remain in the linked
list: because the port does not belong to any agg, so unbind do
nothing for this port.

After a while, bond_3ad_state_machine_handler() will run and
traverse each existing port, trying to bind each port to the
newly selected agg, such as:
	if (!found) {
		if (free_aggregator) {
			...
			port->aggregator->lag_ports = port;
			...
		}
	}
After this operation, the link list looks like this:
	 aggregator1->lag_ports = port1->port2(has been deleted)->port3

After that, just traverse the linked list of agg1 and access the
port2->slave, the crash will happen.

The easiest way to fix it is: if a port does not belong to any agg, delete
it from the list and wait for the state machine to select the agg again

Signed-off-by: Di Zhu <zhudi21@huawei.com>
---
 drivers/net/bonding/bond_3ad.c | 7 +++++++
 1 file changed, 7 insertions(+)

Comments

David Miller June 3, 2021, 10:12 p.m. UTC | #1 
Please prep[ost with an appropriate Fixes: tag, thanks.
diff mbox series

Patch

diff --git a/drivers/net/bonding/bond_3ad.c b/drivers/net/bonding/bond_3ad.c
index 6908822d9773..1d6ff4e1ed28 100644
--- a/drivers/net/bonding/bond_3ad.c
+++ b/drivers/net/bonding/bond_3ad.c
@@ -1793,6 +1793,8 @@  static void ad_agg_selection_logic(struct aggregator *agg,
 static void ad_clear_agg(struct aggregator *aggregator)
 {
 	if (aggregator) {
+		struct port *port, *next;
+
 		aggregator->is_individual = false;
 		aggregator->actor_admin_aggregator_key = 0;
 		aggregator->actor_oper_aggregator_key = 0;
@@ -1801,6 +1803,11 @@  static void ad_clear_agg(struct aggregator *aggregator)
 		aggregator->partner_oper_aggregator_key = 0;
 		aggregator->receive_state = 0;
 		aggregator->transmit_state = 0;
+		for (port = aggregator->lag_ports; port; port = next) {
+			next = port->next_port_in_aggregator;
+			if (port->aggregator == aggregator)
+				port->next_port_in_aggregator = NULL;
+		}
 		aggregator->lag_ports = NULL;
 		aggregator->is_active = 0;
 		aggregator->num_of_ports = 0;