netdev - Re: [PATCH] [PATH xfrm offload] xfrm: bonding: Add xfrm packet offload for active-backup mode

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
Hash Suite for Android: free password hash cracker in your pocket
[<prev] [next>] [thread-next>] [day] [month] [year] [list]
Message-ID: <2152c417f85fd77d795da7fea1d7daadb312ce41.camel@nvidia.com>
Date: Thu, 3 Jul 2025 14:16:43 +0000
From: Cosmin Ratiu <cratiu@...dia.com>
To: "erwan.dufour@...hings.com" <erwan.dufour@...hings.com>,
	"liuhangbin@...il.com" <liuhangbin@...il.com>, "leon@...nel.org"
	<leon@...nel.org>
CC: "davem@...emloft.net" <davem@...emloft.net>, Tariq Toukan
	<tariqt@...dia.com>, "herbert@...dor.apana.org.au"
	<herbert@...dor.apana.org.au>, "steffen.klassert@...unet.com"
	<steffen.klassert@...unet.com>, "jv@...sburgh.net" <jv@...sburgh.net>,
	"netdev@...r.kernel.org" <netdev@...r.kernel.org>, "mrarmonius@...il.com"
	<mrarmonius@...il.com>, Saeed Mahameed <saeedm@...dia.com>
Subject: Re: [PATCH] [PATH xfrm offload] xfrm: bonding: Add xfrm packet
 offload for active-backup mode

+Leon, he cares about IPSec in mlx5.

On Thu, 2025-07-03 at 01:58 +0200, Erwan Dufour wrote:
> Hi Liu,
> 
> > Thanks for your explanation. Unfortunately，the alignment still not
> > works.
> > 
> 
> With pleasure. Thank you very much for providing an example with an
> explanation. 
> Hopefully, there were no mistakes and I managed to correct all the
> errors in the new patch.
> 
> New Patch:
> From 39639cf83712b13271fc3d8bbe3f4d9cd0b38db6 Mon Sep 17 00:00:00
> 2001
> From: Erwan Dufour <erwan.dufour@...hings.com>
> Date: Wed, 2 Jul 2025 22:12:10 +0000
> Subject: [PATCH net-next] xfrm: bonding: Add xfrm packet offload for
>  active-backup mode
> 
> 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
Typo here ^. Should be "dev", not "deb".

> 
> 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.
> 
> Signed-off-by: Erwan Dufour <erwan.dufour@...hings.com>
> ---
>  drivers/net/bonding/bond_main.c               | 251 ++++++++++++++--
> --
>  .../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, 218 insertions(+), 56 deletions(-)
> 
> diff --git a/drivers/net/bonding/bond_main.c
> b/drivers/net/bonding/bond_main.c
> index c4d53e8e7c15..c9505441c863 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)) {
> +				slave_warn(bond_dev, real_dev, "%s:
> failed to add SA\n", __func__);
Lines should be max 80-characters. Please use checkpatch.pl to verify
these things:
scripts/checkpatch.pl -g --max-line-length 80 HEAD-1


[...] I didn't review the bulk of the changes in detail, because:

> 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;
Instead of carrying a mixed list of states and policies, which forces
every piece of code handling this into a model of "if (xs) {} else {}",
prefer two separate lists, one for states and another one for policies.
Then, to make it extra clear that xs and xp are mutually exclusive, an
unnamed union between xs and xp could be used.

Also, in the future, please send new versions of the patch as separate
emails, with subjects such as "[PATCH net-next v2] $title", to interact
better with the kernel patchwork system. Right now, this patch is in a
weird state:
https://patchwork.kernel.org/project/netdevbpf/patch/20250629210623.43497-1-mramonius@gmail.com/


Cosmin.