[PATCH net-next 0/6] Further SCTP changes

[PATCH net-next 0/6] Further SCTP changes

[Daniel Borkmann]

Mostly it's all about getting rid of unnecessary code, and/or making
it more clean and readable. Code has been tested with lksctp-tools
functional test suite and also my small sctp stress test helper that
was used to discover the recent null-ptr derefs.

Changes since last version:
  - Left sctp_put_port() still in sctp_endpoint_destroy() for now
  - Added minor "net: sctp: simplify sctp_get_port" patch

Daniel Borkmann (6):
  net: sctp: remove TEST_FRAME ifdef
  ktime: add ms_to_ktime() and ktime_add_ms() helpers
  net: sctp: migrate cookie life from timeval to ktime
  net: sctp: minor: sctp_seq_dump_local_addrs add missing newline
  net: sctp: decouple cleaning some socket data from endpoint
  net: sctp: simplify sctp_get_port

 include/linux/ktime.h      | 13 +++++++++++++
 include/net/sctp/sctp.h    | 25 -------------------------
 include/net/sctp/structs.h |  6 +++---
 net/sctp/associola.c       |  8 +-------
 net/sctp/endpointola.c     | 18 +++++++++---------
 net/sctp/proc.c            |  2 +-
 net/sctp/sm_make_chunk.c   | 19 ++++++++-----------
 net/sctp/socket.c          | 35 +++++++++++++++++++----------------
 8 files changed, 54 insertions(+), 72 deletions(-)

-- 
1.7.11.7

[PATCH net-next 1/6] net: sctp: remove TEST_FRAME ifdef

[Daniel Borkmann]

We do neither ship a test_frame.h, nor will this be compatible with
the 2.5 out-of-tree lksctp kernel test suite anyway. So remove this
artefact.

Signed-off-by: Daniel Borkmann <dborkman@redhat.com>
---
 include/net/sctp/sctp.h | 7 -------
 1 file changed, 7 deletions(-)

diff --git a/include/net/sctp/sctp.h b/include/net/sctp/sctp.h
index 6321c08..15214a8 100644
--- a/include/net/sctp/sctp.h
+++ b/include/net/sctp/sctp.h
@@ -188,11 +188,6 @@ extern struct kmem_cache *sctp_bucket_cachep __read_mostly;
  *  Section:  Macros, externs, and inlines
  */
 
-
-#ifdef TEST_FRAME
-#include <test_frame.h>
-#else
-
 /* spin lock wrappers. */
 #define sctp_spin_lock_irqsave(lock, flags) spin_lock_irqsave(lock, flags)
 #define sctp_spin_unlock_irqrestore(lock, flags)  \
@@ -218,8 +213,6 @@ extern struct kmem_cache *sctp_bucket_cachep __read_mostly;
 #define SCTP_INC_STATS_USER(net, field) SNMP_INC_STATS_USER((net)->sctp.sctp_statistics, field)
 #define SCTP_DEC_STATS(net, field)      SNMP_DEC_STATS((net)->sctp.sctp_statistics, field)
 
-#endif /* !TEST_FRAME */
-
 /* sctp mib definitions */
 enum {
 	SCTP_MIB_NUM = 0,
-- 
1.7.11.7

[PATCH net-next 2/6] ktime: add ms_to_ktime() and ktime_add_ms() helpers

[Daniel Borkmann]

Add two ktime helper functions that i) convert a given msec value to
a ktime structure and ii) that adds a msec value to a ktime structure.

Signed-off-by: Daniel Borkmann <dborkman@redhat.com>
---
 include/linux/ktime.h | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/include/linux/ktime.h b/include/linux/ktime.h
index bbca128..b4fa5e4 100644
--- a/include/linux/ktime.h
+++ b/include/linux/ktime.h
@@ -323,6 +323,11 @@ static inline ktime_t ktime_add_us(const ktime_t kt, const u64 usec)
 	return ktime_add_ns(kt, usec * 1000);
 }
 
+static inline ktime_t ktime_add_ms(const ktime_t kt, const u64 msec)
+{
+	return ktime_add_ns(kt, msec * NSEC_PER_MSEC);
+}
+
 static inline ktime_t ktime_sub_us(const ktime_t kt, const u64 usec)
 {
 	return ktime_sub_ns(kt, usec * 1000);
@@ -366,7 +371,15 @@ extern void ktime_get_ts(struct timespec *ts);
 static inline ktime_t ns_to_ktime(u64 ns)
 {
 	static const ktime_t ktime_zero = { .tv64 = 0 };
+
 	return ktime_add_ns(ktime_zero, ns);
 }
 
+static inline ktime_t ms_to_ktime(u64 ms)
+{
+	static const ktime_t ktime_zero = { .tv64 = 0 };
+
+	return ktime_add_ms(ktime_zero, ms);
+}
+
 #endif
-- 
1.7.11.7

[PATCH net-next 3/6] net: sctp: migrate cookie life from timeval to ktime

[Daniel Borkmann]

Currently, SCTP code defines its own timeval functions (since timeval
is rarely used inside the kernel by others), namely tv_lt() and
TIMEVAL_ADD() macros, that operate on SCTP cookie expiration.

We might as well remove all those, and operate directly on ktime
structures for a couple of reasons: ktime is available on all archs;
complexity of ktime calculations depending on the arch is less than
(reduces to a simple arithmetic operations on archs with
BITS_PER_LONG == 64 or CONFIG_KTIME_SCALAR) or equal to timeval
functions (other archs); code becomes more readable; macros can be
thrown out.

Signed-off-by: Daniel Borkmann <dborkman@redhat.com>
---
 include/net/sctp/sctp.h    | 18 ------------------
 include/net/sctp/structs.h |  6 +++---
 net/sctp/associola.c       |  8 +-------
 net/sctp/sm_make_chunk.c   | 19 ++++++++-----------
 net/sctp/socket.c          | 14 +++-----------
 5 files changed, 15 insertions(+), 50 deletions(-)

diff --git a/include/net/sctp/sctp.h b/include/net/sctp/sctp.h
index 15214a8..e6b95bc 100644
--- a/include/net/sctp/sctp.h
+++ b/include/net/sctp/sctp.h
@@ -560,24 +560,6 @@ for (pos = chunk->subh.fwdtsn_hdr->skip;\
 /* Round an int up to the next multiple of 4.  */
 #define WORD_ROUND(s) (((s)+3)&~3)
 
-/* Compare two timevals.  */
-#define tv_lt(s, t) \
-   (s.tv_sec < t.tv_sec || (s.tv_sec == t.tv_sec && s.tv_usec < t.tv_usec))
-
-/* Add tv1 to tv2. */
-#define TIMEVAL_ADD(tv1, tv2) \
-({ \
-        suseconds_t usecs = (tv2).tv_usec + (tv1).tv_usec; \
-        time_t secs = (tv2).tv_sec + (tv1).tv_sec; \
-\
-        if (usecs >= 1000000) { \
-                usecs -= 1000000; \
-                secs++; \
-        } \
-        (tv2).tv_sec = secs; \
-        (tv2).tv_usec = usecs; \
-})
-
 /* External references. */
 
 extern struct proto sctp_prot;
diff --git a/include/net/sctp/structs.h b/include/net/sctp/structs.h
index 1bd4c41..e745c92 100644
--- a/include/net/sctp/structs.h
+++ b/include/net/sctp/structs.h
@@ -54,7 +54,7 @@
 #ifndef __sctp_structs_h__
 #define __sctp_structs_h__
 
-#include <linux/time.h>		/* We get struct timespec.    */
+#include <linux/ktime.h>
 #include <linux/socket.h>	/* linux/in.h needs this!!    */
 #include <linux/in.h>		/* We get struct sockaddr_in. */
 #include <linux/in6.h>		/* We get struct in6_addr     */
@@ -284,7 +284,7 @@ struct sctp_cookie {
 	__u32 peer_ttag;
 
 	/* When does this cookie expire? */
-	struct timeval expiration;
+	ktime_t expiration;
 
 	/* Number of inbound/outbound streams which are set
 	 * and negotiated during the INIT process.
@@ -1537,7 +1537,7 @@ struct sctp_association {
 	sctp_state_t state;
 
 	/* The cookie life I award for any cookie.  */
-	struct timeval cookie_life;
+	ktime_t cookie_life;
 
 	/* Overall     : The overall association error count.
 	 * Error Count : [Clear this any time I get something.]
diff --git a/net/sctp/associola.c b/net/sctp/associola.c
index bf6e6bd..9a383a8 100644
--- a/net/sctp/associola.c
+++ b/net/sctp/associola.c
@@ -102,13 +102,7 @@ static struct sctp_association *sctp_association_init(struct sctp_association *a
 	sctp_bind_addr_init(&asoc->base.bind_addr, ep->base.bind_addr.port);
 
 	asoc->state = SCTP_STATE_CLOSED;
-
-	/* Set these values from the socket values, a conversion between
-	 * millsecons to seconds/microseconds must also be done.
-	 */
-	asoc->cookie_life.tv_sec = sp->assocparams.sasoc_cookie_life / 1000;
-	asoc->cookie_life.tv_usec = (sp->assocparams.sasoc_cookie_life % 1000)
-					* 1000;
+	asoc->cookie_life = ms_to_ktime(sp->assocparams.sasoc_cookie_life);
 	asoc->frag_point = 0;
 	asoc->user_frag = sp->user_frag;
 
diff --git a/net/sctp/sm_make_chunk.c b/net/sctp/sm_make_chunk.c
index fc85487..dd71f1f 100644
--- a/net/sctp/sm_make_chunk.c
+++ b/net/sctp/sm_make_chunk.c
@@ -1630,8 +1630,8 @@ static sctp_cookie_param_t *sctp_pack_cookie(const struct sctp_endpoint *ep,
 	cookie->c.adaptation_ind = asoc->peer.adaptation_ind;
 
 	/* Set an expiration time for the cookie.  */
-	do_gettimeofday(&cookie->c.expiration);
-	TIMEVAL_ADD(asoc->cookie_life, cookie->c.expiration);
+	cookie->c.expiration = ktime_add(asoc->cookie_life,
+					 ktime_get());
 
 	/* Copy the peer's init packet.  */
 	memcpy(&cookie->c.peer_init[0], init_chunk->chunk_hdr,
@@ -1680,7 +1680,7 @@ struct sctp_association *sctp_unpack_cookie(
 	unsigned int len;
 	sctp_scope_t scope;
 	struct sk_buff *skb = chunk->skb;
-	struct timeval tv;
+	ktime_t kt;
 	struct hash_desc desc;
 
 	/* Header size is static data prior to the actual cookie, including
@@ -1757,11 +1757,11 @@ no_hmac:
 	 * down the new association establishment instead of every packet.
 	 */
 	if (sock_flag(ep->base.sk, SOCK_TIMESTAMP))
-		skb_get_timestamp(skb, &tv);
+		kt = skb_get_ktime(skb);
 	else
-		do_gettimeofday(&tv);
+		kt = ktime_get();
 
-	if (!asoc && tv_lt(bear_cookie->expiration, tv)) {
+	if (!asoc && ktime_compare(bear_cookie->expiration, kt) < 0) {
 		/*
 		 * Section 3.3.10.3 Stale Cookie Error (3)
 		 *
@@ -1773,9 +1773,7 @@ no_hmac:
 		len = ntohs(chunk->chunk_hdr->length);
 		*errp = sctp_make_op_error_space(asoc, chunk, len);
 		if (*errp) {
-			suseconds_t usecs = (tv.tv_sec -
-				bear_cookie->expiration.tv_sec) * 1000000L +
-				tv.tv_usec - bear_cookie->expiration.tv_usec;
+			suseconds_t usecs = ktime_to_us(ktime_sub(kt, bear_cookie->expiration));
 			__be32 n = htonl(usecs);
 
 			sctp_init_cause(*errp, SCTP_ERROR_STALE_COOKIE,
@@ -2514,8 +2512,7 @@ do_addr_param:
 		/* Suggested Cookie Life span increment's unit is msec,
 		 * (1/1000sec).
 		 */
-		asoc->cookie_life.tv_sec += stale / 1000;
-		asoc->cookie_life.tv_usec += (stale % 1000) * 1000;
+		asoc->cookie_life = ktime_add_ms(asoc->cookie_life, stale);
 		break;
 
 	case SCTP_PARAM_HOST_NAME_ADDRESS:
diff --git a/net/sctp/socket.c b/net/sctp/socket.c
index 32db19b..4c47e55 100644
--- a/net/sctp/socket.c
+++ b/net/sctp/socket.c
@@ -2910,13 +2910,8 @@ static int sctp_setsockopt_associnfo(struct sock *sk, char __user *optval, unsig
 			asoc->max_retrans = assocparams.sasoc_asocmaxrxt;
 		}
 
-		if (assocparams.sasoc_cookie_life != 0) {
-			asoc->cookie_life.tv_sec =
-					assocparams.sasoc_cookie_life / 1000;
-			asoc->cookie_life.tv_usec =
-					(assocparams.sasoc_cookie_life % 1000)
-					* 1000;
-		}
+		if (assocparams.sasoc_cookie_life != 0)
+			asoc->cookie_life = ms_to_ktime(assocparams.sasoc_cookie_life);
 	} else {
 		/* Set the values to the endpoint */
 		struct sctp_sock *sp = sctp_sk(sk);
@@ -5074,10 +5069,7 @@ static int sctp_getsockopt_associnfo(struct sock *sk, int len,
 		assocparams.sasoc_asocmaxrxt = asoc->max_retrans;
 		assocparams.sasoc_peer_rwnd = asoc->peer.rwnd;
 		assocparams.sasoc_local_rwnd = asoc->a_rwnd;
-		assocparams.sasoc_cookie_life = (asoc->cookie_life.tv_sec
-						* 1000) +
-						(asoc->cookie_life.tv_usec
-						/ 1000);
+		assocparams.sasoc_cookie_life = ktime_to_ms(asoc->cookie_life);
 
 		list_for_each(pos, &asoc->peer.transport_addr_list) {
 			cnt ++;
-- 
1.7.11.7

[PATCH net-next 4/6] net: sctp: minor: sctp_seq_dump_local_addrs add missing newline

[Daniel Borkmann]

A trailing newline has been forgotten to add into the WARN().

Signed-off-by: Daniel Borkmann <dborkman@redhat.com>
---
 net/sctp/proc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/sctp/proc.c b/net/sctp/proc.c
index 0c83162..62526c4 100644
--- a/net/sctp/proc.c
+++ b/net/sctp/proc.c
@@ -138,7 +138,7 @@ static void sctp_seq_dump_local_addrs(struct seq_file *seq, struct sctp_ep_commo
 
 		peer = asoc->peer.primary_path;
 		if (unlikely(peer == NULL)) {
-			WARN(1, "Association %p with NULL primary path!", asoc);
+			WARN(1, "Association %p with NULL primary path!\n", asoc);
 			return;
 		}
 
-- 
1.7.11.7

[PATCH net-next 5/6] net: sctp: decouple cleaning some socket data from endpoint

[Daniel Borkmann]

Rather instead of having the endpoint clean the garbage from the
socket, use a sk_destruct handler sctp_destruct_sock(), that does
the job for that when there are no more references on the socket.
At least do this for our crypto transform through crypto_free_hash()
that is allocated when in listening state. Also, for now, move the
sctp_put_port() into the sk if body. At a later point in time we
can still determine if there's an option of placing this into
unhash() or sctp_endpoint_free() without any races. For now, leave
it in sctp_endpoint_destroy() though.

Signed-off-by: Daniel Borkmann <dborkman@redhat.com>
---
 net/sctp/endpointola.c | 18 +++++++++---------
 net/sctp/socket.c      | 16 +++++++++++++++-
 2 files changed, 24 insertions(+), 10 deletions(-)

diff --git a/net/sctp/endpointola.c b/net/sctp/endpointola.c
index a8b2674..8ad7781 100644
--- a/net/sctp/endpointola.c
+++ b/net/sctp/endpointola.c
@@ -247,10 +247,9 @@ void sctp_endpoint_free(struct sctp_endpoint *ep)
 /* Final destructor for endpoint.  */
 static void sctp_endpoint_destroy(struct sctp_endpoint *ep)
 {
-	SCTP_ASSERT(ep->base.dead, "Endpoint is not dead", return);
+	struct sock *sk;
 
-	/* Free up the HMAC transform. */
-	crypto_free_hash(sctp_sk(ep->base.sk)->hmac);
+	SCTP_ASSERT(ep->base.dead, "Endpoint is not dead", return);
 
 	/* Free the digest buffer */
 	kfree(ep->digest);
@@ -271,13 +270,14 @@ static void sctp_endpoint_destroy(struct sctp_endpoint *ep)
 
 	memset(ep->secret_key, 0, sizeof(ep->secret_key));
 
-	/* Remove and free the port */
-	if (sctp_sk(ep->base.sk)->bind_hash)
-		sctp_put_port(ep->base.sk);
-
 	/* Give up our hold on the sock. */
-	if (ep->base.sk)
-		sock_put(ep->base.sk);
+	if ((sk = ep->base.sk)) {
+		/* Remove and free the port */
+		if (sctp_sk(sk)->bind_hash)
+			sctp_put_port(sk);
+
+		sock_put(sk);
+	}
 
 	kfree(ep);
 	SCTP_DBG_OBJCNT_DEC(ep);
diff --git a/net/sctp/socket.c b/net/sctp/socket.c
index 4c47e55..ba9359c 100644
--- a/net/sctp/socket.c
+++ b/net/sctp/socket.c
@@ -93,6 +93,7 @@ static int sctp_wait_for_packet(struct sock * sk, int *err, long *timeo_p);
 static int sctp_wait_for_connect(struct sctp_association *, long *timeo_p);
 static int sctp_wait_for_accept(struct sock *sk, long timeo);
 static void sctp_wait_for_close(struct sock *sk, long timeo);
+static void sctp_destruct_sock(struct sock *sk);
 static struct sctp_af *sctp_sockaddr_af(struct sctp_sock *opt,
 					union sctp_addr *addr, int len);
 static int sctp_bindx_add(struct sock *, struct sockaddr *, int);
@@ -3966,6 +3967,8 @@ static int sctp_init_sock(struct sock *sk)
 
 	sp->hmac = NULL;
 
+	sk->sk_destruct = sctp_destruct_sock;
+
 	SCTP_DBG_OBJCNT_INC(sock);
 
 	local_bh_disable();
@@ -4008,6 +4011,17 @@ static void sctp_destroy_sock(struct sock *sk)
 	local_bh_enable();
 }
 
+/* Triggered when there are no references on the socket anymore */
+static void sctp_destruct_sock(struct sock *sk)
+{
+	struct sctp_sock *sp = sctp_sk(sk);
+
+	/* Free up the HMAC transform. */
+	crypto_free_hash(sp->hmac);
+
+	inet_sock_destruct(sk);
+}
+
 /* API 4.1.7 shutdown() - TCP Style Syntax
  *     int shutdown(int socket, int how);
  *
@@ -6848,7 +6862,7 @@ void sctp_copy_sock(struct sock *newsk, struct sock *sk,
 	newsk->sk_reuse = sk->sk_reuse;
 
 	newsk->sk_shutdown = sk->sk_shutdown;
-	newsk->sk_destruct = inet_sock_destruct;
+	newsk->sk_destruct = sctp_destruct_sock;
 	newsk->sk_family = sk->sk_family;
 	newsk->sk_protocol = IPPROTO_SCTP;
 	newsk->sk_backlog_rcv = sk->sk_prot->backlog_rcv;
-- 
1.7.11.7

[PATCH net-next 6/6] net: sctp: simplify sctp_get_port

[Daniel Borkmann]

No need to have an extra ret variable when we directly can return
the value of sctp_get_port_local().

Signed-off-by: Daniel Borkmann <dborkman@redhat.com>
---
 net/sctp/socket.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/net/sctp/socket.c b/net/sctp/socket.c
index ba9359c..66fcdcf 100644
--- a/net/sctp/socket.c
+++ b/net/sctp/socket.c
@@ -6036,7 +6036,6 @@ fail:
  */
 static int sctp_get_port(struct sock *sk, unsigned short snum)
 {
-	long ret;
 	union sctp_addr addr;
 	struct sctp_af *af = sctp_sk(sk)->pf->af;
 
@@ -6045,9 +6044,7 @@ static int sctp_get_port(struct sock *sk, unsigned short snum)
 	addr.v4.sin_port = htons(snum);
 
 	/* Note: sk->sk_num gets filled in if ephemeral port request. */
-	ret = sctp_get_port_local(sk, &addr);
-
-	return ret ? 1 : 0;
+	return !!sctp_get_port_local(sk, &addr);
 }
 
 /*
-- 
1.7.11.7

Re: [PATCH net-next 5/6] net: sctp: decouple cleaning some socket data from endpoint

[Vlad Yasevich]

On 06/25/2013 11:13 AM, Daniel Borkmann wrote:
> Rather instead of having the endpoint clean the garbage from the
> socket, use a sk_destruct handler sctp_destruct_sock(), that does
> the job for that when there are no more references on the socket.
> At least do this for our crypto transform through crypto_free_hash()
> that is allocated when in listening state. Also, for now, move the
> sctp_put_port() into the sk if body.

This sentence is hard to parse without looking at the patch.  Can
you rephrase.  May be say that we perform sctp_put_port() only when
sk is valid.

> At a later point in time we
> can still determine if there's an option of placing this into
> unhash() or sctp_endpoint_free() without any races. For now, leave
> it in sctp_endpoint_destroy() though.
>
> Signed-off-by: Daniel Borkmann <dborkman@redhat.com>
> ---
>   net/sctp/endpointola.c | 18 +++++++++---------
>   net/sctp/socket.c      | 16 +++++++++++++++-
>   2 files changed, 24 insertions(+), 10 deletions(-)
>
> diff --git a/net/sctp/endpointola.c b/net/sctp/endpointola.c
> index a8b2674..8ad7781 100644
> --- a/net/sctp/endpointola.c
> +++ b/net/sctp/endpointola.c
> @@ -247,10 +247,9 @@ void sctp_endpoint_free(struct sctp_endpoint *ep)
>   /* Final destructor for endpoint.  */
>   static void sctp_endpoint_destroy(struct sctp_endpoint *ep)
>   {
> -	SCTP_ASSERT(ep->base.dead, "Endpoint is not dead", return);
> +	struct sock *sk;
>
> -	/* Free up the HMAC transform. */
> -	crypto_free_hash(sctp_sk(ep->base.sk)->hmac);
> +	SCTP_ASSERT(ep->base.dead, "Endpoint is not dead", return);
>
>   	/* Free the digest buffer */
>   	kfree(ep->digest);
> @@ -271,13 +270,14 @@ static void sctp_endpoint_destroy(struct sctp_endpoint *ep)
>
>   	memset(ep->secret_key, 0, sizeof(ep->secret_key));
>
> -	/* Remove and free the port */
> -	if (sctp_sk(ep->base.sk)->bind_hash)
> -		sctp_put_port(ep->base.sk);
> -
>   	/* Give up our hold on the sock. */
> -	if (ep->base.sk)
> -		sock_put(ep->base.sk);
> +	if ((sk = ep->base.sk)) {

Can you either split the above into separate assignment and test (this 
is what checkpatchs.pl recommends) or at least comment that you mean to 
do assign and test in the above statement.

-vlad
> +		/* Remove and free the port */
> +		if (sctp_sk(sk)->bind_hash)
> +			sctp_put_port(sk);
> +
> +		sock_put(sk);
> +	}
>
>   	kfree(ep);
>   	SCTP_DBG_OBJCNT_DEC(ep);
> diff --git a/net/sctp/socket.c b/net/sctp/socket.c
> index 4c47e55..ba9359c 100644
> --- a/net/sctp/socket.c
> +++ b/net/sctp/socket.c
> @@ -93,6 +93,7 @@ static int sctp_wait_for_packet(struct sock * sk, int *err, long *timeo_p);
>   static int sctp_wait_for_connect(struct sctp_association *, long *timeo_p);
>   static int sctp_wait_for_accept(struct sock *sk, long timeo);
>   static void sctp_wait_for_close(struct sock *sk, long timeo);
> +static void sctp_destruct_sock(struct sock *sk);
>   static struct sctp_af *sctp_sockaddr_af(struct sctp_sock *opt,
>   					union sctp_addr *addr, int len);
>   static int sctp_bindx_add(struct sock *, struct sockaddr *, int);
> @@ -3966,6 +3967,8 @@ static int sctp_init_sock(struct sock *sk)
>
>   	sp->hmac = NULL;
>
> +	sk->sk_destruct = sctp_destruct_sock;
> +
>   	SCTP_DBG_OBJCNT_INC(sock);
>
>   	local_bh_disable();
> @@ -4008,6 +4011,17 @@ static void sctp_destroy_sock(struct sock *sk)
>   	local_bh_enable();
>   }
>
> +/* Triggered when there are no references on the socket anymore */
> +static void sctp_destruct_sock(struct sock *sk)
> +{
> +	struct sctp_sock *sp = sctp_sk(sk);
> +
> +	/* Free up the HMAC transform. */
> +	crypto_free_hash(sp->hmac);
> +
> +	inet_sock_destruct(sk);
> +}
> +
>   /* API 4.1.7 shutdown() - TCP Style Syntax
>    *     int shutdown(int socket, int how);
>    *
> @@ -6848,7 +6862,7 @@ void sctp_copy_sock(struct sock *newsk, struct sock *sk,
>   	newsk->sk_reuse = sk->sk_reuse;
>
>   	newsk->sk_shutdown = sk->sk_shutdown;
> -	newsk->sk_destruct = inet_sock_destruct;
> +	newsk->sk_destruct = sctp_destruct_sock;
>   	newsk->sk_family = sk->sk_family;
>   	newsk->sk_protocol = IPPROTO_SCTP;
>   	newsk->sk_backlog_rcv = sk->sk_prot->backlog_rcv;
>

Re: [PATCH net-next 5/6] net: sctp: decouple cleaning some socket data from endpoint

[Daniel Borkmann]

On 06/25/2013 05:32 PM, Vlad Yasevich wrote:
> On 06/25/2013 11:13 AM, Daniel Borkmann wrote:
>> Rather instead of having the endpoint clean the garbage from the
>> socket, use a sk_destruct handler sctp_destruct_sock(), that does
>> the job for that when there are no more references on the socket.
>> At least do this for our crypto transform through crypto_free_hash()
>> that is allocated when in listening state. Also, for now, move the
>> sctp_put_port() into the sk if body.
>
> This sentence is hard to parse without looking at the patch.  Can
> you rephrase.  May be say that we perform sctp_put_port() only when
> sk is valid.
>
>> At a later point in time we
>> can still determine if there's an option of placing this into
>> unhash() or sctp_endpoint_free() without any races. For now, leave
>> it in sctp_endpoint_destroy() though.
>>
>> Signed-off-by: Daniel Borkmann <dborkman@redhat.com>
>> ---
>>   net/sctp/endpointola.c | 18 +++++++++---------
>>   net/sctp/socket.c      | 16 +++++++++++++++-
>>   2 files changed, 24 insertions(+), 10 deletions(-)
>>
>> diff --git a/net/sctp/endpointola.c b/net/sctp/endpointola.c
>> index a8b2674..8ad7781 100644
>> --- a/net/sctp/endpointola.c
>> +++ b/net/sctp/endpointola.c
>> @@ -247,10 +247,9 @@ void sctp_endpoint_free(struct sctp_endpoint *ep)
>>   /* Final destructor for endpoint.  */
>>   static void sctp_endpoint_destroy(struct sctp_endpoint *ep)
>>   {
>> -    SCTP_ASSERT(ep->base.dead, "Endpoint is not dead", return);
>> +    struct sock *sk;
>>
>> -    /* Free up the HMAC transform. */
>> -    crypto_free_hash(sctp_sk(ep->base.sk)->hmac);
>> +    SCTP_ASSERT(ep->base.dead, "Endpoint is not dead", return);
>>
>>       /* Free the digest buffer */
>>       kfree(ep->digest);
>> @@ -271,13 +270,14 @@ static void sctp_endpoint_destroy(struct sctp_endpoint *ep)
>>
>>       memset(ep->secret_key, 0, sizeof(ep->secret_key));
>>
>> -    /* Remove and free the port */
>> -    if (sctp_sk(ep->base.sk)->bind_hash)
>> -        sctp_put_port(ep->base.sk);
>> -
>>       /* Give up our hold on the sock. */
>> -    if (ep->base.sk)
>> -        sock_put(ep->base.sk);
>> +    if ((sk = ep->base.sk)) {
>
> Can you either split the above into separate assignment and test (this is what checkpatchs.pl recommends) or at least comment that you mean to do assign and test in the above statement.

Ok, sure, I can make this separate and rephrase the above sentence for the log.

Thanks,

Daniel