* [PATCH] sctp: Fix a race between ICMP protocol unreachable and connect()
@ 2010-05-05 19:29 Vlad Yasevich
2010-05-05 19:36 ` [PATCH v2] " Vlad Yasevich
0 siblings, 1 reply; 6+ messages in thread
From: Vlad Yasevich @ 2010-05-05 19:29 UTC (permalink / raw)
To: davem; +Cc: netdev, linux-sctp, Vlad Yasevich
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.
Signed-off-by: Vlad Yasevich <vladislav.yasevich@hp.com>
---
include/net/sctp/sm.h | 1 +
include/net/sctp/structs.h | 3 +++
net/sctp/input.c | 23 +++++++++++++++++++----
net/sctp/sm_sideeffect.c | 35 +++++++++++++++++++++++++++++++++++
net/sctp/transport.c | 2 ++
5 files changed, 60 insertions(+), 4 deletions(-)
diff --git a/include/net/sctp/sm.h b/include/net/sctp/sm.h
index 851c813..61d73e3 100644
--- a/include/net/sctp/sm.h
+++ b/include/net/sctp/sm.h
@@ -279,6 +279,7 @@ int sctp_do_sm(sctp_event_t event_type, sctp_subtype_t subtype,
/* 2nd level prototypes */
void sctp_generate_t3_rtx_event(unsigned long peer);
void sctp_generate_heartbeat_event(unsigned long peer);
+void sctp_generate_proto_unreach_event(unsigned long peer);
void sctp_ootb_pkt_free(struct sctp_packet *);
diff --git a/include/net/sctp/structs.h b/include/net/sctp/structs.h
index 597f8e2..219043a 100644
--- a/include/net/sctp/structs.h
+++ b/include/net/sctp/structs.h
@@ -1010,6 +1010,9 @@ struct sctp_transport {
/* Heartbeat timer is per destination. */
struct timer_list hb_timer;
+ /* Timer to handle ICMP proto unreachable envets */
+ struct timer_list proto_unreach_timer;
+
/* Since we're using per-destination retransmission timers
* (see above), we're also using per-destination "transmitted"
* queues. This probably ought to be a private struct
diff --git a/net/sctp/input.c b/net/sctp/input.c
index 2a57018..94b2eb2 100644
--- a/net/sctp/input.c
+++ b/net/sctp/input.c
@@ -440,11 +440,25 @@ void sctp_icmp_proto_unreachable(struct sock *sk,
{
SCTP_DEBUG_PRINTK("%s\n", __func__);
- sctp_do_sm(SCTP_EVENT_T_OTHER,
- SCTP_ST_OTHER(SCTP_EVENT_ICMP_PROTO_UNREACH),
- asoc->state, asoc->ep, asoc, t,
- GFP_ATOMIC);
+ if (sock_owned_by_user(sk)) {
+ if (timer_pending(&t->proto_unreach_timer))
+ return;
+ else {
+ if (!mod_timer(&t->proto_unreach_timer,
+ jiffies + (HZ/20)))
+ sctp_association_hold(asoc);
+ }
+
+ } else {
+ if (timer_pending(&t->proto_unreach_timer) &&
+ del_timer(&t->proto_unreach_timer))
+ sctp_association_put(asoc);
+ sctp_do_sm(SCTP_EVENT_T_OTHER,
+ SCTP_ST_OTHER(SCTP_EVENT_ICMP_PROTO_UNREACH),
+ asoc->state, asoc->ep, asoc, t,
+ GFP_ATOMIC);
+ }
}
/* Common lookup code for icmp/icmpv6 error handler. */
@@ -599,6 +613,7 @@ void sctp_v4_err(struct sk_buff *skb, __u32 info)
}
else {
if (ICMP_PROT_UNREACH == code) {
+ printk("processing code unreach\n");
sctp_icmp_proto_unreachable(sk, asoc,
transport);
goto out_unlock;
diff --git a/net/sctp/sm_sideeffect.c b/net/sctp/sm_sideeffect.c
index d5ae450..eb1f42f 100644
--- a/net/sctp/sm_sideeffect.c
+++ b/net/sctp/sm_sideeffect.c
@@ -397,6 +397,41 @@ out_unlock:
sctp_transport_put(transport);
}
+/* Handle the timeout of the ICMP protocol unreachable timer. Trigger
+ * the correct state machine transition that will close the association.
+ */
+void sctp_generate_proto_unreach_event(unsigned long data)
+{
+ struct sctp_transport *transport = (struct sctp_transport *) data;
+ struct sctp_association *asoc = transport->asoc;
+
+ sctp_bh_lock_sock(asoc->base.sk);
+ if (sock_owned_by_user(asoc->base.sk)) {
+ SCTP_DEBUG_PRINTK("%s:Sock is busy.\n", __func__);
+
+ /* Try again later. */
+ if (!mod_timer(&transport->proto_unreach_timer,
+ jiffies + (HZ/20)))
+ sctp_association_hold(asoc);
+ goto out_unlock;
+ }
+
+ /* Is this structure just waiting around for us to actually
+ * get destroyed?
+ */
+ if (asoc->base.dead)
+ goto out_unlock;
+
+ sctp_do_sm(SCTP_EVENT_T_OTHER,
+ SCTP_ST_OTHER(SCTP_EVENT_ICMP_PROTO_UNREACH),
+ asoc->state, asoc->ep, asoc, transport, GFP_ATOMIC);
+
+out_unlock:
+ sctp_bh_unlock_sock(asoc->base.sk);
+ sctp_association_put(asoc);
+}
+
+
/* Inject a SACK Timeout event into the state machine. */
static void sctp_generate_sack_event(unsigned long data)
{
diff --git a/net/sctp/transport.c b/net/sctp/transport.c
index be4d63d..4a36803 100644
--- a/net/sctp/transport.c
+++ b/net/sctp/transport.c
@@ -108,6 +108,8 @@ static struct sctp_transport *sctp_transport_init(struct sctp_transport *peer,
(unsigned long)peer);
setup_timer(&peer->hb_timer, sctp_generate_heartbeat_event,
(unsigned long)peer);
+ setup_timer(&peer->proto_unreach_timer,
+ sctp_generate_proto_unreach_event, (unsigned long)peer);
/* Initialize the 64-bit random nonce sent with heartbeat. */
get_random_bytes(&peer->hb_nonce, sizeof(peer->hb_nonce));
--
1.6.0.4
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH v2] sctp: Fix a race between ICMP protocol unreachable and connect()
2010-05-05 19:29 [PATCH] sctp: Fix a race between ICMP protocol unreachable and connect() Vlad Yasevich
@ 2010-05-05 19:36 ` Vlad Yasevich
2010-05-06 7:56 ` David Miller
2010-05-10 2:56 ` Wei Yongjun
0 siblings, 2 replies; 6+ messages in thread
From: Vlad Yasevich @ 2010-05-05 19:36 UTC (permalink / raw)
To: davem; +Cc: netdev, linux-sctp, Vlad Yasevich
[.. 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.
Signed-off-by: Vlad Yasevich <vladislav.yasevich@hp.com>
---
include/net/sctp/sm.h | 1 +
include/net/sctp/structs.h | 3 +++
net/sctp/input.c | 22 ++++++++++++++++++----
net/sctp/sm_sideeffect.c | 35 +++++++++++++++++++++++++++++++++++
net/sctp/transport.c | 2 ++
5 files changed, 59 insertions(+), 4 deletions(-)
diff --git a/include/net/sctp/sm.h b/include/net/sctp/sm.h
index 851c813..61d73e3 100644
--- a/include/net/sctp/sm.h
+++ b/include/net/sctp/sm.h
@@ -279,6 +279,7 @@ int sctp_do_sm(sctp_event_t event_type, sctp_subtype_t subtype,
/* 2nd level prototypes */
void sctp_generate_t3_rtx_event(unsigned long peer);
void sctp_generate_heartbeat_event(unsigned long peer);
+void sctp_generate_proto_unreach_event(unsigned long peer);
void sctp_ootb_pkt_free(struct sctp_packet *);
diff --git a/include/net/sctp/structs.h b/include/net/sctp/structs.h
index 597f8e2..219043a 100644
--- a/include/net/sctp/structs.h
+++ b/include/net/sctp/structs.h
@@ -1010,6 +1010,9 @@ struct sctp_transport {
/* Heartbeat timer is per destination. */
struct timer_list hb_timer;
+ /* Timer to handle ICMP proto unreachable envets */
+ struct timer_list proto_unreach_timer;
+
/* Since we're using per-destination retransmission timers
* (see above), we're also using per-destination "transmitted"
* queues. This probably ought to be a private struct
diff --git a/net/sctp/input.c b/net/sctp/input.c
index 2a57018..ea21924 100644
--- a/net/sctp/input.c
+++ b/net/sctp/input.c
@@ -440,11 +440,25 @@ void sctp_icmp_proto_unreachable(struct sock *sk,
{
SCTP_DEBUG_PRINTK("%s\n", __func__);
- sctp_do_sm(SCTP_EVENT_T_OTHER,
- SCTP_ST_OTHER(SCTP_EVENT_ICMP_PROTO_UNREACH),
- asoc->state, asoc->ep, asoc, t,
- GFP_ATOMIC);
+ if (sock_owned_by_user(sk)) {
+ if (timer_pending(&t->proto_unreach_timer))
+ return;
+ else {
+ if (!mod_timer(&t->proto_unreach_timer,
+ jiffies + (HZ/20)))
+ sctp_association_hold(asoc);
+ }
+
+ } else {
+ if (timer_pending(&t->proto_unreach_timer) &&
+ del_timer(&t->proto_unreach_timer))
+ sctp_association_put(asoc);
+ sctp_do_sm(SCTP_EVENT_T_OTHER,
+ SCTP_ST_OTHER(SCTP_EVENT_ICMP_PROTO_UNREACH),
+ asoc->state, asoc->ep, asoc, t,
+ GFP_ATOMIC);
+ }
}
/* Common lookup code for icmp/icmpv6 error handler. */
diff --git a/net/sctp/sm_sideeffect.c b/net/sctp/sm_sideeffect.c
index d5ae450..eb1f42f 100644
--- a/net/sctp/sm_sideeffect.c
+++ b/net/sctp/sm_sideeffect.c
@@ -397,6 +397,41 @@ out_unlock:
sctp_transport_put(transport);
}
+/* Handle the timeout of the ICMP protocol unreachable timer. Trigger
+ * the correct state machine transition that will close the association.
+ */
+void sctp_generate_proto_unreach_event(unsigned long data)
+{
+ struct sctp_transport *transport = (struct sctp_transport *) data;
+ struct sctp_association *asoc = transport->asoc;
+
+ sctp_bh_lock_sock(asoc->base.sk);
+ if (sock_owned_by_user(asoc->base.sk)) {
+ SCTP_DEBUG_PRINTK("%s:Sock is busy.\n", __func__);
+
+ /* Try again later. */
+ if (!mod_timer(&transport->proto_unreach_timer,
+ jiffies + (HZ/20)))
+ sctp_association_hold(asoc);
+ goto out_unlock;
+ }
+
+ /* Is this structure just waiting around for us to actually
+ * get destroyed?
+ */
+ if (asoc->base.dead)
+ goto out_unlock;
+
+ sctp_do_sm(SCTP_EVENT_T_OTHER,
+ SCTP_ST_OTHER(SCTP_EVENT_ICMP_PROTO_UNREACH),
+ asoc->state, asoc->ep, asoc, transport, GFP_ATOMIC);
+
+out_unlock:
+ sctp_bh_unlock_sock(asoc->base.sk);
+ sctp_association_put(asoc);
+}
+
+
/* Inject a SACK Timeout event into the state machine. */
static void sctp_generate_sack_event(unsigned long data)
{
diff --git a/net/sctp/transport.c b/net/sctp/transport.c
index be4d63d..4a36803 100644
--- a/net/sctp/transport.c
+++ b/net/sctp/transport.c
@@ -108,6 +108,8 @@ static struct sctp_transport *sctp_transport_init(struct sctp_transport *peer,
(unsigned long)peer);
setup_timer(&peer->hb_timer, sctp_generate_heartbeat_event,
(unsigned long)peer);
+ setup_timer(&peer->proto_unreach_timer,
+ sctp_generate_proto_unreach_event, (unsigned long)peer);
/* Initialize the 64-bit random nonce sent with heartbeat. */
get_random_bytes(&peer->hb_nonce, sizeof(peer->hb_nonce));
--
1.6.0.4
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v2] sctp: Fix a race between ICMP protocol unreachable and connect()
2010-05-05 19:36 ` [PATCH v2] " Vlad Yasevich
@ 2010-05-06 7:56 ` David Miller
2010-05-10 2:56 ` Wei Yongjun
1 sibling, 0 replies; 6+ messages in thread
From: David Miller @ 2010-05-06 7:56 UTC (permalink / raw)
To: vladislav.yasevich; +Cc: netdev, linux-sctp
From: Vlad Yasevich <vladislav.yasevich@hp.com>
Date: Wed, 5 May 2010 15:36:06 -0400
> 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
...
> 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.
>
> Signed-off-by: Vlad Yasevich <vladislav.yasevich@hp.com>
Applied and queued up for -stable, thanks Vlad.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] sctp: Fix a race between ICMP protocol unreachable and connect()
2010-05-05 19:36 ` [PATCH v2] " Vlad Yasevich
2010-05-06 7:56 ` David Miller
@ 2010-05-10 2:56 ` Wei Yongjun
2010-05-10 13:59 ` Vlad Yasevich
1 sibling, 1 reply; 6+ messages in thread
From: Wei Yongjun @ 2010-05-10 2:56 UTC (permalink / raw)
To: Vlad Yasevich; +Cc: davem, netdev, linux-sctp
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(-)
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);
}
--
1.6.5.2
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v2] sctp: Fix a race between ICMP protocol unreachable and connect()
2010-05-10 2:56 ` Wei Yongjun
@ 2010-05-10 13:59 ` Vlad Yasevich
2010-05-16 7:46 ` David Miller
0 siblings, 1 reply; 6+ messages in thread
From: Vlad Yasevich @ 2010-05-10 13:59 UTC (permalink / raw)
To: Wei Yongjun; +Cc: davem, netdev, linux-sctp
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] sctp: Fix a race between ICMP protocol unreachable and connect()
2010-05-10 13:59 ` Vlad Yasevich
@ 2010-05-16 7:46 ` David Miller
0 siblings, 0 replies; 6+ messages in thread
From: David Miller @ 2010-05-16 7:46 UTC (permalink / raw)
To: vladislav.yasevich; +Cc: yjwei, netdev, linux-sctp
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.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2010-05-16 7:46 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2010-05-05 19:29 [PATCH] sctp: Fix a race between ICMP protocol unreachable and connect() Vlad Yasevich
2010-05-05 19:36 ` [PATCH v2] " Vlad Yasevich
2010-05-06 7:56 ` David Miller
2010-05-10 2:56 ` Wei Yongjun
2010-05-10 13:59 ` Vlad Yasevich
2010-05-16 7:46 ` David Miller
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).