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

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

[Daniel Borkmann]

v1 -> v2:
  - Apply 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

Daniel Borkmann (4):
  net: sctp: throw BUG if primary_path in sctp_seq_dump_local_addrs 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 on error

 net/sctp/associola.c    |  7 +++----
 net/sctp/proc.c         | 15 ++++++++++++---
 net/sctp/sm_statefuns.c |  8 ++++----
 net/sctp/socket.c       |  6 ++----
 4 files changed, 21 insertions(+), 15 deletions(-)

-- 
1.7.11.7

[PATCH net-next 1/4] net: sctp: sctp_seq_dump_local_addrs: 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 comes up again, throw a BUG and add a comment
why this is the case since it is not too obvious 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 | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/net/sctp/proc.c b/net/sctp/proc.c
index 4e45ee3..f171366 100644
--- a/net/sctp/proc.c
+++ b/net/sctp/proc.c
@@ -134,9 +134,18 @@ 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;
+
+		/* There must be no such case where an association is linked
+		 * into sctp_assoc_hashtable that does not have a primary
+		 * path! This means either sctp_association_free() was called
+		 * without sctp_unhash_established(), or somewhere in the
+		 * interpreter SCTP_CMD_ASOC_NEW was called on a non-fully
+		 * set up association. So do hara-kiri until this is fixed.
+		 */
+		BUG_ON(peer == NULL);
+		primary = &peer->saddr;
 	}
 
 	rcu_read_lock();
-- 
1.7.11.7

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

[Vlad Yasevich]

On 06/13/2013 12:04 PM, Daniel Borkmann wrote:
> 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 comes up again, throw a BUG and add a comment
> why this is the case since it is not too obvious 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 | 15 ++++++++++++---
>   1 file changed, 12 insertions(+), 3 deletions(-)
>
> diff --git a/net/sctp/proc.c b/net/sctp/proc.c
> index 4e45ee3..f171366 100644
> --- a/net/sctp/proc.c
> +++ b/net/sctp/proc.c
> @@ -134,9 +134,18 @@ 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;
> +
> +		/* There must be no such case where an association is linked
> +		 * into sctp_assoc_hashtable that does not have a primary
> +		 * path! This means either sctp_association_free() was called
> +		 * without sctp_unhash_established(), or somewhere in the
> +		 * interpreter SCTP_CMD_ASOC_NEW was called on a non-fully
> +		 * set up association. So do hara-kiri until this is fixed.
> +		 */
> +		BUG_ON(peer == NULL);
> +		primary = &peer->saddr;

I am still trying to convince myself whether this BUG_ON() is the right 
thing to do...

The fact that we reached this association may not necessarily help in 
diagnosing how we managed that and what might be going on.  Also 
crashing the system just because someone read /proc is a bit of an
overkill, especially considering that the system might have stayed up
if the user did not read /proc.

One thought I had was to change the above into something like this:
	if (peer == NULL) {
		WARN(1, "Association %p with NULL primary path", asoc);
		return;
	}

And add the following to handler for SCTP_CMD_NEW_ASOC and may be also 
to sctp_cmd_delete_tcb()

	BUG_ON(asoc->peer.primary_path == NULL);

This way, we would bug on additional and removal paths which have the
possibility of giving us a lot more information about why the condition
occurred in the first place.

-vlad



>   	}
>
>   	rcu_read_lock();
>

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

[Daniel Borkmann]

On 06/14/2013 04:33 PM, Vlad Yasevich wrote:
> On 06/13/2013 12:04 PM, Daniel Borkmann wrote:
>> 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 comes up again, throw a BUG and add a comment
>> why this is the case since it is not too obvious 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 | 15 ++++++++++++---
>>   1 file changed, 12 insertions(+), 3 deletions(-)
>>
>> diff --git a/net/sctp/proc.c b/net/sctp/proc.c
>> index 4e45ee3..f171366 100644
>> --- a/net/sctp/proc.c
>> +++ b/net/sctp/proc.c
>> @@ -134,9 +134,18 @@ 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;
>> +
>> +        /* There must be no such case where an association is linked
>> +         * into sctp_assoc_hashtable that does not have a primary
>> +         * path! This means either sctp_association_free() was called
>> +         * without sctp_unhash_established(), or somewhere in the
>> +         * interpreter SCTP_CMD_ASOC_NEW was called on a non-fully
>> +         * set up association. So do hara-kiri until this is fixed.
>> +         */
>> +        BUG_ON(peer == NULL);
>> +        primary = &peer->saddr;
>
> I am still trying to convince myself whether this BUG_ON() is the right thing to do...
>
> The fact that we reached this association may not necessarily help in diagnosing how we managed that and what might be going on.  Also crashing the system just because someone read /proc is a bit of an
> overkill, especially considering that the system might have stayed up
> if the user did not read /proc.

Well, but this patch actually makes no difference at all. Even if this BUG_ON would
not be there, then the next thing that happens is a NULL ptr dereference in cmp_addr()
on the primary pointer, right as in the stack trace I've sent with the recent patch.

So we might as well tell the user why this is happening before he debugs on this code
for quite a while.

> One thought I had was to change the above into something like this:
>      if (peer == NULL) {
>          WARN(1, "Association %p with NULL primary path", asoc);
>          return;
>      }

Ok, this could be an alternative. It would suck to just have a crash because we want
to print out an asterisk character. :-)

I'm not sure about the side effects if we leave that association in the list and just
warn, maybe it will also trigger a crash sooner or later, but sure, we could do this
like that.

> And add the following to handler for SCTP_CMD_NEW_ASOC and may be also to sctp_cmd_delete_tcb()
>
>      BUG_ON(asoc->peer.primary_path == NULL);
>
> This way, we would bug on additional and removal paths which have the
> possibility of giving us a lot more information about why the condition
> occurred in the first place.

Agreed, sounds good to me. Then let me resubmit the set with the proposed change.

Cheers,

Daniel

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

[Vlad Yasevich]

On 06/14/2013 10:51 AM, Daniel Borkmann wrote:
> On 06/14/2013 04:33 PM, Vlad Yasevich wrote:
>> On 06/13/2013 12:04 PM, Daniel Borkmann wrote:
>>> 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 comes up again, throw a BUG and add a comment
>>> why this is the case since it is not too obvious 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 | 15 ++++++++++++---
>>>   1 file changed, 12 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/net/sctp/proc.c b/net/sctp/proc.c
>>> index 4e45ee3..f171366 100644
>>> --- a/net/sctp/proc.c
>>> +++ b/net/sctp/proc.c
>>> @@ -134,9 +134,18 @@ 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;
>>> +
>>> +        /* There must be no such case where an association is linked
>>> +         * into sctp_assoc_hashtable that does not have a primary
>>> +         * path! This means either sctp_association_free() was called
>>> +         * without sctp_unhash_established(), or somewhere in the
>>> +         * interpreter SCTP_CMD_ASOC_NEW was called on a non-fully
>>> +         * set up association. So do hara-kiri until this is fixed.
>>> +         */
>>> +        BUG_ON(peer == NULL);
>>> +        primary = &peer->saddr;
>>
>> I am still trying to convince myself whether this BUG_ON() is the
>> right thing to do...
>>
>> The fact that we reached this association may not necessarily help in
>> diagnosing how we managed that and what might be going on.  Also
>> crashing the system just because someone read /proc is a bit of an
>> overkill, especially considering that the system might have stayed up
>> if the user did not read /proc.
>
> Well, but this patch actually makes no difference at all. Even if this
> BUG_ON would
> not be there, then the next thing that happens is a NULL ptr dereference
> in cmp_addr()
> on the primary pointer, right as in the stack trace I've sent with the
> recent patch.
>
> So we might as well tell the user why this is happening before he debugs
> on this code
> for quite a while.
>
>> One thought I had was to change the above into something like this:
>>      if (peer == NULL) {
>>          WARN(1, "Association %p with NULL primary path", asoc);
>>          return;
>>      }
>
> Ok, this could be an alternative. It would suck to just have a crash
> because we want
> to print out an asterisk character. :-)
>
> I'm not sure about the side effects if we leave that association in the
> list and just
> warn, maybe it will also trigger a crash sooner or later, but sure, we
> could do this
> like that.

It may trigger the crash later if the user performs some action on the 
association that touches the primary.   That's the reason why I was 
proposing the checks below.

With the checks in command interpreter, we are only left with the 
possibility that primary_path changes to NULL during the association 
lifetime, which code audit doesn't support right now.  If that ever 
changes we would at least have a bit more information to go on.

>
>> And add the following to handler for SCTP_CMD_NEW_ASOC and may be also
>> to sctp_cmd_delete_tcb()
>>
>>      BUG_ON(asoc->peer.primary_path == NULL);
>>
>> This way, we would bug on additional and removal paths which have the
>> possibility of giving us a lot more information about why the condition
>> occurred in the first place.
>
> Agreed, sounds good to me. Then let me resubmit the set with the
> proposed change.
>

OK.

-vlad
> Cheers,
>
> Daniel

[PATCH net-next 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 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 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 0/4] Some SCTP cleanups/improvements

[David Miller]

From: Daniel Borkmann <dborkman@redhat.com>
Date: Thu, 13 Jun 2013 18:04:19 +0200

> v1 -> v2:
>   - Apply 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
> 
> Daniel Borkmann (4):
>   net: sctp: throw BUG if primary_path in sctp_seq_dump_local_addrs 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 on error

Looks good to me, SCTP folks please review.

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

[Neil Horman]

On Thu, Jun 13, 2013 at 06:04:19PM +0200, Daniel Borkmann wrote:
> v1 -> v2:
>   - Apply 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
> 
> Daniel Borkmann (4):
>   net: sctp: throw BUG if primary_path in sctp_seq_dump_local_addrs 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 on error
> 
>  net/sctp/associola.c    |  7 +++----
>  net/sctp/proc.c         | 15 ++++++++++++---
>  net/sctp/sm_statefuns.c |  8 ++++----
>  net/sctp/socket.c       |  6 ++----
>  4 files changed, 21 insertions(+), 15 deletions(-)
> 
> -- 
> 1.7.11.7
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-sctp" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

For the series
Acked-by: Neil Horman <nhorman@tuxdriver.com>