[PATCH net-next v3 0/4] Some SCTP cleanups/improvements

[PATCH net-next v3 0/4] Some SCTP cleanups/improvements

[Daniel Borkmann]

v2 -> v3:
  - Applied feedback from Vlad in first patch, rest is unchanged

v1 -> v2:
  - Applied Neil's feedback, I'll send out the first patch at a later
    point in time though, have not included it in this set
  - Add two other patches that appear to be useful from the last
    couple of days debugging SCTP code

Let it run through lksctp-tools and also my recent stress test tool,
all looks good.

Daniel Borkmann (4):
  net: sctp: sideeffect: throw BUG if primary_path is NULL
  net: sctp: sctp_sf_do_prm_asoc: do SCTP_CMD_INIT_CHOOSE_TRANSPORT first
  net: sctp: minor: remove variable in sctp_init_sock
  net: sctp: sctp_association_init: put refs in reverse order

 net/sctp/associola.c     |  7 +++----
 net/sctp/proc.c          | 19 ++++++++++++++++---
 net/sctp/sm_sideeffect.c |  5 ++++-
 net/sctp/sm_statefuns.c  |  8 ++++----
 net/sctp/socket.c        |  6 ++----
 5 files changed, 29 insertions(+), 16 deletions(-)

-- 
1.7.11.7

[PATCH net-next v3 1/4] net: sctp: sideeffect: throw BUG if primary_path is NULL

[Daniel Borkmann]

This clearly states a BUG somewhere in the SCTP code as e.g. fixed once
in f28156335 ("sctp: Use correct sideffect command in duplicate cookie
handling"). If this ever happens, throw a trace in the sideeffect engine
where assocs clearly must have a primary_path assigned.

When in sctp_seq_dump_local_addrs() also throw a WARN and bail out since
we do not need to panic for printing this one asterisk. Also, it will
avoid the not so obvious case when primary != NULL test passes and at a
later point in time triggering a NULL ptr dereference caused by primary.
While at it, also fix up the white space.

Signed-off-by: Daniel Borkmann <dborkman@redhat.com>
---
 net/sctp/proc.c          | 12 +++++++++---
 net/sctp/sm_sideeffect.c |  5 ++++-
 2 files changed, 13 insertions(+), 4 deletions(-)

diff --git a/net/sctp/proc.c b/net/sctp/proc.c
index 4e45ee3..47fff5f 100644
--- a/net/sctp/proc.c
+++ b/net/sctp/proc.c
@@ -134,9 +134,15 @@ static void sctp_seq_dump_local_addrs(struct seq_file *seq, struct sctp_ep_commo
 	struct sctp_af *af;
 
 	if (epb->type == SCTP_EP_TYPE_ASSOCIATION) {
-	    asoc = sctp_assoc(epb);
-	    peer = asoc->peer.primary_path;
-	    primary = &peer->saddr;
+		asoc = sctp_assoc(epb);
+
+		peer = asoc->peer.primary_path;
+		if (unlikely(peer == NULL)) {
+			WARN(1, "Association %p with NULL primary path!", asoc);
+			return;
+		}
+
+		primary = &peer->saddr;
 	}
 
 	rcu_read_lock();
diff --git a/net/sctp/sm_sideeffect.c b/net/sctp/sm_sideeffect.c
index 8aab894..ff91f47 100644
--- a/net/sctp/sm_sideeffect.c
+++ b/net/sctp/sm_sideeffect.c
@@ -864,6 +864,7 @@ static void sctp_cmd_delete_tcb(sctp_cmd_seq_t *cmds,
 	    (!asoc->temp) && (sk->sk_shutdown != SHUTDOWN_MASK))
 		return;
 
+	BUG_ON(asoc->peer.primary_path == NULL);
 	sctp_unhash_established(asoc);
 	sctp_association_free(asoc);
 }
@@ -1274,8 +1275,10 @@ static int sctp_cmd_interpreter(sctp_event_t event_type,
 				sctp_outq_uncork(&asoc->outqueue);
 				local_cork = 0;
 			}
-			asoc = cmd->obj.asoc;
+
 			/* Register with the endpoint.  */
+			asoc = cmd->obj.asoc;
+			BUG_ON(asoc->peer.primary_path == NULL);
 			sctp_endpoint_add_asoc(ep, asoc);
 			sctp_hash_established(asoc);
 			break;
-- 
1.7.11.7

[PATCH net-next v3 2/4] net: sctp: sctp_sf_do_prm_asoc: do SCTP_CMD_INIT_CHOOSE_TRANSPORT first

[Daniel Borkmann]

While this currently cannot trigger any NULL pointer dereference in
sctp_seq_dump_local_addrs(), better change the order of commands to
prevent a future bug to happen. Although we first add SCTP_CMD_NEW_ASOC
and then set the SCTP_CMD_INIT_CHOOSE_TRANSPORT, it is okay for now,
since this primitive is only called by sctp_connect() or sctp_sendmsg()
with sctp_assoc_add_peer() set first. However, lets do this precaution
and first set the transport and then add it to the association hashlist
to prevent in future something to possibly triggering this.

Signed-off-by: Daniel Borkmann <dborkman@redhat.com>
---
 net/sctp/sm_statefuns.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/net/sctp/sm_statefuns.c b/net/sctp/sm_statefuns.c
index de1a013..b3d1868 100644
--- a/net/sctp/sm_statefuns.c
+++ b/net/sctp/sm_statefuns.c
@@ -4632,16 +4632,16 @@ sctp_disposition_t sctp_sf_do_prm_asoc(struct net *net,
 	if (!repl)
 		goto nomem;
 
+	/* Choose transport for INIT. */
+	sctp_add_cmd_sf(commands, SCTP_CMD_INIT_CHOOSE_TRANSPORT,
+			SCTP_CHUNK(repl));
+
 	/* Cast away the const modifier, as we want to just
 	 * rerun it through as a sideffect.
 	 */
 	my_asoc = (struct sctp_association *)asoc;
 	sctp_add_cmd_sf(commands, SCTP_CMD_NEW_ASOC, SCTP_ASOC(my_asoc));
 
-	/* Choose transport for INIT. */
-	sctp_add_cmd_sf(commands, SCTP_CMD_INIT_CHOOSE_TRANSPORT,
-			SCTP_CHUNK(repl));
-
 	/* After sending the INIT, "A" starts the T1-init timer and
 	 * enters the COOKIE-WAIT state.
 	 */
-- 
1.7.11.7

[PATCH net-next v3 3/4] net: sctp: minor: remove variable in sctp_init_sock

[Daniel Borkmann]

It's only used at this one time, so we could remove it as well.
This is valid and also makes it more explicit/obvious that in case
of error the sp->ep is NULL here, i.e. for the sctp_destroy_sock()
check that was recently added.

Signed-off-by: Daniel Borkmann <dborkman@redhat.com>
---
 net/sctp/socket.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/net/sctp/socket.c b/net/sctp/socket.c
index f631c5f..510dc79 100644
--- a/net/sctp/socket.c
+++ b/net/sctp/socket.c
@@ -3862,7 +3862,6 @@ out:
 SCTP_STATIC int sctp_init_sock(struct sock *sk)
 {
 	struct net *net = sock_net(sk);
-	struct sctp_endpoint *ep;
 	struct sctp_sock *sp;
 
 	SCTP_DEBUG_PRINTK("sctp_init_sock(sk: %p)\n", sk);
@@ -3971,11 +3970,10 @@ SCTP_STATIC int sctp_init_sock(struct sock *sk)
 	 * change the data structure relationships, this may still
 	 * be useful for storing pre-connect address information.
 	 */
-	ep = sctp_endpoint_new(sk, GFP_KERNEL);
-	if (!ep)
+	sp->ep = sctp_endpoint_new(sk, GFP_KERNEL);
+	if (!sp->ep)
 		return -ENOMEM;
 
-	sp->ep = ep;
 	sp->hmac = NULL;
 
 	SCTP_DBG_OBJCNT_INC(sock);
-- 
1.7.11.7

[PATCH net-next v3 4/4] net: sctp: sctp_association_init: put refs in reverse order

[Daniel Borkmann]

In case we need to bail out for whatever reason during assoc
init, we call sctp_endpoint_put() and then sock_put(), however,
we've hold both refs in reverse, non-symmetric order, so first
sctp_endpoint_hold() and then sock_hold().

Reverse this, so that in an error case we have sock_put() and then
sctp_endpoint_put(). Actually shouldn't matter too much, since both
cleanup paths do the right thing, but that way, it is more consistent
with the rest of the code.

Signed-off-by: Daniel Borkmann <dborkman@redhat.com>
---
 net/sctp/associola.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/net/sctp/associola.c b/net/sctp/associola.c
index 91cfd8f..756025c 100644
--- a/net/sctp/associola.c
+++ b/net/sctp/associola.c
@@ -86,10 +86,9 @@ static struct sctp_association *sctp_association_init(struct sctp_association *a
 
 	/* Discarding const is appropriate here.  */
 	asoc->ep = (struct sctp_endpoint *)ep;
-	sctp_endpoint_hold(asoc->ep);
-
-	/* Hold the sock.  */
 	asoc->base.sk = (struct sock *)sk;
+
+	sctp_endpoint_hold(asoc->ep);
 	sock_hold(asoc->base.sk);
 
 	/* Initialize the common base substructure.  */
@@ -343,8 +342,8 @@ static struct sctp_association *sctp_association_init(struct sctp_association *a
 	return asoc;
 
 fail_init:
-	sctp_endpoint_put(asoc->ep);
 	sock_put(asoc->base.sk);
+	sctp_endpoint_put(asoc->ep);
 	return NULL;
 }
 
-- 
1.7.11.7

Re: [PATCH net-next v3 0/4] Some SCTP cleanups/improvements

[Vlad Yasevich]

On 06/14/2013 12:24 PM, Daniel Borkmann wrote:
> v2 -> v3:
>    - Applied feedback from Vlad in first patch, rest is unchanged
>
> v1 -> v2:
>    - Applied Neil's feedback, I'll send out the first patch at a later
>      point in time though, have not included it in this set
>    - Add two other patches that appear to be useful from the last
>      couple of days debugging SCTP code
>
> Let it run through lksctp-tools and also my recent stress test tool,
> all looks good.

Looks good.

Acked-by: Vlad Yasevich <vyasevich@gmail.com>

>
> Daniel Borkmann (4):
>    net: sctp: sideeffect: throw BUG if primary_path is NULL
>    net: sctp: sctp_sf_do_prm_asoc: do SCTP_CMD_INIT_CHOOSE_TRANSPORT first
>    net: sctp: minor: remove variable in sctp_init_sock
>    net: sctp: sctp_association_init: put refs in reverse order
>
>   net/sctp/associola.c     |  7 +++----
>   net/sctp/proc.c          | 19 ++++++++++++++++---
>   net/sctp/sm_sideeffect.c |  5 ++++-
>   net/sctp/sm_statefuns.c  |  8 ++++----
>   net/sctp/socket.c        |  6 ++----
>   5 files changed, 29 insertions(+), 16 deletions(-)
>

Re: [PATCH net-next v3 0/4] Some SCTP cleanups/improvements

[David Miller]

From: Vlad Yasevich <vyasevich@gmail.com>
Date: Fri, 14 Jun 2013 13:00:54 -0400

> On 06/14/2013 12:24 PM, Daniel Borkmann wrote:
>> v2 -> v3:
>>    - Applied feedback from Vlad in first patch, rest is unchanged
>>
>> v1 -> v2:
>>    - Applied Neil's feedback, I'll send out the first patch at a later
>>      point in time though, have not included it in this set
>>    - Add two other patches that appear to be useful from the last
>>      couple of days debugging SCTP code
>>
>> Let it run through lksctp-tools and also my recent stress test tool,
>> all looks good.
> 
> Looks good.
> 
> Acked-by: Vlad Yasevich <vyasevich@gmail.com>

Series applied, thanks.