Hi Cosmin, On Tue, Feb 25, 2025 at 02:00:05PM +0000, Cosmin Ratiu wrote:
This got me to stare at the code again. What if we move the removal of the xs from bond->ipsec from bond_ipsec_del_sa to bond_ipsec_free_sa? bond_ipsec_free_sa, unlike bond_ipsec_del_sa, is not called with x-
lock held. It is called from the xfrm gc task or directly via
xfrm_state_put_sync and therefore wouldn't suffer from the locking issue.
The tricky part is to make sure that inactive bond->ipsec entries (after bond_ipsec_del_sa calls) do not cause issues if there's a migration (bond_ipsec_del_sa_all is called) happening before bond_ipsec_free_sa. Perhaps filtering by x->km.state != XFRM_STATE_DEAD in bond_ipsec_del_sa_all.
What do you think about this idea?
Thanks a lot for the comments. I also skipped the DEAD xs in add_sa_all. What about the patch like:
diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c index e45bba240cbc..0e4db43a833a 100644 --- a/drivers/net/bonding/bond_main.c +++ b/drivers/net/bonding/bond_main.c @@ -537,6 +537,12 @@ static void bond_ipsec_add_sa_all(struct bonding *bond) }
list_for_each_entry(ipsec, &bond->ipsec_list, list) { + /* No need to handle DEAD XFRM, as it has already been + * deleted and will be freed later. + */ + if (ipsec->xs->km.state == XFRM_STATE_DEAD) + continue; + /* If new state is added before ipsec_lock acquired */ if (ipsec->xs->xso.real_dev == real_dev) continue; @@ -592,15 +598,6 @@ static void bond_ipsec_del_sa(struct xfrm_state *xs) real_dev->xfrmdev_ops->xdo_dev_state_delete(xs); out: netdev_put(real_dev, &tracker); - mutex_lock(&bond->ipsec_lock); - list_for_each_entry(ipsec, &bond->ipsec_list, list) { - if (ipsec->xs == xs) { - list_del(&ipsec->list); - kfree(ipsec); - break; - } - } - mutex_unlock(&bond->ipsec_lock); }
static void bond_ipsec_del_sa_all(struct bonding *bond) @@ -617,6 +614,12 @@ static void bond_ipsec_del_sa_all(struct bonding *bond)
mutex_lock(&bond->ipsec_lock); list_for_each_entry(ipsec, &bond->ipsec_list, list) { + /* No need to handle DEAD XFRM, as it has already been + * deleted and will be freed later. + */ + if (ipsec->xs->km.state == XFRM_STATE_DEAD) + continue; + if (!ipsec->xs->xso.real_dev) continue;
@@ -666,6 +669,16 @@ static void bond_ipsec_free_sa(struct xfrm_state *xs) real_dev->xfrmdev_ops->xdo_dev_state_free(xs); out: netdev_put(real_dev, &tracker); + + mutex_lock(&bond->ipsec_lock); + list_for_each_entry(ipsec, &bond->ipsec_list, list) { + if (ipsec->xs == xs) { + list_del(&ipsec->list); + kfree(ipsec); + break; + } + } + mutex_unlock(&bond->ipsec_lock); }
/**