* [PATCH net-next v3] sctp: sctp_close: fix release of bindings for deferred call_rcu's
@ 2013-02-01 14:37 Daniel Borkmann
2013-02-01 14:55 ` Vlad Yasevich
` (2 more replies)
0 siblings, 3 replies; 4+ messages in thread
From: Daniel Borkmann @ 2013-02-01 14:37 UTC (permalink / raw)
To: davem; +Cc: linux-sctp, netdev, Vlad Yasevich
It seems due to RCU usage, i.e. within SCTP's address binding list,
a, say, ``behavioral change'' was introduced which does actually
not conform to the RFC anymore. In particular consider the following
(fictional) scenario to demonstrate this:
do:
Two SOCK_SEQPACKET-style sockets are opened (S1, S2)
S1 is bound to 127.0.0.1, port 1024 [server]
S2 is bound to 127.0.0.1, port 1025 [client]
listen(2) is invoked on S1
From S2 we call one sendmsg(2) with msg.msg_name and
msg.msg_namelen parameters set to the server's
address
S1, S2 are closed
goto do
The first pass of this loop passes successful, while the second round
fails during binding of S1 (address still in use). What is happening?
In the first round, the initial handshake is being done, and, at the
time close(2) is called on S1, a non-graceful shutdown is performed via
ABORT since in S1's receive queue an unprocessed packet is present,
thus stating an error condition. This can be considered as a correct
behavior.
During close also all bound addresses are freed, thus nothing *must*
be active anymore. In reference to RFC2960:
After checking the Verification Tag, the receiving endpoint shall
remove the association from its record, and shall report the
termination to its upper layer. (9.1 Abort of an Association)
Also, no half-open states are supported, thus after an ungraceful
shutdown, we leave nothing behind. However, this seems not to be
happening though. In a real-world scenario, this is exactly where
it breaks the lksctp-tools functional test suite, *for instance*:
./test_sockopt
test_sockopt.c 1 PASS : getsockopt(SCTP_STATUS) on a socket with no assoc
test_sockopt.c 2 PASS : getsockopt(SCTP_STATUS)
test_sockopt.c 3 PASS : getsockopt(SCTP_STATUS) with invalid associd
test_sockopt.c 4 PASS : getsockopt(SCTP_STATUS) with NULL associd
test_sockopt.c 5 BROK : bind: Address already in use
The underlying problem is that sctp_endpoint_destroy() hasn't been
triggered yet while the next bind attempt is being done. It will be
triggered eventually (but too late) by sctp_transport_destroy_rcu()
after one RCU grace period:
sctp_transport_destroy()
sctp_transport_destroy_rcu() ----.
sctp_association_put() [*] <--+--> sctp_packet_free()
sctp_association_destroy() [...]
sctp_endpoint_put() skb->destructor
sctp_endpoint_destroy() sctp_wfree()
sctp_bind_addr_free() sctp_association_put() [*]
Thus, we move out the condition with sctp_association_put() as well as
the sctp_packet_free() invocation and the issue can be solved. We also
better free the SCTP chunks first before putting the ref of the association.
With this patch, the example above (which simulates a similar scenario
as in the implementation of this test case) and therefore also the test
suite run successfully through. Tested by myself.
Cc: Vlad Yasevich <vyasevich@gmail.com>
Signed-off-by: Daniel Borkmann <dborkman@redhat.com>
---
net/sctp/transport.c | 9 +++++----
1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/net/sctp/transport.c b/net/sctp/transport.c
index 4e45bb6..ca5331c 100644
--- a/net/sctp/transport.c
+++ b/net/sctp/transport.c
@@ -168,10 +168,6 @@ static void sctp_transport_destroy_rcu(struct rcu_head *head)
struct sctp_transport *transport;
transport = container_of(head, struct sctp_transport, rcu);
- if (transport->asoc)
- sctp_association_put(transport->asoc);
-
- sctp_packet_free(&transport->packet);
dst_release(transport->dst);
kfree(transport);
@@ -186,6 +182,11 @@ static void sctp_transport_destroy(struct sctp_transport *transport)
SCTP_ASSERT(transport->dead, "Transport is not dead", return);
call_rcu(&transport->rcu, sctp_transport_destroy_rcu);
+
+ sctp_packet_free(&transport->packet);
+
+ if (transport->asoc)
+ sctp_association_put(transport->asoc);
}
/* Start T3_rtx timer if it is not already running and update the heartbeat
--
1.7.11.7
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH net-next v3] sctp: sctp_close: fix release of bindings for deferred call_rcu's
2013-02-01 14:37 [PATCH net-next v3] sctp: sctp_close: fix release of bindings for deferred call_rcu's Daniel Borkmann
@ 2013-02-01 14:55 ` Vlad Yasevich
2013-02-04 12:10 ` Neil Horman
2013-02-04 18:23 ` David Miller
2 siblings, 0 replies; 4+ messages in thread
From: Vlad Yasevich @ 2013-02-01 14:55 UTC (permalink / raw)
To: Daniel Borkmann; +Cc: davem, linux-sctp, netdev
On 02/01/2013 09:37 AM, Daniel Borkmann wrote:
>
> The underlying problem is that sctp_endpoint_destroy() hasn't been
> triggered yet while the next bind attempt is being done. It will be
> triggered eventually (but too late) by sctp_transport_destroy_rcu()
> after one RCU grace period:
>
> sctp_transport_destroy()
> sctp_transport_destroy_rcu() ----.
> sctp_association_put() [*] <--+--> sctp_packet_free()
> sctp_association_destroy() [...]
> sctp_endpoint_put() skb->destructor
> sctp_endpoint_destroy() sctp_wfree()
> sctp_bind_addr_free() sctp_association_put() [*]
>
> Thus, we move out the condition with sctp_association_put() as well as
> the sctp_packet_free() invocation and the issue can be solved. We also
> better free the SCTP chunks first before putting the ref of the association.
>
> With this patch, the example above (which simulates a similar scenario
> as in the implementation of this test case) and therefore also the test
> suite run successfully through. Tested by myself.
>
> Cc: Vlad Yasevich <vyasevich@gmail.com>
> Signed-off-by: Daniel Borkmann <dborkman@redhat.com>
Acked-by: Vlad Yasevich <vyasevich@gmail.com>
-vlad
> ---
> net/sctp/transport.c | 9 +++++----
> 1 file changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/net/sctp/transport.c b/net/sctp/transport.c
> index 4e45bb6..ca5331c 100644
> --- a/net/sctp/transport.c
> +++ b/net/sctp/transport.c
> @@ -168,10 +168,6 @@ static void sctp_transport_destroy_rcu(struct rcu_head *head)
> struct sctp_transport *transport;
>
> transport = container_of(head, struct sctp_transport, rcu);
> - if (transport->asoc)
> - sctp_association_put(transport->asoc);
> -
> - sctp_packet_free(&transport->packet);
>
> dst_release(transport->dst);
> kfree(transport);
> @@ -186,6 +182,11 @@ static void sctp_transport_destroy(struct sctp_transport *transport)
> SCTP_ASSERT(transport->dead, "Transport is not dead", return);
>
> call_rcu(&transport->rcu, sctp_transport_destroy_rcu);
> +
> + sctp_packet_free(&transport->packet);
> +
> + if (transport->asoc)
> + sctp_association_put(transport->asoc);
> }
>
> /* Start T3_rtx timer if it is not already running and update the heartbeat
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH net-next v3] sctp: sctp_close: fix release of bindings for deferred call_rcu's
2013-02-01 14:37 [PATCH net-next v3] sctp: sctp_close: fix release of bindings for deferred call_rcu's Daniel Borkmann
2013-02-01 14:55 ` Vlad Yasevich
@ 2013-02-04 12:10 ` Neil Horman
2013-02-04 18:23 ` David Miller
2 siblings, 0 replies; 4+ messages in thread
From: Neil Horman @ 2013-02-04 12:10 UTC (permalink / raw)
To: Daniel Borkmann; +Cc: davem, linux-sctp, netdev, Vlad Yasevich
On Fri, Feb 01, 2013 at 03:37:43PM +0100, Daniel Borkmann wrote:
> It seems due to RCU usage, i.e. within SCTP's address binding list,
> a, say, ``behavioral change'' was introduced which does actually
> not conform to the RFC anymore. In particular consider the following
> (fictional) scenario to demonstrate this:
>
> do:
> Two SOCK_SEQPACKET-style sockets are opened (S1, S2)
> S1 is bound to 127.0.0.1, port 1024 [server]
> S2 is bound to 127.0.0.1, port 1025 [client]
> listen(2) is invoked on S1
> From S2 we call one sendmsg(2) with msg.msg_name and
> msg.msg_namelen parameters set to the server's
> address
> S1, S2 are closed
> goto do
>
> The first pass of this loop passes successful, while the second round
> fails during binding of S1 (address still in use). What is happening?
> In the first round, the initial handshake is being done, and, at the
> time close(2) is called on S1, a non-graceful shutdown is performed via
> ABORT since in S1's receive queue an unprocessed packet is present,
> thus stating an error condition. This can be considered as a correct
> behavior.
>
> During close also all bound addresses are freed, thus nothing *must*
> be active anymore. In reference to RFC2960:
>
> After checking the Verification Tag, the receiving endpoint shall
> remove the association from its record, and shall report the
> termination to its upper layer. (9.1 Abort of an Association)
>
> Also, no half-open states are supported, thus after an ungraceful
> shutdown, we leave nothing behind. However, this seems not to be
> happening though. In a real-world scenario, this is exactly where
> it breaks the lksctp-tools functional test suite, *for instance*:
>
> ./test_sockopt
> test_sockopt.c 1 PASS : getsockopt(SCTP_STATUS) on a socket with no assoc
> test_sockopt.c 2 PASS : getsockopt(SCTP_STATUS)
> test_sockopt.c 3 PASS : getsockopt(SCTP_STATUS) with invalid associd
> test_sockopt.c 4 PASS : getsockopt(SCTP_STATUS) with NULL associd
> test_sockopt.c 5 BROK : bind: Address already in use
>
> The underlying problem is that sctp_endpoint_destroy() hasn't been
> triggered yet while the next bind attempt is being done. It will be
> triggered eventually (but too late) by sctp_transport_destroy_rcu()
> after one RCU grace period:
>
> sctp_transport_destroy()
> sctp_transport_destroy_rcu() ----.
> sctp_association_put() [*] <--+--> sctp_packet_free()
> sctp_association_destroy() [...]
> sctp_endpoint_put() skb->destructor
> sctp_endpoint_destroy() sctp_wfree()
> sctp_bind_addr_free() sctp_association_put() [*]
>
> Thus, we move out the condition with sctp_association_put() as well as
> the sctp_packet_free() invocation and the issue can be solved. We also
> better free the SCTP chunks first before putting the ref of the association.
>
> With this patch, the example above (which simulates a similar scenario
> as in the implementation of this test case) and therefore also the test
> suite run successfully through. Tested by myself.
>
> Cc: Vlad Yasevich <vyasevich@gmail.com>
> Signed-off-by: Daniel Borkmann <dborkman@redhat.com>
> ---
> net/sctp/transport.c | 9 +++++----
> 1 file changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/net/sctp/transport.c b/net/sctp/transport.c
> index 4e45bb6..ca5331c 100644
> --- a/net/sctp/transport.c
> +++ b/net/sctp/transport.c
> @@ -168,10 +168,6 @@ static void sctp_transport_destroy_rcu(struct rcu_head *head)
> struct sctp_transport *transport;
>
> transport = container_of(head, struct sctp_transport, rcu);
> - if (transport->asoc)
> - sctp_association_put(transport->asoc);
> -
> - sctp_packet_free(&transport->packet);
>
> dst_release(transport->dst);
> kfree(transport);
> @@ -186,6 +182,11 @@ static void sctp_transport_destroy(struct sctp_transport *transport)
> SCTP_ASSERT(transport->dead, "Transport is not dead", return);
>
> call_rcu(&transport->rcu, sctp_transport_destroy_rcu);
> +
> + sctp_packet_free(&transport->packet);
> +
> + if (transport->asoc)
> + sctp_association_put(transport->asoc);
> }
>
Yup, this looks good, thanks!
Acked-by: Neil Horman <nhorman@tuxdriver.com>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH net-next v3] sctp: sctp_close: fix release of bindings for deferred call_rcu's
2013-02-01 14:37 [PATCH net-next v3] sctp: sctp_close: fix release of bindings for deferred call_rcu's Daniel Borkmann
2013-02-01 14:55 ` Vlad Yasevich
2013-02-04 12:10 ` Neil Horman
@ 2013-02-04 18:23 ` David Miller
2 siblings, 0 replies; 4+ messages in thread
From: David Miller @ 2013-02-04 18:23 UTC (permalink / raw)
To: dborkman; +Cc: linux-sctp, netdev, vyasevich
From: Daniel Borkmann <dborkman@redhat.com>
Date: Fri, 1 Feb 2013 15:37:43 +0100
> It seems due to RCU usage, i.e. within SCTP's address binding list,
> a, say, ``behavioral change'' was introduced which does actually
> not conform to the RFC anymore. In particular consider the following
> (fictional) scenario to demonstrate this:
>
> do:
> Two SOCK_SEQPACKET-style sockets are opened (S1, S2)
> S1 is bound to 127.0.0.1, port 1024 [server]
> S2 is bound to 127.0.0.1, port 1025 [client]
> listen(2) is invoked on S1
> From S2 we call one sendmsg(2) with msg.msg_name and
> msg.msg_namelen parameters set to the server's
> address
> S1, S2 are closed
> goto do
>
> The first pass of this loop passes successful, while the second round
> fails during binding of S1 (address still in use). What is happening?
> In the first round, the initial handshake is being done, and, at the
> time close(2) is called on S1, a non-graceful shutdown is performed via
> ABORT since in S1's receive queue an unprocessed packet is present,
> thus stating an error condition. This can be considered as a correct
> behavior.
>
> During close also all bound addresses are freed, thus nothing *must*
> be active anymore. In reference to RFC2960:
>
> After checking the Verification Tag, the receiving endpoint shall
> remove the association from its record, and shall report the
> termination to its upper layer. (9.1 Abort of an Association)
>
> Also, no half-open states are supported, thus after an ungraceful
> shutdown, we leave nothing behind. However, this seems not to be
> happening though. In a real-world scenario, this is exactly where
> it breaks the lksctp-tools functional test suite, *for instance*:
>
> ./test_sockopt
> test_sockopt.c 1 PASS : getsockopt(SCTP_STATUS) on a socket with no assoc
> test_sockopt.c 2 PASS : getsockopt(SCTP_STATUS)
> test_sockopt.c 3 PASS : getsockopt(SCTP_STATUS) with invalid associd
> test_sockopt.c 4 PASS : getsockopt(SCTP_STATUS) with NULL associd
> test_sockopt.c 5 BROK : bind: Address already in use
>
> The underlying problem is that sctp_endpoint_destroy() hasn't been
> triggered yet while the next bind attempt is being done. It will be
> triggered eventually (but too late) by sctp_transport_destroy_rcu()
> after one RCU grace period:
>
> sctp_transport_destroy()
> sctp_transport_destroy_rcu() ----.
> sctp_association_put() [*] <--+--> sctp_packet_free()
> sctp_association_destroy() [...]
> sctp_endpoint_put() skb->destructor
> sctp_endpoint_destroy() sctp_wfree()
> sctp_bind_addr_free() sctp_association_put() [*]
>
> Thus, we move out the condition with sctp_association_put() as well as
> the sctp_packet_free() invocation and the issue can be solved. We also
> better free the SCTP chunks first before putting the ref of the association.
>
> With this patch, the example above (which simulates a similar scenario
> as in the implementation of this test case) and therefore also the test
> suite run successfully through. Tested by myself.
>
> Cc: Vlad Yasevich <vyasevich@gmail.com>
> Signed-off-by: Daniel Borkmann <dborkman@redhat.com>
Applied.
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2013-02-04 18:23 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-02-01 14:37 [PATCH net-next v3] sctp: sctp_close: fix release of bindings for deferred call_rcu's Daniel Borkmann
2013-02-01 14:55 ` Vlad Yasevich
2013-02-04 12:10 ` Neil Horman
2013-02-04 18:23 ` 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).