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@xxxxxxxxxxxxx
> >>>>>CC: Vlad Yasevich <vyasevich@xxxxxxxxx>
> >>>>>CC: "David S. Miller" <davem@xxxxxxxxxxxxx>
> >>>>>CC: linux-sctp@xxxxxxxxxxxxxxx
> >>>>>---
> >>>>> 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 linux-sctp" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
