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

[PATCHv4 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.

v4: Ratelimit pr_warn (Sabrina Dubroca)
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 | 97 ++++++++++++++++++++++++++++-----
 1 file changed, 84 insertions(+), 13 deletions(-)

-- 
2.45.0

[PATCHv4 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>
Reviewed-by: Nikolay Aleksandrov <razor@blackwall.org>
Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
---
 drivers/net/bonding/bond_main.c | 47 ++++++++++++++++++++++++---------
 1 file changed, 34 insertions(+), 13 deletions(-)

diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index f9633a6f8571..fe10ac66f26e 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -418,6 +418,38 @@ 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)
+		pr_warn_ratelimited("%s: (slave %s): not same with IPsec offload real dev %s\n",
+				    bond_dev->name, slave->dev->name, 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 +627,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

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

[Jakub Kicinski]

On Wed, 21 Aug 2024 18:50:01 +0800 Hangbin Liu wrote:
> +/**
> + * 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
> + **/

in addition to the feedback on v3, nit: document the return value in
kdoc for non-void functions

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

[Hangbin Liu]

Hi Jakub,
On Tue, Aug 27, 2024 at 01:06:19PM -0700, Jakub Kicinski wrote:
> On Wed, 21 Aug 2024 18:50:01 +0800 Hangbin Liu wrote:
> > +/**
> > + * 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
> > + **/
> 
> in addition to the feedback on v3, nit: document the return value in
> kdoc for non-void functions

I already document the return value. Do you want me to change the format like:

/**
 * bond_ipsec_dev - Get active device for IPsec offload,
 *                  caller must hold rcu_read_lock.
 * @xs: pointer to transformer state struct
 *
 * Return the device for ipsec offload, or NULL if not exist.
 **/

BTW, The patch now has conflicts with latest net-next, I can do a rebase if
you want.

Thanks
Hangbin

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

[Jakub Kicinski]

On Wed, 28 Aug 2024 09:14:37 +0800 Hangbin Liu wrote:
> On Tue, Aug 27, 2024 at 01:06:19PM -0700, Jakub Kicinski wrote:
> > On Wed, 21 Aug 2024 18:50:01 +0800 Hangbin Liu wrote:  
> > > +/**
> > > + * 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
> > > + **/  
> > 
> > in addition to the feedback on v3, nit: document the return value in
> > kdoc for non-void functions  
> 
> I already document the return value. Do you want me to change the format like:
> 
> /**
>  * bond_ipsec_dev - Get active device for IPsec offload,
>  *                  caller must hold rcu_read_lock.
>  * @xs: pointer to transformer state struct
>  *
>  * Return the device for ipsec offload, or NULL if not exist.
>  **/

Yes, but still a bit too much info in the "short description"
how about:

/**
 * bond_ipsec_dev - Get active device for IPsec offload
 * @xs: pointer to transformer state struct
 *
 * Context: caller must hold rcu_read_lock.
 *
 * Return the device for ipsec offload, or NULL if not exist.
 **/

> BTW, The patch now has conflicts with latest net-next, I can do a rebase if
> you want.

net or net-next? the patches from nvidia went into net.
If it conflicts with net-next please rebase.
If it conflicts with net -- could you wait with repost
until after the net PR to Linus? And then rebase & post?

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

[Hangbin Liu]

On Tue, Aug 27, 2024 at 07:28:01PM -0700, Jakub Kicinski wrote:
> 
> Yes, but still a bit too much info in the "short description"
> how about:
> 
> /**
>  * bond_ipsec_dev - Get active device for IPsec offload
>  * @xs: pointer to transformer state struct
>  *
>  * Context: caller must hold rcu_read_lock.
>  *
>  * Return the device for ipsec offload, or NULL if not exist.
>  **/

OK, thanks for the update :)

> 
> > BTW, The patch now has conflicts with latest net-next, I can do a rebase if
> > you want.
> 
> net or net-next? the patches from nvidia went into net.
> If it conflicts with net-next please rebase.

It conflicts with Nikolay's bond xfrm fixes, which already in net-next.
I will do a rebase.

Thanks
Hangbin

> If it conflicts with net -- could you wait with repost
> until after the net PR to Linus? And then rebase & post?

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

[Simon Horman]

On Wed, Aug 28, 2024 at 09:14:37AM +0800, Hangbin Liu wrote:
> Hi Jakub,
> On Tue, Aug 27, 2024 at 01:06:19PM -0700, Jakub Kicinski wrote:
> > On Wed, 21 Aug 2024 18:50:01 +0800 Hangbin Liu wrote:
> > > +/**
> > > + * 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
> > > + **/
> > 
> > in addition to the feedback on v3, nit: document the return value in
> > kdoc for non-void functions
> 
> I already document the return value. Do you want me to change the format like:
> 
> /**
>  * bond_ipsec_dev - Get active device for IPsec offload,
>  *                  caller must hold rcu_read_lock.
>  * @xs: pointer to transformer state struct
>  *
>  * Return the device for ipsec offload, or NULL if not exist.
>  **/

nit-pick: I think that the ./scripts/kernel-doc expects "Return: " or
          "Returns: "

> BTW, The patch now has conflicts with latest net-next, I can do a rebase if
> you want.
> 
> Thanks
> Hangbin
> 

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

[Hangbin Liu]

On Wed, Aug 28, 2024 at 03:43:26PM +0100, Simon Horman wrote:
> On Wed, Aug 28, 2024 at 09:14:37AM +0800, Hangbin Liu wrote:
> > Hi Jakub,
> > On Tue, Aug 27, 2024 at 01:06:19PM -0700, Jakub Kicinski wrote:
> > > On Wed, 21 Aug 2024 18:50:01 +0800 Hangbin Liu wrote:
> > > > +/**
> > > > + * 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
> > > > + **/
> > > 
> > > in addition to the feedback on v3, nit: document the return value in
> > > kdoc for non-void functions
> > 
> > I already document the return value. Do you want me to change the format like:
> > 
> > /**
> >  * bond_ipsec_dev - Get active device for IPsec offload,
> >  *                  caller must hold rcu_read_lock.
> >  * @xs: pointer to transformer state struct
> >  *
> >  * Return the device for ipsec offload, or NULL if not exist.
> >  **/
> 
> nit-pick: I think that the ./scripts/kernel-doc expects "Return: " or
>           "Returns: "

Thanks, I will post a new version for this.

Hangbin

[PATCHv4 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 fe10ac66f26e..2fffc5956545 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -650,10 +650,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_ratelimited("%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

[PATCHv4 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 2fffc5956545..e4e73664cd35 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -674,11 +674,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_ratelimited("%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: [PATCHv4 net-next 0/3] Bonding: support new xfrm state offload functions

[Jay Vosburgh]

Hangbin Liu <liuhangbin@gmail.com> wrote:

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

	These look ok to me from a code point of view, but I'm not
familiar enough with IPsec to comment on whether those aspects are
correct.  A cursory examiniation suggests that none of the new functions
being called might sleep.

	For the series:

Acked-by: Jay Vosburgh <jv@jvosburgh.net>

	-J

>v4: Ratelimit pr_warn (Sabrina Dubroca)
>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 | 97 ++++++++++++++++++++++++++++-----
> 1 file changed, 84 insertions(+), 13 deletions(-)
>
>-- 
>2.45.0

---
	-Jay Vosburgh, jv@jvosburgh.net