| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <aGJiZrvRKXm74wd2@fedora>
Date: Mon, 30 Jun 2025 10:09:42 +0000
From: Hangbin Liu <liuhangbin@...il.com>
To: Erwan Dufour <mrarmonius@...il.com>
Cc: netdev@...r.kernel.org, steffen.klassert@...unet.com,
herbert@...dor.apana.org.au, davem@...emloft.net, jv@...sburgh.net,
saeedm@...dia.com, tariqt@...dia.com, erwan.dufour@...hings.com,
Cosmin Ratiu <cratiu@...dia.com>
Subject: Re: [PATCH] [PATH xfrm offload] xfrm: bonding: Add xfrm packet
offload for active-backup mode
Hi Erwan,
Cc Cosmin as he updated bond ipsec a lot.
Maybe the subject prefix could be [PATCH net-next].
On Sun, Jun 29, 2025 at 11:06:23PM +0200, Erwan Dufour wrote:
> From: Erwan Dufour <erwan.dufour@...hings.com>
>
> Implement XFRM policy offload functions for bond device in active-backup mode.
> - xdo_dev_policy_add = bond_ipsec_add_sp
> - xdo_dev_policy_delete = bond_ipsec_del_sp
> _ xdo_deb_policy_free = bond_ipsec_free_sp
>
> Modification of the function signature for copying on SA models.
> Also add netdevice pointer to avoid to use real_dev which is obsolete and deleted for policy.
>
> Store the bond's xfrm policies in the struct bond_ipsec.
> Also rename these functions:
> - bond_ipsec_del_sa_all -> bond_ipsec_del_sa_sp_all
> - bond_ipsec_add_sa_all -> bond_ipsec_add_sa_sp_all
> Now bond_ipsec_{del,add}_sa_sp_all remove/add also the bond's policies stores in same struct as SA.
>
> Tested on Mellanox ConnectX-6 Dx Crypto Enable Cards.
> ---
> drivers/net/bonding/bond_main.c | 279 +++++++++++++++---
> .../mellanox/mlx5/core/en_accel/ipsec.c | 10 +-
> include/linux/netdevice.h | 6 +-
> include/net/bonding.h | 1 +
> include/net/xfrm.h | 4 +-
> net/xfrm/xfrm_device.c | 2 +-
> 6 files changed, 246 insertions(+), 56 deletions(-)
>
> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
> index c4d53e8e7c15..85017635f9b5 100644
> --- a/drivers/net/bonding/bond_main.c
> +++ b/drivers/net/bonding/bond_main.c
> @@ -512,7 +512,7 @@ static int bond_ipsec_add_sa(struct net_device *bond_dev,
> return err;
> }
>
> -static void bond_ipsec_add_sa_all(struct bonding *bond)
> +static void bond_ipsec_add_sa_sp_all(struct bonding *bond)
> {
> struct net_device *bond_dev = bond->dev;
> struct net_device *real_dev;
> @@ -536,29 +536,44 @@ static void bond_ipsec_add_sa_all(struct bonding *bond)
> }
>
> list_for_each_entry(ipsec, &bond->ipsec_list, list) {
> - /* If new state is added before ipsec_lock acquired */
> - if (ipsec->xs->xso.real_dev == real_dev)
> - continue;
> + if (ipsec->xs) {
> + /* If new state is added before ipsec_lock acquired */
> + if (ipsec->xs->xso.real_dev == real_dev)
> + continue;
>
> - if (real_dev->xfrmdev_ops->xdo_dev_state_add(real_dev,
> - ipsec->xs, NULL)) {
> - slave_warn(bond_dev, real_dev, "%s: failed to add SA\n", __func__);
> - continue;
> - }
> + if (real_dev->xfrmdev_ops->xdo_dev_state_add(real_dev,
> + ipsec->xs, NULL)) {
Please fix the code alignment. And all others in the code.
> +
> +/**
> + * bond_ipsec_del_sp - clear out this specific SP
> + * @bond_dev: pointer to net device
> + * @xs: pointer to transformer policy struct
> + **/
> +static void bond_ipsec_del_sp(struct net_device *bond_dev, struct xfrm_policy *xp)
> +{
> + struct net_device *real_dev;
> + netdevice_tracker tracker;
> + struct bond_ipsec *ipsec;
> + struct bonding *bond;
> + struct slave *slave;
> +
> + if (!bond_dev)
> + return;
> +
> + rcu_read_lock();
> + bond = netdev_priv(bond_dev);
> + slave = rcu_dereference(bond->curr_active_slave);
> + real_dev = slave ? slave->dev : NULL;
> + netdev_hold(real_dev, &tracker, GFP_ATOMIC);
> + rcu_read_unlock();
> +
> + if (!slave)
> + goto out;
> +
> + if (!xp->xdo.real_dev)
> + goto out;
> +
> + WARN_ON(xp->xdo.real_dev != real_dev);
> +
> + if (!real_dev->xfrmdev_ops ||
> + !real_dev->xfrmdev_ops->xdo_dev_policy_delete ||
> + netif_is_bond_master(real_dev)) {
> + slave_warn(bond_dev, real_dev, "%s: no slave xdo_dev_policy_delete\n", __func__);
> + goto out;
> + }
> +
> + real_dev->xfrmdev_ops->xdo_dev_policy_delete(real_dev, xp);
> +out:
> + netdev_put(real_dev, &tracker);
> + mutex_lock(&bond->ipsec_lock);
> + list_for_each_entry(ipsec, &bond->ipsec_list, list) {
> + if (ipsec->xp == xp) {
> + list_del(&ipsec->list);
> + kfree(ipsec);
> + break;
> + }
> + }
> + mutex_unlock(&bond->ipsec_lock);
> +}
In xfrm_add_policy() err out, it calls xfrm_dev_policy_delete() first and
then xfrm_dev_policy_free(). So why we free ipsec->list in bond_ipsec_del_sp()
but no bond_ipsec_free_sp()?
BTW, if (ipsec->xp == xp), should we delete the whole ipsec_list? Is it
possible ipsec->xs still exist?
Thanks
Hangbin
> +
> +static void bond_ipsec_free_sp(struct net_device *bond_dev, struct xfrm_policy *xp)
> +{
> + struct net_device *real_dev;
> + netdevice_tracker tracker;
> + struct bonding *bond;
> + struct slave *slave;
> +
> + if (!bond_dev)
> + return;
> +
> + rcu_read_lock();
> + bond = netdev_priv(bond_dev);
> + slave = rcu_dereference(bond->curr_active_slave);
> + real_dev = slave ? slave->dev : NULL;
> + netdev_hold(real_dev, &tracker, GFP_ATOMIC);
> + rcu_read_unlock();
> +
> + if (!slave)
> + goto out;
> +
> + if (!xp->xdo.real_dev)
> + goto out;
> +
> + WARN_ON(xp->xdo.real_dev != real_dev);
> +
> + if (real_dev && real_dev->xfrmdev_ops &&
> + real_dev->xfrmdev_ops->xdo_dev_policy_free)
> + real_dev->xfrmdev_ops->xdo_dev_policy_free(real_dev, xp);
> +out:
> + netdev_put(real_dev, &tracker);
> +}
> +
> static const struct xfrmdev_ops bond_xfrmdev_ops = {
> .xdo_dev_state_add = bond_ipsec_add_sa,
> .xdo_dev_state_delete = bond_ipsec_del_sa,
> @@ -738,6 +924,9 @@ static const struct xfrmdev_ops bond_xfrmdev_ops = {
> .xdo_dev_offload_ok = bond_ipsec_offload_ok,
> .xdo_dev_state_advance_esn = bond_advance_esn_state,
> .xdo_dev_state_update_stats = bond_xfrm_update_stats,
> + .xdo_dev_policy_add = bond_ipsec_add_sp,
> + .xdo_dev_policy_delete = bond_ipsec_del_sp,
> + .xdo_dev_policy_free = bond_ipsec_free_sp,
> };
> #endif /* CONFIG_XFRM_OFFLOAD */
>
> @@ -1277,7 +1466,7 @@ void bond_change_active_slave(struct bonding *bond, struct slave *new_active)
> return;
>
> #ifdef CONFIG_XFRM_OFFLOAD
> - bond_ipsec_del_sa_all(bond);
> + bond_ipsec_del_sa_sp_all(bond);
> #endif /* CONFIG_XFRM_OFFLOAD */
>
> if (new_active) {
> @@ -1352,7 +1541,7 @@ void bond_change_active_slave(struct bonding *bond, struct slave *new_active)
> }
>
> #ifdef CONFIG_XFRM_OFFLOAD
> - bond_ipsec_add_sa_all(bond);
> + bond_ipsec_add_sa_sp_all(bond);
> #endif /* CONFIG_XFRM_OFFLOAD */
>
> /* resend IGMP joins since active slave has changed or
> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_accel/ipsec.c b/drivers/net/ethernet/mellanox/mlx5/core/en_accel/ipsec.c
> index 77f61cd28a79..f5e3fc054f41 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/en_accel/ipsec.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_accel/ipsec.c
> @@ -1161,15 +1161,15 @@ mlx5e_ipsec_build_accel_pol_attrs(struct mlx5e_ipsec_pol_entry *pol_entry,
> attrs->prio = x->priority;
> }
>
> -static int mlx5e_xfrm_add_policy(struct xfrm_policy *x,
> +static int mlx5e_xfrm_add_policy(struct net_device *dev,
> + struct xfrm_policy *x,
> struct netlink_ext_ack *extack)
> {
> - struct net_device *netdev = x->xdo.dev;
> struct mlx5e_ipsec_pol_entry *pol_entry;
> struct mlx5e_priv *priv;
> int err;
>
> - priv = netdev_priv(netdev);
> + priv = netdev_priv(dev);
> if (!priv->ipsec) {
> NL_SET_ERR_MSG_MOD(extack, "Device doesn't support IPsec packet offload");
> return -EOPNOTSUPP;
> @@ -1207,7 +1207,7 @@ static int mlx5e_xfrm_add_policy(struct xfrm_policy *x,
> return err;
> }
>
> -static void mlx5e_xfrm_del_policy(struct xfrm_policy *x)
> +static void mlx5e_xfrm_del_policy(struct net_device *dev, struct xfrm_policy *x)
> {
> struct mlx5e_ipsec_pol_entry *pol_entry = to_ipsec_pol_entry(x);
>
> @@ -1215,7 +1215,7 @@ static void mlx5e_xfrm_del_policy(struct xfrm_policy *x)
> mlx5_eswitch_unblock_ipsec(pol_entry->ipsec->mdev);
> }
>
> -static void mlx5e_xfrm_free_policy(struct xfrm_policy *x)
> +static void mlx5e_xfrm_free_policy(struct net_device *dev, struct xfrm_policy *x)
> {
> struct mlx5e_ipsec_pol_entry *pol_entry = to_ipsec_pol_entry(x);
>
> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> index adb14db25798..7c3d74d28ef4 100644
> --- a/include/linux/netdevice.h
> +++ b/include/linux/netdevice.h
> @@ -1024,9 +1024,9 @@ struct xfrmdev_ops {
> struct xfrm_state *x);
> void (*xdo_dev_state_advance_esn) (struct xfrm_state *x);
> void (*xdo_dev_state_update_stats) (struct xfrm_state *x);
> - int (*xdo_dev_policy_add) (struct xfrm_policy *x, struct netlink_ext_ack *extack);
> - void (*xdo_dev_policy_delete) (struct xfrm_policy *x);
> - void (*xdo_dev_policy_free) (struct xfrm_policy *x);
> + int (*xdo_dev_policy_add) (struct net_device *dev, struct xfrm_policy *x, struct netlink_ext_ack *extack);
> + void (*xdo_dev_policy_delete) (struct net_device *dev, struct xfrm_policy *x);
> + void (*xdo_dev_policy_free) (struct net_device *dev, struct xfrm_policy *x);
> };
> #endif
>
> diff --git a/include/net/bonding.h b/include/net/bonding.h
> index 95f67b308c19..6ac079673f87 100644
> --- a/include/net/bonding.h
> +++ b/include/net/bonding.h
> @@ -207,6 +207,7 @@ struct bond_up_slave {
> struct bond_ipsec {
> struct list_head list;
> struct xfrm_state *xs;
> + struct xfrm_policy *xp;
> };
>
> /*
> diff --git a/include/net/xfrm.h b/include/net/xfrm.h
> index a21e276dbe44..ffae7cc1f989 100644
> --- a/include/net/xfrm.h
> +++ b/include/net/xfrm.h
> @@ -2116,7 +2116,7 @@ static inline void xfrm_dev_policy_delete(struct xfrm_policy *x)
> struct net_device *dev = xdo->dev;
>
> if (dev && dev->xfrmdev_ops && dev->xfrmdev_ops->xdo_dev_policy_delete)
> - dev->xfrmdev_ops->xdo_dev_policy_delete(x);
> + dev->xfrmdev_ops->xdo_dev_policy_delete(dev, x);
> }
>
> static inline void xfrm_dev_policy_free(struct xfrm_policy *x)
> @@ -2126,7 +2126,7 @@ static inline void xfrm_dev_policy_free(struct xfrm_policy *x)
>
> if (dev && dev->xfrmdev_ops) {
> if (dev->xfrmdev_ops->xdo_dev_policy_free)
> - dev->xfrmdev_ops->xdo_dev_policy_free(x);
> + dev->xfrmdev_ops->xdo_dev_policy_free(dev, x);
> xdo->dev = NULL;
> netdev_put(dev, &xdo->dev_tracker);
> }
> diff --git a/net/xfrm/xfrm_device.c b/net/xfrm/xfrm_device.c
> index 81fd486b5e56..643679b8d13c 100644
> --- a/net/xfrm/xfrm_device.c
> +++ b/net/xfrm/xfrm_device.c
> @@ -394,7 +394,7 @@ int xfrm_dev_policy_add(struct net *net, struct xfrm_policy *xp,
> return -EINVAL;
> }
>
> - err = dev->xfrmdev_ops->xdo_dev_policy_add(xp, extack);
> + err = dev->xfrmdev_ops->xdo_dev_policy_add(dev, xp, extack);
> if (err) {
> xdo->dev = NULL;
> xdo->type = XFRM_DEV_OFFLOAD_UNSPECIFIED;
> --
> 2.43.0
>
Powered by blists - more mailing lists