sctp: fix test for end of loop

Dan Carpenter wrote, at 09/06/2010 08:26 PM:

net/sctp/sm_statefuns.c static int sctp_sf_check_restart_addrs(const struct sctp_association *new_asoc, · line 1276

Quoted 1 line

net/sctp/sm_statefuns.c +1 -0

+	    &new_addr->transports != &new_asoc->peer.transport_addr_list) {

why did you add this check?

Signature · 4 lines

-- 
Best Regards
-----
Shan Wei

"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 <[email protected]>

"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 <[email protected]>

---
Compile tested only.

net/sctp/sm_statefuns.c +0 -1

index 24b2cd5..cb76d2e 100644
@@ -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,

net/sctp/sm_statefuns.c +0 -1

 		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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

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.

From: Dan Carpenter <[email protected]>
Date: Mon, 6 Sep 2010 14:26:39 +0200

Quoted 9 lines

"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 <[email protected]>

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!

net/sctp/sm_statefuns.c +4 -14

index 8b28443..3d5bbae7 100644
@@ -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

net/sctp/sm_statefuns.c +1 -1

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

I would ack this.

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 <[email protected]>

---
 net/sctp/sm_statefuns.c |   46 +++++++++++++++++++++++-----------------------
 1 files changed, 23 insertions(+), 23 deletions(-)

net/sctp/sm_statefuns.c +23 -23

index 8b28443..4b4eb7c 100644
@@ -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

Quoted 6 lines

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

Quoted 1 line

Signed-off-by Joe Perches <[email protected]>

Thanks Joe

I like this. Nice and clean. :)

Acked-by Vlad Yasevich <[email protected]>