On Thu, Mar 06, 2025 at 10:02:34AM +0000, Hangbin Liu wrote:
Set xs->xso.real_dev = NULL is a good idea. As we will break in bond_ipsec_del_sa()/bond_ipsec_free_sa() when there is no xs->xso.real_dev.
For bond_ipsec_add_sa_all(), I will move the xso.real_dev = real_dev after .xdo_dev_state_add() in case the following situation.
Hmm, do we still need to the spin_lock in bond_ipsec_add_sa_all()? With xs->xso.real_dev = NULL after bond_ipsec_del_sa_all(), it looks there is no need the spin_lock in bond_ipsec_add_sa_all(). e.g.
diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c index 04b677d0c45b..3ada51c63207 100644 --- a/drivers/net/bonding/bond_main.c +++ b/drivers/net/bonding/bond_main.c @@ -537,15 +537,27 @@ static void bond_ipsec_add_sa_all(struct bonding *bond) } list_for_each_entry(ipsec, &bond->ipsec_list, list) {
spin_lock_bh(&ipsec->xs->lock);/* Skip dead xfrm states, they'll be freed later. */if (ipsec->xs->km.state == XFRM_STATE_DEAD) {spin_unlock_bh(&ipsec->xs->lock);continue;}- /* If new state is added before ipsec_lock acquired */
if (ipsec->xs->xso.real_dev == real_dev)
if (ipsec->xs->xso.real_dev == real_dev) {spin_unlock_bh(&ipsec->xs->lock); continue;}
ipsec->xs->xso.real_dev = real_dev;
/* Set real_dev after .xdo_dev_state_add in case* __xfrm_state_delete() is called in parallel*/ipsec->xs->xso.real_dev = real_dev;
OK, please ignore this, the .xdo_dev_state_add() need xso.real_dev to be set first. Then I'm still wonder how to avoid the race before .xdo_dev_state_add() is called, e.g.
bond_ipsec_add_sa_all() spin_lock_bh(&ipsec->xs->lock); ipsec->xs->xso.real_dev = real_dev; spin_unlock(&ipsec->x->lock); __xfrm_state_delete - bond_ipsec_del_sa() - .xdo_dev_state_delete() - bond_ipsec_free_sa() - .xdo_dev_state_free() .xdo_dev_state_add()
Thanks Hangbin