Re: [PATCHv3 net-next 2/3] bonding: Add ESN support to IPSec HW offload

[Sabrina Dubroca <sd@queasysnail.net>]

Hi Hangbin,

(adding Steffen since we're getting a bit into details of IPsec)

2024-08-20, 08:48:39 +0800, Hangbin Liu wrote:
> Currently, users can see that bonding supports IPSec HW offload via ethtool.
> However, this functionality does not work with NICs like Mellanox cards when
> ESN (Extended Sequence Numbers) is enabled, as ESN functions are not yet
> supported. This patch adds ESN support to the bonding IPSec device offload,
> ensuring proper functionality with NICs that support ESN.
> 
> Reviewed-by: Nikolay Aleksandrov <razor@blackwall.org>
> Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
> ---
>  drivers/net/bonding/bond_main.c | 25 +++++++++++++++++++++++++
>  1 file changed, 25 insertions(+)
> 
> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
> index 560e3416f6f5..24747fceef66 100644
> --- a/drivers/net/bonding/bond_main.c
> +++ b/drivers/net/bonding/bond_main.c
> @@ -651,10 +651,35 @@ static bool bond_ipsec_offload_ok(struct sk_buff *skb, struct xfrm_state *xs)
>  	return err;
>  }
>  
> +/**
> + * bond_advance_esn_state - ESN support for IPSec HW offload
> + * @xs: pointer to transformer state struct
> + **/
> +static void bond_advance_esn_state(struct xfrm_state *xs)
> +{
> +	struct net_device *real_dev;
> +
> +	rcu_read_lock();
> +	real_dev = bond_ipsec_dev(xs);
> +	if (!real_dev)
> +		goto out;
> +
> +	if (!real_dev->xfrmdev_ops ||
> +	    !real_dev->xfrmdev_ops->xdo_dev_state_advance_esn) {
> +		pr_warn("%s: %s doesn't support xdo_dev_state_advance_esn\n", __func__, real_dev->name);

xdo_dev_state_advance_esn is called on the receive path for every
packet when ESN is enabled (xfrm_input -> xfrm_replay_advance ->
xfrm_replay_advance_esn -> xfrm_dev_state_advance_esn), this needs to
be ratelimited.


But this CB is required to make offload with ESN work. If it's not
implemented on a lower device, I expect things will break. I wonder
what the best behavior would be:

 - just warn (this patch) -- but things will break for users

 - don't allow mixing devices that support ESN with devices that don't
   in the same bond (really bad if the user doesn't care about ESN)

 - don't allow enslaving devices that don't support ESN if an xfrm
   state using ESN has been added to the bond, and don't allow
   creating ESN states if the bond has one non-ESN slave

 - disable re-offloading the state if we have to migrate it from an
   ESN-enabled to a non-ESN device (when changing the active slave)
   -- and fall back to the (slow) SW implementation

 - other options that I'm not thinking about?


Sorry I'm only noticing this at v3 :(

-- 
Sabrina