Re: [PATCH v2] bonding: 3ad: fix a crash in agg_device_up()

zhudi [off-list ref] wrote:

quoted

From: Di Zhu <redacted>

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.

	Presumably by "no active ports" you refer to the following block
near the end of bond_3ad_unbind_slave() (where a port being deleted is
removed from an aggregator):

				if (prev_port)
					prev_port->next_port_in_aggregator
= temp_port->next_port_in_aggregator;
				else
					temp_aggregator->lag_ports =
temp_port->next_port_in_aggregator;
				temp_aggregator->num_of_ports--;
				if (__agg_active_ports(temp_aggregator) ==
0) {
					select_new_active_agg =
temp_aggregator->is_active;
					ad_clear_agg(temp_aggregator);
					if (select_new_active_agg) {
						slave_info(bond->dev, slave-

quoted

dev, "Removing an active aggregator\n");

						/* select new active
aggregator */

	ad_agg_selection_logic(__get_first_agg(port),

&dummy_slave_update);

	Yes

	by what mechanism are you causing the ports within a particular
aggregator to all be inactive (port->is_enabled == false)?  Am I correct
in speculating that you're removing carrier from "port1" and "port3"
while simultaneously removing "port2" from the bond?  I believe this
would all need to occur within a single gap between passes of the state
machine logic.

	The logic in bond_3ad_handle_link_change() that sets
port->is_enabled = false happens on link state change, and is
immediately followed (under the same acquisition of bond->mode_lock) by
a call to ad_agg_selection_logic().  So, in principle, when the port is
set to inactive (is_enabled == false), it should be reselected to a
different aggregator (likely as an individual port) on the next run of
the state machine.

 	We have four physical network(enp3s0, enp4s0, enp5s0, enp6s0) cards 
bound to the bonding network card and test script looks like this:
	while [1]; do
	"echo 1 > /sys/bus/pci/devices/${pci_slot[0]}/remove" &
	"echo 1 > /sys/bus/pci/devices/${pci_slot[1]}/remove" &
        	"echo 1 > /sys/bus/pci/devices/${pci_slot[2]}/remove" &
        	"echo 1 > /sys/bus/pci/devices/${pci_slot[3]}/remove" &
       	 "echo 1 > /sys/bus/pci/rescan"
	done

According to dmesg,  the sequence of events is as follows:
	1）bond_mii_monitor()：print the status of the four physical network cards as link down
		hinic network card driver firstly call netif_carrier_off() changing the dev->state, 
		so mii thread can see state change.
	2）enp3s0 is unbind and release
		Then hinic network card driver call unregister_netdev()
		   ->call_netdevice_notifiers(NETDEV_UNREGISTER, dev);
		      -> bond_netdev_event()
		         -> __bond_release_one()->bond_3ad_unbind_slave()

		firstly, the link list look like this:
			Aggregator3->lag_ports = enp3s0->enp5s0-> enp4s0-> enp6s0
		Unbind process first selects a new agg4 and the link list look like this:
			Aggregator4->lag_ports = enp3s0->enp5s0-> enp4s0-> enp6s0
			Aggregator3->lag_ports = NULL
		Unbind process then run code block you described above, remove enp3s0 from port list and 
		call ad_clear_agg()(all ports are inactive) :
			Aggregator4->lag_ports = NULL
			enp5s0-> enp4s0-> enp6s0 (This patch is to break the linked list to ensure that no one can access it after deletion)
	3）enp4s0 is unbind and release
		The process is the same as enp3s0 and now the linked list looks like this:
			enp5s0-> enp4s0(has been deleted)-> enp6s0
	4）enp5s0 up
		enp5s0 network cards were reactivated(pci rescan operation) ,without completing the pci remove operation.
		State machine select the enp5s0 to join agg5(this is a new LAG), so the link list looks like this:
			Aggregator5->lag_ports = enp5s0-> enp4s0(has been deleted)-> enp6s0
	5）enp6s0 up
		The process is the same as enp6s0 and now the linked list looks like this:
			Aggregator5->lag_ports = enp5s0-> enp4s0(has been deleted)-> enp6s0
			Aggregator6->lag_ports = enp6s0 （joined new LAG）

		This is the final state that remain in vmcore and yes, enp6s0 exists in two linked lists at the same time.

	The above event sequence will cause the problem of patch description

quoted

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.

	As I understand the above, the bug causes a deleted (freed)
object to be left on the linked list (port1 -> port2 -> port3 in the
above).  I'm not quite sure how the code block above allows this to
happen, as the port being deleted is unlinked (at the top of the code
block).

	In any event, assuming that I'm missing something and it does
leave the freed port in the list, this fix may be the easiest, but I
don't believe it's correct, as it leaves opportunity for something else
to traverse the linked list and dereference the freed element.

	First, all port list operations are protected by bond->mode_lock
and in ad_clear_agg(), we break the link list immediately, It's guaranteed that
 no one will be able to access it after release.

	

	The code sample I included above occurs immediately after
removing a port from the aggregator's list of ports, and, in light of
your analysis (and assuming I've got the correct code block above), I
believe that the "__agg_active_ports(temp_aggregator) == 0" test should
really test to determine if there are no ports at all in the aggregator
(i.e., temp_aggregator->num_of_ports == 0), not only test for active
ports.  This way, if any ports remain in the aggregator, then
ad_clear_agg() would not be called, and the linked list would not end up
detached.

	There is a time gap between the change of link state and the
operation of state machine, during which unbinding events occurs, it will
see "__agg_active_ports(temp_aggregator) == 0" but temp_aggregator->num_of_ports != 0.

	In addition, the link state change does not necessarily cause the state machine to reselect the aggregator,
because ad_agg_selection_logic() not always possible to choose a best aggregator(such as, 
all NIC devices on bond are all in the down state)

	Comments?

	-J

quoted

Fixes: 0622cab0341c ("bonding: fix 802.3ad aggregator reselection")
Signed-off-by: Di Zhu <redacted>
---
/* v2 */
-David Miller [off-list ref]
-add Fixes: tag
---
drivers/net/bonding/bond_3ad.c | 7 +++++++
1 file changed, 7 insertions(+)

diff --git a/drivers/net/bonding/bond_3ad.c

b/drivers/net/bonding/bond_3ad.c

quoted

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,

quoted

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)

quoted

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

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