Re: [patch] xfrm_policy destructor fix

[Herbert Xu <herbert@gondor.apana.org.au>]

David S. Miller <davem@davemloft.net> wrote:
> On Fri, 25 Mar 2005 15:34:41 +0100
> Ingo Molnar <mingo@elte.hu> 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.
> 
> 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 <herbert@gondor.apana.org.au>

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
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);
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/