[PATCH] sctp: Clean up type-punning in sctp_cmd_t union

Lots of points in the sctp_cmd_interpreter function treat the sctp_cmd_t arg as
a void pointer, even though they are written as various other types.  Theres no
need for this as doing so just leads to possible type-punning issues that could
cause crashes, and if we remain type-consistent we can actually just remove the
void * member of the union entirely.

Signed-off-by: Neil Horman <nhorman@tuxdriver.com
CC: Vlad Yasevich <redacted>
CC: "David S. Miller" <davem@davemloft.net>
CC: linux-sctp@vger.kernel.org
---
 include/net/sctp/command.h  |  7 ++++---
 include/net/sctp/ulpqueue.h |  2 +-
 net/sctp/sm_sideeffect.c    | 45 ++++++++++++++++++++++-----------------------
 net/sctp/ulpqueue.c         |  3 +--
 4 files changed, 28 insertions(+), 29 deletions(-)

diff --git a/include/net/sctp/command.h b/include/net/sctp/command.h
index 712b3be..7f1b0f3 100644
--- a/include/net/sctp/command.h
+++ b/include/net/sctp/command.h

@@ -131,7 +131,6 @@ typedef union {
 	sctp_state_t state;
 	sctp_event_timeout_t to;
 	unsigned long zero;
-	void *ptr;
 	struct sctp_chunk *chunk;
 	struct sctp_association *asoc;
 	struct sctp_transport *transport;

@@ -154,9 +153,12 @@ typedef union {
  * which takes an __s32 and returns a sctp_arg_t containing the
  * __s32.  So, after foo = SCTP_I32(arg), foo.i32 == arg.
  */
+#define SCTP_NULL_BYTE 0xAA
 static inline sctp_arg_t SCTP_NULL(void)
 {
-	sctp_arg_t retval; retval.ptr = NULL; return retval;
+	sctp_arg_t retval;
+	memset(&retval, SCTP_NULL_BYTE, sizeof(sctp_arg_t));
+	return retval;
 }
 static inline sctp_arg_t SCTP_NOFORCE(void)
 {

@@ -181,7 +183,6 @@ SCTP_ARG_CONSTRUCTOR(ERROR,     int, error)
 SCTP_ARG_CONSTRUCTOR(PERR,      __be16, err)	/* protocol error */
 SCTP_ARG_CONSTRUCTOR(STATE,	sctp_state_t, state)
 SCTP_ARG_CONSTRUCTOR(TO,	sctp_event_timeout_t, to)
-SCTP_ARG_CONSTRUCTOR(PTR,	void *, ptr)
 SCTP_ARG_CONSTRUCTOR(CHUNK,	struct sctp_chunk *, chunk)
 SCTP_ARG_CONSTRUCTOR(ASOC,	struct sctp_association *, asoc)
 SCTP_ARG_CONSTRUCTOR(TRANSPORT,	struct sctp_transport *, transport)

diff --git a/include/net/sctp/ulpqueue.h b/include/net/sctp/ulpqueue.h
index 2e5ee0d..ff1b8ba7 100644
--- a/include/net/sctp/ulpqueue.h
+++ b/include/net/sctp/ulpqueue.h

@@ -72,7 +72,7 @@ int sctp_ulpq_tail_event(struct sctp_ulpq *, struct sctp_ulpevent *ev);
 void sctp_ulpq_renege(struct sctp_ulpq *, struct sctp_chunk *, gfp_t);
 
 /* Perform partial delivery. */
-void sctp_ulpq_partial_delivery(struct sctp_ulpq *, struct sctp_chunk *, gfp_t);
+void sctp_ulpq_partial_delivery(struct sctp_ulpq *, gfp_t);
 
 /* Abort the partial delivery. */
 void sctp_ulpq_abort_pd(struct sctp_ulpq *, gfp_t);

diff --git a/net/sctp/sm_sideeffect.c b/net/sctp/sm_sideeffect.c
index 6773d78..6eecf7e 100644
--- a/net/sctp/sm_sideeffect.c
+++ b/net/sctp/sm_sideeffect.c

@@ -1268,14 +1268,14 @@ static int sctp_cmd_interpreter(sctp_event_t event_type,
 				sctp_outq_uncork(&asoc->outqueue);
 				local_cork = 0;
 			}
-			asoc = cmd->obj.ptr;
+			asoc = cmd->obj.asoc;
 			/* Register with the endpoint.  */
 			sctp_endpoint_add_asoc(ep, asoc);
 			sctp_hash_established(asoc);
 			break;
 
 		case SCTP_CMD_UPDATE_ASSOC:
-		       sctp_assoc_update(asoc, cmd->obj.ptr);
+		       sctp_assoc_update(asoc, cmd->obj.asoc);
 		       break;
 
 		case SCTP_CMD_PURGE_OUTQUEUE:

@@ -1315,7 +1315,7 @@ static int sctp_cmd_interpreter(sctp_event_t event_type,
 			break;
 
 		case SCTP_CMD_PROCESS_FWDTSN:
-			sctp_cmd_process_fwdtsn(&asoc->ulpq, cmd->obj.ptr);
+			sctp_cmd_process_fwdtsn(&asoc->ulpq, cmd->obj.chunk);
 			break;
 
 		case SCTP_CMD_GEN_SACK:

@@ -1331,7 +1331,7 @@ static int sctp_cmd_interpreter(sctp_event_t event_type,
 		case SCTP_CMD_PROCESS_SACK:
 			/* Process an inbound SACK.  */
 			error = sctp_cmd_process_sack(commands, asoc,
-						      cmd->obj.ptr);
+						      cmd->obj.chunk);
 			break;
 
 		case SCTP_CMD_GEN_INIT_ACK:

@@ -1352,15 +1352,15 @@ static int sctp_cmd_interpreter(sctp_event_t event_type,
 			 * layer which will bail.
 			 */
 			error = sctp_cmd_process_init(commands, asoc, chunk,
-						      cmd->obj.ptr, gfp);
+						      cmd->obj.init, gfp);
 			break;
 
 		case SCTP_CMD_GEN_COOKIE_ECHO:
 			/* Generate a COOKIE ECHO chunk.  */
 			new_obj = sctp_make_cookie_echo(asoc, chunk);
 			if (!new_obj) {
-				if (cmd->obj.ptr)
-					sctp_chunk_free(cmd->obj.ptr);
+				if (cmd->obj.chunk)
+					sctp_chunk_free(cmd->obj.chunk);
 				goto nomem;
 			}
 			sctp_add_cmd_sf(commands, SCTP_CMD_REPLY,

@@ -1369,9 +1369,9 @@ static int sctp_cmd_interpreter(sctp_event_t event_type,
 			/* If there is an ERROR chunk to be sent along with
 			 * the COOKIE_ECHO, send it, too.
 			 */
-			if (cmd->obj.ptr)
+			if (cmd->obj.chunk)
 				sctp_add_cmd_sf(commands, SCTP_CMD_REPLY,
-						SCTP_CHUNK(cmd->obj.ptr));
+						SCTP_CHUNK(cmd->obj.chunk));
 
 			if (new_obj->transport) {
 				new_obj->transport->init_sent_count++;

@@ -1417,18 +1417,18 @@ static int sctp_cmd_interpreter(sctp_event_t event_type,
 		case SCTP_CMD_CHUNK_ULP:
 			/* Send a chunk to the sockets layer.  */
 			SCTP_DEBUG_PRINTK("sm_sideff: %s %p, %s %p.\n",
-					  "chunk_up:", cmd->obj.ptr,
+					  "chunk_up:", cmd->obj.chunk,
 					  "ulpq:", &asoc->ulpq);
-			sctp_ulpq_tail_data(&asoc->ulpq, cmd->obj.ptr,
+			sctp_ulpq_tail_data(&asoc->ulpq, cmd->obj.chunk,
 					    GFP_ATOMIC);
 			break;
 
 		case SCTP_CMD_EVENT_ULP:
 			/* Send a notification to the sockets layer.  */
 			SCTP_DEBUG_PRINTK("sm_sideff: %s %p, %s %p.\n",
-					  "event_up:",cmd->obj.ptr,
+					  "event_up:",cmd->obj.ulpevent,
 					  "ulpq:",&asoc->ulpq);
-			sctp_ulpq_tail_event(&asoc->ulpq, cmd->obj.ptr);
+			sctp_ulpq_tail_event(&asoc->ulpq, cmd->obj.ulpevent);
 			break;
 
 		case SCTP_CMD_REPLY:

@@ -1438,12 +1438,12 @@ static int sctp_cmd_interpreter(sctp_event_t event_type,
 				local_cork = 1;
 			}
 			/* Send a chunk to our peer.  */
-			error = sctp_outq_tail(&asoc->outqueue, cmd->obj.ptr);
+			error = sctp_outq_tail(&asoc->outqueue, cmd->obj.chunk);
 			break;
 
 		case SCTP_CMD_SEND_PKT:
 			/* Send a full packet to our peer.  */
-			packet = cmd->obj.ptr;
+			packet = cmd->obj.packet;
 			sctp_packet_transmit(packet);
 			sctp_ootb_pkt_free(packet);
 			break;

@@ -1480,7 +1480,7 @@ static int sctp_cmd_interpreter(sctp_event_t event_type,
 			break;
 
 		case SCTP_CMD_SETUP_T2:
-			sctp_cmd_setup_t2(commands, asoc, cmd->obj.ptr);
+			sctp_cmd_setup_t2(commands, asoc, cmd->obj.chunk);
 			break;
 
 		case SCTP_CMD_TIMER_START_ONCE:

@@ -1514,7 +1514,7 @@ static int sctp_cmd_interpreter(sctp_event_t event_type,
 			break;
 
 		case SCTP_CMD_INIT_CHOOSE_TRANSPORT:
-			chunk = cmd->obj.ptr;
+			chunk = cmd->obj.chunk;
 			t = sctp_assoc_choose_alter_transport(asoc,
 						asoc->init_last_sent_to);
 			asoc->init_last_sent_to = t;

@@ -1665,17 +1665,16 @@ static int sctp_cmd_interpreter(sctp_event_t event_type,
 			break;
 
 		case SCTP_CMD_PART_DELIVER:
-			sctp_ulpq_partial_delivery(&asoc->ulpq, cmd->obj.ptr,
-						   GFP_ATOMIC);
+			sctp_ulpq_partial_delivery(&asoc->ulpq, GFP_ATOMIC);
 			break;
 
 		case SCTP_CMD_RENEGE:
-			sctp_ulpq_renege(&asoc->ulpq, cmd->obj.ptr,
+			sctp_ulpq_renege(&asoc->ulpq, cmd->obj.chunk,
 					 GFP_ATOMIC);
 			break;
 
 		case SCTP_CMD_SETUP_T4:
-			sctp_cmd_setup_t4(commands, asoc, cmd->obj.ptr);
+			sctp_cmd_setup_t4(commands, asoc, cmd->obj.chunk);
 			break;
 
 		case SCTP_CMD_PROCESS_OPERR:

@@ -1734,8 +1733,8 @@ static int sctp_cmd_interpreter(sctp_event_t event_type,
 			break;
 
 		default:
-			pr_warn("Impossible command: %u, %p\n",
-				cmd->verb, cmd->obj.ptr);
+			pr_warn("Impossible command: %u\n",
+				cmd->verb);
 			break;
 		}
 

diff --git a/net/sctp/ulpqueue.c b/net/sctp/ulpqueue.c
index 360d869..ada1746 100644
--- a/net/sctp/ulpqueue.c
+++ b/net/sctp/ulpqueue.c

@@ -997,7 +997,6 @@ static __u16 sctp_ulpq_renege_frags(struct sctp_ulpq *ulpq, __u16 needed)
 
 /* Partial deliver the first message as there is pressure on rwnd. */
 void sctp_ulpq_partial_delivery(struct sctp_ulpq *ulpq,
-				struct sctp_chunk *chunk,
 				gfp_t gfp)
 {
 	struct sctp_ulpevent *event;

@@ -1060,7 +1059,7 @@ void sctp_ulpq_renege(struct sctp_ulpq *ulpq, struct sctp_chunk *chunk,
 		sctp_tsnmap_mark(&asoc->peer.tsn_map, tsn, chunk->transport);
 		sctp_ulpq_tail_data(ulpq, chunk, gfp);
 
-		sctp_ulpq_partial_delivery(ulpq, chunk, gfp);
+		sctp_ulpq_partial_delivery(ulpq, gfp);
 	}
 
 	sk_mem_reclaim(asoc->base.sk);

-- 
1.7.11.7

On 10/25/2012 04:47 PM, Neil Horman wrote:

What's this for?  Can't we just use retval.zero?

-vlad

On Thu, Oct 25, 2012 at 05:42:15PM -0400, Vlad Yasevich wrote:

On 10/25/2012 04:47 PM, Neil Horman wrote:

quoted

Lots of points in the sctp_cmd_interpreter function treat the sctp_cmd_t arg as
a void pointer, even though they are written as various other types.  Theres no
need for this as doing so just leads to possible type-punning issues that could
cause crashes, and if we remain type-consistent we can actually just remove the
void * member of the union entirely.

Signed-off-by: Neil Horman <nhorman@tuxdriver.com
CC: Vlad Yasevich <redacted>
CC: "David S. Miller" <davem@davemloft.net>
CC: linux-sctp@vger.kernel.org
---
 include/net/sctp/command.h  |  7 ++++---
 include/net/sctp/ulpqueue.h |  2 +-
 net/sctp/sm_sideeffect.c    | 45 ++++++++++++++++++++++-----------------------
 net/sctp/ulpqueue.c         |  3 +--
 4 files changed, 28 insertions(+), 29 deletions(-)

diff --git a/include/net/sctp/command.h b/include/net/sctp/command.h
index 712b3be..7f1b0f3 100644
--- a/include/net/sctp/command.h
+++ b/include/net/sctp/command.h

@@ -131,7 +131,6 @@ typedef union {
 	sctp_state_t state;
 	sctp_event_timeout_t to;
 	unsigned long zero;
-	void *ptr;
 	struct sctp_chunk *chunk;
 	struct sctp_association *asoc;
 	struct sctp_transport *transport;

@@ -154,9 +153,12 @@ typedef union {
  * which takes an __s32 and returns a sctp_arg_t containing the
  * __s32.  So, after foo = SCTP_I32(arg), foo.i32 == arg.
  */
+#define SCTP_NULL_BYTE 0xAA
 static inline sctp_arg_t SCTP_NULL(void)
 {
-	sctp_arg_t retval; retval.ptr = NULL; return retval;
+	sctp_arg_t retval;
+	memset(&retval, SCTP_NULL_BYTE, sizeof(sctp_arg_t));
+	return retval;

What's this for?  Can't we just use retval.zero?

-vlad

My intent was to highlight any users of sctp_arg_t when SCTP_NULL was passed.
My thinking was that the 0xAA byte patern would be a good indicator.  Although,
admittedly I didn't see the zero argument there.  Looking at it though, the zero
member of the union is effectively unused.  Strictly speaking its used for
initalization of sctp_arg_t, but its done somewhat poorly, since theres no
guarantee that an unsigned long will be the largest member of that union.  Doing
the memset guarantees the whole instance is set to a predefined value.

I could go either way with this, would you rather we just have SCTP_NULL return
retval = { .zero = 0}; or would you rather remove the zero initialization from
SCTP_[NO]FORCE, and SCTP_ARG_CONSTRUCTOR and do the memset.  I think the memset
reduces to a single 64 bit assignment as long as the union doesn't exceed that
size anyway, and it ensures that you initalize the whole union's storage if it
does in the future.  And if we remove the initialization step (I don't see that
its needed in the three macros above anyway), then we can remove the zero member
as well.

Let me know what you want to do here, and I can respin this.
Best
Neil

On 10/25/2012 07:58 PM, Neil Horman wrote:

On Thu, Oct 25, 2012 at 05:42:15PM -0400, Vlad Yasevich wrote:

quoted

On 10/25/2012 04:47 PM, Neil Horman wrote:

quoted

Lots of points in the sctp_cmd_interpreter function treat the sctp_cmd_t arg as
a void pointer, even though they are written as various other types.  Theres no
need for this as doing so just leads to possible type-punning issues that could
cause crashes, and if we remain type-consistent we can actually just remove the
void * member of the union entirely.

Signed-off-by: Neil Horman <nhorman@tuxdriver.com
CC: Vlad Yasevich <redacted>
CC: "David S. Miller" <davem@davemloft.net>
CC: linux-sctp@vger.kernel.org
---
  include/net/sctp/command.h  |  7 ++++---
  include/net/sctp/ulpqueue.h |  2 +-
  net/sctp/sm_sideeffect.c    | 45 ++++++++++++++++++++++-----------------------
  net/sctp/ulpqueue.c         |  3 +--
  4 files changed, 28 insertions(+), 29 deletions(-)

diff --git a/include/net/sctp/command.h b/include/net/sctp/command.h
index 712b3be..7f1b0f3 100644
--- a/include/net/sctp/command.h
+++ b/include/net/sctp/command.h

@@ -131,7 +131,6 @@ typedef union {
  	sctp_state_t state;
  	sctp_event_timeout_t to;
  	unsigned long zero;
-	void *ptr;
  	struct sctp_chunk *chunk;
  	struct sctp_association *asoc;
  	struct sctp_transport *transport;

@@ -154,9 +153,12 @@ typedef union {
   * which takes an __s32 and returns a sctp_arg_t containing the
   * __s32.  So, after foo = SCTP_I32(arg), foo.i32 == arg.
   */
+#define SCTP_NULL_BYTE 0xAA
  static inline sctp_arg_t SCTP_NULL(void)
  {
-	sctp_arg_t retval; retval.ptr = NULL; return retval;
+	sctp_arg_t retval;
+	memset(&retval, SCTP_NULL_BYTE, sizeof(sctp_arg_t));
+	return retval;

What's this for?  Can't we just use retval.zero?

-vlad

My intent was to highlight any users of sctp_arg_t when SCTP_NULL was passed.
My thinking was that the 0xAA byte patern would be a good indicator.  Although,
admittedly I didn't see the zero argument there.  Looking at it though, the zero
member of the union is effectively unused.  Strictly speaking its used for
initalization of sctp_arg_t, but its done somewhat poorly, since theres no
guarantee that an unsigned long will be the largest member of that union.  Doing
the memset guarantees the whole instance is set to a predefined value.

I could go either way with this, would you rather we just have SCTP_NULL return
retval = { .zero = 0}; or would you rather remove the zero initialization from
SCTP_[NO]FORCE, and SCTP_ARG_CONSTRUCTOR and do the memset.  I think the memset
reduces to a single 64 bit assignment as long as the union doesn't exceed that
size anyway, and it ensures that you initalize the whole union's storage if it
does in the future.  And if we remove the initialization step (I don't see that
its needed in the three macros above anyway), then we can remove the zero member
as well.

You need the initialization step, otherwise things might fail (they did on IA64 a while back).  That's why the zero member was added.  You can go with memset if you want, but I was primarily wondering why the 0xAA pattern was there.

-vlad

Let me know what you want to do here, and I can respin this.
Best
Neil

Subject: [PATCH] sctp: Clean up type-punning in sctp_cmd_t union

...

You really don't want to be taking the address of a local
variable that would normally be held in a register.
It stops the compiler doing a lot of optimisations.
In this case the structure is also being returned by value,
not nice except that all(?) modern ABI do pass small structures
in registers.

An assignment of some member of the union to NULL would
seem most appropriate.
OTOH the code that uses this must be someones 'bright idea (tm)'
that probably wasn't such a good idea after all.

	David

On Thu, Oct 25, 2012 at 11:48:16PM -0400, Vlad Yasevich wrote:

On 10/25/2012 07:58 PM, Neil Horman wrote:

quoted

On Thu, Oct 25, 2012 at 05:42:15PM -0400, Vlad Yasevich wrote:

quoted

On 10/25/2012 04:47 PM, Neil Horman wrote:

quoted

Lots of points in the sctp_cmd_interpreter function treat the sctp_cmd_t arg as
a void pointer, even though they are written as various other types.  Theres no
need for this as doing so just leads to possible type-punning issues that could
cause crashes, and if we remain type-consistent we can actually just remove the
void * member of the union entirely.

Signed-off-by: Neil Horman <nhorman@tuxdriver.com
CC: Vlad Yasevich <redacted>
CC: "David S. Miller" <davem@davemloft.net>
CC: linux-sctp@vger.kernel.org
---
 include/net/sctp/command.h  |  7 ++++---
 include/net/sctp/ulpqueue.h |  2 +-
 net/sctp/sm_sideeffect.c    | 45 ++++++++++++++++++++++-----------------------
 net/sctp/ulpqueue.c         |  3 +--
 4 files changed, 28 insertions(+), 29 deletions(-)

diff --git a/include/net/sctp/command.h b/include/net/sctp/command.h
index 712b3be..7f1b0f3 100644
--- a/include/net/sctp/command.h
+++ b/include/net/sctp/command.h

@@ -131,7 +131,6 @@ typedef union {
 	sctp_state_t state;
 	sctp_event_timeout_t to;
 	unsigned long zero;
-	void *ptr;
 	struct sctp_chunk *chunk;
 	struct sctp_association *asoc;
 	struct sctp_transport *transport;

@@ -154,9 +153,12 @@ typedef union {
  * which takes an __s32 and returns a sctp_arg_t containing the
  * __s32.  So, after foo = SCTP_I32(arg), foo.i32 == arg.
  */
+#define SCTP_NULL_BYTE 0xAA
 static inline sctp_arg_t SCTP_NULL(void)
 {
-	sctp_arg_t retval; retval.ptr = NULL; return retval;
+	sctp_arg_t retval;
+	memset(&retval, SCTP_NULL_BYTE, sizeof(sctp_arg_t));
+	return retval;

What's this for?  Can't we just use retval.zero?

-vlad

My intent was to highlight any users of sctp_arg_t when SCTP_NULL was passed.
My thinking was that the 0xAA byte patern would be a good indicator.  Although,
admittedly I didn't see the zero argument there.  Looking at it though, the zero
member of the union is effectively unused.  Strictly speaking its used for
initalization of sctp_arg_t, but its done somewhat poorly, since theres no
guarantee that an unsigned long will be the largest member of that union.  Doing
the memset guarantees the whole instance is set to a predefined value.

I could go either way with this, would you rather we just have SCTP_NULL return
retval = { .zero = 0}; or would you rather remove the zero initialization from
SCTP_[NO]FORCE, and SCTP_ARG_CONSTRUCTOR and do the memset.  I think the memset
reduces to a single 64 bit assignment as long as the union doesn't exceed that
size anyway, and it ensures that you initalize the whole union's storage if it
does in the future.  And if we remove the initialization step (I don't see that
its needed in the three macros above anyway), then we can remove the zero member
as well.

You need the initialization step, otherwise things might fail (they
did on IA64 a while back).  That's why the zero member was added.
You can go with memset if you want, but I was primarily wondering
why the 0xAA pattern was there.

The AA I did was just meant as a pattern marker, so that, should someone use an
instance of sctp_arg_t that was passed in as SCTP_NULL(), it would be visually
obvious in the stack trace, but I suppose its not really needed given that NULL
is equally clear.  And since Dave pointed out the lack of optimization
opportunity when using a store to an address rather than a register, I think I
should probably just revert it and use zero as you initially suggested.

The need for the initalization in SCTP_[NO]FORCE and SCTP_ARG_CONSTRUCTOR
concerns me though.  All its doing is setting part of the storage to zero, and
then overwriting it again with whatever type spcific member you're assigning
from the corresponding SCTP_* macro.  That kind of sounds to me like ia64 might
have fallen to some amount of type-punning problem.  do you have a link to
discussion about that problem?

Regards
Neil

-vlad

quoted

Let me know what you want to do here, and I can respin this.
Best
Neil

On Fri, Oct 26, 2012 at 10:00:13AM +0100, David Laight wrote:

quoted

Subject: [PATCH] sctp: Clean up type-punning in sctp_cmd_t union

...

quoted

+#define SCTP_NULL_BYTE 0xAA
 static inline sctp_arg_t SCTP_NULL(void)
 {
-	sctp_arg_t retval; retval.ptr = NULL; return retval;
+	sctp_arg_t retval;
+	memset(&retval, SCTP_NULL_BYTE, sizeof(sctp_arg_t));
+	return retval;
 }

You really don't want to be taking the address of a local
variable that would normally be held in a register.
It stops the compiler doing a lot of optimisations.
In this case the structure is also being returned by value,
not nice except that all(?) modern ABI do pass small structures
in registers.

Thank you dave, I hadn't considered the implications of missed optimizations
when storing to a register here.

An assignment of some member of the union to NULL would
seem most appropriate.
OTOH the code that uses this must be someones 'bright idea (tm)'
that probably wasn't such a good idea after all.

nobody uses it currently, Its just that some state machine side effects don't
require an argument, but you still have to pass one in, so I was trying to
enhance this to make it obvious in a backtrace should someone inadvertently use
it in the future.  I'm going to revert it though, a big fat zero is probably
just as clear.
Neil

	David

Lots of points in the sctp_cmd_interpreter function treat the sctp_cmd_t arg as
a void pointer, even though they are written as various other types.  Theres no
need for this as doing so just leads to possible type-punning issues that could
cause crashes, and if we remain type-consistent we can actually just remove the
void * member of the union entirely.

Change Notes:

v2)
	* Dropped chunk that modified SCTP_NULL to create a marker pattern
	 should anyone try to use a SCTP_NULL() assigned sctp_arg_t, Assigning
	 to .zero provides the same effect and should be faster, per Vlad Y.

Signed-off-by: Neil Horman <nhorman@tuxdriver.com
CC: Vlad Yasevich <redacted>
CC: "David S. Miller" <davem@davemloft.net>
CC: linux-sctp@vger.kernel.org
---
 include/net/sctp/command.h  |  4 +---
 include/net/sctp/ulpqueue.h |  2 +-
 net/sctp/sm_sideeffect.c    | 45 ++++++++++++++++++++++-----------------------
 net/sctp/ulpqueue.c         |  3 +--
 4 files changed, 25 insertions(+), 29 deletions(-)

diff --git a/include/net/sctp/command.h b/include/net/sctp/command.h
index 712b3be..8d52cd7 100644
--- a/include/net/sctp/command.h
+++ b/include/net/sctp/command.h

@@ -131,7 +131,6 @@ typedef union {
 	sctp_state_t state;
 	sctp_event_timeout_t to;
 	unsigned long zero;
-	void *ptr;
 	struct sctp_chunk *chunk;
 	struct sctp_association *asoc;
 	struct sctp_transport *transport;

@@ -156,7 +155,7 @@ typedef union {
  */
 static inline sctp_arg_t SCTP_NULL(void)
 {
-	sctp_arg_t retval; retval.ptr = NULL; return retval;
+	sctp_arg_t retval = { .zero = 0UL}; return retval;
 }
 static inline sctp_arg_t SCTP_NOFORCE(void)
 {

@@ -181,7 +180,6 @@ SCTP_ARG_CONSTRUCTOR(ERROR,     int, error)
 SCTP_ARG_CONSTRUCTOR(PERR,      __be16, err)	/* protocol error */
 SCTP_ARG_CONSTRUCTOR(STATE,	sctp_state_t, state)
 SCTP_ARG_CONSTRUCTOR(TO,	sctp_event_timeout_t, to)
-SCTP_ARG_CONSTRUCTOR(PTR,	void *, ptr)
 SCTP_ARG_CONSTRUCTOR(CHUNK,	struct sctp_chunk *, chunk)
 SCTP_ARG_CONSTRUCTOR(ASOC,	struct sctp_association *, asoc)
 SCTP_ARG_CONSTRUCTOR(TRANSPORT,	struct sctp_transport *, transport)

diff --git a/include/net/sctp/ulpqueue.h b/include/net/sctp/ulpqueue.h
index 2e5ee0d..ff1b8ba7 100644
--- a/include/net/sctp/ulpqueue.h
+++ b/include/net/sctp/ulpqueue.h

@@ -72,7 +72,7 @@ int sctp_ulpq_tail_event(struct sctp_ulpq *, struct sctp_ulpevent *ev);
 void sctp_ulpq_renege(struct sctp_ulpq *, struct sctp_chunk *, gfp_t);
 
 /* Perform partial delivery. */
-void sctp_ulpq_partial_delivery(struct sctp_ulpq *, struct sctp_chunk *, gfp_t);
+void sctp_ulpq_partial_delivery(struct sctp_ulpq *, gfp_t);
 
 /* Abort the partial delivery. */
 void sctp_ulpq_abort_pd(struct sctp_ulpq *, gfp_t);

diff --git a/net/sctp/sm_sideeffect.c b/net/sctp/sm_sideeffect.c
index 6773d78..6eecf7e 100644
--- a/net/sctp/sm_sideeffect.c
+++ b/net/sctp/sm_sideeffect.c

@@ -1268,14 +1268,14 @@ static int sctp_cmd_interpreter(sctp_event_t event_type,
 				sctp_outq_uncork(&asoc->outqueue);
 				local_cork = 0;
 			}
-			asoc = cmd->obj.ptr;
+			asoc = cmd->obj.asoc;
 			/* Register with the endpoint.  */
 			sctp_endpoint_add_asoc(ep, asoc);
 			sctp_hash_established(asoc);
 			break;
 
 		case SCTP_CMD_UPDATE_ASSOC:
-		       sctp_assoc_update(asoc, cmd->obj.ptr);
+		       sctp_assoc_update(asoc, cmd->obj.asoc);
 		       break;
 
 		case SCTP_CMD_PURGE_OUTQUEUE:

@@ -1315,7 +1315,7 @@ static int sctp_cmd_interpreter(sctp_event_t event_type,
 			break;
 
 		case SCTP_CMD_PROCESS_FWDTSN:
-			sctp_cmd_process_fwdtsn(&asoc->ulpq, cmd->obj.ptr);
+			sctp_cmd_process_fwdtsn(&asoc->ulpq, cmd->obj.chunk);
 			break;
 
 		case SCTP_CMD_GEN_SACK:

@@ -1331,7 +1331,7 @@ static int sctp_cmd_interpreter(sctp_event_t event_type,
 		case SCTP_CMD_PROCESS_SACK:
 			/* Process an inbound SACK.  */
 			error = sctp_cmd_process_sack(commands, asoc,
-						      cmd->obj.ptr);
+						      cmd->obj.chunk);
 			break;
 
 		case SCTP_CMD_GEN_INIT_ACK:

@@ -1352,15 +1352,15 @@ static int sctp_cmd_interpreter(sctp_event_t event_type,
 			 * layer which will bail.
 			 */
 			error = sctp_cmd_process_init(commands, asoc, chunk,
-						      cmd->obj.ptr, gfp);
+						      cmd->obj.init, gfp);
 			break;
 
 		case SCTP_CMD_GEN_COOKIE_ECHO:
 			/* Generate a COOKIE ECHO chunk.  */
 			new_obj = sctp_make_cookie_echo(asoc, chunk);
 			if (!new_obj) {
-				if (cmd->obj.ptr)
-					sctp_chunk_free(cmd->obj.ptr);
+				if (cmd->obj.chunk)
+					sctp_chunk_free(cmd->obj.chunk);
 				goto nomem;
 			}
 			sctp_add_cmd_sf(commands, SCTP_CMD_REPLY,

@@ -1369,9 +1369,9 @@ static int sctp_cmd_interpreter(sctp_event_t event_type,
 			/* If there is an ERROR chunk to be sent along with
 			 * the COOKIE_ECHO, send it, too.
 			 */
-			if (cmd->obj.ptr)
+			if (cmd->obj.chunk)
 				sctp_add_cmd_sf(commands, SCTP_CMD_REPLY,
-						SCTP_CHUNK(cmd->obj.ptr));
+						SCTP_CHUNK(cmd->obj.chunk));
 
 			if (new_obj->transport) {
 				new_obj->transport->init_sent_count++;

@@ -1417,18 +1417,18 @@ static int sctp_cmd_interpreter(sctp_event_t event_type,
 		case SCTP_CMD_CHUNK_ULP:
 			/* Send a chunk to the sockets layer.  */
 			SCTP_DEBUG_PRINTK("sm_sideff: %s %p, %s %p.\n",
-					  "chunk_up:", cmd->obj.ptr,
+					  "chunk_up:", cmd->obj.chunk,
 					  "ulpq:", &asoc->ulpq);
-			sctp_ulpq_tail_data(&asoc->ulpq, cmd->obj.ptr,
+			sctp_ulpq_tail_data(&asoc->ulpq, cmd->obj.chunk,
 					    GFP_ATOMIC);
 			break;
 
 		case SCTP_CMD_EVENT_ULP:
 			/* Send a notification to the sockets layer.  */
 			SCTP_DEBUG_PRINTK("sm_sideff: %s %p, %s %p.\n",
-					  "event_up:",cmd->obj.ptr,
+					  "event_up:",cmd->obj.ulpevent,
 					  "ulpq:",&asoc->ulpq);
-			sctp_ulpq_tail_event(&asoc->ulpq, cmd->obj.ptr);
+			sctp_ulpq_tail_event(&asoc->ulpq, cmd->obj.ulpevent);
 			break;
 
 		case SCTP_CMD_REPLY:

@@ -1438,12 +1438,12 @@ static int sctp_cmd_interpreter(sctp_event_t event_type,
 				local_cork = 1;
 			}
 			/* Send a chunk to our peer.  */
-			error = sctp_outq_tail(&asoc->outqueue, cmd->obj.ptr);
+			error = sctp_outq_tail(&asoc->outqueue, cmd->obj.chunk);
 			break;
 
 		case SCTP_CMD_SEND_PKT:
 			/* Send a full packet to our peer.  */
-			packet = cmd->obj.ptr;
+			packet = cmd->obj.packet;
 			sctp_packet_transmit(packet);
 			sctp_ootb_pkt_free(packet);
 			break;

@@ -1480,7 +1480,7 @@ static int sctp_cmd_interpreter(sctp_event_t event_type,
 			break;
 
 		case SCTP_CMD_SETUP_T2:
-			sctp_cmd_setup_t2(commands, asoc, cmd->obj.ptr);
+			sctp_cmd_setup_t2(commands, asoc, cmd->obj.chunk);
 			break;
 
 		case SCTP_CMD_TIMER_START_ONCE:

@@ -1514,7 +1514,7 @@ static int sctp_cmd_interpreter(sctp_event_t event_type,
 			break;
 
 		case SCTP_CMD_INIT_CHOOSE_TRANSPORT:
-			chunk = cmd->obj.ptr;
+			chunk = cmd->obj.chunk;
 			t = sctp_assoc_choose_alter_transport(asoc,
 						asoc->init_last_sent_to);
 			asoc->init_last_sent_to = t;

@@ -1665,17 +1665,16 @@ static int sctp_cmd_interpreter(sctp_event_t event_type,
 			break;
 
 		case SCTP_CMD_PART_DELIVER:
-			sctp_ulpq_partial_delivery(&asoc->ulpq, cmd->obj.ptr,
-						   GFP_ATOMIC);
+			sctp_ulpq_partial_delivery(&asoc->ulpq, GFP_ATOMIC);
 			break;
 
 		case SCTP_CMD_RENEGE:
-			sctp_ulpq_renege(&asoc->ulpq, cmd->obj.ptr,
+			sctp_ulpq_renege(&asoc->ulpq, cmd->obj.chunk,
 					 GFP_ATOMIC);
 			break;
 
 		case SCTP_CMD_SETUP_T4:
-			sctp_cmd_setup_t4(commands, asoc, cmd->obj.ptr);
+			sctp_cmd_setup_t4(commands, asoc, cmd->obj.chunk);
 			break;
 
 		case SCTP_CMD_PROCESS_OPERR:

@@ -1734,8 +1733,8 @@ static int sctp_cmd_interpreter(sctp_event_t event_type,
 			break;
 
 		default:
-			pr_warn("Impossible command: %u, %p\n",
-				cmd->verb, cmd->obj.ptr);
+			pr_warn("Impossible command: %u\n",
+				cmd->verb);
 			break;
 		}
 

diff --git a/net/sctp/ulpqueue.c b/net/sctp/ulpqueue.c
index 360d869..ada1746 100644
--- a/net/sctp/ulpqueue.c
+++ b/net/sctp/ulpqueue.c

@@ -997,7 +997,6 @@ static __u16 sctp_ulpq_renege_frags(struct sctp_ulpq *ulpq, __u16 needed)
 
 /* Partial deliver the first message as there is pressure on rwnd. */
 void sctp_ulpq_partial_delivery(struct sctp_ulpq *ulpq,
-				struct sctp_chunk *chunk,
 				gfp_t gfp)
 {
 	struct sctp_ulpevent *event;

@@ -1060,7 +1059,7 @@ void sctp_ulpq_renege(struct sctp_ulpq *ulpq, struct sctp_chunk *chunk,
 		sctp_tsnmap_mark(&asoc->peer.tsn_map, tsn, chunk->transport);
 		sctp_ulpq_tail_data(ulpq, chunk, gfp);
 
-		sctp_ulpq_partial_delivery(ulpq, chunk, gfp);
+		sctp_ulpq_partial_delivery(ulpq, gfp);
 	}
 
 	sk_mem_reclaim(asoc->base.sk);

-- 
1.7.11.7

On 10/26/2012 09:24 AM, Neil Horman wrote:

On Thu, Oct 25, 2012 at 11:48:16PM -0400, Vlad Yasevich wrote:

quoted

On 10/25/2012 07:58 PM, Neil Horman wrote:

quoted

On Thu, Oct 25, 2012 at 05:42:15PM -0400, Vlad Yasevich wrote:

quoted

On 10/25/2012 04:47 PM, Neil Horman wrote:

quoted

Lots of points in the sctp_cmd_interpreter function treat the sctp_cmd_t arg as
a void pointer, even though they are written as various other types.  Theres no
need for this as doing so just leads to possible type-punning issues that could
cause crashes, and if we remain type-consistent we can actually just remove the
void * member of the union entirely.

Signed-off-by: Neil Horman <nhorman@tuxdriver.com
CC: Vlad Yasevich <redacted>
CC: "David S. Miller" <davem@davemloft.net>
CC: linux-sctp@vger.kernel.org
---
  include/net/sctp/command.h  |  7 ++++---
  include/net/sctp/ulpqueue.h |  2 +-
  net/sctp/sm_sideeffect.c    | 45 ++++++++++++++++++++++-----------------------
  net/sctp/ulpqueue.c         |  3 +--
  4 files changed, 28 insertions(+), 29 deletions(-)

diff --git a/include/net/sctp/command.h b/include/net/sctp/command.h
index 712b3be..7f1b0f3 100644
--- a/include/net/sctp/command.h
+++ b/include/net/sctp/command.h

@@ -131,7 +131,6 @@ typedef union {
  	sctp_state_t state;
  	sctp_event_timeout_t to;
  	unsigned long zero;
-	void *ptr;
  	struct sctp_chunk *chunk;
  	struct sctp_association *asoc;
  	struct sctp_transport *transport;

@@ -154,9 +153,12 @@ typedef union {
   * which takes an __s32 and returns a sctp_arg_t containing the
   * __s32.  So, after foo = SCTP_I32(arg), foo.i32 == arg.
   */
+#define SCTP_NULL_BYTE 0xAA
  static inline sctp_arg_t SCTP_NULL(void)
  {
-	sctp_arg_t retval; retval.ptr = NULL; return retval;
+	sctp_arg_t retval;
+	memset(&retval, SCTP_NULL_BYTE, sizeof(sctp_arg_t));
+	return retval;

What's this for?  Can't we just use retval.zero?

-vlad

My intent was to highlight any users of sctp_arg_t when SCTP_NULL was passed.
My thinking was that the 0xAA byte patern would be a good indicator.  Although,
admittedly I didn't see the zero argument there.  Looking at it though, the zero
member of the union is effectively unused.  Strictly speaking its used for
initalization of sctp_arg_t, but its done somewhat poorly, since theres no
guarantee that an unsigned long will be the largest member of that union.  Doing
the memset guarantees the whole instance is set to a predefined value.

I could go either way with this, would you rather we just have SCTP_NULL return
retval = { .zero = 0}; or would you rather remove the zero initialization from
SCTP_[NO]FORCE, and SCTP_ARG_CONSTRUCTOR and do the memset.  I think the memset
reduces to a single 64 bit assignment as long as the union doesn't exceed that
size anyway, and it ensures that you initalize the whole union's storage if it
does in the future.  And if we remove the initialization step (I don't see that
its needed in the three macros above anyway), then we can remove the zero member
as well.

You need the initialization step, otherwise things might fail (they
did on IA64 a while back).  That's why the zero member was added.
You can go with memset if you want, but I was primarily wondering
why the 0xAA pattern was there.

The AA I did was just meant as a pattern marker, so that, should someone use an
instance of sctp_arg_t that was passed in as SCTP_NULL(), it would be visually
obvious in the stack trace, but I suppose its not really needed given that NULL
is equally clear.  And since Dave pointed out the lack of optimization
opportunity when using a store to an address rather than a register, I think I
should probably just revert it and use zero as you initially suggested.

The need for the initalization in SCTP_[NO]FORCE and SCTP_ARG_CONSTRUCTOR
concerns me though.  All its doing is setting part of the storage to zero, and
then overwriting it again with whatever type spcific member you're assigning
from the corresponding SCTP_* macro.  That kind of sounds to me like ia64 might
have fallen to some amount of type-punning problem.  do you have a link to
discussion about that problem?

Look at commit 19c7e9ee that introduced this.  I don't remember all the details any more, but the problem only occurred on ia64 (probably due its speculative load handling).

-vlad

Regards
Neil

quoted

-vlad

quoted

Let me know what you want to do here, and I can respin this.
Best
Neil

On 10/26/2012 09:41 AM, Neil Horman wrote:

Lots of points in the sctp_cmd_interpreter function treat the sctp_cmd_t arg as
a void pointer, even though they are written as various other types.  Theres no
need for this as doing so just leads to possible type-punning issues that could
cause crashes, and if we remain type-consistent we can actually just remove the
void * member of the union entirely.

Change Notes:

v2)
	* Dropped chunk that modified SCTP_NULL to create a marker pattern
	 should anyone try to use a SCTP_NULL() assigned sctp_arg_t, Assigning
	 to .zero provides the same effect and should be faster, per Vlad Y.

Signed-off-by: Neil Horman <nhorman@tuxdriver.com
CC: Vlad Yasevich <redacted>

Acked-by: Vlad Yasevich <redacted>

-vlad

On Fri, Oct 26, 2012 at 03:12:11PM -0400, Vlad Yasevich wrote:

On 10/26/2012 09:24 AM, Neil Horman wrote:

quoted

On Thu, Oct 25, 2012 at 11:48:16PM -0400, Vlad Yasevich wrote:

quoted

On 10/25/2012 07:58 PM, Neil Horman wrote:

quoted

On Thu, Oct 25, 2012 at 05:42:15PM -0400, Vlad Yasevich wrote:

quoted

On 10/25/2012 04:47 PM, Neil Horman wrote:

quoted

Lots of points in the sctp_cmd_interpreter function treat the sctp_cmd_t arg as
a void pointer, even though they are written as various other types.  Theres no
need for this as doing so just leads to possible type-punning issues that could
cause crashes, and if we remain type-consistent we can actually just remove the
void * member of the union entirely.

Signed-off-by: Neil Horman <nhorman@tuxdriver.com
CC: Vlad Yasevich <redacted>
CC: "David S. Miller" <davem@davemloft.net>
CC: linux-sctp@vger.kernel.org
---
 include/net/sctp/command.h  |  7 ++++---
 include/net/sctp/ulpqueue.h |  2 +-
 net/sctp/sm_sideeffect.c    | 45 ++++++++++++++++++++++-----------------------
 net/sctp/ulpqueue.c         |  3 +--
 4 files changed, 28 insertions(+), 29 deletions(-)

diff --git a/include/net/sctp/command.h b/include/net/sctp/command.h
index 712b3be..7f1b0f3 100644
--- a/include/net/sctp/command.h
+++ b/include/net/sctp/command.h

@@ -131,7 +131,6 @@ typedef union {
 	sctp_state_t state;
 	sctp_event_timeout_t to;
 	unsigned long zero;
-	void *ptr;
 	struct sctp_chunk *chunk;
 	struct sctp_association *asoc;
 	struct sctp_transport *transport;

@@ -154,9 +153,12 @@ typedef union {
  * which takes an __s32 and returns a sctp_arg_t containing the
  * __s32.  So, after foo = SCTP_I32(arg), foo.i32 == arg.
  */
+#define SCTP_NULL_BYTE 0xAA
 static inline sctp_arg_t SCTP_NULL(void)
 {
-	sctp_arg_t retval; retval.ptr = NULL; return retval;
+	sctp_arg_t retval;
+	memset(&retval, SCTP_NULL_BYTE, sizeof(sctp_arg_t));
+	return retval;

What's this for?  Can't we just use retval.zero?

-vlad

My intent was to highlight any users of sctp_arg_t when SCTP_NULL was passed.
My thinking was that the 0xAA byte patern would be a good indicator.  Although,
admittedly I didn't see the zero argument there.  Looking at it though, the zero
member of the union is effectively unused.  Strictly speaking its used for
initalization of sctp_arg_t, but its done somewhat poorly, since theres no
guarantee that an unsigned long will be the largest member of that union.  Doing
the memset guarantees the whole instance is set to a predefined value.

I could go either way with this, would you rather we just have SCTP_NULL return
retval = { .zero = 0}; or would you rather remove the zero initialization from
SCTP_[NO]FORCE, and SCTP_ARG_CONSTRUCTOR and do the memset.  I think the memset
reduces to a single 64 bit assignment as long as the union doesn't exceed that
size anyway, and it ensures that you initalize the whole union's storage if it
does in the future.  And if we remove the initialization step (I don't see that
its needed in the three macros above anyway), then we can remove the zero member
as well.

You need the initialization step, otherwise things might fail (they
did on IA64 a while back).  That's why the zero member was added.
You can go with memset if you want, but I was primarily wondering
why the 0xAA pattern was there.

The AA I did was just meant as a pattern marker, so that, should someone use an
instance of sctp_arg_t that was passed in as SCTP_NULL(), it would be visually
obvious in the stack trace, but I suppose its not really needed given that NULL
is equally clear.  And since Dave pointed out the lack of optimization
opportunity when using a store to an address rather than a register, I think I
should probably just revert it and use zero as you initially suggested.

The need for the initalization in SCTP_[NO]FORCE and SCTP_ARG_CONSTRUCTOR
concerns me though.  All its doing is setting part of the storage to zero, and
then overwriting it again with whatever type spcific member you're assigning
from the corresponding SCTP_* macro.  That kind of sounds to me like ia64 might
have fallen to some amount of type-punning problem.  do you have a link to
discussion about that problem?

Look at commit 19c7e9ee that introduced this.  I don't remember all
the details any more, but the problem only occurred on ia64
(probably due its speculative load handling).

-vlad

Thanks Vlad, I'll have a look see.
Regards
Neil

From: Neil Horman <nhorman@tuxdriver.com>
Date: Fri, 26 Oct 2012 16:35:04 -0400

On Fri, Oct 26, 2012 at 03:12:11PM -0400, Vlad Yasevich wrote:

quoted

Look at commit 19c7e9ee that introduced this.  I don't remember all
the details any more, but the problem only occurred on ia64
(probably due its speculative load handling).

-vlad

Thanks Vlad, I'll have a look see.

Ok, so this IA64 issue is all about accesses to uninitialized memory.

I think Neil's change is thus the most desirable thing to do.  Simple
memset the object to zero.

Let the compiler optimize or not optimize things as it sees fit, to
make sure the object is completely initialized.

memset() expands to __builtin_memset(), and therefore the compiler
can and will eliminate initializations to overlapping areas if such
eliminations are possible.

On Fri, Oct 26, 2012 at 05:10:19PM -0400, David Miller wrote:

From: Neil Horman <nhorman@tuxdriver.com>
Date: Fri, 26 Oct 2012 16:35:04 -0400

quoted

On Fri, Oct 26, 2012 at 03:12:11PM -0400, Vlad Yasevich wrote:

quoted

Look at commit 19c7e9ee that introduced this.  I don't remember all
the details any more, but the problem only occurred on ia64
(probably due its speculative load handling).

-vlad

Thanks Vlad, I'll have a look see.

Ok, so this IA64 issue is all about accesses to uninitialized memory.

I think Neil's change is thus the most desirable thing to do.  Simple
memset the object to zero.

Let the compiler optimize or not optimize things as it sees fit, to
make sure the object is completely initialized.

memset() expands to __builtin_memset(), and therefore the compiler
can and will eliminate initializations to overlapping areas if such
eliminations are possible.

If thats the case, then I'll need to duplicate the memset in all three call
sites.  I've got a busy weekend comming up, but I'll respin this monday barring
no objections or counter-arguments

Best
Neil

On 10/26/2012 09:42 PM, Neil Horman wrote:

On Fri, Oct 26, 2012 at 05:10:19PM -0400, David Miller wrote:

quoted

From: Neil Horman <nhorman@tuxdriver.com>
Date: Fri, 26 Oct 2012 16:35:04 -0400

quoted

On Fri, Oct 26, 2012 at 03:12:11PM -0400, Vlad Yasevich wrote:

quoted

Look at commit 19c7e9ee that introduced this.  I don't remember all
the details any more, but the problem only occurred on ia64
(probably due its speculative load handling).

-vlad

Thanks Vlad, I'll have a look see.

Ok, so this IA64 issue is all about accesses to uninitialized memory.

I think Neil's change is thus the most desirable thing to do.  Simple
memset the object to zero.

Let the compiler optimize or not optimize things as it sees fit, to
make sure the object is completely initialized.

memset() expands to __builtin_memset(), and therefore the compiler
can and will eliminate initializations to overlapping areas if such
eliminations are possible.

If thats the case, then I'll need to duplicate the memset in all three call
sites.  I've got a busy weekend comming up, but I'll respin this monday barring
no objections or counter-arguments

Best
Neil

Yes, it should be done in all 3 call sights.  If you are going with memset, you can remove .zero element as well.

-vlad

quoted

On Fri, Oct 26, 2012 at 03:12:11PM -0400, Vlad Yasevich wrote:

quoted

Look at commit 19c7e9ee that introduced this.  I don't remember all
the details any more, but the problem only occurred on ia64
(probably due its speculative load handling).

-vlad

Thanks Vlad, I'll have a look see.

Ok, so this IA64 issue is all about accesses to uninitialized memory.

I think Neil's change is thus the most desirable thing to do.  Simple
memset the object to zero.

I'd guess it an artifact of the C memory aliasing rules and unions.
If you write to a location as a one type but read it as another type
(and neither type is char) then the compiler is generally allowed to
schedule the read before the write (and may even optimise the write away).
This shouldn't happen if the fields are both members of the same union,
but casting some other address to a 'union *' separately for the
write and read isn't enough.

This probably means that this entire type-punning argument passing
scheme is actually broken.

	David

On Fri, Oct 26, 2012 at 03:12:54PM -0400, Vlad Yasevich wrote:

On 10/26/2012 09:41 AM, Neil Horman wrote:

quoted

Lots of points in the sctp_cmd_interpreter function treat the sctp_cmd_t arg as
a void pointer, even though they are written as various other types.  Theres no
need for this as doing so just leads to possible type-punning issues that could
cause crashes, and if we remain type-consistent we can actually just remove the
void * member of the union entirely.

Change Notes:

v2)
* Dropped chunk that modified SCTP_NULL to create a marker pattern
 should anyone try to use a SCTP_NULL() assigned sctp_arg_t, Assigning
 to .zero provides the same effect and should be faster, per Vlad Y.

Signed-off-by: Neil Horman <nhorman@tuxdriver.com
CC: Vlad Yasevich <redacted>

Acked-by: Vlad Yasevich <redacted>

-vlad

Thanks for the ACk Vlad, do you have any thoughts regarding Dave M's comments
above on this .zero/memset question?  He makes a good point regarding complete
initalization I think.
Neil

On Mon, Oct 29, 2012 at 03:07:07PM -0000, David Laight wrote:

quoted

quoted

On Fri, Oct 26, 2012 at 03:12:11PM -0400, Vlad Yasevich wrote:

quoted

Look at commit 19c7e9ee that introduced this.  I don't remember all
the details any more, but the problem only occurred on ia64
(probably due its speculative load handling).

-vlad

Thanks Vlad, I'll have a look see.

Ok, so this IA64 issue is all about accesses to uninitialized memory.

I think Neil's change is thus the most desirable thing to do.  Simple
memset the object to zero.

I'd guess it an artifact of the C memory aliasing rules and unions.
If you write to a location as a one type but read it as another type
(and neither type is char) then the compiler is generally allowed to
schedule the read before the write (and may even optimise the write away).
This shouldn't happen if the fields are both members of the same union,
but casting some other address to a 'union *' separately for the
write and read isn't enough.

I don't think thats whats going on at all.  The problem, as commit
19c7e9eef503dc1ae926f3d26c56f88bee568d7b describes it, is that ia64 was
speculatively loading a value from memory of type sctp_arg_t.  If the
initialization step set the value of one of the smaller members of the union
(say a short), then the load stage may still load a larger amount of data (given
that the union is larger than its smallest members).  That results in part of
the load being uninitalized, and ia64 declaring it Not A Thing, and consequently
trapping out.

The fix previously was to just initazlie the unsigned long member zero in that
union before setting the actuall type member that was requried so as to ensure
all the data was initalized.  That works well enough, but it presumes that
unsigned long is the largest member of the union, which is risky.  Its better to
memset the union to 0 for sizeof bytes to ensure future proofing.

I've sent an updated version of the patch to do the memset. Its marked as V3,
but I fat fingered the in-reply-to so it didn't show up in this thread.
Apologies.

This probably means that this entire type-punning argument passing
scheme is actually broken.

No, It works, as long as you read the same member as you write, and initalize
the entire union, which is what this patch does.
Neil

	David



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

From: Neil Horman <nhorman@tuxdriver.com>
Date: Mon, 29 Oct 2012 14:59:32 -0400

I don't think thats whats going on at all.  The problem, as commit
19c7e9eef503dc1ae926f3d26c56f88bee568d7b describes it, is that ia64 was
speculatively loading a value from memory of type sctp_arg_t.  If the
initialization step set the value of one of the smaller members of the union
(say a short), then the load stage may still load a larger amount of data (given
that the union is larger than its smallest members).  That results in part of
the load being uninitalized, and ia64 declaring it Not A Thing, and consequently
trapping out.

Agreed.

The fix previously was to just initazlie the unsigned long member zero in that
union before setting the actuall type member that was requried so as to ensure
all the data was initalized.  That works well enough, but it presumes that
unsigned long is the largest member of the union, which is risky.  Its better to
memset the union to 0 for sizeof bytes to ensure future proofing.

Also agreed.