On 3/7/25 05:19, Hangbin Liu wrote:
> The fixed commit placed mutex_lock() inside spin_lock_bh(), which triggers
> a warning:
>
> BUG: sleeping function called from invalid context at...
>
> Fix this by moving the IPsec deletion operation to bond_ipsec_free_sa,
> which is not held by spin_lock_bh().
>
> Additionally, there are also some race conditions as bond_ipsec_del_sa_all()
> and __xfrm_state_delete could running in parallel without any lock.
> e.g.
>
> bond_ipsec_del_sa_all() __xfrm_state_delete()
> - .xdo_dev_state_delete - bond_ipsec_del_sa()
> - .xdo_dev_state_free - .xdo_dev_state_delete()
> - bond_ipsec_free_sa()
> bond active_slave changes - .xdo_dev_state_free()
>
> bond_ipsec_add_sa_all()
> - ipsec->xs->xso.real_dev = real_dev;
> - xdo_dev_state_add
>
> To fix this, let's add xs->lock during bond_ipsec_del_sa_all(), and delete
> the IPsec list when the XFRM state is DEAD, which could prevent
> xdo_dev_state_free() from being triggered again in bond_ipsec_free_sa().
>
> In bond_ipsec_add_sa(), if .xdo_dev_state_add() failed, the xso.real_dev
> is set without clean. Which will cause trouble if __xfrm_state_delete is
> called at the same time. Reset the xso.real_dev to NULL if state add failed.
>
> Despite the above fixes, there are still races in bond_ipsec_add_sa()
> and bond_ipsec_add_sa_all(). If __xfrm_state_delete() is called immediately
> after we set the xso.real_dev and before .xdo_dev_state_add() is finished,
> like
>
> ipsec->xs->xso.real_dev = real_dev;
> __xfrm_state_delete
> - bond_ipsec_del_sa()
> - .xdo_dev_state_delete()
> - bond_ipsec_free_sa()
> - .xdo_dev_state_free()
> .xdo_dev_state_add()
>
> But there is no good solution yet. So I just added a FIXME note in here
> and hope we can fix it in future.
>
> Fixes: 2aeeef906d5a ("bonding: change ipsec_lock from spin lock to mutex")
> Reported-by: Jakub Kicinski <k...@kernel.org>
> Closes: https://lore.kernel.org/netdev/20241212062734.182a0...@kernel.org
> Suggested-by: Cosmin Ratiu <cra...@nvidia.com>
> Signed-off-by: Hangbin Liu <liuhang...@gmail.com>
> ---
> drivers/net/bonding/bond_main.c | 69 ++++++++++++++++++++++++---------
> 1 file changed, 51 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
> index e45bba240cbc..dd3d0d41d98f 100644
> --- a/drivers/net/bonding/bond_main.c
> +++ b/drivers/net/bonding/bond_main.c
> @@ -506,6 +506,7 @@ static int bond_ipsec_add_sa(struct xfrm_state *xs,
> list_add(&ipsec->list, &bond->ipsec_list);
> mutex_unlock(&bond->ipsec_lock);
> } else {
> + xs->xso.real_dev = NULL;
> kfree(ipsec);
> }
> out:
> @@ -541,7 +542,15 @@ static void bond_ipsec_add_sa_all(struct bonding *bond)
> if (ipsec->xs->xso.real_dev == real_dev)
> continue;
>
> + /* Skip dead xfrm states, they'll be freed later. */
> + if (ipsec->xs->km.state == XFRM_STATE_DEAD)
> + continue;
As we commented earlier, reading this state without x->lock is wrong.
> +
> ipsec->xs->xso.real_dev = real_dev;
> + /* FIXME: there is a race that before .xdo_dev_state_add()
> + * is called, the __xfrm_state_delete() is called in parallel,
> + * which will call .xdo_dev_state_delete() and
> xdo_dev_state_free()
> + */
> if (real_dev->xfrmdev_ops->xdo_dev_state_add(ipsec->xs, NULL)) {
> slave_warn(bond_dev, real_dev, "%s: failed to add
> SA\n", __func__);
> ipsec->xs->xso.real_dev = NULL;
[snip]
TBH, keeping buggy code with a comment doesn't sound good to me. I'd rather
remove this
support than tell people "good luck, it might crash". It's better to be safe
until a
correct design is in place which takes care of these issues.
Cheers,
Nik
