| Message ID | 1351198075-20385-1-git-send-email-nhorman@tuxdriver.com |
|---|---|
| State | Superseded, archived |
| Delegated to: | David Miller |
| Headers | show |
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 > } > 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); > -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. Let me know what you want to do here, and I can respin this. Best Neil -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -vlad > Let me know what you want to do here, and I can respin this. > Best > Neil > -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> Subject: [PATCH] sctp: Clean up type-punning in sctp_cmd_t union ... > +#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. 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 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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? Regards Neil > -vlad > >Let me know what you want to do here, and I can respin this. > >Best > >Neil > > > > -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Oct 26, 2012 at 10:00:13AM +0100, David Laight wrote: > > Subject: [PATCH] sctp: Clean up type-punning in sctp_cmd_t union > ... > > +#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 > > > > -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 >>> >> >> -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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@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 > 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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: >> 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. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 > > > On Fri, Oct 26, 2012 at 03:12:11PM -0400, Vlad Yasevich wrote: > >> 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 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 10/26/2012 09:42 PM, Neil Horman wrote: > 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 >> >>> On Fri, Oct 26, 2012 at 03:12:11PM -0400, Vlad Yasevich wrote: >>>> 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 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> > On Fri, Oct 26, 2012 at 03:12:11PM -0400, Vlad Yasevich wrote: > >> 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 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Oct 29, 2012 at 03:07:07PM -0000, David Laight wrote: > > > On Fri, Oct 26, 2012 at 03:12:11PM -0400, Vlad Yasevich wrote: > > >> 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 > -- To unsubscribe from this list: send the line "unsubscribe netdev" 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. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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);
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(-)