[XFRM]: Fix leak of expired xfrm_states

[XFRM]: Fix leak of expired xfrm_states

[Patrick McHardy]

[-- Attachment #1: Type: text/plain, Size: 112 bytes --]

This patch fixes a xfrm_state leak, which appears to be a
regression from the reference count simplifications.


[-- Attachment #2: x --]
[-- Type: text/plain, Size: 1176 bytes --]

commit 817252c2a475371f9764883c7d0f0cde63b3cfe8
Author: Patrick McHardy <kaber@trash.net>
Date:   Mon Nov 26 16:00:50 2007 +0100

    [XFRM]: Fix leak of expired xfrm_states
    
    The xfrm_timer calls __xfrm_state_delete, which drops the final reference
    manually without triggering destruction of the state.
    
    Signed-off-by: Patrick McHardy <kaber@trash.net>

diff --git a/net/xfrm/xfrm_state.c b/net/xfrm/xfrm_state.c
index 224b44e..11e9a48 100644
--- a/net/xfrm/xfrm_state.c
+++ b/net/xfrm/xfrm_state.c
@@ -416,7 +416,7 @@ static inline unsigned long make_jiffies(long secs)
 
 static void xfrm_timer_handler(unsigned long data)
 {
-	struct xfrm_state *x = (struct xfrm_state*)data;
+	struct xfrm_state *x = (struct xfrm_state*)data, *del = NULL;
 	unsigned long now = get_seconds();
 	long next = LONG_MAX;
 	int warn = 0;
@@ -479,6 +479,8 @@ expired:
 		goto resched;
 	}
 
+	del = x;
+	xfrm_state_hold(del);
 	err = __xfrm_state_delete(x);
 	if (!err && x->id.spi)
 		km_state_expired(x, 1, 0);
@@ -488,6 +490,8 @@ expired:
 
 out:
 	spin_unlock(&x->lock);
+	if (del)
+		xfrm_state_put(del);
 }
 
 static void xfrm_replay_timer_handler(unsigned long data);

Re: [XFRM]: Fix leak of expired xfrm_states

[Herbert Xu]

On Mon, Nov 26, 2007 at 04:05:27PM +0100, Patrick McHardy wrote:
> This patch fixes a xfrm_state leak, which appears to be a
> regression from the reference count simplifications.

I was going to say this was a good find :)

But digging deeper it seems that it might not be a bug after
all.  Even though the ref count on x may now drop to zero, it
won't be freed until del_timer_sync returns which should be
sufficient, no?

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

Re: [XFRM]: Fix leak of expired xfrm_states

[Patrick McHardy]

Herbert Xu wrote:
> On Mon, Nov 26, 2007 at 04:05:27PM +0100, Patrick McHardy wrote:
>> This patch fixes a xfrm_state leak, which appears to be a
>> regression from the reference count simplifications.
> 
> I was going to say this was a good find :)
> 
> But digging deeper it seems that it might not be a bug after
> all.  Even though the ref count on x may now drop to zero, it
> won't be freed until del_timer_sync returns which should be
> sufficient, no?


It actually won't get freed at all currently since nothing is
calling __xfrm_state_destroy(). __xfrm_state_delete() uses
__xfrm_state_put(), which only decrements the refcount, but
doesn't perform destruction.

This is visible when looking at the xfrm[46]_mode_{tunnel,transport}
module reference counts, they climb higher and higher over time.

Re: [XFRM]: Fix leak of expired xfrm_states

[Herbert Xu]

On Mon, Nov 26, 2007 at 04:51:42PM +0100, Patrick McHardy wrote:
>
> It actually won't get freed at all currently since nothing is
> calling __xfrm_state_destroy(). __xfrm_state_delete() uses
> __xfrm_state_put(), which only decrements the refcount, but
> doesn't perform destruction.
> 
> This is visible when looking at the xfrm[46]_mode_{tunnel,transport}
> module reference counts, they climb higher and higher over time.

Oh I see.  How about just removing those double underscores then?

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

Re: [XFRM]: Fix leak of expired xfrm_states

[Patrick McHardy]

Herbert Xu wrote:
> On Mon, Nov 26, 2007 at 04:51:42PM +0100, Patrick McHardy wrote:
>> It actually won't get freed at all currently since nothing is
>> calling __xfrm_state_destroy(). __xfrm_state_delete() uses
>> __xfrm_state_put(), which only decrements the refcount, but
>> doesn't perform destruction.
>>
>> This is visible when looking at the xfrm[46]_mode_{tunnel,transport}
>> module reference counts, they climb higher and higher over time.
> 
> Oh I see.  How about just removing those double underscores then?


That should work as long as we keep the del_timer_sync to avoid
a use-after-free. It seems a bit fragile though.

Re: [XFRM]: Fix leak of expired xfrm_states

[Herbert Xu]

On Mon, Nov 26, 2007 at 04:56:01PM +0100, Patrick McHardy wrote:
>
> That should work as long as we keep the del_timer_sync to avoid
> a use-after-free. It seems a bit fragile though.

Well we're relying on the del_timer_sync already to avoid the
ref count on the timer.  Otherwise if the admin deletes the
SA while the timer is running it'll go up in smoke too.

If you look in the history you'll find that the same patch
that removed the ref count on the timer introduced the call
to del_timer_sync :)

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

Re: [XFRM]: Fix leak of expired xfrm_states

[Patrick McHardy]

[-- Attachment #1: Type: text/plain, Size: 752 bytes --]

Herbert Xu wrote:
> On Mon, Nov 26, 2007 at 04:56:01PM +0100, Patrick McHardy wrote:
>> That should work as long as we keep the del_timer_sync to avoid
>> a use-after-free. It seems a bit fragile though.
> 
> Well we're relying on the del_timer_sync already to avoid the
> ref count on the timer.  Otherwise if the admin deletes the
> SA while the timer is running it'll go up in smoke too.
> 
> If you look in the history you'll find that the same patch
> that removed the ref count on the timer introduced the call
> to del_timer_sync :)


OK, here's a patch to use xfrm_state_put in __xfrm_state_delete().
I've checked the other callers and it should be fine. lock ordering
between x->lock and xfrm_state_gc_lock also doesn't seem to be an
issue.



[-- Attachment #2: x --]
[-- Type: text/plain, Size: 975 bytes --]

commit ba63b1baf5d8a63f3bb3097a7201de75c1b77e2d
Author: Patrick McHardy <kaber@trash.net>
Date:   Mon Nov 26 16:00:50 2007 +0100

    [XFRM]: Fix leak of expired xfrm_states

    The xfrm_timer calls __xfrm_state_delete, which drops the final reference
    manually without triggering destruction of the state. Change it to use
    xfrm_state_put to add the state to the gc list when we're dropping the
    last reference. The timer function may still continue to use the state
    safely since the final destruction does a del_timer_sync().

    Signed-off-by: Patrick McHardy <kaber@trash.net>

diff --git a/net/xfrm/xfrm_state.c b/net/xfrm/xfrm_state.c
index 224b44e..cf43c49 100644
--- a/net/xfrm/xfrm_state.c
+++ b/net/xfrm/xfrm_state.c
@@ -552,7 +552,7 @@ int __xfrm_state_delete(struct xfrm_state *x)
 		 * The xfrm_state_alloc call gives a reference, and that
 		 * is what we are dropping here.
 		 */
-		__xfrm_state_put(x);
+		xfrm_state_put(x);
 		err = 0;
 	}
 

Re: [XFRM]: Fix leak of expired xfrm_states

[Herbert Xu]

On Mon, Nov 26, 2007 at 05:52:15PM +0100, Patrick McHardy wrote:
>
> OK, here's a patch to use xfrm_state_put in __xfrm_state_delete().
> I've checked the other callers and it should be fine. lock ordering
> between x->lock and xfrm_state_gc_lock also doesn't seem to be an
> issue.

Patch applied.  Thanks a lot Patrick!
-- 
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