On Fri, May 31, 2019 at 09:42:42AM -0300, Marcelo Ricardo Leitner wrote:

On Thu, May 30, 2019 at 03:56:34PM -0400, Neil Horman wrote:

quoted

On Thu, May 30, 2019 at 12:17:05PM -0300, Marcelo Ricardo Leitner wrote:

...

quoted

quoted

--- a/net/sctp/sm_sideeffect.c
+++ b/net/sctp/sm_sideeffect.c

@@ -898,6 +898,11 @@ static void sctp_cmd_new_state(struct sctp_cmd_seq *cmds,
 						asoc->rto_initial;
 	}
 
+	if (sctp_state(asoc, ESTABLISHED)) {
+		kfree(asoc->peer.cookie);
+		asoc->peer.cookie = NULL;
+	}
+

Not sure I follow why this is needed.  It doesn't hurt anything of course, but
if we're freeing in sctp_association_free, we don't need to duplicate the
operation here, do we?

This one would be to avoid storing the cookie throughout the entire
association lifetime, as the cookie is only needed during the
handshake.
While the free in sctp_association_free will handle the freeing in
case the association never enters established state.

Ok, I see we do that with the peer_random and other allocated values as well
when they are no longer needed, but ew, I hate freeing in multiple places like
that.  I'll fix this up on monday, but I wonder if we can't consolidate that
somehow

Neil

quoted

quoted

 	if (sctp_state(asoc, ESTABLISHED) ||
 	    sctp_state(asoc, CLOSED) ||
 	    sctp_state(asoc, SHUTDOWN_RECEIVED)) {

Also untested, just sharing the idea.

  Marcelo