[PATCH net-next 0/5] sctp: fix and clean up

[PATCH net-next 0/5] sctp: fix and clean up

[Wang Weidong]

This patch series include: remove the duplicate initialize,
remove else path, fix one lost free, convert func to boolean,
fix some typos.

Wang Weidong (5):
  sctp: remove the duplicate initialize
  sctp: remove the else path
  sctp: if we done update ssnmap, free the new ssnmap
  sctp: convert sctp_peer_needs_update to boolean
  sctp: fix some typos in associola.c

 net/sctp/associola.c | 63 ++++++++++------------------------------------------
 1 file changed, 12 insertions(+), 51 deletions(-)

-- 
1.7.12

[PATCH net-next 1/5] sctp: remove the duplicate initialize

[Wang Weidong]

kzalloc had initialize the allocated memroy. Therefore, remove the
initialize with 0 and the memset.

Signed-off-by: Wang Weidong <wangweidong1@huawei.com>
---
 net/sctp/associola.c | 41 -----------------------------------------
 1 file changed, 41 deletions(-)

diff --git a/net/sctp/associola.c b/net/sctp/associola.c
index cef5099..667f984 100644
--- a/net/sctp/associola.c
+++ b/net/sctp/associola.c
@@ -90,14 +90,12 @@ static struct sctp_association *sctp_association_init(struct sctp_association *a
 
 	/* Initialize the object handling fields.  */
 	atomic_set(&asoc->base.refcnt, 1);
-	asoc->base.dead = false;
 
 	/* Initialize the bind addr area.  */
 	sctp_bind_addr_init(&asoc->base.bind_addr, ep->base.bind_addr.port);
 
 	asoc->state = SCTP_STATE_CLOSED;
 	asoc->cookie_life = ms_to_ktime(sp->assocparams.sasoc_cookie_life);
-	asoc->frag_point = 0;
 	asoc->user_frag = sp->user_frag;
 
 	/* Set the association max_retrans and RTO values from the
@@ -110,8 +108,6 @@ static struct sctp_association *sctp_association_init(struct sctp_association *a
 	asoc->rto_max = msecs_to_jiffies(sp->rtoinfo.srto_max);
 	asoc->rto_min = msecs_to_jiffies(sp->rtoinfo.srto_min);
 
-	asoc->overall_error_count = 0;
-
 	/* Initialize the association's heartbeat interval based on the
 	 * sock configured value.
 	 */
@@ -138,12 +134,9 @@ static struct sctp_association *sctp_association_init(struct sctp_association *a
 	asoc->max_burst = sp->max_burst;
 
 	/* initialize association timers */
-	asoc->timeouts[SCTP_EVENT_TIMEOUT_NONE] = 0;
 	asoc->timeouts[SCTP_EVENT_TIMEOUT_T1_COOKIE] = asoc->rto_initial;
 	asoc->timeouts[SCTP_EVENT_TIMEOUT_T1_INIT] = asoc->rto_initial;
 	asoc->timeouts[SCTP_EVENT_TIMEOUT_T2_SHUTDOWN] = asoc->rto_initial;
-	asoc->timeouts[SCTP_EVENT_TIMEOUT_T3_RTX] = 0;
-	asoc->timeouts[SCTP_EVENT_TIMEOUT_T4_RTO] = 0;
 
 	/* sctpimpguide Section 2.12.2
 	 * If the 'T5-shutdown-guard' timer is used, it SHOULD be set to the
@@ -152,7 +145,6 @@ static struct sctp_association *sctp_association_init(struct sctp_association *a
 	asoc->timeouts[SCTP_EVENT_TIMEOUT_T5_SHUTDOWN_GUARD]
 		= 5 * asoc->rto_max;
 
-	asoc->timeouts[SCTP_EVENT_TIMEOUT_HEARTBEAT] = 0;
 	asoc->timeouts[SCTP_EVENT_TIMEOUT_SACK] = asoc->sackdelay;
 	asoc->timeouts[SCTP_EVENT_TIMEOUT_AUTOCLOSE] =
 		min_t(unsigned long, sp->autoclose, net->sctp.max_autoclose) * HZ;
@@ -173,11 +165,6 @@ static struct sctp_association *sctp_association_init(struct sctp_association *a
 	asoc->max_init_timeo =
 		 msecs_to_jiffies(sp->initmsg.sinit_max_init_timeo);
 
-	/* Allocate storage for the ssnmap after the inbound and outbound
-	 * streams have been negotiated during Init.
-	 */
-	asoc->ssnmap = NULL;
-
 	/* Set the local window size for receive.
 	 * This is also the rcvbuf space per association.
 	 * RFC 6 - A SCTP receiver MUST be able to receive a minimum of
@@ -190,25 +177,15 @@ static struct sctp_association *sctp_association_init(struct sctp_association *a
 
 	asoc->a_rwnd = asoc->rwnd;
 
-	asoc->rwnd_over = 0;
-	asoc->rwnd_press = 0;
-
 	/* Use my own max window until I learn something better.  */
 	asoc->peer.rwnd = SCTP_DEFAULT_MAXWINDOW;
 
-	/* Set the sndbuf size for transmit.  */
-	asoc->sndbuf_used = 0;
-
 	/* Initialize the receive memory counter */
 	atomic_set(&asoc->rmem_alloc, 0);
 
 	init_waitqueue_head(&asoc->wait);
 
 	asoc->c.my_vtag = sctp_generate_tag(ep);
-	asoc->peer.i.init_tag = 0;     /* INIT needs a vtag of 0. */
-	asoc->c.peer_vtag = 0;
-	asoc->c.my_ttag   = 0;
-	asoc->c.peer_ttag = 0;
 	asoc->c.my_port = ep->base.bind_addr.port;
 
 	asoc->c.initial_tsn = sctp_generate_tsn(ep);
@@ -219,7 +196,6 @@ static struct sctp_association *sctp_association_init(struct sctp_association *a
 	asoc->adv_peer_ack_point = asoc->ctsn_ack_point;
 	asoc->highest_sacked = asoc->ctsn_ack_point;
 	asoc->last_cwr_tsn = asoc->ctsn_ack_point;
-	asoc->unack_data = 0;
 
 	/* ADDIP Section 4.1 Asconf Chunk Procedures
 	 *
@@ -238,7 +214,6 @@ static struct sctp_association *sctp_association_init(struct sctp_association *a
 
 	/* Make an empty list of remote transport addresses.  */
 	INIT_LIST_HEAD(&asoc->peer.transport_addr_list);
-	asoc->peer.transport_count = 0;
 
 	/* RFC 2960 5.1 Normal Establishment of an Association
 	 *
@@ -252,7 +227,6 @@ static struct sctp_association *sctp_association_init(struct sctp_association *a
 	 * already received one packet.]
 	 */
 	asoc->peer.sack_needed = 1;
-	asoc->peer.sack_cnt = 0;
 	asoc->peer.sack_generation = 1;
 
 	/* Assume that the peer will tell us if he recognizes ASCONF
@@ -260,12 +234,8 @@ static struct sctp_association *sctp_association_init(struct sctp_association *a
 	 * The sctp_addip_noauth option is there for backward compatibilty
 	 * and will revert old behavior.
 	 */
-	asoc->peer.asconf_capable = 0;
 	if (net->sctp.addip_noauth)
 		asoc->peer.asconf_capable = 1;
-	asoc->asconf_addr_del_pending = NULL;
-	asoc->src_out_of_asoc_ok = 0;
-	asoc->new_transport = NULL;
 
 	/* Create an input queue.  */
 	sctp_inq_init(&asoc->base.inqueue);
@@ -277,12 +247,6 @@ static struct sctp_association *sctp_association_init(struct sctp_association *a
 	if (!sctp_ulpq_init(&asoc->ulpq, asoc))
 		goto fail_init;
 
-	memset(&asoc->peer.tsn_map, 0, sizeof(struct sctp_tsnmap));
-
-	asoc->need_ecne = 0;
-
-	asoc->assoc_id = 0;
-
 	/* Assume that peer would support both address types unless we are
 	 * told otherwise.
 	 */
@@ -300,9 +264,6 @@ static struct sctp_association *sctp_association_init(struct sctp_association *a
 	asoc->default_timetolive = sp->default_timetolive;
 	asoc->default_rcv_context = sp->default_rcv_context;
 
-	/* SCTP_GET_ASSOC_STATS COUNTERS */
-	memset(&asoc->stats, 0, sizeof(struct sctp_priv_assoc_stats));
-
 	/* AUTH related initializations */
 	INIT_LIST_HEAD(&asoc->endpoint_shared_keys);
 	err = sctp_auth_asoc_copy_shkeys(ep, asoc, gfp);
@@ -310,9 +271,7 @@ static struct sctp_association *sctp_association_init(struct sctp_association *a
 		goto fail_init;
 
 	asoc->active_key_id = ep->active_key_id;
-	asoc->asoc_shared_key = NULL;
 
-	asoc->default_hmac_id = 0;
 	/* Save the hmacs and chunks list into this association */
 	if (ep->auth_hmacs_list)
 		memcpy(asoc->c.auth_hmacs, ep->auth_hmacs_list,
-- 
1.7.12

[PATCH net-next 2/5] sctp: remove the else path

[Wang Weidong]

Make the code more simplification.

Signed-off-by: Wang Weidong <wangweidong1@huawei.com>
---
 net/sctp/associola.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/net/sctp/associola.c b/net/sctp/associola.c
index 667f984..2d53d4c 100644
--- a/net/sctp/associola.c
+++ b/net/sctp/associola.c
@@ -954,15 +954,13 @@ int sctp_cmp_addr_exact(const union sctp_addr *ss1,
  */
 struct sctp_chunk *sctp_get_ecne_prepend(struct sctp_association *asoc)
 {
-	struct sctp_chunk *chunk;
+	struct sctp_chunk *chunk = NULL;
 
 	/* Send ECNE if needed.
 	 * Not being able to allocate a chunk here is not deadly.
 	 */
 	if (asoc->need_ecne)
 		chunk = sctp_make_ecne(asoc, asoc->last_ecne_tsn);
-	else
-		chunk = NULL;
 
 	return chunk;
 }
-- 
1.7.12

[PATCH net-next 3/5] sctp: if we done update ssnmap, free the new ssnmap

[Wang Weidong]

I found that free the old parameters after update the peer parameters.
So we should free the new ssnmap which not updated.

Signed-off-by: Wang Weidong <wangweidong1@huawei.com>
---
 net/sctp/associola.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/net/sctp/associola.c b/net/sctp/associola.c
index 2d53d4c..bca1ad1 100644
--- a/net/sctp/associola.c
+++ b/net/sctp/associola.c
@@ -1223,6 +1223,10 @@ void sctp_assoc_update(struct sctp_association *asoc,
 		}
 	}
 
+	/* if we don't update the ssnmap, we should free it. */
+	if (new->ssnmap)
+		sctp_ssnmap_free(new->ssnmap);
+
 	/* SCTP-AUTH: Save the peer parameters from the new assocaitions
 	 * and also move the association shared keys over
 	 */
-- 
1.7.12

[PATCH net-next 4/5] sctp: convert sctp_peer_needs_update to boolean

[Wang Weidong]

sctp_peer_needs_update only return 0 or 1.

Signed-off-by: Wang Weidong <wangweidong1@huawei.com>
---
 net/sctp/associola.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/net/sctp/associola.c b/net/sctp/associola.c
index bca1ad1..c6cfff6 100644
--- a/net/sctp/associola.c
+++ b/net/sctp/associola.c
@@ -1355,7 +1355,7 @@ void sctp_assoc_sync_pmtu(struct sock *sk, struct sctp_association *asoc)
 }
 
 /* Should we send a SACK to update our peer? */
-static inline int sctp_peer_needs_update(struct sctp_association *asoc)
+static inline bool sctp_peer_needs_update(struct sctp_association *asoc)
 {
 	struct net *net = sock_net(asoc->base.sk);
 	switch (asoc->state) {
@@ -1367,12 +1367,12 @@ static inline int sctp_peer_needs_update(struct sctp_association *asoc)
 		    ((asoc->rwnd - asoc->a_rwnd) >= max_t(__u32,
 			   (asoc->base.sk->sk_rcvbuf >> net->sctp.rwnd_upd_shift),
 			   asoc->pathmtu)))
-			return 1;
+			return true;
 		break;
 	default:
 		break;
 	}
-	return 0;
+	return false;
 }
 
 /* Increase asoc's rwnd by len and send any window update SACK if needed. */
-- 
1.7.12

[PATCH net-next 5/5] sctp: fix some typos in associola.c

[Wang Weidong]

fix some typos

Signed-off-by: Wang Weidong <wangweidong1@huawei.com>
---
 net/sctp/associola.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/net/sctp/associola.c b/net/sctp/associola.c
index c6cfff6..fadf31d 100644
--- a/net/sctp/associola.c
+++ b/net/sctp/associola.c
@@ -128,7 +128,7 @@ static struct sctp_association *sctp_association_init(struct sctp_association *a
 	 */
 	asoc->param_flags = sp->param_flags;
 
-	/* Initialize the maximum mumber of new data packets that can be sent
+	/* Initialize the maximum number of new data packets that can be sent
 	 * in a burst.
 	 */
 	asoc->max_burst = sp->max_burst;
@@ -231,7 +231,7 @@ static struct sctp_association *sctp_association_init(struct sctp_association *a
 
 	/* Assume that the peer will tell us if he recognizes ASCONF
 	 * as part of INIT exchange.
-	 * The sctp_addip_noauth option is there for backward compatibilty
+	 * The sctp_addip_noauth option is there for backward compatibility
 	 * and will revert old behavior.
 	 */
 	if (net->sctp.addip_noauth)
@@ -1227,7 +1227,7 @@ void sctp_assoc_update(struct sctp_association *asoc,
 	if (new->ssnmap)
 		sctp_ssnmap_free(new->ssnmap);
 
-	/* SCTP-AUTH: Save the peer parameters from the new assocaitions
+	/* SCTP-AUTH: Save the peer parameters from the new associations
 	 * and also move the association shared keys over
 	 */
 	kfree(asoc->peer.peer_random);
@@ -1452,7 +1452,7 @@ void sctp_assoc_rwnd_decrease(struct sctp_association *asoc, unsigned int len)
 
 	/* If we've reached or overflowed our receive buffer, announce
 	 * a 0 rwnd if rwnd would still be positive.  Store the
-	 * the pottential pressure overflow so that the window can be restored
+	 * the potential pressure overflow so that the window can be restored
 	 * back to original value.
 	 */
 	if (rx_count >= asoc->base.sk->sk_rcvbuf)
-- 
1.7.12

Re: [PATCH net-next 2/5] sctp: remove the else path

[Joe Perches]

On Fri, 2013-11-08 at 10:55 +0800, Wang Weidong wrote:
> Make the code more simplification.
> 
> Signed-off-by: Wang Weidong <wangweidong1@huawei.com>
> ---
>  net/sctp/associola.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/net/sctp/associola.c b/net/sctp/associola.c
> index 667f984..2d53d4c 100644
> --- a/net/sctp/associola.c
> +++ b/net/sctp/associola.c
> @@ -954,15 +954,13 @@ int sctp_cmp_addr_exact(const union sctp_addr *ss1,
>   */
>  struct sctp_chunk *sctp_get_ecne_prepend(struct sctp_association *asoc)
>  {
> -	struct sctp_chunk *chunk;
> +	struct sctp_chunk *chunk = NULL;
>  
>  	/* Send ECNE if needed.
>  	 * Not being able to allocate a chunk here is not deadly.
>  	 */
>  	if (asoc->need_ecne)
>  		chunk = sctp_make_ecne(asoc, asoc->last_ecne_tsn);
> -	else
> -		chunk = NULL;
>  
>  	return chunk;
>  }

If you really want to make the code simple how about:

struct sctp_chunk *sctp_get_ecne_prepend(struct sctp_association *asoc)
{
	if (!asoc->need_ecne)
		return NULL;

	return sctp_make_ecne(asoc, asoc->last_ecne_tsn);
}

Re: [PATCH net-next 2/5] sctp: remove the else path

[wangweidong]

On 2013/11/8 11:00, Joe Perches wrote:
> On Fri, 2013-11-08 at 10:55 +0800, Wang Weidong wrote:
>> Make the code more simplification.
>>
>> Signed-off-by: Wang Weidong <wangweidong1@huawei.com>
>> ---
>>  net/sctp/associola.c | 4 +---
>>  1 file changed, 1 insertion(+), 3 deletions(-)
>>
>> diff --git a/net/sctp/associola.c b/net/sctp/associola.c
>> index 667f984..2d53d4c 100644
>> --- a/net/sctp/associola.c
>> +++ b/net/sctp/associola.c
>> @@ -954,15 +954,13 @@ int sctp_cmp_addr_exact(const union sctp_addr *ss1,
>>   */
>>  struct sctp_chunk *sctp_get_ecne_prepend(struct sctp_association *asoc)
>>  {
>> -	struct sctp_chunk *chunk;
>> +	struct sctp_chunk *chunk = NULL;
>>  
>>  	/* Send ECNE if needed.
>>  	 * Not being able to allocate a chunk here is not deadly.
>>  	 */
>>  	if (asoc->need_ecne)
>>  		chunk = sctp_make_ecne(asoc, asoc->last_ecne_tsn);
>> -	else
>> -		chunk = NULL;
>>  
>>  	return chunk;
>>  }
> 
> If you really want to make the code simple how about:
> 
> struct sctp_chunk *sctp_get_ecne_prepend(struct sctp_association *asoc)
> {
> 	if (!asoc->need_ecne)
> 		return NULL;
> 
> 	return sctp_make_ecne(asoc, asoc->last_ecne_tsn);
> }
> 
> 

Yeah, you are right. I will fix it in v2.
Thanks. 

> 
> 

Re: [PATCH net-next 3/5] sctp: if we done update ssnmap, free the new ssnmap

[Vlad Yasevich]

On 11/07/2013 09:55 PM, Wang Weidong wrote:
> I found that free the old parameters after update the peer parameters.
> So we should free the new ssnmap which not updated.
>
> Signed-off-by: Wang Weidong <wangweidong1@huawei.com>
> ---
>   net/sctp/associola.c | 4 ++++
>   1 file changed, 4 insertions(+)
>
> diff --git a/net/sctp/associola.c b/net/sctp/associola.c
> index 2d53d4c..bca1ad1 100644
> --- a/net/sctp/associola.c
> +++ b/net/sctp/associola.c
> @@ -1223,6 +1223,10 @@ void sctp_assoc_update(struct sctp_association *asoc,
>   		}
>   	}
>
> +	/* if we don't update the ssnmap, we should free it. */
> +	if (new->ssnmap)
> +		sctp_ssnmap_free(new->ssnmap);
> +
>   	/* SCTP-AUTH: Save the peer parameters from the new assocaitions
>   	 * and also move the association shared keys over
>   	 */
>

Not necessary.  This will be done when the 'new' association is destroyed.

-vlad

Re: [PATCH net-next 3/5] sctp: if we done update ssnmap, free the new ssnmap

[wangweidong]

On 2013/11/8 11:49, Vlad Yasevich wrote:
> On 11/07/2013 09:55 PM, Wang Weidong wrote:
>> I found that free the old parameters after update the peer parameters.
>> So we should free the new ssnmap which not updated.
>>
>> Signed-off-by: Wang Weidong <wangweidong1@huawei.com>
>> ---
>>   net/sctp/associola.c | 4 ++++
>>   1 file changed, 4 insertions(+)
>>
>> diff --git a/net/sctp/associola.c b/net/sctp/associola.c
>> index 2d53d4c..bca1ad1 100644
>> --- a/net/sctp/associola.c
>> +++ b/net/sctp/associola.c
>> @@ -1223,6 +1223,10 @@ void sctp_assoc_update(struct sctp_association *asoc,
>>           }
>>       }
>>
>> +    /* if we don't update the ssnmap, we should free it. */
>> +    if (new->ssnmap)
>> +        sctp_ssnmap_free(new->ssnmap);
>> +
>>       /* SCTP-AUTH: Save the peer parameters from the new assocaitions
>>        * and also move the association shared keys over
>>        */
>>
> 
> Not necessary.  This will be done when the 'new' association is destroyed.
> 
> -vlad
>

Yeah, it is not necessary done in sctp_assoc_update.
Thanks. 

> 

Re: [PATCH net-next 0/5] sctp: fix and clean up

[David Miller]

Such patches are not appropriate to submit at this time.

Please wait until the current merge window is completed and
the net-next tree is openned back up.

Thank you.

Re: [PATCH net-next 2/5] sctp: remove the else path

[Sergei Shtylyov]

Hello.

On 11/08/2013 05:55 AM, Wang Weidong wrote:

> Make the code more simplification.

> Signed-off-by: Wang Weidong <wangweidong1@huawei.com>
> ---
>   net/sctp/associola.c | 4 +---
>   1 file changed, 1 insertion(+), 3 deletions(-)

> diff --git a/net/sctp/associola.c b/net/sctp/associola.c
> index 667f984..2d53d4c 100644
> --- a/net/sctp/associola.c
> +++ b/net/sctp/associola.c
> @@ -954,15 +954,13 @@ int sctp_cmp_addr_exact(const union sctp_addr *ss1,
>    */
>   struct sctp_chunk *sctp_get_ecne_prepend(struct sctp_association *asoc)
>   {
> -	struct sctp_chunk *chunk;
> +	struct sctp_chunk *chunk = NULL;
>
>   	/* Send ECNE if needed.
>   	 * Not being able to allocate a chunk here is not deadly.
>   	 */
>   	if (asoc->need_ecne)
>   		chunk = sctp_make_ecne(asoc, asoc->last_ecne_tsn);
> -	else
> -		chunk = NULL;

    This is hardly better/simpler than it was.

WBR, Sergei

Re: [PATCH net-next 2/5] sctp: remove the else path

[wangweidong]

On 2013/11/9 3:19, Sergei Shtylyov wrote:
> Hello.
> 
> On 11/08/2013 05:55 AM, Wang Weidong wrote:
> 
>> Make the code more simplification.
> 
>> Signed-off-by: Wang Weidong <wangweidong1@huawei.com>
>> ---
>>   net/sctp/associola.c | 4 +---
>>   1 file changed, 1 insertion(+), 3 deletions(-)
> 
>> diff --git a/net/sctp/associola.c b/net/sctp/associola.c
>> index 667f984..2d53d4c 100644
>> --- a/net/sctp/associola.c
>> +++ b/net/sctp/associola.c
>> @@ -954,15 +954,13 @@ int sctp_cmp_addr_exact(const union sctp_addr *ss1,
>>    */
>>   struct sctp_chunk *sctp_get_ecne_prepend(struct sctp_association *asoc)
>>   {
>> -    struct sctp_chunk *chunk;
>> +    struct sctp_chunk *chunk = NULL;
>>
>>       /* Send ECNE if needed.
>>        * Not being able to allocate a chunk here is not deadly.
>>        */
>>       if (asoc->need_ecne)
>>           chunk = sctp_make_ecne(asoc, asoc->last_ecne_tsn);
>> -    else
>> -        chunk = NULL;
> 
>    This is hardly better/simpler than it was.
> 
> WBR, Sergei
> 

Well, you are right. Joe had suggested a good idea to me. And I had fixed
it in the v2.
Thanks. 

> 
>