diff mbox series

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

Message ID 20240821105003.547460-2-liuhangbin@gmail.com (mailing list archive)
State Superseded
Delegated to: Netdev Maintainers
Headers show
Series Bonding: support new xfrm state offload functions | expand

Checks

Context Check Description
netdev/series_format success Posting correctly formatted
netdev/tree_selection success Clearly marked for net-next
netdev/ynl success Generated files up to date; no warnings/errors; no diff in generated;
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 16 this patch: 16
netdev/build_tools success No tools touched, skip
netdev/cc_maintainers warning 2 maintainers not CCed: jv@jvosburgh.net andy@greyhouse.net
netdev/build_clang success Errors and warnings before: 16 this patch: 16
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 16 this patch: 16
netdev/checkpatch warning WARNING: line length of 94 exceeds 80 columns
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc fail Errors and warnings before: 13 this patch: 14
netdev/source_inline success Was 0 now: 0

Commit Message

Hangbin Liu Aug. 21, 2024, 10:50 a.m. UTC 
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(-)

Comments

Jakub Kicinski Aug. 27, 2024, 8:06 p.m. UTC | #1 
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
Hangbin Liu Aug. 28, 2024, 1:14 a.m. UTC | #2 
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
Jakub Kicinski Aug. 28, 2024, 2:28 a.m. UTC | #3 
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?
Hangbin Liu Aug. 28, 2024, 3:30 a.m. UTC | #4 
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?
Simon Horman Aug. 28, 2024, 2:43 p.m. UTC | #5 
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
>
Hangbin Liu Aug. 29, 2024, 9:28 a.m. UTC | #6 
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
diff mbox series

Patch

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