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

[liaichun <liaichun@huawei.com>]

> > Aichun Li <liaichun@huawei.com> wrote:
> >
> > >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
> > [...]
> > >     PC: ffff000000e2fcd0  [ad_agg_selection_logic+328]
> > [...]
> > >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 <moyufeng@huawei.com> 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?

> >
> > >Signed-off-by: Aichun Li <liaichun@huawei.com>
> > >---
> > > 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.
> >
> > >@@ -1516,6 +1516,7 @@ static void ad_port_selection_logic(struct port
> > *port, bool *update_slave_arr)
> > > 				  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.
> > 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.
> >
> > 	-J
> >
> > ---
> > 	-Jay Vosburgh, jay.vosburgh@canonical.com
> 
> ---
> 	-Aichun Li <liaichun@huawei.com>