| Message ID | 20100214144415.GA8115@x200 |
|---|---|
| State | Superseded, archived |
| Delegated to: | stephen hemminger |
| Headers | show |
On Sun, Feb 14, 2010 at 04:44:15PM +0200, Alexey Dobriyan wrote: > Consider using ipcomp with tunnel mode: > > pfkey_add -> xfrm_state_init -> x->type->init_state() == ipcomp4_init_state > > 1. If ipcomp_tunnel_attach() fails, xfrm_state private data (x->data) are freed > first time (synchronously), but stale pointer is left. > 2. xfrm_state_init() failed, all right, we're going to do error unwind > but this time asynchronously and we're going to double free x->data > asynchronously. Sorry, I don't see the async path, where is it? Cheers,
On Mon, Feb 15, 2010 at 2:18 AM, Herbert Xu <herbert@gondor.apana.org.au> wrote: > On Sun, Feb 14, 2010 at 04:44:15PM +0200, Alexey Dobriyan wrote: >> Consider using ipcomp with tunnel mode: >> >> pfkey_add -> xfrm_state_init -> x->type->init_state() == ipcomp4_init_state >> >> 1. If ipcomp_tunnel_attach() fails, xfrm_state private data (x->data) are freed >> first time (synchronously), but stale pointer is left. >> 2. xfrm_state_init() failed, all right, we're going to do error unwind >> but this time asynchronously and we're going to double free x->data >> asynchronously. > > Sorry, I don't see the async path, where is it? pfkey_add pfkey_msg2xfrm_state xfrm_init_state [fails] xfrm_state_put __xfrm_state_destroy [puts xfrm_state into GC list, schedule work] xfrm_state_gc_task xfrm_state_gc_destroy x->type->destructor -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Feb 15, 2010 at 09:32:33AM +0200, Alexey Dobriyan wrote: > > pfkey_add > pfkey_msg2xfrm_state > xfrm_init_state [fails] > xfrm_state_put > __xfrm_state_destroy [puts xfrm_state into GC list, schedule work] > xfrm_state_gc_task > xfrm_state_gc_destroy > x->type->destructor Doh, I was looking at the buggy xfrm_state_clone path (which incidently needs to be fixed to use xfrm_state_put). How about just getting rid of the ipcomp_destroy call in ipcomp's init_state functions? Calling the destructor twice is a bit iffy even if we can make it work by setting various things to NULL and testing for it. Cheers,
--- a/net/xfrm/xfrm_ipcomp.c +++ b/net/xfrm/xfrm_ipcomp.c @@ -332,6 +332,7 @@ void ipcomp_destroy(struct xfrm_state *x) ipcomp_free_data(ipcd); mutex_unlock(&ipcomp_resource_mutex); kfree(ipcd); + x->data = NULL; } EXPORT_SYMBOL_GPL(ipcomp_destroy);
Consider using ipcomp with tunnel mode: pfkey_add -> xfrm_state_init -> x->type->init_state() == ipcomp4_init_state 1. If ipcomp_tunnel_attach() fails, xfrm_state private data (x->data) are freed first time (synchronously), but stale pointer is left. 2. xfrm_state_init() failed, all right, we're going to do error unwind but this time asynchronously and we're going to double free x->data asynchronously. Fix by clearing x->data pointer, so second time it'll be fine. Note, second time can happen in quite arbitrary time, double free messages were seen in completely irrelevant functions, e. g. INFO: Allocated in icmp_sk_init INFO: Freed in icmp_sk_exit [<ffffffff810b5ceb>] kfree+0xab/0x140 [<ffffffff810719ae>] free_sect_attrs (!) [<ffffffff81072353>] free_module The only common thing was kmalloc-16 cache. Signed-off-by: Alexey Dobriyan <adobriyan@gmail.com> --- net/xfrm/xfrm_ipcomp.c | 1 + 1 file changed, 1 insertion(+) -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html