[patch] sctp: fix test for end of loop

[patch] sctp: fix test for end of loop

[Dan Carpenter]

"new_addr" is the list cursor here and it's always non-NULL.

We're trying to test if we exited because the loop ended or we hit the
break statement.  Really testing !found is enough so long as 
"new_asoc->peer.transport_addr_list" is not empty and I believe it never
is empty at this point.  So this is never really a bug with the current
code.

Signed-off-by: Dan Carpenter <error27@gmail.com>
---
Compile tested only.

diff --git a/net/sctp/sm_statefuns.c b/net/sctp/sm_statefuns.c
index 24b2cd5..cb76d2e 100644
--- a/net/sctp/sm_statefuns.c
+++ b/net/sctp/sm_statefuns.c
@@ -1254,7 +1254,6 @@ static int sctp_sf_check_restart_addrs(const struct sctp_association *new_asoc,
 	/* Search through all current addresses and make sure
 	 * we aren't adding any new ones.
 	 */
-	new_addr = NULL;
 	found = 0;
 
 	list_for_each_entry(new_addr, &new_asoc->peer.transport_addr_list,
@@ -1273,7 +1272,8 @@ static int sctp_sf_check_restart_addrs(const struct sctp_association *new_asoc,
 	}
 
 	/* If a new address was added, ABORT the sender. */
-	if (!found && new_addr) {
+	if (!found &&
+	    &new_addr->transports != &new_asoc->peer.transport_addr_list) {
 		sctp_sf_send_restart_abort(&new_addr->ipaddr, init, commands);
 	}
 

Re: [patch] sctp: fix test for end of loop

[Shan Wei]

Dan Carpenter wrote, at 09/06/2010 08:26 PM:
> +	    &new_addr->transports != &new_asoc->peer.transport_addr_list) {

why did you add this check?


-- 
Best Regards
-----
Shan Wei

Re: [patch] sctp: fix test for end of loop

[Dan Carpenter]

On Tue, Sep 07, 2010 at 04:46:58PM +0800, Shan Wei wrote:
> Dan Carpenter wrote, at 09/06/2010 08:26 PM:
> > +	    &new_addr->transports != &new_asoc->peer.transport_addr_list) {
> 
> why did you add this check?
> 

That's the check which tells us if we broke out of the loop or if we
came to the end of the list.

As I explained before, the only way that the check matters is if the
list is empty.  With the current code I do not think we ever call this
function with an empty list, so that check is not needed.  But the code
could change I suppose and it doesn't hurt to be cautious.  On the other
hand, I'm fine with removing the check as well.

regards,
dan carpenter

> 
> -- 
> Best Regards
> -----
> Shan Wei

Re: [patch] sctp: fix test for end of loop

[David Miller]

From: Dan Carpenter <error27@gmail.com>
Date: Mon, 6 Sep 2010 14:26:39 +0200

> "new_addr" is the list cursor here and it's always non-NULL.
> 
> We're trying to test if we exited because the loop ended or we hit the
> break statement.  Really testing !found is enough so long as 
> "new_asoc->peer.transport_addr_list" is not empty and I believe it never
> is empty at this point.  So this is never really a bug with the current
> code.
> 
> Signed-off-by: Dan Carpenter <error27@gmail.com>

If you don't mind, I still think the code is confusing after your
patch even if the result is correct.

What do you think about the following kind of approach instead?

Thanks!

diff --git a/net/sctp/sm_statefuns.c b/net/sctp/sm_statefuns.c
index 8b28443..3d5bbae7 100644
--- a/net/sctp/sm_statefuns.c
+++ b/net/sctp/sm_statefuns.c
@@ -1241,7 +1241,7 @@ static int sctp_sf_check_restart_addrs(const struct sctp_association *new_asoc,
 				       sctp_cmd_seq_t *commands)
 {
 	struct sctp_transport *new_addr, *addr;
-	int found;
+	int ret = 1;
 
 	/* Implementor's Guide - Sectin 5.2.2
 	 * ...
@@ -1254,31 +1254,28 @@ static int sctp_sf_check_restart_addrs(const struct sctp_association *new_asoc,
 	/* Search through all current addresses and make sure
 	 * we aren't adding any new ones.
 	 */
-	new_addr = NULL;
-	found = 0;
-
 	list_for_each_entry(new_addr, &new_asoc->peer.transport_addr_list,
 			transports) {
-		found = 0;
 		list_for_each_entry(addr, &asoc->peer.transport_addr_list,
 				transports) {
 			if (sctp_cmp_addr_exact(&new_addr->ipaddr,
-						&addr->ipaddr)) {
-				found = 1;
-				break;
-			}
+						&addr->ipaddr))
+				goto next;
 		}
-		if (!found)
-			break;
-	}
 
-	/* If a new address was added, ABORT the sender. */
-	if (!found && new_addr) {
+		/* 'new_addr' could not be found in the transport address
+		 * list of 'asoc', abort.
+		 */
 		sctp_sf_send_restart_abort(&new_addr->ipaddr, init, commands);
+		ret = 0;
+		break;
+
+	next:
+		;
 	}
 
 	/* Return success if all addresses were found. */
-	return found;
+	return ret;
 }
 
 /* Populate the verification/tie tags based on overlapping INIT

Re: [patch] sctp: fix test for end of loop

[Vlad Yasevich]

On 09/08/2010 04:24 PM, David Miller wrote:
> From: Dan Carpenter <error27@gmail.com>
> Date: Mon, 6 Sep 2010 14:26:39 +0200
> 
>> "new_addr" is the list cursor here and it's always non-NULL.
>>
>> We're trying to test if we exited because the loop ended or we hit the
>> break statement.  Really testing !found is enough so long as 
>> "new_asoc->peer.transport_addr_list" is not empty and I believe it never
>> is empty at this point.  So this is never really a bug with the current
>> code.
>>
>> Signed-off-by: Dan Carpenter <error27@gmail.com>
> 
> If you don't mind, I still think the code is confusing after your
> patch even if the result is correct.
> 
> What do you think about the following kind of approach instead?
> 
> Thanks!
> 
> diff --git a/net/sctp/sm_statefuns.c b/net/sctp/sm_statefuns.c
> index 8b28443..3d5bbae7 100644
> --- a/net/sctp/sm_statefuns.c
> +++ b/net/sctp/sm_statefuns.c
> @@ -1241,7 +1241,7 @@ static int sctp_sf_check_restart_addrs(const struct sctp_association *new_asoc,
>  				       sctp_cmd_seq_t *commands)
>  {
>  	struct sctp_transport *new_addr, *addr;
> -	int found;
> +	int ret = 1;
>  
>  	/* Implementor's Guide - Sectin 5.2.2
>  	 * ...
> @@ -1254,31 +1254,28 @@ static int sctp_sf_check_restart_addrs(const struct sctp_association *new_asoc,
>  	/* Search through all current addresses and make sure
>  	 * we aren't adding any new ones.
>  	 */
> -	new_addr = NULL;
> -	found = 0;
> -
>  	list_for_each_entry(new_addr, &new_asoc->peer.transport_addr_list,
>  			transports) {
> -		found = 0;
>  		list_for_each_entry(addr, &asoc->peer.transport_addr_list,
>  				transports) {
>  			if (sctp_cmp_addr_exact(&new_addr->ipaddr,
> -						&addr->ipaddr)) {
> -				found = 1;
> -				break;
> -			}
> +						&addr->ipaddr))
> +				goto next;
>  		}
> -		if (!found)
> -			break;
> -	}
>  
> -	/* If a new address was added, ABORT the sender. */
> -	if (!found && new_addr) {
> +		/* 'new_addr' could not be found in the transport address
> +		 * list of 'asoc', abort.
> +		 */
>  		sctp_sf_send_restart_abort(&new_addr->ipaddr, init, commands);
> +		ret = 0;
> +		break;
> +
> +	next:
> +		;
>  	}
>  

This would certainly make things clearer as well.  It essentially does what I suggested
(moving the abort into the loop once we know we have a new address) and clean up all
the 'found' mess at the same time.

The empty goto tag would give my old profs an apoplexy though. :)

I would ack this.

-vlad
>  	/* Return success if all addresses were found. */
> -	return found;
> +	return ret;
>  }
>  
>  /* Populate the verification/tie tags based on overlapping INIT
> 

Re: [patch] sctp: fix test for end of loop

[David Miller]

From: Vlad Yasevich <vladislav.yasevich@hp.com>
Date: Wed, 08 Sep 2010 16:34:46 -0400

> I would ack this.

So why don't you? :-)

sctp: Fix test for end of loop in sctp_sf_check_restart_addrs().

Based upon a patch by Dan Carpenter.

Signed-off-by: David S. Miller <davem@davemloft.net>

diff --git a/net/sctp/sm_statefuns.c b/net/sctp/sm_statefuns.c
index 8b28443..3d5bbae7 100644
--- a/net/sctp/sm_statefuns.c
+++ b/net/sctp/sm_statefuns.c
@@ -1241,7 +1241,7 @@ static int sctp_sf_check_restart_addrs(const struct sctp_association *new_asoc,
 				       sctp_cmd_seq_t *commands)
 {
 	struct sctp_transport *new_addr, *addr;
-	int found;
+	int ret = 1;
 
 	/* Implementor's Guide - Sectin 5.2.2
 	 * ...
@@ -1254,31 +1254,28 @@ static int sctp_sf_check_restart_addrs(const struct sctp_association *new_asoc,
 	/* Search through all current addresses and make sure
 	 * we aren't adding any new ones.
 	 */
-	new_addr = NULL;
-	found = 0;
-
 	list_for_each_entry(new_addr, &new_asoc->peer.transport_addr_list,
 			transports) {
-		found = 0;
 		list_for_each_entry(addr, &asoc->peer.transport_addr_list,
 				transports) {
 			if (sctp_cmp_addr_exact(&new_addr->ipaddr,
-						&addr->ipaddr)) {
-				found = 1;
-				break;
-			}
+						&addr->ipaddr))
+				goto next;
 		}
-		if (!found)
-			break;
-	}
 
-	/* If a new address was added, ABORT the sender. */
-	if (!found && new_addr) {
+		/* 'new_addr' could not be found in the transport address
+		 * list of 'asoc', abort.
+		 */
 		sctp_sf_send_restart_abort(&new_addr->ipaddr, init, commands);
+		ret = 0;
+		break;
+
+	next:
+		;
 	}
 
 	/* Return success if all addresses were found. */
-	return found;
+	return ret;
 }
 
 /* Populate the verification/tie tags based on overlapping INIT

[Alternative PATCH net-next] sctp: fix test for end of loop

[Joe Perches]

Perhaps something like this is clearer?

Add a list_has_sctp_addr function to simplify loop

Based on a patches by Dan Carpenter and David Miller

Signed-off-by: Joe Perches <joe@perches.com>
---
 net/sctp/sm_statefuns.c |   46 +++++++++++++++++++++++-----------------------
 1 files changed, 23 insertions(+), 23 deletions(-)

diff --git a/net/sctp/sm_statefuns.c b/net/sctp/sm_statefuns.c
index 8b28443..4b4eb7c 100644
--- a/net/sctp/sm_statefuns.c
+++ b/net/sctp/sm_statefuns.c
@@ -1232,6 +1232,18 @@ out:
 	return 0;
 }
 
+static bool list_has_sctp_addr(const struct list_head *list,
+			       union sctp_addr *ipaddr)
+{
+	struct sctp_transport *addr;
+
+	list_for_each_entry(addr, list, transports) {
+		if (sctp_cmp_addr_exact(ipaddr, &addr->ipaddr))
+			return true;
+	}
+
+	return false;
+}
 /* A restart is occurring, check to make sure no new addresses
  * are being added as we may be under a takeover attack.
  */
@@ -1240,10 +1252,10 @@ static int sctp_sf_check_restart_addrs(const struct sctp_association *new_asoc,
 				       struct sctp_chunk *init,
 				       sctp_cmd_seq_t *commands)
 {
-	struct sctp_transport *new_addr, *addr;
-	int found;
+	struct sctp_transport *new_addr;
+	int ret = 1;
 
-	/* Implementor's Guide - Sectin 5.2.2
+	/* Implementor's Guide - Section 5.2.2
 	 * ...
 	 * Before responding the endpoint MUST check to see if the
 	 * unexpected INIT adds new addresses to the association. If new
@@ -1254,31 +1266,19 @@ static int sctp_sf_check_restart_addrs(const struct sctp_association *new_asoc,
 	/* Search through all current addresses and make sure
 	 * we aren't adding any new ones.
 	 */
-	new_addr = NULL;
-	found = 0;
-
 	list_for_each_entry(new_addr, &new_asoc->peer.transport_addr_list,
-			transports) {
-		found = 0;
-		list_for_each_entry(addr, &asoc->peer.transport_addr_list,
-				transports) {
-			if (sctp_cmp_addr_exact(&new_addr->ipaddr,
-						&addr->ipaddr)) {
-				found = 1;
-				break;
-			}
-		}
-		if (!found)
+			    transports) {
+		if (!list_has_sctp_addr(&asoc->peer.transport_addr_list,
+					&new_addr->ipaddr)) {
+			sctp_sf_send_restart_abort(&new_addr->ipaddr, init,
+						   commands);
+			ret = 0;
 			break;
-	}
-
-	/* If a new address was added, ABORT the sender. */
-	if (!found && new_addr) {
-		sctp_sf_send_restart_abort(&new_addr->ipaddr, init, commands);
+		}
 	}
 
 	/* Return success if all addresses were found. */
-	return found;
+	return ret;
 }
 
 /* Populate the verification/tie tags based on overlapping INIT

Re: [Alternative PATCH net-next] sctp: fix test for end of loop

[Vlad Yasevich]

On 09/08/2010 05:04 PM, Joe Perches wrote:
> Perhaps something like this is clearer?
> 
> Add a list_has_sctp_addr function to simplify loop
> 
> Based on a patches by Dan Carpenter and David Miller
> 
> Signed-off-by: Joe Perches <joe@perches.com>

Thanks Joe

I like this.  Nice and clean. :)

Acked-by: Vlad Yasevich <vladislav.yasevich@hp.com>

-vlad

> ---
>  net/sctp/sm_statefuns.c |   46 +++++++++++++++++++++++-----------------------
>  1 files changed, 23 insertions(+), 23 deletions(-)
> 
> diff --git a/net/sctp/sm_statefuns.c b/net/sctp/sm_statefuns.c
> index 8b28443..4b4eb7c 100644
> --- a/net/sctp/sm_statefuns.c
> +++ b/net/sctp/sm_statefuns.c
> @@ -1232,6 +1232,18 @@ out:
>  	return 0;
>  }
>  
> +static bool list_has_sctp_addr(const struct list_head *list,
> +			       union sctp_addr *ipaddr)
> +{
> +	struct sctp_transport *addr;
> +
> +	list_for_each_entry(addr, list, transports) {
> +		if (sctp_cmp_addr_exact(ipaddr, &addr->ipaddr))
> +			return true;
> +	}
> +
> +	return false;
> +}
>  /* A restart is occurring, check to make sure no new addresses
>   * are being added as we may be under a takeover attack.
>   */
> @@ -1240,10 +1252,10 @@ static int sctp_sf_check_restart_addrs(const struct sctp_association *new_asoc,
>  				       struct sctp_chunk *init,
>  				       sctp_cmd_seq_t *commands)
>  {
> -	struct sctp_transport *new_addr, *addr;
> -	int found;
> +	struct sctp_transport *new_addr;
> +	int ret = 1;
>  
> -	/* Implementor's Guide - Sectin 5.2.2
> +	/* Implementor's Guide - Section 5.2.2
>  	 * ...
>  	 * Before responding the endpoint MUST check to see if the
>  	 * unexpected INIT adds new addresses to the association. If new
> @@ -1254,31 +1266,19 @@ static int sctp_sf_check_restart_addrs(const struct sctp_association *new_asoc,
>  	/* Search through all current addresses and make sure
>  	 * we aren't adding any new ones.
>  	 */
> -	new_addr = NULL;
> -	found = 0;
> -
>  	list_for_each_entry(new_addr, &new_asoc->peer.transport_addr_list,
> -			transports) {
> -		found = 0;
> -		list_for_each_entry(addr, &asoc->peer.transport_addr_list,
> -				transports) {
> -			if (sctp_cmp_addr_exact(&new_addr->ipaddr,
> -						&addr->ipaddr)) {
> -				found = 1;
> -				break;
> -			}
> -		}
> -		if (!found)
> +			    transports) {
> +		if (!list_has_sctp_addr(&asoc->peer.transport_addr_list,
> +					&new_addr->ipaddr)) {
> +			sctp_sf_send_restart_abort(&new_addr->ipaddr, init,
> +						   commands);
> +			ret = 0;
>  			break;
> -	}
> -
> -	/* If a new address was added, ABORT the sender. */
> -	if (!found && new_addr) {
> -		sctp_sf_send_restart_abort(&new_addr->ipaddr, init, commands);
> +		}
>  	}
>  
>  	/* Return success if all addresses were found. */
> -	return found;
> +	return ret;
>  }
>  
>  /* Populate the verification/tie tags based on overlapping INIT
> 
> 

Re: [Alternative PATCH net-next] sctp: fix test for end of loop

[David Miller]

From: Vlad Yasevich <vladislav.yasevich@hp.com>
Date: Thu, 09 Sep 2010 09:57:31 -0400

> On 09/08/2010 05:04 PM, Joe Perches wrote:
>> Perhaps something like this is clearer?
>> 
>> Add a list_has_sctp_addr function to simplify loop
>> 
>> Based on a patches by Dan Carpenter and David Miller
>> 
>> Signed-off-by: Joe Perches <joe@perches.com>
> 
> Thanks Joe
> 
> I like this.  Nice and clean. :)
> 
> Acked-by: Vlad Yasevich <vladislav.yasevich@hp.com>

Applied to net-2.6 since this is reasonably important bug fix.

Thanks guys!

Re: [patch] sctp: fix test for end of loop

[Vlad Yasevich]

On 09/06/2010 08:26 AM, Dan Carpenter wrote:
> "new_addr" is the list cursor here and it's always non-NULL.
> 
> We're trying to test if we exited because the loop ended or we hit the
> break statement.  Really testing !found is enough so long as 
> "new_asoc->peer.transport_addr_list" is not empty and I believe it never
> is empty at this point.  So this is never really a bug with the current
> code.
> 
> Signed-off-by: Dan Carpenter <error27@gmail.com>
> ---
> Compile tested only.
> 
> diff --git a/net/sctp/sm_statefuns.c b/net/sctp/sm_statefuns.c
> index 24b2cd5..cb76d2e 100644
> --- a/net/sctp/sm_statefuns.c
> +++ b/net/sctp/sm_statefuns.c
> @@ -1254,7 +1254,6 @@ static int sctp_sf_check_restart_addrs(const struct sctp_association *new_asoc,
>  	/* Search through all current addresses and make sure
>  	 * we aren't adding any new ones.
>  	 */
> -	new_addr = NULL;
>  	found = 0;
>  
>  	list_for_each_entry(new_addr, &new_asoc->peer.transport_addr_list,
> @@ -1273,7 +1272,8 @@ static int sctp_sf_check_restart_addrs(const struct sctp_association *new_asoc,
>  	}
>  
>  	/* If a new address was added, ABORT the sender. */
> -	if (!found && new_addr) {
> +	if (!found &&
> +	    &new_addr->transports != &new_asoc->peer.transport_addr_list) {

I am really not fond of this code.  I think it would be better to move
this abort code into the loop and break out once things are aborted.

This way we can get rid of the whole found check after the outer loop happens
and it cleans things up nicely.

-vlad

>  		sctp_sf_send_restart_abort(&new_addr->ipaddr, init, commands);
>  	}
>  
> --
> 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
> 

Re: [patch] sctp: fix test for end of loop

[David Miller]

From: Vlad Yasevich <vladislav.yasevich@hp.com>
Date: Wed, 08 Sep 2010 16:26:45 -0400

> I am really not fond of this code.  I think it would be better to move
> this abort code into the loop and break out once things are aborted.
> 
> This way we can get rid of the whole found check after the outer loop happens
> and it cleans things up nicely.

Vlad see my followup.