[PATCHv3 net-next 0/3] Bonding: support new xfrm state offload functions

[PATCHv3 net-next 0/3] Bonding: support new xfrm state offload functions

[Hangbin Liu]

Add 2 new xfrm state offload functions xdo_dev_state_advance_esn and
xdo_dev_state_update_stats for bonding. The xdo_dev_state_free will be
added by Jianbo's patchset [1]. I will add the bonding xfrm policy offload
in future.

I planned to add the new XFRM state offload functions after Jianbo's
patchset [1], but it seems that may take some time. Therefore, I am
posting these two patches to net-next now, as our users are waiting for
this functionality. If Jianbo's patch is applied first, I can update these
patches accordingly.

[1] https://lore.kernel.org/netdev/20240815142103.2253886-2-tariqt@nvidia.com

v3: Re-format bond_ipsec_dev, use slave_warn instead of WARN_ON (Nikolay Aleksandrov)
    Fix bond_ipsec_dev defination, add *. (Simon Horman, kernel test robot)
    Fix "real" typo (kernel test robot)
v2: Add a function to process the common device checking (Nikolay Aleksandrov)
    Remove unused variable (Simon Horman)
v1: lore.kernel.org/netdev/20240816035518.203704-1-liuhangbin@gmail.com

Hangbin Liu (3):
  bonding: add common function to check ipsec device
  bonding: Add ESN support to IPSec HW offload
  bonding: support xfrm state update

 drivers/net/bonding/bond_main.c | 98 ++++++++++++++++++++++++++++-----
 1 file changed, 85 insertions(+), 13 deletions(-)

-- 
2.45.0

[PATCHv3 net-next 1/3] bonding: add common function to check ipsec device

[Hangbin Liu]

This patch adds a common function to check the status of IPSec devices.
This function will be useful for future implementations, such as IPSec ESN
and state offload callbacks.

Suggested-by: Nikolay Aleksandrov <razor@blackwall.org>
Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
---
 drivers/net/bonding/bond_main.c | 48 ++++++++++++++++++++++++---------
 1 file changed, 35 insertions(+), 13 deletions(-)

diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index f9633a6f8571..560e3416f6f5 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -418,6 +418,39 @@ static int bond_vlan_rx_kill_vid(struct net_device *bond_dev,
 /*---------------------------------- XFRM -----------------------------------*/
 
 #ifdef CONFIG_XFRM_OFFLOAD
+/**
+ * bond_ipsec_dev - return the device for ipsec offload, or NULL if not exist
+ *                  caller must hold rcu_read_lock.
+ * @xs: pointer to transformer state struct
+ **/
+static struct net_device *bond_ipsec_dev(struct xfrm_state *xs)
+{
+	struct net_device *bond_dev = xs->xso.dev;
+	struct bonding *bond;
+	struct slave *slave;
+
+	if (!bond_dev)
+		return NULL;
+
+	bond = netdev_priv(bond_dev);
+	if (BOND_MODE(bond) != BOND_MODE_ACTIVEBACKUP)
+		return NULL;
+
+	slave = rcu_dereference(bond->curr_active_slave);
+	if (!slave)
+		return NULL;
+
+	if (!xs->xso.real_dev)
+		return NULL;
+
+	if (xs->xso.real_dev != slave->dev)
+		slave_warn(bond_dev, slave->dev,
+			   "not same with IPsec offload real dev %s\n",
+			   xs->xso.real_dev->name);
+
+	return slave->dev;
+}
+
 /**
  * bond_ipsec_add_sa - program device with a security association
  * @xs: pointer to transformer state struct
@@ -595,23 +628,12 @@ static void bond_ipsec_del_sa_all(struct bonding *bond)
  **/
 static bool bond_ipsec_offload_ok(struct sk_buff *skb, struct xfrm_state *xs)
 {
-	struct net_device *bond_dev = xs->xso.dev;
 	struct net_device *real_dev;
-	struct slave *curr_active;
-	struct bonding *bond;
 	int err;
 
-	bond = netdev_priv(bond_dev);
 	rcu_read_lock();
-	curr_active = rcu_dereference(bond->curr_active_slave);
-	real_dev = curr_active->dev;
-
-	if (BOND_MODE(bond) != BOND_MODE_ACTIVEBACKUP) {
-		err = false;
-		goto out;
-	}
-
-	if (!xs->xso.real_dev) {
+	real_dev = bond_ipsec_dev(xs);
+	if (!real_dev) {
 		err = false;
 		goto out;
 	}
-- 
2.45.0

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

[Hangbin Liu]

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);
+		goto out;
+	}
+
+	real_dev->xfrmdev_ops->xdo_dev_state_advance_esn(xs);
+out:
+	rcu_read_unlock();
+}
+
 static const struct xfrmdev_ops bond_xfrmdev_ops = {
 	.xdo_dev_state_add = bond_ipsec_add_sa,
 	.xdo_dev_state_delete = bond_ipsec_del_sa,
 	.xdo_dev_offload_ok = bond_ipsec_offload_ok,
+	.xdo_dev_state_advance_esn = bond_advance_esn_state,
 };
 #endif /* CONFIG_XFRM_OFFLOAD */
 
-- 
2.45.0

[PATCHv3 net-next 3/3] bonding: support xfrm state update

[Hangbin Liu]

The patch add xfrm statistics update for bonding IPsec offload.

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 24747fceef66..4a4a1d9c8cca 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -675,11 +675,36 @@ static void bond_advance_esn_state(struct xfrm_state *xs)
 	rcu_read_unlock();
 }
 
+/**
+ * bond_xfrm_update_stats - Update xfrm state
+ * @xs: pointer to transformer state struct
+ **/
+static void bond_xfrm_update_stats(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_update_stats) {
+		pr_warn("%s: %s doesn't support xdo_dev_state_update_stats\n", __func__, real_dev->name);
+		goto out;
+	}
+
+	real_dev->xfrmdev_ops->xdo_dev_state_update_stats(xs);
+out:
+	rcu_read_unlock();
+}
+
 static const struct xfrmdev_ops bond_xfrmdev_ops = {
 	.xdo_dev_state_add = bond_ipsec_add_sa,
 	.xdo_dev_state_delete = bond_ipsec_del_sa,
 	.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,
 };
 #endif /* CONFIG_XFRM_OFFLOAD */
 
-- 
2.45.0

Re: [PATCHv3 net-next 1/3] bonding: add common function to check ipsec device

[Nikolay Aleksandrov]

On 20/08/2024 03:48, Hangbin Liu wrote:
> This patch adds a common function to check the status of IPSec devices.
> This function will be useful for future implementations, such as IPSec ESN
> and state offload callbacks.
> 
> Suggested-by: Nikolay Aleksandrov <razor@blackwall.org>
> Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
> ---
>   drivers/net/bonding/bond_main.c | 48 ++++++++++++++++++++++++---------
>   1 file changed, 35 insertions(+), 13 deletions(-)
> 

Looks good to me,
Reviewed-by: Nikolay Aleksandrov <razor@blackwall.org>

Re: [PATCHv3 net-next 3/3] bonding: support xfrm state update

[Sabrina Dubroca]

Hi Hangbin,

2024-08-20, 08:48:40 +0800, Hangbin Liu wrote:
> The patch add xfrm statistics update for bonding IPsec offload.
> 
> 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 24747fceef66..4a4a1d9c8cca 100644
> --- a/drivers/net/bonding/bond_main.c
> +++ b/drivers/net/bonding/bond_main.c
> @@ -675,11 +675,36 @@ static void bond_advance_esn_state(struct xfrm_state *xs)
>  	rcu_read_unlock();
>  }
>  
> +/**
> + * bond_xfrm_update_stats - Update xfrm state
> + * @xs: pointer to transformer state struct
> + **/
> +static void bond_xfrm_update_stats(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_update_stats) {
> +		pr_warn("%s: %s doesn't support xdo_dev_state_update_stats\n", __func__, real_dev->name);

I'm not convinced we should warn here. Most drivers don't implement
xdo_dev_state_update_stats, so if we're using one of those drivers
(for example netdevsim), we'll get one line in dmesg for every "ip
xfrm state" command run by the user. At most it should be ratelimited,
but since it's an optional callback, I think no warning would be fine.

-- 
Sabrina

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

[Sabrina Dubroca]

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

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

[Hangbin Liu]

On Tue, Aug 20, 2024 at 05:33:58PM +0200, Sabrina Dubroca wrote:
> > +	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.

You are right. Warn on adding/deleting is OK. But during packets receiving
we need to limit it.

> 
> 
> 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

I would prefer this way, which is what we do currently. The warn could
let user know the ESN is not supported. They should use ESN supported device
or disable it.

Thanks
Hangbin

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

[Steffen Klassert]

On Tue, Aug 20, 2024 at 05:33:58PM +0200, Sabrina Dubroca wrote:
> 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.

How does xfrm_state offload work on bonding?
Does every slave have its own negotiated SA?

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

[Hangbin Liu]

On Wed, Aug 21, 2024 at 02:30:41PM +0200, Steffen Klassert wrote:
> > > +/**
> > > + * 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.
> 
> How does xfrm_state offload work on bonding?
> Does every slave have its own negotiated SA?

Yes and no. Bonding only supports xfrm offload with active-backup mode. So only
current active slave keep the SA. When active slave changes, the sa on
previous slave is deleted and re-added on new active slave.

Thanks
Hangbin

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

[Sabrina Dubroca]

2024-08-21, 21:26:00 +0800, Hangbin Liu wrote:
> On Wed, Aug 21, 2024 at 02:30:41PM +0200, Steffen Klassert wrote:
> > > > +/**
> > > > + * 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.
> > 
> > How does xfrm_state offload work on bonding?
> > Does every slave have its own negotiated SA?
> 
> Yes and no. Bonding only supports xfrm offload with active-backup mode. So only
> current active slave keep the SA. When active slave changes, the sa on
> previous slave is deleted and re-added on new active slave.

It's the same SA, there's no DELSA+NEWSA when we change the active
slave (but we call xdo_dev_state_delete/xdo_dev_state_add to inform
the driver/HW), and only a single NEWSA to install the offloaded SA on
the bond device (which calls the active slave's xdo_dev_state_add).

-- 
Sabrina

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

[Steffen Klassert]

On Wed, Aug 21, 2024 at 03:39:48PM +0200, Sabrina Dubroca wrote:
> 2024-08-21, 21:26:00 +0800, Hangbin Liu wrote:
> > On Wed, Aug 21, 2024 at 02:30:41PM +0200, Steffen Klassert wrote:
> > > > > +/**
> > > > > + * 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.
> > > 
> > > How does xfrm_state offload work on bonding?
> > > Does every slave have its own negotiated SA?
> > 
> > Yes and no. Bonding only supports xfrm offload with active-backup mode. So only
> > current active slave keep the SA. When active slave changes, the sa on
> > previous slave is deleted and re-added on new active slave.
> 
> It's the same SA, there's no DELSA+NEWSA when we change the active
> slave (but we call xdo_dev_state_delete/xdo_dev_state_add to inform
> the driver/HW), and only a single NEWSA to install the offloaded SA on
> the bond device (which calls the active slave's xdo_dev_state_add).

Maybe I miss something, but how is the sequence number, replay window
etc. transfered from the old to the new active slave?

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

[Sabrina Dubroca]

2024-08-21, 16:03:01 +0200, Steffen Klassert wrote:
> On Wed, Aug 21, 2024 at 03:39:48PM +0200, Sabrina Dubroca wrote:
> > 2024-08-21, 21:26:00 +0800, Hangbin Liu wrote:
> > > On Wed, Aug 21, 2024 at 02:30:41PM +0200, Steffen Klassert wrote:
> > > > > > +/**
> > > > > > + * 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.
> > > > 
> > > > How does xfrm_state offload work on bonding?
> > > > Does every slave have its own negotiated SA?
> > > 
> > > Yes and no. Bonding only supports xfrm offload with active-backup mode. So only
> > > current active slave keep the SA. When active slave changes, the sa on
> > > previous slave is deleted and re-added on new active slave.
> > 
> > It's the same SA, there's no DELSA+NEWSA when we change the active
> > slave (but we call xdo_dev_state_delete/xdo_dev_state_add to inform
> > the driver/HW), and only a single NEWSA to install the offloaded SA on
> > the bond device (which calls the active slave's xdo_dev_state_add).
> 
> Maybe I miss something, but how is the sequence number, replay window
> etc. transfered from the old to the new active slave?

With crypto offload, the stack sees the headers so it manages to keep
track and update its data, so it should have no problem feeding it
back to the next driver?

Note that if something in that area is broken, it would be broken
regardless of ESN.

-- 
Sabrina

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

[Hangbin Liu]

On Wed, Aug 21, 2024 at 03:39:48PM +0200, Sabrina Dubroca wrote:
> > > > > +	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.
> > > 
> > > How does xfrm_state offload work on bonding?
> > > Does every slave have its own negotiated SA?
> > 
> > Yes and no. Bonding only supports xfrm offload with active-backup mode. So only
> > current active slave keep the SA. When active slave changes, the sa on
> > previous slave is deleted and re-added on new active slave.
> 
> It's the same SA, there's no DELSA+NEWSA when we change the active
> slave (but we call xdo_dev_state_delete/xdo_dev_state_add to inform
> the driver/HW), and only a single NEWSA to install the offloaded SA on
> the bond device (which calls the active slave's xdo_dev_state_add).

Yes, thanks for the clarification. The SA is not changed, we just delete it
on old active slave

slave->dev->xfrmdev_ops->xdo_dev_state_delete(ipsec->xs);

And add to now one.

ipsec->xs->xso.real_dev = slave->dev;
slave->dev->xfrmdev_ops->xdo_dev_state_add(ipsec->xs, NULL)

Thanks
Hangbin

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

[Steffen Klassert]

On Thu, Aug 22, 2024 at 08:33:17AM +0800, Hangbin Liu wrote:
> On Wed, Aug 21, 2024 at 03:39:48PM +0200, Sabrina Dubroca wrote:
> > > > > > +	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.
> > > > 
> > > > How does xfrm_state offload work on bonding?
> > > > Does every slave have its own negotiated SA?
> > > 
> > > Yes and no. Bonding only supports xfrm offload with active-backup mode. So only
> > > current active slave keep the SA. When active slave changes, the sa on
> > > previous slave is deleted and re-added on new active slave.
> > 
> > It's the same SA, there's no DELSA+NEWSA when we change the active
> > slave (but we call xdo_dev_state_delete/xdo_dev_state_add to inform
> > the driver/HW), and only a single NEWSA to install the offloaded SA on
> > the bond device (which calls the active slave's xdo_dev_state_add).
> 
> Yes, thanks for the clarification. The SA is not changed, we just delete it
> on old active slave
> 
> slave->dev->xfrmdev_ops->xdo_dev_state_delete(ipsec->xs);
> 
> And add to now one.
> 
> ipsec->xs->xso.real_dev = slave->dev;
> slave->dev->xfrmdev_ops->xdo_dev_state_add(ipsec->xs, NULL)

Using the same key on two different devices is very dangerous.
Counter mode algorithms have the requirement that the IV
must not repeat. If you use the same key on two devices,
you can't guarantee that. If both devices use an internal
counter (initialized to one) to generate the IV, then the
IV repeats for the mumber of packets that were already
sent on the old device. The algorithm is cryptographically
broken in that case.

Instead of moving the existing state, it is better to
request a rekey. Maybe by setting the old state to
'expired' and then send a km_state_expired() message.

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

[Steffen Klassert]

On Wed, Aug 21, 2024 at 07:20:46PM +0200, Sabrina Dubroca wrote:
> 2024-08-21, 16:03:01 +0200, Steffen Klassert wrote:
> > 
> > Maybe I miss something, but how is the sequence number, replay window
> > etc. transfered from the old to the new active slave?
> 
> With crypto offload, the stack sees the headers so it manages to keep
> track and update its data, so it should have no problem feeding it
> back to the next driver?

Right, I was thinking about the packet offload case.

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

[Hangbin Liu]

Hi Steffen,
On Thu, Aug 22, 2024 at 09:10:47AM +0200, Steffen Klassert wrote:
> > Yes, thanks for the clarification. The SA is not changed, we just delete it
> > on old active slave
> > 
> > slave->dev->xfrmdev_ops->xdo_dev_state_delete(ipsec->xs);
> > 
> > And add to now one.
> > 
> > ipsec->xs->xso.real_dev = slave->dev;
> > slave->dev->xfrmdev_ops->xdo_dev_state_add(ipsec->xs, NULL)
> 
> Using the same key on two different devices is very dangerous.
> Counter mode algorithms have the requirement that the IV
> must not repeat. If you use the same key on two devices,
> you can't guarantee that. If both devices use an internal
> counter (initialized to one) to generate the IV, then the
> IV repeats for the mumber of packets that were already
> sent on the old device. The algorithm is cryptographically
> broken in that case.
> 
> Instead of moving the existing state, it is better to
> request a rekey. Maybe by setting the old state to
> 'expired' and then send a km_state_expired() message.

Thanks for your comments. I'm not familiar with IPsec state.
Do you mean something like

diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index f74bacf071fc..8a51d0812564 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -477,6 +477,7 @@ static void bond_ipsec_add_sa_all(struct bonding *bond)
 	struct net_device *bond_dev = bond->dev;
 	struct bond_ipsec *ipsec;
 	struct slave *slave;
+	struct km_event c;
 
 	rcu_read_lock();
 	slave = rcu_dereference(bond->curr_active_slave);
@@ -498,6 +499,13 @@ static void bond_ipsec_add_sa_all(struct bonding *bond)
 	spin_lock_bh(&bond->ipsec_lock);
 	list_for_each_entry(ipsec, &bond->ipsec_list, list) {
 		ipsec->xs->xso.real_dev = slave->dev;
+
+		ipsec->xs->km.state = XFRM_STATE_VALID;
+		c.data.hard = 1;
+		c.portid = 0;
+		c.event = XFRM_MSG_NEWSA;
+		km_state_notify(x, &c);
+
 		if (slave->dev->xfrmdev_ops->xdo_dev_state_add(ipsec->xs, NULL)) {
 			slave_warn(bond_dev, slave->dev, "%s: failed to add SA\n", __func__);
 			ipsec->xs->xso.real_dev = NULL;
@@ -580,6 +588,8 @@ static void bond_ipsec_del_sa_all(struct bonding *bond)
 				   "%s: no slave xdo_dev_state_delete\n",
 				   __func__);
 		} else {
+			ipsec->xs->km.state = XFRM_STATE_EXPIRED;
+			km_state_expired(ipsec->xs, 1, 0);
 			slave->dev->xfrmdev_ops->xdo_dev_state_delete(ipsec->xs);
 		}
 	}

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

[Sabrina Dubroca]

2024-08-22, 09:10:47 +0200, Steffen Klassert wrote:
> On Thu, Aug 22, 2024 at 08:33:17AM +0800, Hangbin Liu wrote:
> > On Wed, Aug 21, 2024 at 03:39:48PM +0200, Sabrina Dubroca wrote:
> > > > > > > +	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.
> > > > > 
> > > > > How does xfrm_state offload work on bonding?
> > > > > Does every slave have its own negotiated SA?
> > > > 
> > > > Yes and no. Bonding only supports xfrm offload with active-backup mode. So only
> > > > current active slave keep the SA. When active slave changes, the sa on
> > > > previous slave is deleted and re-added on new active slave.
> > > 
> > > It's the same SA, there's no DELSA+NEWSA when we change the active
> > > slave (but we call xdo_dev_state_delete/xdo_dev_state_add to inform
> > > the driver/HW), and only a single NEWSA to install the offloaded SA on
> > > the bond device (which calls the active slave's xdo_dev_state_add).
> > 
> > Yes, thanks for the clarification. The SA is not changed, we just delete it
> > on old active slave
> > 
> > slave->dev->xfrmdev_ops->xdo_dev_state_delete(ipsec->xs);
> > 
> > And add to now one.
> > 
> > ipsec->xs->xso.real_dev = slave->dev;
> > slave->dev->xfrmdev_ops->xdo_dev_state_add(ipsec->xs, NULL)
> 
> Using the same key on two different devices is very dangerous.

It's only used by one device at a time, we only support offload with
"active-backup" mode, where only the current active slave can send
packets.

> Counter mode algorithms have the requirement that the IV
> must not repeat. If you use the same key on two devices,
> you can't guarantee that. If both devices use an internal
> counter (initialized to one) to generate the IV, then the
> IV repeats for the mumber of packets that were already
> sent on the old device. The algorithm is cryptographically
> broken in that case.

Aren't they basing the IV on the sequence number filled in the header?
If not, then I guess this stuff has been broken since 2020 :(
(18cb261afd7b ("bonding: support hardware encryption offload to slaves"))

> Instead of moving the existing state, it is better to
> request a rekey. Maybe by setting the old state to
> 'expired' and then send a km_state_expired() message.

But then you're going to drop packets during the whole rekey?

-- 
Sabrina

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

[Steffen Klassert]

On Thu, Aug 22, 2024 at 10:39:11AM +0200, Sabrina Dubroca wrote:
> 2024-08-22, 09:10:47 +0200, Steffen Klassert wrote:
> > On Thu, Aug 22, 2024 at 08:33:17AM +0800, Hangbin Liu wrote:
> > > On Wed, Aug 21, 2024 at 03:39:48PM +0200, Sabrina Dubroca wrote:
> > > > > > > > +	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.
> > > > > > 
> > > > > > How does xfrm_state offload work on bonding?
> > > > > > Does every slave have its own negotiated SA?
> > > > > 
> > > > > Yes and no. Bonding only supports xfrm offload with active-backup mode. So only
> > > > > current active slave keep the SA. When active slave changes, the sa on
> > > > > previous slave is deleted and re-added on new active slave.
> > > > 
> > > > It's the same SA, there's no DELSA+NEWSA when we change the active
> > > > slave (but we call xdo_dev_state_delete/xdo_dev_state_add to inform
> > > > the driver/HW), and only a single NEWSA to install the offloaded SA on
> > > > the bond device (which calls the active slave's xdo_dev_state_add).
> > > 
> > > Yes, thanks for the clarification. The SA is not changed, we just delete it
> > > on old active slave
> > > 
> > > slave->dev->xfrmdev_ops->xdo_dev_state_delete(ipsec->xs);
> > > 
> > > And add to now one.
> > > 
> > > ipsec->xs->xso.real_dev = slave->dev;
> > > slave->dev->xfrmdev_ops->xdo_dev_state_add(ipsec->xs, NULL)
> > 
> > Using the same key on two different devices is very dangerous.
> 
> It's only used by one device at a time, we only support offload with
> "active-backup" mode, where only the current active slave can send
> packets.
> 
> > Counter mode algorithms have the requirement that the IV
> > must not repeat. If you use the same key on two devices,
> > you can't guarantee that. If both devices use an internal
> > counter (initialized to one) to generate the IV, then the
> > IV repeats for the mumber of packets that were already
> > sent on the old device. The algorithm is cryptographically
> > broken in that case.
> 
> Aren't they basing the IV on the sequence number filled in the header?
> If not, then I guess this stuff has been broken since 2020 :(
> (18cb261afd7b ("bonding: support hardware encryption offload to slaves"))

Linux does that, but it is not guaranteed that other devices do that
too. It is perfectly Ok to use some internal counter (or anything
elase that does not repeat) to generate the IV.

> 
> > Instead of moving the existing state, it is better to
> > request a rekey. Maybe by setting the old state to
> > 'expired' and then send a km_state_expired() message.
> 
> But then you're going to drop packets during the whole rekey?

Yes, I know. That would be the downside of that.

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

[Steffen Klassert]

On Thu, Aug 22, 2024 at 04:33:36PM +0800, Hangbin Liu wrote:
> Hi Steffen,
> On Thu, Aug 22, 2024 at 09:10:47AM +0200, Steffen Klassert wrote:
> > > Yes, thanks for the clarification. The SA is not changed, we just delete it
> > > on old active slave
> > > 
> > > slave->dev->xfrmdev_ops->xdo_dev_state_delete(ipsec->xs);
> > > 
> > > And add to now one.
> > > 
> > > ipsec->xs->xso.real_dev = slave->dev;
> > > slave->dev->xfrmdev_ops->xdo_dev_state_add(ipsec->xs, NULL)
> > 
> > Using the same key on two different devices is very dangerous.
> > Counter mode algorithms have the requirement that the IV
> > must not repeat. If you use the same key on two devices,
> > you can't guarantee that. If both devices use an internal
> > counter (initialized to one) to generate the IV, then the
> > IV repeats for the mumber of packets that were already
> > sent on the old device. The algorithm is cryptographically
> > broken in that case.
> > 
> > Instead of moving the existing state, it is better to
> > request a rekey. Maybe by setting the old state to
> > 'expired' and then send a km_state_expired() message.
> 
> Thanks for your comments. I'm not familiar with IPsec state.
> Do you mean something like
> 
> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
> index f74bacf071fc..8a51d0812564 100644
> --- a/drivers/net/bonding/bond_main.c
> +++ b/drivers/net/bonding/bond_main.c
> @@ -477,6 +477,7 @@ static void bond_ipsec_add_sa_all(struct bonding *bond)
>  	struct net_device *bond_dev = bond->dev;
>  	struct bond_ipsec *ipsec;
>  	struct slave *slave;
> +	struct km_event c;
>  
>  	rcu_read_lock();
>  	slave = rcu_dereference(bond->curr_active_slave);
> @@ -498,6 +499,13 @@ static void bond_ipsec_add_sa_all(struct bonding *bond)
>  	spin_lock_bh(&bond->ipsec_lock);
>  	list_for_each_entry(ipsec, &bond->ipsec_list, list) {
>  		ipsec->xs->xso.real_dev = slave->dev;
> +
> +		ipsec->xs->km.state = XFRM_STATE_VALID;
> +		c.data.hard = 1;
> +		c.portid = 0;
> +		c.event = XFRM_MSG_NEWSA;
> +		km_state_notify(x, &c);

The xfrm stack does that already when inserting the state.

> +
>  		if (slave->dev->xfrmdev_ops->xdo_dev_state_add(ipsec->xs, NULL)) {
>  			slave_warn(bond_dev, slave->dev, "%s: failed to add SA\n", __func__);
>  			ipsec->xs->xso.real_dev = NULL;
> @@ -580,6 +588,8 @@ static void bond_ipsec_del_sa_all(struct bonding *bond)
>  				   "%s: no slave xdo_dev_state_delete\n",
>  				   __func__);
>  		} else {
> +			ipsec->xs->km.state = XFRM_STATE_EXPIRED;

I think you also need to set 'x->km.dying = 1'.

> +			km_state_expired(ipsec->xs, 1, 0);

Please test this at least with libreswan and strongswan. The state is
actually not expired, so not sure if the IKE daemons behave as we want
in that case.

Downside of this approach is that you loose some packets until the new
SA is negotiated, as Sabrina mentioned.

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

[Hangbin Liu]

Hi Steffen,
On Fri, Aug 23, 2024 at 10:24:46AM +0200, Steffen Klassert wrote:
> > Thanks for your comments. I'm not familiar with IPsec state.
> > Do you mean something like
> > 
> > diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
> > index f74bacf071fc..8a51d0812564 100644
> > --- a/drivers/net/bonding/bond_main.c
> > +++ b/drivers/net/bonding/bond_main.c
> > @@ -477,6 +477,7 @@ static void bond_ipsec_add_sa_all(struct bonding *bond)
> >  	struct net_device *bond_dev = bond->dev;
> >  	struct bond_ipsec *ipsec;
> >  	struct slave *slave;
> > +	struct km_event c;
> >  
> >  	rcu_read_lock();
> >  	slave = rcu_dereference(bond->curr_active_slave);
> > @@ -498,6 +499,13 @@ static void bond_ipsec_add_sa_all(struct bonding *bond)
> >  	spin_lock_bh(&bond->ipsec_lock);
> >  	list_for_each_entry(ipsec, &bond->ipsec_list, list) {
> >  		ipsec->xs->xso.real_dev = slave->dev;
> > +
> > +		ipsec->xs->km.state = XFRM_STATE_VALID;
> > +		c.data.hard = 1;
> > +		c.portid = 0;
> > +		c.event = XFRM_MSG_NEWSA;
> > +		km_state_notify(x, &c);
> 
> The xfrm stack does that already when inserting the state.

Thanks for this info.

> 
> > +
> >  		if (slave->dev->xfrmdev_ops->xdo_dev_state_add(ipsec->xs, NULL)) {
> >  			slave_warn(bond_dev, slave->dev, "%s: failed to add SA\n", __func__);
> >  			ipsec->xs->xso.real_dev = NULL;
> > @@ -580,6 +588,8 @@ static void bond_ipsec_del_sa_all(struct bonding *bond)
> >  				   "%s: no slave xdo_dev_state_delete\n",
> >  				   __func__);
> >  		} else {
> > +			ipsec->xs->km.state = XFRM_STATE_EXPIRED;
> 
> I think you also need to set 'x->km.dying = 1'.
> 
> > +			km_state_expired(ipsec->xs, 1, 0);
> 
> Please test this at least with libreswan and strongswan. The state is
> actually not expired, so not sure if the IKE daemons behave as we want
> in that case.

OK, this fix should be not related the current patch set. I will post this fix
after more tests.

Thanks
Hangbin
> 
> Downside of this approach is that you loose some packets until the new
> SA is negotiated, as Sabrina mentioned.