[patch] xfrm_policy destructor fix

[patch] xfrm_policy destructor fix

[Ingo Molnar]

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 <mingo@elte.hu>

--- linux/net/xfrm/xfrm_policy.c.orig
+++ linux/net/xfrm/xfrm_policy.c
@@ -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

Re: [patch] xfrm_policy destructor fix

[David S. Miller]

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.
> 
> Signed-off-by: Ingo Molnar <mingo@elte.hu>

Good catch Ingo, patch applied.  Thanks.

Re: [patch] xfrm_policy destructor fix

[Herbert Xu]

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

Re: [patch] xfrm_policy destructor fix

[David S. Miller]

On Sat, 26 Mar 2005 12:11:45 +1100
Herbert Xu <herbert@gondor.apana.org.au> wrote:

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

Applied, thanks Herbert.