Date:	Fri, 26 Oct 2012 16:35:04 -0400
From:	Neil Horman <nhorman@...driver.com>
To:	Vlad Yasevich <vyasevich@...il.com>
Cc:	netdev@...r.kernel.org, "David S. Miller" <davem@...emloft.net>,
	linux-sctp@...r.kernel.org
Subject: Re: [PATCH] sctp: Clean up type-punning in sctp_cmd_t union

On Fri, Oct 26, 2012 at 03:12:11PM -0400, Vlad Yasevich wrote:
> On 10/26/2012 09:24 AM, Neil Horman wrote:
> >On Thu, Oct 25, 2012 at 11:48:16PM -0400, Vlad Yasevich wrote:
> >>On 10/25/2012 07:58 PM, Neil Horman wrote:
> >>>On Thu, Oct 25, 2012 at 05:42:15PM -0400, Vlad Yasevich wrote:
> >>>>On 10/25/2012 04:47 PM, 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.
> >>>>>
> >>>>>Signed-off-by: Neil Horman <nhorman@...driver.com
> >>>>>CC: Vlad Yasevich <vyasevich@...il.com>
> >>>>>CC: "David S. Miller" <davem@...emloft.net>
> >>>>>CC: linux-sctp@...r.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
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Powered by blists - more mailing lists