Skip to contentSkip to conversation

xfrm_policy destructor fix

PATCH Applied Ingo Molnar 3 participants 4 messages Mar 25, 2005 – Apr 1, 2005

Subsystems (2)
Networking Networking IPsec
lore
Share & tools
lore

Ingo Molnar

author
#0 lore raw

PATCH 1/1 xfrm_policy destructor fix

the patch below fixes a bug that i encountered while running a PREEMPT_RT kernel, but i believe it should be fixed in the generic kernel too. xfrm_policy_kill() queues a destroyed policy structure to the GC list, and unlocks the policy->lock spinlock _after_ that point. This created a scenario where GC processing got to the new structure first, and kfree()d it - then the write_unlock_bh() was done on the already kfreed structure. There is no guarantee that GC processing will be done after policy->lock has been dropped and softirq processing has been enabled.

Signed-off-by Ingo Molnar <[email protected]>
net/xfrm/xfrm_policy.c renamed from net/xfrm/xfrm_policy.c.orig +10 -6
@@ -301,18 +301,22 @@ static void xfrm_policy_gc_task(void *da
 static void xfrm_policy_kill(struct xfrm_policy *policy)
 {
 	write_lock_bh(&policy->lock);
-	if (policy->dead)
-		goto out;
-
+	if (policy->dead) {
+		write_unlock_bh(&policy->lock);
+		return;
+	}
 	policy->dead = 1;
 
 	spin_lock(&xfrm_policy_gc_lock);
 	list_add(&policy->list, &xfrm_policy_gc_list);
+	/*
+	 * Unlock the policy (out of order unlocking), to make sure
+	 * the GC context does not free it with an active lock:
+	 */
+	write_unlock_bh(&policy->lock);
 	spin_unlock(&xfrm_policy_gc_lock);
-	schedule_work(&xfrm_policy_gc_work);
 
-out:
-	write_unlock_bh(&policy->lock);
+	schedule_work(&xfrm_policy_gc_work);
 }
 
 /* Generate new index... KAME seems to generate them ordered by cost
maintainer
#1 lore raw
Quoted 11 lines · On Fri, 25 Mar 2005 15:34:41 +0100 Ingo Molnar <[email protected]> wrote:

the patch below fixes a bug that i encountered while running a PREEMPT_RT kernel, but i believe it should be fixed in the generic kernel too. xfrm_policy_kill() queues a destroyed policy structure to the GC list, and unlocks the policy->lock spinlock _after_ that point. This created a scenario where GC processing got to the new structure first, and kfree()d it - then the write_unlock_bh() was done on the already kfreed structure. There is no guarantee that GC processing will be done after policy->lock has been dropped and softirq processing has been enabled.

Signed-off-by Ingo Molnar <[email protected]>

Good catch Ingo, patch applied. Thanks.

maintainer
#2 lore raw
Quoted 13 lines · David S. Miller
Quoted 9 lines · On Fri, 25 Mar 2005 15:34:41 +0100 Ingo Molnar <[email protected]> wrote:

the patch below fixes a bug that i encountered while running a PREEMPT_RT kernel, but i believe it should be fixed in the generic kernel too. xfrm_policy_kill() queues a destroyed policy structure to the GC list, and unlocks the policy->lock spinlock _after_ that point. This created a scenario where GC processing got to the new structure first, and kfree()d it - then the write_unlock_bh() was done on the already kfreed structure. There is no guarantee that GC processing will be done after policy->lock has been dropped and softirq processing has been enabled.

Quoted 1 line

Good catch Ingo, patch applied. Thanks.

Sorry, that was my fault. I dropped the GC code inside the locks without thinking.

In fact, the GC list linking doesn't need to sit inside the locks at all. The locks are there to guard the setting of policy->dead only.

So here is a patch to simplify xfrm_policy_kill() by moving the GC linking after the write_unlock_bh().

Actually, as the code stands, xfrm_policy_kill() should/will never be called twice on the same policy. So we can add a warning to catch that.

Signed-off-by Herbert Xu <[email protected]>

Cheers,

Signature · 45 lines
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
--
===== net/xfrm/xfrm_policy.c 1.74 vs edited =====
--- 1.74/net/xfrm/xfrm_policy.c	2005-03-26 04:25:09 +11:00
+++ edited/net/xfrm/xfrm_policy.c	2005-03-26 12:07:08 +11:00
@@ -13,6 +13,7 @@
  * 	
  */
 
+#include <asm/bug.h>
 #include <linux/config.h>
 #include <linux/slab.h>
 #include <linux/kmod.h>
@@ -300,20 +301,20 @@
 
 static void xfrm_policy_kill(struct xfrm_policy *policy)
 {
+	int dead;
+
 	write_lock_bh(&policy->lock);
-	if (policy->dead) {
-		write_unlock_bh(&policy->lock);
+	dead = policy->dead;
+	policy->dead = 1;
+	write_unlock_bh(&policy->lock);
+
+	if (unlikely(dead)) {
+		WARN_ON(1);
 		return;
 	}
-	policy->dead = 1;
 
 	spin_lock(&xfrm_policy_gc_lock);
 	list_add(&policy->list, &xfrm_policy_gc_list);
-	/*
-	 * Unlock the policy (out of order unlocking), to make sure
-	 * the GC context does not free it with an active lock:
-	 */
-	write_unlock_bh(&policy->lock);
 	spin_unlock(&xfrm_policy_gc_lock);
 
 	schedule_work(&xfrm_policy_gc_work);
maintainer Applied
#3 lore raw
Quoted 8 lines · On Sat, 26 Mar 2005 12:11:45 +1100 Herbert Xu <[email protected]>…

So here is a patch to simplify xfrm_policy_kill() by moving the GC linking after the write_unlock_bh().

Actually, as the code stands, xfrm_policy_kill() should/will never be called twice on the same policy. So we can add a warning to catch that.

Signed-off-by Herbert Xu <[email protected]>

Applied, thanks Herbert.