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

[Vlad Yasevich <vyasevich@gmail.com>]

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@tuxdriver.com
>>>>> CC: Vlad Yasevich <vyasevich@gmail.com>
>>>>> 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
>
>> -vlad
>>> Let me know what you want to do here, and I can respin this.
>>> Best
>>> Neil
>>>
>>
>>