From: Vlad Yasevich <vladislav.yasevich@hp•com>
To: Wei Yongjun <yjwei@cn•fujitsu.com>
Cc: davem@davemloft•net, netdev@vger•kernel.org, linux-sctp@vger•kernel.org
Subject: Re: [PATCH v2] sctp: Fix a race between ICMP protocol unreachable and connect()
Date: Mon, 10 May 2010 09:59:32 -0400 [thread overview]
Message-ID: <4BE81144.8020806@hp.com> (raw)
In-Reply-To: <4BE775C7.9070702@cn.fujitsu.com>
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
next prev parent reply other threads:[~2010-05-10 13:59 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
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 [this message]
2010-05-16 7:46 ` David Miller
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=4BE81144.8020806@hp.com \
--to=vladislav.yasevich@hp$(echo .)com \
--cc=davem@davemloft$(echo .)net \
--cc=linux-sctp@vger$(echo .)kernel.org \
--cc=netdev@vger$(echo .)kernel.org \
--cc=yjwei@cn$(echo .)fujitsu.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox