[PATCH net-next 0/9] Minor SCTP cleanups all over the place

[PATCH net-next 0/9] Minor SCTP cleanups all over the place

[Daniel Borkmann]

That's a batch of cleanups for the SCTP protocol. Probably all
deadly boring and trivial patches, but it always bugs me. :-) I
guess this is still from the very SCTP beginning in Linux. Well,
at least we can now remove 41 lines of code in total. lksctp-tools
test suite run through without problems.

Daniel Borkmann (9):
  net: sctp: sctp_ssnmap: remove 'malloced' element from struct
  net: sctp: sctp_inq: remove dead code
  net: sctp: sctp_outq: remove 'malloced' from its struct
  net: sctp: sctp_outq: consolidate chars into bitfield
  net: sctp: outqueue: simplify sctp_outq_uncork function
  net: sctp: sctp_transport: remove unused variable
  net: sctp: sctp_bind_addr: remove dead code
  net: sctp: sctp_ulpq: remove 'malloced' struct member
  net: sctp: sctp_ulpq: do not use char as a struct member

 include/net/sctp/structs.h  | 24 ++++--------------------
 include/net/sctp/ulpqueue.h |  3 +--
 net/sctp/bind_addr.c        |  7 -------
 net/sctp/inqueue.c          |  7 -------
 net/sctp/outqueue.c         | 11 ++---------
 net/sctp/ssnmap.c           | 23 ++++++++++++-----------
 net/sctp/transport.c        |  1 -
 net/sctp/ulpqueue.c         |  3 ---
 8 files changed, 19 insertions(+), 60 deletions(-)

-- 
1.7.11.7

[PATCH net-next 1/9] net: sctp: sctp_ssnmap: remove 'malloced' element from struct

[Daniel Borkmann]

sctp_ssnmap_init() can only be called from sctp_ssnmap_new()
where malloced is always set to 1. Thus, when we call
sctp_ssnmap_free() the test for map->malloced evaluates always
to true.

Signed-off-by: Daniel Borkmann <dborkman@redhat.com>
---
 include/net/sctp/structs.h |  1 -
 net/sctp/ssnmap.c          | 23 ++++++++++++-----------
 2 files changed, 12 insertions(+), 12 deletions(-)

diff --git a/include/net/sctp/structs.h b/include/net/sctp/structs.h
index e12aa77..3c1bb8d 100644
--- a/include/net/sctp/structs.h
+++ b/include/net/sctp/structs.h
@@ -399,7 +399,6 @@ struct sctp_stream {
 struct sctp_ssnmap {
 	struct sctp_stream in;
 	struct sctp_stream out;
-	int malloced;
 };
 
 struct sctp_ssnmap *sctp_ssnmap_new(__u16 in, __u16 out,
diff --git a/net/sctp/ssnmap.c b/net/sctp/ssnmap.c
index 825ea94..da86035 100644
--- a/net/sctp/ssnmap.c
+++ b/net/sctp/ssnmap.c
@@ -74,7 +74,6 @@ struct sctp_ssnmap *sctp_ssnmap_new(__u16 in, __u16 out,
 	if (!sctp_ssnmap_init(retval, in, out))
 		goto fail_map;
 
-	retval->malloced = 1;
 	SCTP_DBG_OBJCNT_INC(ssnmap);
 
 	return retval;
@@ -118,14 +117,16 @@ void sctp_ssnmap_clear(struct sctp_ssnmap *map)
 /* Dispose of a ssnmap.  */
 void sctp_ssnmap_free(struct sctp_ssnmap *map)
 {
-	if (map && map->malloced) {
-		int size;
-
-		size = sctp_ssnmap_size(map->in.len, map->out.len);
-		if (size <= KMALLOC_MAX_SIZE)
-			kfree(map);
-		else
-			free_pages((unsigned long)map, get_order(size));
-		SCTP_DBG_OBJCNT_DEC(ssnmap);
-	}
+	int size;
+
+	if (unlikely(!map))
+		return;
+
+	size = sctp_ssnmap_size(map->in.len, map->out.len);
+	if (size <= KMALLOC_MAX_SIZE)
+		kfree(map);
+	else
+		free_pages((unsigned long)map, get_order(size));
+
+	SCTP_DBG_OBJCNT_DEC(ssnmap);
 }
-- 
1.7.11.7

[PATCH net-next 2/9] net: sctp: sctp_inq: remove dead code

[Daniel Borkmann]

sctp_inq is never kmalloced, since it's integrated into sctp_ep_common
and only initialized from eps and assocs. Therefore, remove the dead
code from there.

Signed-off-by: Daniel Borkmann <dborkman@redhat.com>
---
 include/net/sctp/structs.h | 2 --
 net/sctp/inqueue.c         | 7 -------
 2 files changed, 9 deletions(-)

diff --git a/include/net/sctp/structs.h b/include/net/sctp/structs.h
index 3c1bb8d..125a19c 100644
--- a/include/net/sctp/structs.h
+++ b/include/net/sctp/structs.h
@@ -991,8 +991,6 @@ struct sctp_inq {
 	 * messages.
 	 */
 	struct work_struct immediate;
-
-	int malloced;	     /* Is this structure kfree()able?	*/
 };
 
 void sctp_inq_init(struct sctp_inq *);
diff --git a/net/sctp/inqueue.c b/net/sctp/inqueue.c
index 2d5ad28..3221d07 100644
--- a/net/sctp/inqueue.c
+++ b/net/sctp/inqueue.c
@@ -58,8 +58,6 @@ void sctp_inq_init(struct sctp_inq *queue)
 
 	/* Create a task for delivering data.  */
 	INIT_WORK(&queue->immediate, NULL);
-
-	queue->malloced = 0;
 }
 
 /* Release the memory associated with an SCTP inqueue.  */
@@ -80,11 +78,6 @@ void sctp_inq_free(struct sctp_inq *queue)
 		sctp_chunk_free(queue->in_progress);
 		queue->in_progress = NULL;
 	}
-
-	if (queue->malloced) {
-		/* Dump the master memory segment.  */
-		kfree(queue);
-	}
 }
 
 /* Put a new packet in an SCTP inqueue.
-- 
1.7.11.7

[PATCH net-next 3/9] net: sctp: sctp_outq: remove 'malloced' from its struct

[Daniel Borkmann]

sctp_outq is embedded into sctp_association, and thus never
kmalloced in any way. Also, malloced is always 0, thus kfree()
is never called. Therefore, remove that dead piece of code.

Signed-off-by: Daniel Borkmann <dborkman@redhat.com>
---
 include/net/sctp/structs.h | 3 ---
 net/sctp/outqueue.c        | 6 ------
 2 files changed, 9 deletions(-)

diff --git a/include/net/sctp/structs.h b/include/net/sctp/structs.h
index 125a19c..73fd5de 100644
--- a/include/net/sctp/structs.h
+++ b/include/net/sctp/structs.h
@@ -1059,9 +1059,6 @@ struct sctp_outq {
 
 	/* Is this structure empty?  */
 	char empty;
-
-	/* Are we kfree()able? */
-	char malloced;
 };
 
 void sctp_outq_init(struct sctp_association *, struct sctp_outq *);
diff --git a/net/sctp/outqueue.c b/net/sctp/outqueue.c
index 01dca75..d4c137e 100644
--- a/net/sctp/outqueue.c
+++ b/net/sctp/outqueue.c
@@ -217,8 +217,6 @@ void sctp_outq_init(struct sctp_association *asoc, struct sctp_outq *q)
 	q->outstanding_bytes = 0;
 	q->empty = 1;
 	q->cork  = 0;
-
-	q->malloced = 0;
 	q->out_qlen = 0;
 }
 
@@ -295,10 +293,6 @@ void sctp_outq_free(struct sctp_outq *q)
 {
 	/* Throw away leftover chunks. */
 	__sctp_outq_teardown(q);
-
-	/* If we were kmalloc()'d, free the memory.  */
-	if (q->malloced)
-		kfree(q);
 }
 
 /* Put a new chunk in an sctp_outq.  */
-- 
1.7.11.7

[PATCH net-next 4/9] net: sctp: sctp_outq: consolidate chars into bitfield

[Daniel Borkmann]

sctp_outq structure members fast_rtx, cork and empty are all
of type char. Consolidate them into a single __u8 bitfield since
they either carry 0 or 1. Have the rest 5 bits for future flags.

Signed-off-by: Daniel Borkmann <dborkman@redhat.com>
---
 include/net/sctp/structs.h | 11 +++--------
 1 file changed, 3 insertions(+), 8 deletions(-)

diff --git a/include/net/sctp/structs.h b/include/net/sctp/structs.h
index 73fd5de..3de5985 100644
--- a/include/net/sctp/structs.h
+++ b/include/net/sctp/structs.h
@@ -1051,14 +1051,9 @@ struct sctp_outq {
 	/* How many unackd bytes do we have in-flight?	*/
 	__u32 outstanding_bytes;
 
-	/* Are we doing fast-rtx on this queue */
-	char fast_rtx;
-
-	/* Corked? */
-	char cork;
-
-	/* Is this structure empty?  */
-	char empty;
+	__u8 fast_rtx:1,	/* Are we doing fast-rtx on this queue */
+	     cork:1,		/* Corked? */
+	     empty:1;		/* Is this structure empty?  */
 };
 
 void sctp_outq_init(struct sctp_association *, struct sctp_outq *);
-- 
1.7.11.7

[PATCH net-next 5/9] net: sctp: outqueue: simplify sctp_outq_uncork function

[Daniel Borkmann]

Just a minor edit to simplify the function. No need for this
error variable here.

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

diff --git a/net/sctp/outqueue.c b/net/sctp/outqueue.c
index d4c137e..32a4625 100644
--- a/net/sctp/outqueue.c
+++ b/net/sctp/outqueue.c
@@ -701,11 +701,10 @@ redo:
 /* Cork the outqueue so queued chunks are really queued. */
 int sctp_outq_uncork(struct sctp_outq *q)
 {
-	int error = 0;
 	if (q->cork)
 		q->cork = 0;
-	error = sctp_outq_flush(q, 0);
-	return error;
+
+	return sctp_outq_flush(q, 0);
 }
 
 
-- 
1.7.11.7

[PATCH net-next 6/9] net: sctp: sctp_transport: remove unused variable

[Daniel Borkmann]

sctp_transport's member 'malloced' is set to 1, never evaluated
and the structure is kfreed anyway. So just remove it.

Signed-off-by: Daniel Borkmann <dborkman@redhat.com>
---
 include/net/sctp/structs.h | 5 +----
 net/sctp/transport.c       | 1 -
 2 files changed, 1 insertion(+), 5 deletions(-)

diff --git a/include/net/sctp/structs.h b/include/net/sctp/structs.h
index 3de5985..945e952 100644
--- a/include/net/sctp/structs.h
+++ b/include/net/sctp/structs.h
@@ -779,10 +779,7 @@ struct sctp_transport {
 		hb_sent:1,
 
 		/* Is the Path MTU update pending on this tranport */
-		pmtu_pending:1,
-
-		/* Is this structure kfree()able? */
-		malloced:1;
+		pmtu_pending:1;
 
 	/* Has this transport moved the ctsn since we last sacked */
 	__u32 sack_generation;
diff --git a/net/sctp/transport.c b/net/sctp/transport.c
index fafd2a4..098f1d5f 100644
--- a/net/sctp/transport.c
+++ b/net/sctp/transport.c
@@ -123,7 +123,6 @@ struct sctp_transport *sctp_transport_new(struct net *net,
 	if (!sctp_transport_init(net, transport, addr, gfp))
 		goto fail_init;
 
-	transport->malloced = 1;
 	SCTP_DBG_OBJCNT_INC(transport);
 
 	return transport;
-- 
1.7.11.7

[PATCH net-next 7/9] net: sctp: sctp_bind_addr: remove dead code

[Daniel Borkmann]

The sctp_bind_addr structure has a 'malloced' member that is
always set to 0, thus in sctp_bind_addr_free() the kfree()
part can never be called. This part is embedded into
sctp_ep_common anyway and never alloced.

Signed-off-by: Daniel Borkmann <dborkman@redhat.com>
---
 include/net/sctp/structs.h | 2 --
 net/sctp/bind_addr.c       | 7 -------
 2 files changed, 9 deletions(-)

diff --git a/include/net/sctp/structs.h b/include/net/sctp/structs.h
index 945e952..b4e9894 100644
--- a/include/net/sctp/structs.h
+++ b/include/net/sctp/structs.h
@@ -1088,8 +1088,6 @@ struct sctp_bind_addr {
 	 *	peer(s) in INIT and INIT ACK chunks.
 	 */
 	struct list_head address_list;
-
-	int malloced;	     /* Are we kfree()able?  */
 };
 
 void sctp_bind_addr_init(struct sctp_bind_addr *, __u16 port);
diff --git a/net/sctp/bind_addr.c b/net/sctp/bind_addr.c
index d886b3b..41145fe 100644
--- a/net/sctp/bind_addr.c
+++ b/net/sctp/bind_addr.c
@@ -131,8 +131,6 @@ int sctp_bind_addr_dup(struct sctp_bind_addr *dest,
  */
 void sctp_bind_addr_init(struct sctp_bind_addr *bp, __u16 port)
 {
-	bp->malloced = 0;
-
 	INIT_LIST_HEAD(&bp->address_list);
 	bp->port = port;
 }
@@ -155,11 +153,6 @@ void sctp_bind_addr_free(struct sctp_bind_addr *bp)
 {
 	/* Empty the bind address list. */
 	sctp_bind_addr_clean(bp);
-
-	if (bp->malloced) {
-		kfree(bp);
-		SCTP_DBG_OBJCNT_DEC(bind_addr);
-	}
 }
 
 /* Add an address to the bind address list in the SCTP_bind_addr structure. */
-- 
1.7.11.7

[PATCH net-next 8/9] net: sctp: sctp_ulpq: remove 'malloced' struct member

[Daniel Borkmann]

The structure sctp_ulpq is embedded into sctp_association and never
separately allocated, also ulpq->malloced is always 0, so that
kfree() is never called. Therefore, remove this code.

Signed-off-by: Daniel Borkmann <dborkman@redhat.com>
---
 include/net/sctp/ulpqueue.h | 1 -
 net/sctp/ulpqueue.c         | 3 ---
 2 files changed, 4 deletions(-)

diff --git a/include/net/sctp/ulpqueue.h b/include/net/sctp/ulpqueue.h
index ff1b8ba7..00e50ba 100644
--- a/include/net/sctp/ulpqueue.h
+++ b/include/net/sctp/ulpqueue.h
@@ -49,7 +49,6 @@
 
 /* A structure to carry information to the ULP (e.g. Sockets API) */
 struct sctp_ulpq {
-	char malloced;
 	char pd_mode;
 	struct sctp_association *asoc;
 	struct sk_buff_head reasm;
diff --git a/net/sctp/ulpqueue.c b/net/sctp/ulpqueue.c
index 0fd5b3d..04e3d47 100644
--- a/net/sctp/ulpqueue.c
+++ b/net/sctp/ulpqueue.c
@@ -68,7 +68,6 @@ struct sctp_ulpq *sctp_ulpq_init(struct sctp_ulpq *ulpq,
 	skb_queue_head_init(&ulpq->reasm);
 	skb_queue_head_init(&ulpq->lobby);
 	ulpq->pd_mode  = 0;
-	ulpq->malloced = 0;
 
 	return ulpq;
 }
@@ -96,8 +95,6 @@ void sctp_ulpq_flush(struct sctp_ulpq *ulpq)
 void sctp_ulpq_free(struct sctp_ulpq *ulpq)
 {
 	sctp_ulpq_flush(ulpq);
-	if (ulpq->malloced)
-		kfree(ulpq);
 }
 
 /* Process an incoming DATA chunk.  */
-- 
1.7.11.7

[PATCH net-next 9/9] net: sctp: sctp_ulpq: do not use char as a struct member

[Daniel Borkmann]

Replace char with a more appropriate __u8 bitfield for pd_mode.

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

diff --git a/include/net/sctp/ulpqueue.h b/include/net/sctp/ulpqueue.h
index 00e50ba..a2bebd4 100644
--- a/include/net/sctp/ulpqueue.h
+++ b/include/net/sctp/ulpqueue.h
@@ -49,10 +49,10 @@
 
 /* A structure to carry information to the ULP (e.g. Sockets API) */
 struct sctp_ulpq {
-	char pd_mode;
 	struct sctp_association *asoc;
 	struct sk_buff_head reasm;
 	struct sk_buff_head lobby;
+	__u8 pd_mode:1;
 };
 
 /* Prototypes. */
-- 
1.7.11.7

RE: [PATCH net-next 4/9] net: sctp: sctp_outq: consolidate chars into bitfield

[David Laight]

>  	__u32 outstanding_bytes;
> 
> -	/* Are we doing fast-rtx on this queue */
> -	char fast_rtx;
> -
> -	/* Corked? */
> -	char cork;
> -
> -	/* Is this structure empty?  */
> -	char empty;
> +	__u8 fast_rtx:1,	/* Are we doing fast-rtx on this queue */
> +	     cork:1,		/* Corked? */
> +	     empty:1;		/* Is this structure empty?  */
>  };

Use of bitfields just makes the code slower.
The only real excuse for using them is to reduce the size
of a structure that is allocated a lot.

In the above you are just increasing the padding from
1 byte to 3 bytes.

	David

Re: [PATCH net-next 4/9] net: sctp: sctp_outq: consolidate chars into bitfield

[Daniel Borkmann]

On 04/17/2013 10:43 AM, David Laight wrote:
>>   	__u32 outstanding_bytes;
>>
>> -	/* Are we doing fast-rtx on this queue */
>> -	char fast_rtx;
>> -
>> -	/* Corked? */
>> -	char cork;
>> -
>> -	/* Is this structure empty?  */
>> -	char empty;
>> +	__u8 fast_rtx:1,	/* Are we doing fast-rtx on this queue */
>> +	     cork:1,		/* Corked? */
>> +	     empty:1;		/* Is this structure empty?  */
>>   };
>
> Use of bitfields just makes the code slower.
> The only real excuse for using them is to reduce the size
> of a structure that is allocated a lot.

sctp_outq is _embedded_ into an sctp_association structure, which
has [size: 2280, cachelines: 36, members: 76]! A next step would be
to try to reorder its elements carefully and see if we can reduce
the size by filling some holes.

Re: [PATCH net-next 1/9] net: sctp: sctp_ssnmap: remove 'malloced' element from struct

[Neil Horman]

On Tue, Apr 16, 2013 at 11:07:10PM +0200, Daniel Borkmann wrote:
> sctp_ssnmap_init() can only be called from sctp_ssnmap_new()
> where malloced is always set to 1. Thus, when we call
> sctp_ssnmap_free() the test for map->malloced evaluates always
> to true.
> 
> Signed-off-by: Daniel Borkmann <dborkman@redhat.com>
> ---
>  include/net/sctp/structs.h |  1 -
>  net/sctp/ssnmap.c          | 23 ++++++++++++-----------
>  2 files changed, 12 insertions(+), 12 deletions(-)
> 
> diff --git a/include/net/sctp/structs.h b/include/net/sctp/structs.h
> index e12aa77..3c1bb8d 100644
> --- a/include/net/sctp/structs.h
> +++ b/include/net/sctp/structs.h
> @@ -399,7 +399,6 @@ struct sctp_stream {
>  struct sctp_ssnmap {
>  	struct sctp_stream in;
>  	struct sctp_stream out;
> -	int malloced;
>  };
>  
>  struct sctp_ssnmap *sctp_ssnmap_new(__u16 in, __u16 out,
> diff --git a/net/sctp/ssnmap.c b/net/sctp/ssnmap.c
> index 825ea94..da86035 100644
> --- a/net/sctp/ssnmap.c
> +++ b/net/sctp/ssnmap.c
> @@ -74,7 +74,6 @@ struct sctp_ssnmap *sctp_ssnmap_new(__u16 in, __u16 out,
>  	if (!sctp_ssnmap_init(retval, in, out))
>  		goto fail_map;
>  
> -	retval->malloced = 1;
>  	SCTP_DBG_OBJCNT_INC(ssnmap);
>  
>  	return retval;
> @@ -118,14 +117,16 @@ void sctp_ssnmap_clear(struct sctp_ssnmap *map)
>  /* Dispose of a ssnmap.  */
>  void sctp_ssnmap_free(struct sctp_ssnmap *map)
>  {
> -	if (map && map->malloced) {
> -		int size;
> -
> -		size = sctp_ssnmap_size(map->in.len, map->out.len);
> -		if (size <= KMALLOC_MAX_SIZE)
> -			kfree(map);
> -		else
> -			free_pages((unsigned long)map, get_order(size));
> -		SCTP_DBG_OBJCNT_DEC(ssnmap);
> -	}
> +	int size;
> +
> +	if (unlikely(!map))
> +		return;
> +
> +	size = sctp_ssnmap_size(map->in.len, map->out.len);
> +	if (size <= KMALLOC_MAX_SIZE)
> +		kfree(map);
> +	else
> +		free_pages((unsigned long)map, get_order(size));
> +
> +	SCTP_DBG_OBJCNT_DEC(ssnmap);
>  }
> -- 
> 1.7.11.7
> 
I definately like what you're doing here, as the use of the ->malloced member
always struck me as a half-assed way to try and avoid a double free that someone
couldn't track down during this code's initial development.  That said, I'm
wondering if the !map check is going to fail at some point, given that the call
site for sctp_ssnmap_free never sets asoc->ssnmap to NULL after its call.  Maybe
worthwhile adding such a NULL assoginment to the call site to ensure that we
don't accidentally trigger a double free?

Neil

Re: [PATCH net-next 2/9] net: sctp: sctp_inq: remove dead code

[Neil Horman]

On Tue, Apr 16, 2013 at 11:07:11PM +0200, Daniel Borkmann wrote:
> sctp_inq is never kmalloced, since it's integrated into sctp_ep_common
> and only initialized from eps and assocs. Therefore, remove the dead
> code from there.
> 
> Signed-off-by: Daniel Borkmann <dborkman@redhat.com>
> ---
>  include/net/sctp/structs.h | 2 --
>  net/sctp/inqueue.c         | 7 -------
>  2 files changed, 9 deletions(-)
> 
> diff --git a/include/net/sctp/structs.h b/include/net/sctp/structs.h
> index 3c1bb8d..125a19c 100644
> --- a/include/net/sctp/structs.h
> +++ b/include/net/sctp/structs.h
> @@ -991,8 +991,6 @@ struct sctp_inq {
>  	 * messages.
>  	 */
>  	struct work_struct immediate;
> -
> -	int malloced;	     /* Is this structure kfree()able?	*/
>  };
>  
>  void sctp_inq_init(struct sctp_inq *);
> diff --git a/net/sctp/inqueue.c b/net/sctp/inqueue.c
> index 2d5ad28..3221d07 100644
> --- a/net/sctp/inqueue.c
> +++ b/net/sctp/inqueue.c
> @@ -58,8 +58,6 @@ void sctp_inq_init(struct sctp_inq *queue)
>  
>  	/* Create a task for delivering data.  */
>  	INIT_WORK(&queue->immediate, NULL);
> -
> -	queue->malloced = 0;
>  }
>  
>  /* Release the memory associated with an SCTP inqueue.  */
> @@ -80,11 +78,6 @@ void sctp_inq_free(struct sctp_inq *queue)
>  		sctp_chunk_free(queue->in_progress);
>  		queue->in_progress = NULL;
>  	}
> -
> -	if (queue->malloced) {
> -		/* Dump the master memory segment.  */
> -		kfree(queue);
> -	}
>  }
>  
Acked-by: Neil Horman <nhorman@tuxdriver.com>

Re: [PATCH net-next 1/9] net: sctp: sctp_ssnmap: remove 'malloced' element from struct

[Daniel Borkmann]

On 04/17/2013 02:45 PM, Neil Horman wrote:
> On Tue, Apr 16, 2013 at 11:07:10PM +0200, Daniel Borkmann wrote:
>> sctp_ssnmap_init() can only be called from sctp_ssnmap_new()
>> where malloced is always set to 1. Thus, when we call
>> sctp_ssnmap_free() the test for map->malloced evaluates always
>> to true.
>>
>> Signed-off-by: Daniel Borkmann <dborkman@redhat.com>
>> ---
>>   include/net/sctp/structs.h |  1 -
>>   net/sctp/ssnmap.c          | 23 ++++++++++++-----------
>>   2 files changed, 12 insertions(+), 12 deletions(-)
>>
>> diff --git a/include/net/sctp/structs.h b/include/net/sctp/structs.h
>> index e12aa77..3c1bb8d 100644
>> --- a/include/net/sctp/structs.h
>> +++ b/include/net/sctp/structs.h
>> @@ -399,7 +399,6 @@ struct sctp_stream {
>>   struct sctp_ssnmap {
>>   	struct sctp_stream in;
>>   	struct sctp_stream out;
>> -	int malloced;
>>   };
>>
>>   struct sctp_ssnmap *sctp_ssnmap_new(__u16 in, __u16 out,
>> diff --git a/net/sctp/ssnmap.c b/net/sctp/ssnmap.c
>> index 825ea94..da86035 100644
>> --- a/net/sctp/ssnmap.c
>> +++ b/net/sctp/ssnmap.c
>> @@ -74,7 +74,6 @@ struct sctp_ssnmap *sctp_ssnmap_new(__u16 in, __u16 out,
>>   	if (!sctp_ssnmap_init(retval, in, out))
>>   		goto fail_map;
>>
>> -	retval->malloced = 1;
>>   	SCTP_DBG_OBJCNT_INC(ssnmap);
>>
>>   	return retval;
>> @@ -118,14 +117,16 @@ void sctp_ssnmap_clear(struct sctp_ssnmap *map)
>>   /* Dispose of a ssnmap.  */
>>   void sctp_ssnmap_free(struct sctp_ssnmap *map)
>>   {
>> -	if (map && map->malloced) {
>> -		int size;
>> -
>> -		size = sctp_ssnmap_size(map->in.len, map->out.len);
>> -		if (size <= KMALLOC_MAX_SIZE)
>> -			kfree(map);
>> -		else
>> -			free_pages((unsigned long)map, get_order(size));
>> -		SCTP_DBG_OBJCNT_DEC(ssnmap);
>> -	}
>> +	int size;
>> +
>> +	if (unlikely(!map))
>> +		return;
>> +
>> +	size = sctp_ssnmap_size(map->in.len, map->out.len);
>> +	if (size <= KMALLOC_MAX_SIZE)
>> +		kfree(map);
>> +	else
>> +		free_pages((unsigned long)map, get_order(size));
>> +
>> +	SCTP_DBG_OBJCNT_DEC(ssnmap);
>>   }
>> --
>> 1.7.11.7
>>
> I definately like what you're doing here, as the use of the ->malloced member
> always struck me as a half-assed way to try and avoid a double free that someone
> couldn't track down during this code's initial development.  That said, I'm
> wondering if the !map check is going to fail at some point, given that the call
> site for sctp_ssnmap_free never sets asoc->ssnmap to NULL after its call.  Maybe
> worthwhile adding such a NULL assoginment to the call site to ensure that we
> don't accidentally trigger a double free?

I'll test that with lksctp-tools suite and come back to you today.

Thanks,
Daniel

Re: [PATCH net-next 3/9] net: sctp: sctp_outq: remove 'malloced' from its struct

[Neil Horman]

On Tue, Apr 16, 2013 at 11:07:12PM +0200, Daniel Borkmann wrote:
> sctp_outq is embedded into sctp_association, and thus never
> kmalloced in any way. Also, malloced is always 0, thus kfree()
> is never called. Therefore, remove that dead piece of code.
> 
> Signed-off-by: Daniel Borkmann <dborkman@redhat.com>
> ---
>  include/net/sctp/structs.h | 3 ---
>  net/sctp/outqueue.c        | 6 ------
>  2 files changed, 9 deletions(-)
> 
> diff --git a/include/net/sctp/structs.h b/include/net/sctp/structs.h
> index 125a19c..73fd5de 100644
> --- a/include/net/sctp/structs.h
> +++ b/include/net/sctp/structs.h
> @@ -1059,9 +1059,6 @@ struct sctp_outq {
>  
>  	/* Is this structure empty?  */
>  	char empty;
> -
> -	/* Are we kfree()able? */
> -	char malloced;
>  };
>  
>  void sctp_outq_init(struct sctp_association *, struct sctp_outq *);
> diff --git a/net/sctp/outqueue.c b/net/sctp/outqueue.c
> index 01dca75..d4c137e 100644
> --- a/net/sctp/outqueue.c
> +++ b/net/sctp/outqueue.c
> @@ -217,8 +217,6 @@ void sctp_outq_init(struct sctp_association *asoc, struct sctp_outq *q)
>  	q->outstanding_bytes = 0;
>  	q->empty = 1;
>  	q->cork  = 0;
> -
> -	q->malloced = 0;
>  	q->out_qlen = 0;
>  }
>  
> @@ -295,10 +293,6 @@ void sctp_outq_free(struct sctp_outq *q)
>  {
>  	/* Throw away leftover chunks. */
>  	__sctp_outq_teardown(q);
> -
> -	/* If we were kmalloc()'d, free the memory.  */
> -	if (q->malloced)
> -		kfree(q);
>  }
>  
>  /* Put a new chunk in an sctp_outq.  */
> -- 
> 1.7.11.7
> 
Acked-by: Neil Horman <nhorman@tuxdriver.com>

Re: [PATCH net-next 4/9] net: sctp: sctp_outq: consolidate chars into bitfield

[Neil Horman]

On Wed, Apr 17, 2013 at 11:26:21AM +0200, Daniel Borkmann wrote:
> On 04/17/2013 10:43 AM, David Laight wrote:
> >>  	__u32 outstanding_bytes;
> >>
> >>-	/* Are we doing fast-rtx on this queue */
> >>-	char fast_rtx;
> >>-
> >>-	/* Corked? */
> >>-	char cork;
> >>-
> >>-	/* Is this structure empty?  */
> >>-	char empty;
> >>+	__u8 fast_rtx:1,	/* Are we doing fast-rtx on this queue */
> >>+	     cork:1,		/* Corked? */
> >>+	     empty:1;		/* Is this structure empty?  */
> >>  };
> >
> >Use of bitfields just makes the code slower.
> >The only real excuse for using them is to reduce the size
> >of a structure that is allocated a lot.
> 
> sctp_outq is _embedded_ into an sctp_association structure, which
> has [size: 2280, cachelines: 36, members: 76]! A next step would be
> to try to reorder its elements carefully and see if we can reduce
> the size by filling some holes.
Unrelated note: How did you get pahole to give you that information?  For some
reason pahole can never glean any useful information about struct
sctp_association for me, despite having dwarf info embedded into the cu.

Neil

> --
> 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 net-next 4/9] net: sctp: sctp_outq: consolidate chars into bitfield

[Neil Horman]

On Tue, Apr 16, 2013 at 11:07:13PM +0200, Daniel Borkmann wrote:
> sctp_outq structure members fast_rtx, cork and empty are all
> of type char. Consolidate them into a single __u8 bitfield since
> they either carry 0 or 1. Have the rest 5 bits for future flags.
> 
> Signed-off-by: Daniel Borkmann <dborkman@redhat.com>
> ---
>  include/net/sctp/structs.h | 11 +++--------
>  1 file changed, 3 insertions(+), 8 deletions(-)
> 
> diff --git a/include/net/sctp/structs.h b/include/net/sctp/structs.h
> index 73fd5de..3de5985 100644
> --- a/include/net/sctp/structs.h
> +++ b/include/net/sctp/structs.h
> @@ -1051,14 +1051,9 @@ struct sctp_outq {
>  	/* How many unackd bytes do we have in-flight?	*/
>  	__u32 outstanding_bytes;
>  
> -	/* Are we doing fast-rtx on this queue */
> -	char fast_rtx;
> -
> -	/* Corked? */
> -	char cork;
> -
> -	/* Is this structure empty?  */
> -	char empty;
> +	__u8 fast_rtx:1,	/* Are we doing fast-rtx on this queue */
> +	     cork:1,		/* Corked? */
> +	     empty:1;		/* Is this structure empty?  */
>  };
>  
>  void sctp_outq_init(struct sctp_association *, struct sctp_outq *);
> -- 
> 1.7.11.7
> 
Acked-by: Neil Horman <nhorman@tuxdriver.com>

Re: [PATCH net-next 5/9] net: sctp: outqueue: simplify sctp_outq_uncork function

[Neil Horman]

On Tue, Apr 16, 2013 at 11:07:14PM +0200, Daniel Borkmann wrote:
> Just a minor edit to simplify the function. No need for this
> error variable here.
> 
> Signed-off-by: Daniel Borkmann <dborkman@redhat.com>
> ---
>  net/sctp/outqueue.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/net/sctp/outqueue.c b/net/sctp/outqueue.c
> index d4c137e..32a4625 100644
> --- a/net/sctp/outqueue.c
> +++ b/net/sctp/outqueue.c
> @@ -701,11 +701,10 @@ redo:
>  /* Cork the outqueue so queued chunks are really queued. */
>  int sctp_outq_uncork(struct sctp_outq *q)
>  {
> -	int error = 0;
>  	if (q->cork)
>  		q->cork = 0;
> -	error = sctp_outq_flush(q, 0);
> -	return error;
> +
> +	return sctp_outq_flush(q, 0);
>  }
>  
>  
> -- 
> 1.7.11.7
> 
> --
> 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
> 
Acked-by: Neil Horman <nhorman@tuxdriver.com>

Re: [PATCH net-next 6/9] net: sctp: sctp_transport: remove unused variable

[Neil Horman]

On Tue, Apr 16, 2013 at 11:07:15PM +0200, Daniel Borkmann wrote:
> sctp_transport's member 'malloced' is set to 1, never evaluated
> and the structure is kfreed anyway. So just remove it.
> 
> Signed-off-by: Daniel Borkmann <dborkman@redhat.com>
> ---
>  include/net/sctp/structs.h | 5 +----
>  net/sctp/transport.c       | 1 -
>  2 files changed, 1 insertion(+), 5 deletions(-)
> 
> diff --git a/include/net/sctp/structs.h b/include/net/sctp/structs.h
> index 3de5985..945e952 100644
> --- a/include/net/sctp/structs.h
> +++ b/include/net/sctp/structs.h
> @@ -779,10 +779,7 @@ struct sctp_transport {
>  		hb_sent:1,
>  
>  		/* Is the Path MTU update pending on this tranport */
> -		pmtu_pending:1,
> -
> -		/* Is this structure kfree()able? */
> -		malloced:1;
> +		pmtu_pending:1;
>  
>  	/* Has this transport moved the ctsn since we last sacked */
>  	__u32 sack_generation;
> diff --git a/net/sctp/transport.c b/net/sctp/transport.c
> index fafd2a4..098f1d5f 100644
> --- a/net/sctp/transport.c
> +++ b/net/sctp/transport.c
> @@ -123,7 +123,6 @@ struct sctp_transport *sctp_transport_new(struct net *net,
>  	if (!sctp_transport_init(net, transport, addr, gfp))
>  		goto fail_init;
>  
> -	transport->malloced = 1;
>  	SCTP_DBG_OBJCNT_INC(transport);
>  
>  	return transport;
> -- 
> 1.7.11.7
> 
> --
> 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
> 
Acked-by: Neil Horman <nhorman@tuxdriver.com>

Re: [PATCH net-next 7/9] net: sctp: sctp_bind_addr: remove dead code

[Neil Horman]

On Tue, Apr 16, 2013 at 11:07:16PM +0200, Daniel Borkmann wrote:
> The sctp_bind_addr structure has a 'malloced' member that is
> always set to 0, thus in sctp_bind_addr_free() the kfree()
> part can never be called. This part is embedded into
> sctp_ep_common anyway and never alloced.
> 
> Signed-off-by: Daniel Borkmann <dborkman@redhat.com>
> ---
>  include/net/sctp/structs.h | 2 --
>  net/sctp/bind_addr.c       | 7 -------
>  2 files changed, 9 deletions(-)
> 
> diff --git a/include/net/sctp/structs.h b/include/net/sctp/structs.h
> index 945e952..b4e9894 100644
> --- a/include/net/sctp/structs.h
> +++ b/include/net/sctp/structs.h
> @@ -1088,8 +1088,6 @@ struct sctp_bind_addr {
>  	 *	peer(s) in INIT and INIT ACK chunks.
>  	 */
>  	struct list_head address_list;
> -
> -	int malloced;	     /* Are we kfree()able?  */
>  };
>  
>  void sctp_bind_addr_init(struct sctp_bind_addr *, __u16 port);
> diff --git a/net/sctp/bind_addr.c b/net/sctp/bind_addr.c
> index d886b3b..41145fe 100644
> --- a/net/sctp/bind_addr.c
> +++ b/net/sctp/bind_addr.c
> @@ -131,8 +131,6 @@ int sctp_bind_addr_dup(struct sctp_bind_addr *dest,
>   */
>  void sctp_bind_addr_init(struct sctp_bind_addr *bp, __u16 port)
>  {
> -	bp->malloced = 0;
> -
>  	INIT_LIST_HEAD(&bp->address_list);
>  	bp->port = port;
>  }
> @@ -155,11 +153,6 @@ void sctp_bind_addr_free(struct sctp_bind_addr *bp)
>  {
>  	/* Empty the bind address list. */
>  	sctp_bind_addr_clean(bp);
> -
> -	if (bp->malloced) {
> -		kfree(bp);
> -		SCTP_DBG_OBJCNT_DEC(bind_addr);
> -	}
>  }
>  
>  /* Add an address to the bind address list in the SCTP_bind_addr structure. */
> -- 
> 1.7.11.7
> 
> --
> 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
> 
Acked-by: Neil Horman <nhorman@tuxdriver.com>

Re: [PATCH net-next 4/9] net: sctp: sctp_outq: consolidate chars into bitfield

[Vlad Yasevich]

On 04/17/2013 11:27 AM, Neil Horman wrote:
> On Wed, Apr 17, 2013 at 11:26:21AM +0200, Daniel Borkmann wrote:
>> On 04/17/2013 10:43 AM, David Laight wrote:
>>>>   	__u32 outstanding_bytes;
>>>>
>>>> -	/* Are we doing fast-rtx on this queue */
>>>> -	char fast_rtx;
>>>> -
>>>> -	/* Corked? */
>>>> -	char cork;
>>>> -
>>>> -	/* Is this structure empty?  */
>>>> -	char empty;
>>>> +	__u8 fast_rtx:1,	/* Are we doing fast-rtx on this queue */
>>>> +	     cork:1,		/* Corked? */
>>>> +	     empty:1;		/* Is this structure empty?  */
>>>>   };
>>>
>>> Use of bitfields just makes the code slower.
>>> The only real excuse for using them is to reduce the size
>>> of a structure that is allocated a lot.
>>
>> sctp_outq is _embedded_ into an sctp_association structure, which
>> has [size: 2280, cachelines: 36, members: 76]! A next step would be
>> to try to reorder its elements carefully and see if we can reduce
>> the size by filling some holes.
> Unrelated note: How did you get pahole to give you that information?  For some
> reason pahole can never glean any useful information about struct
> sctp_association for me, despite having dwarf info embedded into the cu.
>

pahole -C sctp_association <sctp object file>  works for me.

-vlad

> Neil
>
>> --
>> 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
>>
> --
> 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 net-next 9/9] net: sctp: sctp_ulpq: do not use char as a struct member

[Vlad Yasevich]

On 04/16/2013 05:07 PM, Daniel Borkmann wrote:
> Replace char with a more appropriate __u8 bitfield for pd_mode.
>
> Signed-off-by: Daniel Borkmann <dborkman@redhat.com>
> ---
>   include/net/sctp/ulpqueue.h | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/include/net/sctp/ulpqueue.h b/include/net/sctp/ulpqueue.h
> index 00e50ba..a2bebd4 100644
> --- a/include/net/sctp/ulpqueue.h
> +++ b/include/net/sctp/ulpqueue.h
> @@ -49,10 +49,10 @@
>
>   /* A structure to carry information to the ULP (e.g. Sockets API) */
>   struct sctp_ulpq {
> -	char pd_mode;
>   	struct sctp_association *asoc;
>   	struct sk_buff_head reasm;
>   	struct sk_buff_head lobby;
> +	__u8 pd_mode:1;
>   };
>
>   /* Prototypes. */
>

This one is a bit pointless.  You are not saving any space and making 
generated code worse.

-vlad

Re: [PATCH net-next 4/9] net: sctp: sctp_outq: consolidate chars into bitfield

[Daniel Borkmann]

On 04/17/2013 05:27 PM, Neil Horman wrote:
> On Wed, Apr 17, 2013 at 11:26:21AM +0200, Daniel Borkmann wrote:
>> On 04/17/2013 10:43 AM, David Laight wrote:
>>>>   	__u32 outstanding_bytes;
>>>>
>>>> -	/* Are we doing fast-rtx on this queue */
>>>> -	char fast_rtx;
>>>> -
>>>> -	/* Corked? */
>>>> -	char cork;
>>>> -
>>>> -	/* Is this structure empty?  */
>>>> -	char empty;
>>>> +	__u8 fast_rtx:1,	/* Are we doing fast-rtx on this queue */
>>>> +	     cork:1,		/* Corked? */
>>>> +	     empty:1;		/* Is this structure empty?  */
>>>>   };
>>>
>>> Use of bitfields just makes the code slower.
>>> The only real excuse for using them is to reduce the size
>>> of a structure that is allocated a lot.
>>
>> sctp_outq is _embedded_ into an sctp_association structure, which
>> has [size: 2280, cachelines: 36, members: 76]! A next step would be
>> to try to reorder its elements carefully and see if we can reduce
>> the size by filling some holes.
> Unrelated note: How did you get pahole to give you that information?  For some
> reason pahole can never glean any useful information about struct
> sctp_association for me, despite having dwarf info embedded into the cu.

I made a ``make net/sctp/'' and run pahole against net/sctp/sctp.o redirecting
the output into a file, thus I have all possible headers visible.

Re: [PATCH net-next 4/9] net: sctp: sctp_outq: consolidate chars into bitfield

[Neil Horman]

On Wed, Apr 17, 2013 at 11:35:49AM -0400, Vlad Yasevich wrote:
> On 04/17/2013 11:27 AM, Neil Horman wrote:
> >On Wed, Apr 17, 2013 at 11:26:21AM +0200, Daniel Borkmann wrote:
> >>On 04/17/2013 10:43 AM, David Laight wrote:
> >>>>  	__u32 outstanding_bytes;
> >>>>
> >>>>-	/* Are we doing fast-rtx on this queue */
> >>>>-	char fast_rtx;
> >>>>-
> >>>>-	/* Corked? */
> >>>>-	char cork;
> >>>>-
> >>>>-	/* Is this structure empty?  */
> >>>>-	char empty;
> >>>>+	__u8 fast_rtx:1,	/* Are we doing fast-rtx on this queue */
> >>>>+	     cork:1,		/* Corked? */
> >>>>+	     empty:1;		/* Is this structure empty?  */
> >>>>  };
> >>>
> >>>Use of bitfields just makes the code slower.
> >>>The only real excuse for using them is to reduce the size
> >>>of a structure that is allocated a lot.
> >>
> >>sctp_outq is _embedded_ into an sctp_association structure, which
> >>has [size: 2280, cachelines: 36, members: 76]! A next step would be
> >>to try to reorder its elements carefully and see if we can reduce
> >>the size by filling some holes.
> >Unrelated note: How did you get pahole to give you that information?  For some
> >reason pahole can never glean any useful information about struct
> >sctp_association for me, despite having dwarf info embedded into the cu.
> >
> 
> pahole -C sctp_association <sctp object file>  works for me.
> 
> -vlad
> 
Strange, that doesn't work for me, and the DW_TAG_structure_type entry is in
place in the debug_info section (though interestingly no byte offsets are noted
for any members). I'll have to look at that further.

Thanks
Neil

> >Neil
> >
> >>--
> >>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
> >>
> >--
> >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
> >
> 
> --
> 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 net-next 9/9] net: sctp: sctp_ulpq: do not use char as a struct member

[Daniel Borkmann]

On 04/17/2013 05:36 PM, Vlad Yasevich wrote:
> On 04/16/2013 05:07 PM, Daniel Borkmann wrote:
>> Replace char with a more appropriate __u8 bitfield for pd_mode.
>>
>> Signed-off-by: Daniel Borkmann <dborkman@redhat.com>
>> ---
>>   include/net/sctp/ulpqueue.h | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/include/net/sctp/ulpqueue.h b/include/net/sctp/ulpqueue.h
>> index 00e50ba..a2bebd4 100644
>> --- a/include/net/sctp/ulpqueue.h
>> +++ b/include/net/sctp/ulpqueue.h
>> @@ -49,10 +49,10 @@
>>
>>   /* A structure to carry information to the ULP (e.g. Sockets API) */
>>   struct sctp_ulpq {
>> -    char pd_mode;
>>       struct sctp_association *asoc;
>>       struct sk_buff_head reasm;
>>       struct sk_buff_head lobby;
>> +    __u8 pd_mode:1;
>>   };
>>
>>   /* Prototypes. */
>
> This one is a bit pointless.  You are not saving any space and making generated code worse.

Right, this one should be dropped, if Dave wants me to resubmit from
0 to 8, I can do that, otherwise just this one can be left out.

Re: [PATCH net-next 4/9] net: sctp: sctp_outq: consolidate chars into bitfield

[Vlad Yasevich]

On 04/17/2013 05:26 AM, Daniel Borkmann wrote:
> On 04/17/2013 10:43 AM, David Laight wrote:
>>>       __u32 outstanding_bytes;
>>>
>>> -    /* Are we doing fast-rtx on this queue */
>>> -    char fast_rtx;
>>> -
>>> -    /* Corked? */
>>> -    char cork;
>>> -
>>> -    /* Is this structure empty?  */
>>> -    char empty;
>>> +    __u8 fast_rtx:1,    /* Are we doing fast-rtx on this queue */
>>> +         cork:1,        /* Corked? */
>>> +         empty:1;        /* Is this structure empty?  */
>>>   };
>>
>> Use of bitfields just makes the code slower.
>> The only real excuse for using them is to reduce the size
>> of a structure that is allocated a lot.
>
> sctp_outq is _embedded_ into an sctp_association structure, which
> has [size: 2280, cachelines: 36, members: 76]! A next step would be
> to try to reorder its elements carefully and see if we can reduce
> the size by filling some holes.

I remember attempting this undertaking that led me down the rabbit hole 
so fast that my head started to spin.  It is a badly needed project, but 
be prepared to spend a lot of time on it.

Not sure if the space saving is worth the bad code generated for 
bit-fields, especially considering that empty and cork are used on 
almost every packet.

-vlad

> --
> 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 net-next 9/9] net: sctp: sctp_ulpq: do not use char as a struct member

[Neil Horman]

On Tue, Apr 16, 2013 at 11:07:18PM +0200, Daniel Borkmann wrote:
> Replace char with a more appropriate __u8 bitfield for pd_mode.
> 
> Signed-off-by: Daniel Borkmann <dborkman@redhat.com>
> ---
>  include/net/sctp/ulpqueue.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/include/net/sctp/ulpqueue.h b/include/net/sctp/ulpqueue.h
> index 00e50ba..a2bebd4 100644
> --- a/include/net/sctp/ulpqueue.h
> +++ b/include/net/sctp/ulpqueue.h
> @@ -49,10 +49,10 @@
>  
>  /* A structure to carry information to the ULP (e.g. Sockets API) */
>  struct sctp_ulpq {
> -	char pd_mode;
>  	struct sctp_association *asoc;
>  	struct sk_buff_head reasm;
>  	struct sk_buff_head lobby;
> +	__u8 pd_mode:1;
>  };
>  

Can you move the char (or __u8 if you prefer) to right after the reasm meber?  I
think theres a hole there that you can fill in
Neil

Re: [PATCH net-next 8/9] net: sctp: sctp_ulpq: remove 'malloced' struct member

[Neil Horman]

On Tue, Apr 16, 2013 at 11:07:17PM +0200, Daniel Borkmann wrote:
> The structure sctp_ulpq is embedded into sctp_association and never
> separately allocated, also ulpq->malloced is always 0, so that
> kfree() is never called. Therefore, remove this code.
> 
> Signed-off-by: Daniel Borkmann <dborkman@redhat.com>
> ---
>  include/net/sctp/ulpqueue.h | 1 -
>  net/sctp/ulpqueue.c         | 3 ---
>  2 files changed, 4 deletions(-)
> 
> diff --git a/include/net/sctp/ulpqueue.h b/include/net/sctp/ulpqueue.h
> index ff1b8ba7..00e50ba 100644
> --- a/include/net/sctp/ulpqueue.h
> +++ b/include/net/sctp/ulpqueue.h
> @@ -49,7 +49,6 @@
>  
>  /* A structure to carry information to the ULP (e.g. Sockets API) */
>  struct sctp_ulpq {
> -	char malloced;
>  	char pd_mode;
>  	struct sctp_association *asoc;
>  	struct sk_buff_head reasm;
> diff --git a/net/sctp/ulpqueue.c b/net/sctp/ulpqueue.c
> index 0fd5b3d..04e3d47 100644
> --- a/net/sctp/ulpqueue.c
> +++ b/net/sctp/ulpqueue.c
> @@ -68,7 +68,6 @@ struct sctp_ulpq *sctp_ulpq_init(struct sctp_ulpq *ulpq,
>  	skb_queue_head_init(&ulpq->reasm);
>  	skb_queue_head_init(&ulpq->lobby);
>  	ulpq->pd_mode  = 0;
> -	ulpq->malloced = 0;
>  
>  	return ulpq;
>  }
> @@ -96,8 +95,6 @@ void sctp_ulpq_flush(struct sctp_ulpq *ulpq)
>  void sctp_ulpq_free(struct sctp_ulpq *ulpq)
>  {
>  	sctp_ulpq_flush(ulpq);
> -	if (ulpq->malloced)
> -		kfree(ulpq);
>  }
>  
>  /* Process an incoming DATA chunk.  */
> -- 
> 1.7.11.7
> 
> --
> 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
> 
Acked-by: Neil Horman <nhorman@tuxdriver.com>

RE: [PATCH net-next 9/9] net: sctp: sctp_ulpq: do not use char as a struct member

[David Laight]

> >  /* A structure to carry information to the ULP (e.g. Sockets API) */
> >  struct sctp_ulpq {
> > -	char pd_mode;
> >  	struct sctp_association *asoc;
> >  	struct sk_buff_head reasm;
> >  	struct sk_buff_head lobby;
> > +	__u8 pd_mode:1;
> >  };
> >
> 
> Can you move the char (or __u8 if you prefer) to right after the reasm meber?  I
> think theres a hole there that you can fill in

If there is a hole after 'reasm' there will also be one after 'lobby'.
In any case it can't be worse than the 7 byte pad after pd_mode
in the existing file.

In the other structure there is a uint32 preceding the three
flag bytes - so unless some other stuff is moved making them
bitfields won't change any sizes at all.

FWIW we do use SCTP (for M3UA).
The code has to be an order of magnitude slower than TCP!
Never mind the difficulty in generating long ethernet frames
(since the data flow means you have to disable nagle).

	David.

Re: [PATCH net-next 1/9] net: sctp: sctp_ssnmap: remove 'malloced' element from struct

[Daniel Borkmann]

On 04/17/2013 02:52 PM, Daniel Borkmann wrote:
> On 04/17/2013 02:45 PM, Neil Horman wrote:
>> On Tue, Apr 16, 2013 at 11:07:10PM +0200, Daniel Borkmann wrote:
>>> sctp_ssnmap_init() can only be called from sctp_ssnmap_new()
>>> where malloced is always set to 1. Thus, when we call
>>> sctp_ssnmap_free() the test for map->malloced evaluates always
>>> to true.
>>>
>>> Signed-off-by: Daniel Borkmann <dborkman@redhat.com>
>>> ---
>>>   include/net/sctp/structs.h |  1 -
>>>   net/sctp/ssnmap.c          | 23 ++++++++++++-----------
>>>   2 files changed, 12 insertions(+), 12 deletions(-)
>>>
>>> diff --git a/include/net/sctp/structs.h b/include/net/sctp/structs.h
>>> index e12aa77..3c1bb8d 100644
>>> --- a/include/net/sctp/structs.h
>>> +++ b/include/net/sctp/structs.h
>>> @@ -399,7 +399,6 @@ struct sctp_stream {
>>>   struct sctp_ssnmap {
>>>       struct sctp_stream in;
>>>       struct sctp_stream out;
>>> -    int malloced;
>>>   };
>>>
>>>   struct sctp_ssnmap *sctp_ssnmap_new(__u16 in, __u16 out,
>>> diff --git a/net/sctp/ssnmap.c b/net/sctp/ssnmap.c
>>> index 825ea94..da86035 100644
>>> --- a/net/sctp/ssnmap.c
>>> +++ b/net/sctp/ssnmap.c
>>> @@ -74,7 +74,6 @@ struct sctp_ssnmap *sctp_ssnmap_new(__u16 in, __u16 out,
>>>       if (!sctp_ssnmap_init(retval, in, out))
>>>           goto fail_map;
>>>
>>> -    retval->malloced = 1;
>>>       SCTP_DBG_OBJCNT_INC(ssnmap);
>>>
>>>       return retval;
>>> @@ -118,14 +117,16 @@ void sctp_ssnmap_clear(struct sctp_ssnmap *map)
>>>   /* Dispose of a ssnmap.  */
>>>   void sctp_ssnmap_free(struct sctp_ssnmap *map)
>>>   {
>>> -    if (map && map->malloced) {
>>> -        int size;
>>> -
>>> -        size = sctp_ssnmap_size(map->in.len, map->out.len);
>>> -        if (size <= KMALLOC_MAX_SIZE)
>>> -            kfree(map);
>>> -        else
>>> -            free_pages((unsigned long)map, get_order(size));
>>> -        SCTP_DBG_OBJCNT_DEC(ssnmap);
>>> -    }
>>> +    int size;
>>> +
>>> +    if (unlikely(!map))
>>> +        return;
>>> +
>>> +    size = sctp_ssnmap_size(map->in.len, map->out.len);
>>> +    if (size <= KMALLOC_MAX_SIZE)
>>> +        kfree(map);
>>> +    else
>>> +        free_pages((unsigned long)map, get_order(size));
>>> +
>>> +    SCTP_DBG_OBJCNT_DEC(ssnmap);
>>>   }
>>> --
>>> 1.7.11.7
>>>
>> I definately like what you're doing here, as the use of the ->malloced member
>> always struck me as a half-assed way to try and avoid a double free that someone
>> couldn't track down during this code's initial development.  That said, I'm
>> wondering if the !map check is going to fail at some point, given that the call
>> site for sctp_ssnmap_free never sets asoc->ssnmap to NULL after its call.  Maybe
>> worthwhile adding such a NULL assoginment to the call site to ensure that we
>> don't accidentally trigger a double free?
>
> I'll test that with lksctp-tools suite and come back to you today.

Just did that.

I've poisoned the pointers, so that they would throw a WARN_ON() if they have
already been seen. Also, I've put a WARN_ON() before sctp_ssnmap_new() in
sctp_process_init(), in case asoc->ssnmap was not NULL. I've run the lksctp-tools
suite for v4/v6 and nothing was thrown, also it all passed.

That said, I think that the !map check is there because we init the asoc first
with a NULL ssnmap. I suggest, if Dave wants to and if there are no other
objections, that we could apply to net-next the patches ...

   * [1/9] http://patchwork.ozlabs.org/patch/237101/
   * [2/9] http://patchwork.ozlabs.org/patch/237102/
   * [3/9] http://patchwork.ozlabs.org/patch/237103/
   * [5/9] http://patchwork.ozlabs.org/patch/237105/
   * [6/9] http://patchwork.ozlabs.org/patch/237109/
   * [7/9] http://patchwork.ozlabs.org/patch/237106/
   * [8/9] http://patchwork.ozlabs.org/patch/237107/

... as is. I've just tested it, they apply cleanly on top of each other without
the missing. Alternatively, I could resend the set without the two that we cut
out (nr 4 and 9). How you prefer, let me know.

For the remaining two, I think it needs some further analysis, thus I'd say that
we could leave it as is for now and address this at a later point in time.

Thanks,

Daniel

Re: [PATCH net-next 0/9] Minor SCTP cleanups all over the place

[David Miller]

From: Daniel Borkmann <dborkman@redhat.com>
Date: Tue, 16 Apr 2013 23:07:09 +0200

> That's a batch of cleanups for the SCTP protocol. Probably all
> deadly boring and trivial patches, but it always bugs me. :-) I
> guess this is still from the very SCTP beginning in Linux. Well,
> at least we can now remove 41 lines of code in total. lksctp-tools
> test suite run through without problems.
> 
> Daniel Borkmann (9):
>   net: sctp: sctp_ssnmap: remove 'malloced' element from struct
>   net: sctp: sctp_inq: remove dead code
>   net: sctp: sctp_outq: remove 'malloced' from its struct
>   net: sctp: sctp_outq: consolidate chars into bitfield
>   net: sctp: outqueue: simplify sctp_outq_uncork function
>   net: sctp: sctp_transport: remove unused variable
>   net: sctp: sctp_bind_addr: remove dead code
>   net: sctp: sctp_ulpq: remove 'malloced' struct member
>   net: sctp: sctp_ulpq: do not use char as a struct member

All applied except for #4 and #9, thanks.

Re: [PATCH net-next 0/9] Minor SCTP cleanups all over the place

[Daniel Borkmann]

On 04/17/2013 08:13 PM, David Miller wrote:

> All applied except for #4 and #9, thanks.

Thanks David !

Re: [PATCH net-next 1/9] net: sctp: sctp_ssnmap: remove 'malloced' element from struct

[Neil Horman]

On Wed, Apr 17, 2013 at 07:17:18PM +0200, Daniel Borkmann wrote:
> On 04/17/2013 02:52 PM, Daniel Borkmann wrote:
> >On 04/17/2013 02:45 PM, Neil Horman wrote:
> >>On Tue, Apr 16, 2013 at 11:07:10PM +0200, Daniel Borkmann wrote:
> >>>sctp_ssnmap_init() can only be called from sctp_ssnmap_new()
> >>>where malloced is always set to 1. Thus, when we call
> >>>sctp_ssnmap_free() the test for map->malloced evaluates always
> >>>to true.
> >>>
> >>>Signed-off-by: Daniel Borkmann <dborkman@redhat.com>
> >>>---
> >>>  include/net/sctp/structs.h |  1 -
> >>>  net/sctp/ssnmap.c          | 23 ++++++++++++-----------
> >>>  2 files changed, 12 insertions(+), 12 deletions(-)
> >>>
> >>>diff --git a/include/net/sctp/structs.h b/include/net/sctp/structs.h
> >>>index e12aa77..3c1bb8d 100644
> >>>--- a/include/net/sctp/structs.h
> >>>+++ b/include/net/sctp/structs.h
> >>>@@ -399,7 +399,6 @@ struct sctp_stream {
> >>>  struct sctp_ssnmap {
> >>>      struct sctp_stream in;
> >>>      struct sctp_stream out;
> >>>-    int malloced;
> >>>  };
> >>>
> >>>  struct sctp_ssnmap *sctp_ssnmap_new(__u16 in, __u16 out,
> >>>diff --git a/net/sctp/ssnmap.c b/net/sctp/ssnmap.c
> >>>index 825ea94..da86035 100644
> >>>--- a/net/sctp/ssnmap.c
> >>>+++ b/net/sctp/ssnmap.c
> >>>@@ -74,7 +74,6 @@ struct sctp_ssnmap *sctp_ssnmap_new(__u16 in, __u16 out,
> >>>      if (!sctp_ssnmap_init(retval, in, out))
> >>>          goto fail_map;
> >>>
> >>>-    retval->malloced = 1;
> >>>      SCTP_DBG_OBJCNT_INC(ssnmap);
> >>>
> >>>      return retval;
> >>>@@ -118,14 +117,16 @@ void sctp_ssnmap_clear(struct sctp_ssnmap *map)
> >>>  /* Dispose of a ssnmap.  */
> >>>  void sctp_ssnmap_free(struct sctp_ssnmap *map)
> >>>  {
> >>>-    if (map && map->malloced) {
> >>>-        int size;
> >>>-
> >>>-        size = sctp_ssnmap_size(map->in.len, map->out.len);
> >>>-        if (size <= KMALLOC_MAX_SIZE)
> >>>-            kfree(map);
> >>>-        else
> >>>-            free_pages((unsigned long)map, get_order(size));
> >>>-        SCTP_DBG_OBJCNT_DEC(ssnmap);
> >>>-    }
> >>>+    int size;
> >>>+
> >>>+    if (unlikely(!map))
> >>>+        return;
> >>>+
> >>>+    size = sctp_ssnmap_size(map->in.len, map->out.len);
> >>>+    if (size <= KMALLOC_MAX_SIZE)
> >>>+        kfree(map);
> >>>+    else
> >>>+        free_pages((unsigned long)map, get_order(size));
> >>>+
> >>>+    SCTP_DBG_OBJCNT_DEC(ssnmap);
> >>>  }
> >>>--
> >>>1.7.11.7
> >>>
> >>I definately like what you're doing here, as the use of the ->malloced member
> >>always struck me as a half-assed way to try and avoid a double free that someone
> >>couldn't track down during this code's initial development.  That said, I'm
> >>wondering if the !map check is going to fail at some point, given that the call
> >>site for sctp_ssnmap_free never sets asoc->ssnmap to NULL after its call.  Maybe
> >>worthwhile adding such a NULL assoginment to the call site to ensure that we
> >>don't accidentally trigger a double free?
> >
> >I'll test that with lksctp-tools suite and come back to you today.
> 
> Just did that.
> 
> I've poisoned the pointers, so that they would throw a WARN_ON() if they have
> already been seen. Also, I've put a WARN_ON() before sctp_ssnmap_new() in
> sctp_process_init(), in case asoc->ssnmap was not NULL. I've run the lksctp-tools
> suite for v4/v6 and nothing was thrown, also it all passed.
> 
> That said, I think that the !map check is there because we init the asoc first
> with a NULL ssnmap. I suggest, if Dave wants to and if there are no other
> objections, that we could apply to net-next the patches ...
> 
>   * [1/9] http://patchwork.ozlabs.org/patch/237101/
>   * [2/9] http://patchwork.ozlabs.org/patch/237102/
>   * [3/9] http://patchwork.ozlabs.org/patch/237103/
>   * [5/9] http://patchwork.ozlabs.org/patch/237105/
>   * [6/9] http://patchwork.ozlabs.org/patch/237109/
>   * [7/9] http://patchwork.ozlabs.org/patch/237106/
>   * [8/9] http://patchwork.ozlabs.org/patch/237107/
> 
> ... as is. I've just tested it, they apply cleanly on top of each other without
> the missing. Alternatively, I could resend the set without the two that we cut
> out (nr 4 and 9). How you prefer, let me know.
> 
Dave's the final word, but I think the typical workflow is a resend to the list.

Neil