RE: [PATCH net v2]bonding: check port and aggregator when select

quoted

Aichun Li [off-list ref] wrote:

quoted

When the network service is repeatedly restarted in 802.3ad, there is
a low  probability that oops occurs.
Test commands:systemctl restart network

1.crash: __enable_port():port->slave is NULL

[...]

quoted

    PC: ffff000000e2fcd0  [ad_agg_selection_logic+328]

[...]

quoted

2.I also have another call stack, same as in another person's post:
https://lore.kernel.org/netdev/52630cba-cc60-a024-8dd0-8319e5245044@ huawei.com

	What hardware platform are you using here?

	moyufeng [off-list ref] appears to be using the same platform,
and I've not had any success so far with the provided script to
reproduce the issue.  I'm using an x86_64 system, however, so I wonder
if perhaps this platform needs a barrier somewhere that x86 does not,
or there's something different in the timing of the device driver close logic.

	Yes, I'm using an arm64 system.
    And i'm different from moyufeng. I'm a physical machine, and he's a virtual
machine.

I'm sorry, I haven't heard from you in a while. I was wondering if you've reproduced this question?

quoted

quoted

Signed-off-by: Aichun Li <redacted>
---
drivers/net/bonding/bond_3ad.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)

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

b/drivers/net/bonding/bond_3ad.c index aa001b16765a..9c8894631bdd
100644

--- a/drivers/net/bonding/bond_3ad.c
+++ b/drivers/net/bonding/bond_3ad.c

@@ -183,7 +183,7 @@ static inline void __enable_port(struct port

*port) {
	struct slave *slave = port->slave;

-	if ((slave->link == BOND_LINK_UP) && bond_slave_is_up(slave))
+	if (slave && (slave->link == BOND_LINK_UP) &&
+bond_slave_is_up(slave))
		bond_set_slave_active_flags(slave, BOND_SLAVE_NOTIFY_LATER); }

	This change seems like a band aid to cover the real problem.
The caller of __enable_port is ad_agg_selection_logic, and it
shouldn't be possible for port->slave to be NULL when assigned to an

aggregator.

quoted

quoted

@@ -1516,6 +1516,7 @@ static void ad_port_selection_logic(struct port

*port, bool *update_slave_arr)

quoted

				  port->actor_port_number,
				  port->aggregator->aggregator_identifier);
		} else {
+			port->aggregator = &(SLAVE_AD_INFO(slave)->aggregator);
			slave_err(bond->dev, port->slave->dev,
				  "Port %d did not find a suitable aggregator\n",
				  port->actor_port_number);

	This change isn't correct; it's assigning the port to a more or less
random aggregator.  This would eliminate the panic, but isn't doing the right

thing.

quoted

At this point in the code, the selection logic has failed to find an
aggregator that matches, and also failed to find a free aggregator.

	I do need to fix up the failure handling here when it hits the "did
not find a suitable agg" case; the code here is simply wrong, and has
been wrong since the beginning.  I'll hack the driver to induce this
situation rather than reproducing whatever problem is making it unable
to find a suitable aggregator.

    Thank you for your reply and look forward to your solution.

quoted

	-J

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

---
	-Aichun Li [off-list ref]