netdev - Re: [PATCHv2 net 1/3] bonding: move mutex lock to a work queue for XFRM GC tasks

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: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <Z77jc8AB3D2xWczl@fedora>
Date: Wed, 26 Feb 2025 09:48:35 +0000
From: Hangbin Liu <liuhangbin@...il.com>
To: Cosmin Ratiu <cratiu@...dia.com>
Cc: "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
	"andrew+netdev@...n.ch" <andrew+netdev@...n.ch>,
	"jarod@...hat.com" <jarod@...hat.com>,
	"razor@...ckwall.org" <razor@...ckwall.org>,
	"davem@...emloft.net" <davem@...emloft.net>,
	Tariq Toukan <tariqt@...dia.com>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"shuah@...nel.org" <shuah@...nel.org>,
	"steffen.klassert@...unet.com" <steffen.klassert@...unet.com>,
	"jv@...sburgh.net" <jv@...sburgh.net>,
	"pabeni@...hat.com" <pabeni@...hat.com>,
	"horms@...nel.org" <horms@...nel.org>,
	"edumazet@...gle.com" <edumazet@...gle.com>,
	"kuba@...nel.org" <kuba@...nel.org>,
	"linux-kselftest@...r.kernel.org" <linux-kselftest@...r.kernel.org>,
	Jianbo Liu <jianbol@...dia.com>
Subject: Re: [PATCHv2 net 1/3] bonding: move mutex lock to a work queue for
 XFRM GC tasks

Hi Cosmin,
On Tue, Feb 25, 2025 at 02:00:05PM +0000, Cosmin Ratiu wrote:
> This got me to stare at the code again. What if we move the removal of
> the xs from bond->ipsec from bond_ipsec_del_sa to bond_ipsec_free_sa?
> bond_ipsec_free_sa, unlike bond_ipsec_del_sa, is not called with x-
> >lock held. It is called from the xfrm gc task or directly via
> xfrm_state_put_sync and therefore wouldn't suffer from the locking
> issue.
> 
> The tricky part is to make sure that inactive bond->ipsec entries
> (after bond_ipsec_del_sa calls) do not cause issues if there's a
> migration (bond_ipsec_del_sa_all is called) happening before
> bond_ipsec_free_sa. Perhaps filtering by x->km.state != XFRM_STATE_DEAD
> in bond_ipsec_del_sa_all.
> 
> What do you think about this idea?

Thanks a lot for the comments. I also skipped the DEAD xs in add_sa_all.
What about the patch like:

diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index e45bba240cbc..0e4db43a833a 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -537,6 +537,12 @@ static void bond_ipsec_add_sa_all(struct bonding *bond)
 	}
 
 	list_for_each_entry(ipsec, &bond->ipsec_list, list) {
+		/* No need to handle DEAD XFRM, as it has already been
+		 * deleted and will be freed later.
+		 */
+		if (ipsec->xs->km.state == XFRM_STATE_DEAD)
+			continue;
+
 		/* If new state is added before ipsec_lock acquired */
 		if (ipsec->xs->xso.real_dev == real_dev)
 			continue;
@@ -592,15 +598,6 @@ static void bond_ipsec_del_sa(struct xfrm_state *xs)
 	real_dev->xfrmdev_ops->xdo_dev_state_delete(xs);
 out:
 	netdev_put(real_dev, &tracker);
-	mutex_lock(&bond->ipsec_lock);
-	list_for_each_entry(ipsec, &bond->ipsec_list, list) {
-		if (ipsec->xs == xs) {
-			list_del(&ipsec->list);
-			kfree(ipsec);
-			break;
-		}
-	}
-	mutex_unlock(&bond->ipsec_lock);
 }
 
 static void bond_ipsec_del_sa_all(struct bonding *bond)
@@ -617,6 +614,12 @@ static void bond_ipsec_del_sa_all(struct bonding *bond)
 
 	mutex_lock(&bond->ipsec_lock);
 	list_for_each_entry(ipsec, &bond->ipsec_list, list) {
+		/* No need to handle DEAD XFRM, as it has already been
+		 * deleted and will be freed later.
+		 */
+		if (ipsec->xs->km.state == XFRM_STATE_DEAD)
+			continue;
+
 		if (!ipsec->xs->xso.real_dev)
 			continue;
 
@@ -666,6 +669,16 @@ static void bond_ipsec_free_sa(struct xfrm_state *xs)
 		real_dev->xfrmdev_ops->xdo_dev_state_free(xs);
 out:
 	netdev_put(real_dev, &tracker);
+
+	mutex_lock(&bond->ipsec_lock);
+	list_for_each_entry(ipsec, &bond->ipsec_list, list) {
+		if (ipsec->xs == xs) {
+			list_del(&ipsec->list);
+			kfree(ipsec);
+			break;
+		}
+	}
+	mutex_unlock(&bond->ipsec_lock);
 }
 
 /**
-- 
2.46.0