Patchwork [v2] sctp: Fix a race between ICMP protocol unreachable and connect()

login
register
mail settings
Submitter Wei Yongjun
Date May 10, 2010, 2:56 a.m.
Message ID <4BE775C7.9070702@cn.fujitsu.com>
Download mbox | patch
Permalink /patch/52006/
State Accepted
Delegated to: David Miller
Headers show

Comments

Wei Yongjun - May 10, 2010, 2:56 a.m.
Hi Vlad:

> [.. removed leftover debuggin printk. should probably be queued for stable
>  as well... ]
>
> ICMP protocol unreachable handling completely disregarded
> the fact that the user may have locket the socket.  It proceeded
> to destroy the association, even though the user may have
> held the lock and had a ref on the association.  This resulted
> in the following:
>
> Attempt to release alive inet socket f6afcc00
>
> =========================
> [ BUG: held lock freed! ]
> -------------------------
> somenu/2672 is freeing memory f6afcc00-f6afcfff, with a lock still held
> there!
>  (sk_lock-AF_INET){+.+.+.}, at: [<c122098a>] sctp_connect+0x13/0x4c
> 1 lock held by somenu/2672:
>  #0:  (sk_lock-AF_INET){+.+.+.}, at: [<c122098a>] sctp_connect+0x13/0x4c
>
> stack backtrace:
> Pid: 2672, comm: somenu Not tainted 2.6.32-telco #55
> Call Trace:
>  [<c1232266>] ? printk+0xf/0x11
>  [<c1038553>] debug_check_no_locks_freed+0xce/0xff
>  [<c10620b4>] kmem_cache_free+0x21/0x66
>  [<c1185f25>] __sk_free+0x9d/0xab
>  [<c1185f9c>] sk_free+0x1c/0x1e
>  [<c1216e38>] sctp_association_put+0x32/0x89
>  [<c1220865>] __sctp_connect+0x36d/0x3f4
>  [<c122098a>] ? sctp_connect+0x13/0x4c
>  [<c102d073>] ? autoremove_wake_function+0x0/0x33
>  [<c12209a8>] sctp_connect+0x31/0x4c
>  [<c11d1e80>] inet_dgram_connect+0x4b/0x55
>  [<c11834fa>] sys_connect+0x54/0x71
>  [<c103a3a2>] ? lock_release_non_nested+0x88/0x239
>  [<c1054026>] ? might_fault+0x42/0x7c
>  [<c1054026>] ? might_fault+0x42/0x7c
>  [<c11847ab>] sys_socketcall+0x6d/0x178
>  [<c10da994>] ? trace_hardirqs_on_thunk+0xc/0x10
>  [<c1002959>] syscall_call+0x7/0xb
>
> This was because the sctp_wait_for_connect() would aqcure the socket
> lock and then proceed to release the last reference count on the
> association, thus cause the fully destruction path to finish freeing
> the socket.
>
> The simplest solution is to start a very short timer in case the socket
> is owned by user.  When the timer expires, we can do some verification
> and be able to do the release properly.
>   

After reviewed this patch, I think we should delete active ICMP proto
unreachable timer when free transport. since I don't reproduce this BUG,
so I just do compile test only, sorry.

[PATCH] sctp: delete active ICMP proto unreachable timer when free transport

transport may be free before ICMP proto unreachable timer expire, so
we should delete active ICMP proto unreachable timer when transport
is going away.

Signed-off-by: Wei Yongjun <yjwei@cn.fujitsu.com>
---
 net/sctp/transport.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)
Vlad Yasevich - May 10, 2010, 1:59 p.m.
Wei Yongjun wrote:
> Hi Vlad:
> 
>> [.. removed leftover debuggin printk. should probably be queued for stable
>>  as well... ]
>>
>> ICMP protocol unreachable handling completely disregarded
>> the fact that the user may have locket the socket.  It proceeded
>> to destroy the association, even though the user may have
>> held the lock and had a ref on the association.  This resulted
>> in the following:
>>
>> Attempt to release alive inet socket f6afcc00
>>
>> =========================
>> [ BUG: held lock freed! ]
>> -------------------------
>> somenu/2672 is freeing memory f6afcc00-f6afcfff, with a lock still held
>> there!
>>  (sk_lock-AF_INET){+.+.+.}, at: [<c122098a>] sctp_connect+0x13/0x4c
>> 1 lock held by somenu/2672:
>>  #0:  (sk_lock-AF_INET){+.+.+.}, at: [<c122098a>] sctp_connect+0x13/0x4c
>>
>> stack backtrace:
>> Pid: 2672, comm: somenu Not tainted 2.6.32-telco #55
>> Call Trace:
>>  [<c1232266>] ? printk+0xf/0x11
>>  [<c1038553>] debug_check_no_locks_freed+0xce/0xff
>>  [<c10620b4>] kmem_cache_free+0x21/0x66
>>  [<c1185f25>] __sk_free+0x9d/0xab
>>  [<c1185f9c>] sk_free+0x1c/0x1e
>>  [<c1216e38>] sctp_association_put+0x32/0x89
>>  [<c1220865>] __sctp_connect+0x36d/0x3f4
>>  [<c122098a>] ? sctp_connect+0x13/0x4c
>>  [<c102d073>] ? autoremove_wake_function+0x0/0x33
>>  [<c12209a8>] sctp_connect+0x31/0x4c
>>  [<c11d1e80>] inet_dgram_connect+0x4b/0x55
>>  [<c11834fa>] sys_connect+0x54/0x71
>>  [<c103a3a2>] ? lock_release_non_nested+0x88/0x239
>>  [<c1054026>] ? might_fault+0x42/0x7c
>>  [<c1054026>] ? might_fault+0x42/0x7c
>>  [<c11847ab>] sys_socketcall+0x6d/0x178
>>  [<c10da994>] ? trace_hardirqs_on_thunk+0xc/0x10
>>  [<c1002959>] syscall_call+0x7/0xb
>>
>> This was because the sctp_wait_for_connect() would aqcure the socket
>> lock and then proceed to release the last reference count on the
>> association, thus cause the fully destruction path to finish freeing
>> the socket.
>>
>> The simplest solution is to start a very short timer in case the socket
>> is owned by user.  When the timer expires, we can do some verification
>> and be able to do the release properly.
>>   
> 
> After reviewed this patch, I think we should delete active ICMP proto
> unreachable timer when free transport. since I don't reproduce this BUG,
> so I just do compile test only, sorry.

Hi Wei

To reproduce with the original code (before my patch), add the following rule

# iptables -A INPUT -p sctp -j REJECT --reject-with icmp-proto-unreachable

and then try connecting to the localhost.  I was never able to reproduce the race
on any kind of network, but on localhost it happens every time.

> 
> [PATCH] sctp: delete active ICMP proto unreachable timer when free transport
> 
> transport may be free before ICMP proto unreachable timer expire, so
> we should delete active ICMP proto unreachable timer when transport
> is going away.
> 
> Signed-off-by: Wei Yongjun <yjwei@cn.fujitsu.com>
> ---
>  net/sctp/transport.c |    4 ++++
>  1 files changed, 4 insertions(+), 0 deletions(-)
> 
> diff --git a/net/sctp/transport.c b/net/sctp/transport.c
> index 4a36803..165d54e 100644
> --- a/net/sctp/transport.c
> +++ b/net/sctp/transport.c
> @@ -173,6 +173,10 @@ void sctp_transport_free(struct sctp_transport *transport)
>  	    del_timer(&transport->T3_rtx_timer))
>  		sctp_transport_put(transport);
>  
> +	/* Delete the ICMP proto unreachable timer if it's active. */
> +	if (timer_pending(&transport->proto_unreach_timer) &&
> +	    del_timer(&transport->proto_unreach_timer))
> +		sctp_association_put(transport->asoc);
>  
>  	sctp_transport_put(transport);
>  }

ACK.  This fixes a race against close().  Although that will be fairly hard to
do, it is possible.

Acked-by: Vlad Yasevich <vladislav.yasevich@hp.com>

-vlad
--
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
David Miller - May 16, 2010, 7:46 a.m.
From: Vlad Yasevich <vladislav.yasevich@hp.com>
Date: Mon, 10 May 2010 09:59:32 -0400

> 
> 
> Wei Yongjun wrote:
>> [PATCH] sctp: delete active ICMP proto unreachable timer when free transport
>> 
>> transport may be free before ICMP proto unreachable timer expire, so
>> we should delete active ICMP proto unreachable timer when transport
>> is going away.
>> 
>> Signed-off-by: Wei Yongjun <yjwei@cn.fujitsu.com>
 ...
> ACK.  This fixes a race against close().  Although that will be fairly hard to
> do, it is possible.
> 
> Acked-by: Vlad Yasevich <vladislav.yasevich@hp.com>

Applied, thanks.
--
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

Patch

diff --git a/net/sctp/transport.c b/net/sctp/transport.c
index 4a36803..165d54e 100644
--- a/net/sctp/transport.c
+++ b/net/sctp/transport.c
@@ -173,6 +173,10 @@  void sctp_transport_free(struct sctp_transport *transport)
 	    del_timer(&transport->T3_rtx_timer))
 		sctp_transport_put(transport);
 
+	/* Delete the ICMP proto unreachable timer if it's active. */
+	if (timer_pending(&transport->proto_unreach_timer) &&
+	    del_timer(&transport->proto_unreach_timer))
+		sctp_association_put(transport->asoc);
 
 	sctp_transport_put(transport);
 }