Re: [PATCHv2 net 1/3] bonding: move mutex lock to a work queue for XFRM GC tasks

[Cosmin Ratiu <cratiu@nvidia.com>]

On Wed, 2025-02-26 at 09:48 +0000, Hangbin Liu wrote:
> 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:

This is what I had in mind, thanks for proposing it. Maybe you should
package it in a new submission with a proper title/etc.?
I'll do the initial review here.

> 
> 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.
> +		 */

Nit: Maybe rephrase that as "Skip dead xfrm states, they'll 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 this doesn't free dead entries now and bond_ipsec_add_sa_all is
called soon after, the pending bond_ipsec_free_sa() call will then hit
the WARN_ON(xs->xso.real_dev != real_dev) before attempting to call
free on the wrong device.
To fix that, these entries should be freed here and the WARN_ON in
bond_ipsec_free_sa() should be converted to an if...goto out, so that 
bond_ipsec_free_sa() calls would hit one of these conditions:
1. "if (!slave)", when no active device exists.
2. "if (!xs->xso.real_dev)", when xdo_dev_state_add() failed.
3. "if (xs->xso.real_dev != real_dev)", when a DEAD xs was already
freed by bond_ipsec_del_sa_all() migration to a new device.
In all 3 cases, xdo_dev_state_free() shouldn't be called, only xs
removed from the bond->ipsec list.

I hope I didn't miss any corner case.

>  		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);
>  }
>  
>  /**

Cosmin.