sctp: fix test for end of loop

PATCH In review Dan Carpenter 5 participants 11 messages Sep 6, 2010 – Sep 9, 2010

Subsystems (4)

Networking Network drivers Kernel janitors SCTP protocol

lore

Share & tools

lore

Dan Carpenter

author

2010-09-06 12:26 UTC #0 lore raw

PATCH 1/1 sctp: fix test for end of loop

"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 +2 -2

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,

@@ -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);
 	}

Shan Wei

2010-09-07 08:46 UTC #1 lore raw

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 · 3 lines

Best Regards
-----
Shan Wei

Dan Carpenter

author

2010-09-07 11:31 UTC #2 lore raw

Quoted 5 lines · Shan Wei

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?

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

Quoted 5 lines

Signature · 4 lines
-- 
Best Regards
-----
Shan Wei

David Miller

maintainer

2010-09-08 20:24 UTC #3 lore raw

In reply to Dan Carpenter

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 +12 ~~-15~~

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
+		 * 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

Vlad Yasevich

2010-09-08 20:34 UTC #4 lore raw

Quoted 60 lines · David Miller

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 static int sctp_sf_check_restart_addrs(const struct sctp_association *new_asoc, · line 1276

Quoted 10 lines

net/sctp/sm_statefuns.c +7 -0

+		 * 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

Quoted 7 lines

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

David Miller

maintainer

2010-09-08 20:37 UTC #5 lore raw

From: Vlad Yasevich <[email protected]>
Date: Wed, 08 Sep 2010 16:34:46 -0400

Quoted 1 line

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

net/sctp/sm_statefuns.c +12 ~~-15~~

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
+		 * 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

Joe Perches

2010-09-08 21:04 UTC #6 lore raw

PATCH 1/1 sctp: fix test for end of loop

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

Vlad Yasevich

Acked-by on v1 1/1

2010-09-09 13:57 UTC #7 lore raw

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

Thanks Joe

I like this. Nice and clean. :)

Acked-by Vlad Yasevich <[email protected]>

-vlad

Quoted 83 lines

---
 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

David Miller

maintainer Applied

2010-09-09 22:00 UTC #8 lore raw

From: Vlad Yasevich <[email protected]>
Date: Thu, 09 Sep 2010 09:57:31 -0400

Quoted 7 lines · Joe Perches

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 7 lines

Quoted 1 line
Signed-off-by Joe Perches <[email protected]>
Thanks Joe
I like this. Nice and clean. :)
Acked-by Vlad Yasevich <[email protected]>

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

Thanks guys!

Vlad Yasevich

2010-09-08 20:26 UTC #9 lore raw

In reply to Dan Carpenter

Quoted 25 lines · 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 <[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 static int sctp_sf_check_restart_addrs(const struct sctp_association *new_asoc, · line 1276

Quoted 6 lines

net/sctp/sm_statefuns.c +2 -1

@@ -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

Quoted 8 lines

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

David Miller

maintainer

2010-09-08 20:30 UTC #10 lore raw

From: Vlad Yasevich <[email protected]>
Date: Wed, 08 Sep 2010 16:26:45 -0400

Quoted 5 lines

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.