On Fri, Mar 07, 2025 at 03:19:01AM +0000, Hangbin Liu wrote:
...
> @@ -616,9 +615,22 @@ static void bond_ipsec_del_sa_all(struct bonding *bond)
> return;
>
> mutex_lock(&bond->ipsec_lock);
> - list_for_each_entry(ipsec, &bond->ipsec_list, list) {
> - if (!ipsec->xs->xso.real_dev)
> + list_for_each_entry_safe(ipsec, tmp_ipsec, &bond->ipsec_list, list) {
> + spin_lock_bh(&ipsec->xs->lock);
> + if (!ipsec->xs->xso.real_dev) {
> + spin_unlock_bh(&ipsec->xs->lock);
> continue;
> + }
> +
> + if (ipsec->xs->km.state == XFRM_STATE_DEAD) {
> + list_del(&ipsec->list);
> + kfree(ipsec);
Hi Hangbin,
Apologies if this was covered elsewhere, but ipsec is kfree'd here...
> + /* Need to free device here, or the xs->xso.real_dev
> + * may changed in bond_ipsec_add_sa_all and free
> + * on old device will never be called.
> + */
> + goto next;
> + }
>
> if (!real_dev->xfrmdev_ops ||
> !real_dev->xfrmdev_ops->xdo_dev_state_delete ||
> @@ -626,11 +638,20 @@ static void bond_ipsec_del_sa_all(struct bonding *bond)
> slave_warn(bond_dev, real_dev,
> "%s: no slave xdo_dev_state_delete\n",
> __func__);
> - } else {
> - real_dev->xfrmdev_ops->xdo_dev_state_delete(ipsec->xs);
> - if (real_dev->xfrmdev_ops->xdo_dev_state_free)
> - real_dev->xfrmdev_ops->xdo_dev_state_free(ipsec->xs);
> + spin_unlock_bh(&ipsec->xs->lock);
> + continue;
> }
> +
> + real_dev->xfrmdev_ops->xdo_dev_state_delete(ipsec->xs);
> +
> +next:
> + /* set real_dev to NULL in case __xfrm_state_delete() is called in parallel */
> + ipsec->xs->xso.real_dev = NULL;
... and the dereferenced here.
Flagged by Smatch.
> +
> + /* Unlock before freeing device state, it could sleep. */
> + spin_unlock_bh(&ipsec->xs->lock);
> + if (real_dev->xfrmdev_ops->xdo_dev_state_free)
> + real_dev->xfrmdev_ops->xdo_dev_state_free(ipsec->xs);
> }
> mutex_unlock(&bond->ipsec_lock);
> }
...
