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

From: "zhudi (J)" <[email-masked]>
Date: Sat, 5 Jun 2021 08:56:50 +0000

> zhudi <[email-masked]> wrote:
> 
> >From: Di Zhu <[email-masked]>
> >
> >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-
> >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

> >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
> 
> >Fixes: 0622cab0341c ("bonding: fix 802.3ad aggregator reselection")
> >Signed-off-by: Di Zhu <[email-masked]>
> >---
> >/* v2 */
> >-David Miller <[email-masked]>
> > -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
> >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;
> >--
> >2.23.0
> >
> 
> ---
> 	-Jay Vosburgh, [email-masked]