Message ID | 1470259393-22861-2-git-send-email-phil@nwl.cc |
---|---|
State | Changes Requested, archived |
Delegated to: | David Miller |
Headers | show |
From: Phil Sutter > Sent: 03 August 2016 22:23 > This is required to correctly interpret INET_DIAG_INFO messages exported > by sctp_diag module. ... > diff --git a/include/linux/sctp.h b/include/linux/sctp.h > index de1f64318fc4e..fcb4c36461732 100644 > --- a/include/linux/sctp.h > +++ b/include/linux/sctp.h > @@ -705,70 +705,6 @@ typedef struct sctp_auth_chunk { > sctp_authhdr_t auth_hdr; > } __packed sctp_auth_chunk_t; > > -struct sctp_info { > - __u32 sctpi_tag; ... > - __u32 __reserved3; > -}; > - > struct sctp_infox { > struct sctp_info *sctpinfo; > struct sctp_association *asoc; > diff --git a/include/uapi/linux/sctp.h b/include/uapi/linux/sctp.h > index d304f4c9792c4..a406adcc0793e 100644 > --- a/include/uapi/linux/sctp.h > +++ b/include/uapi/linux/sctp.h > @@ -944,4 +944,68 @@ struct sctp_default_prinfo { > __u16 pr_policy; > }; > > +struct sctp_info { > + __u32 sctpi_tag; Should these be uint32_t (etc) for userspace? > + __u32 sctpi_state; ... > + __u16 __reserved1; Is it worth adding some extra pad here in case anything extra needs to be added to this set of data? ... > + __u32 __reserved3; Think I'd definitely add a few words of pad here. Or at least make absolutely sure the interface passes the buffer length and allows for kernels that report different length buffers. > +}; David
On Thu, Aug 04, 2016 at 09:13:03AM +0000, David Laight wrote: > From: Phil Sutter > > Sent: 03 August 2016 22:23 > > This is required to correctly interpret INET_DIAG_INFO messages exported > > by sctp_diag module. > ... > > diff --git a/include/linux/sctp.h b/include/linux/sctp.h > > index de1f64318fc4e..fcb4c36461732 100644 > > --- a/include/linux/sctp.h > > +++ b/include/linux/sctp.h > > @@ -705,70 +705,6 @@ typedef struct sctp_auth_chunk { > > sctp_authhdr_t auth_hdr; > > } __packed sctp_auth_chunk_t; > > > > -struct sctp_info { > > - __u32 sctpi_tag; > ... > > - __u32 __reserved3; > > -}; > > - > > struct sctp_infox { > > struct sctp_info *sctpinfo; > > struct sctp_association *asoc; > > diff --git a/include/uapi/linux/sctp.h b/include/uapi/linux/sctp.h > > index d304f4c9792c4..a406adcc0793e 100644 > > --- a/include/uapi/linux/sctp.h > > +++ b/include/uapi/linux/sctp.h > > @@ -944,4 +944,68 @@ struct sctp_default_prinfo { > > __u16 pr_policy; > > }; > > > > +struct sctp_info { > > + __u32 sctpi_tag; > > Should these be uint32_t (etc) for userspace? Grepping through include/uapi in my clone of net-next, I see 271 results for uint32_t but 4595 ones for __u32. So while not necessarily correct, it seems to be the far more popular choice. Do you see any benefit in using the uint*_t typedefs instead? > > + __u32 sctpi_state; > ... > > + __u16 __reserved1; > > Is it worth adding some extra pad here in case anything extra needs > to be added to this set of data? > > ... > > + __u32 __reserved3; > > Think I'd definitely add a few words of pad here. > Or at least make absolutely sure the interface passes the buffer length and > allows for kernels that report different length buffers. I merely copy and pasted the struct from include/linux/sctp.h without thinking about it's layout. Xin, what are your thoughts about this? Thanks, Phil
On Thu, Aug 4, 2016 at 5:27 PM, Phil Sutter <phil@nwl.cc> wrote: > On Thu, Aug 04, 2016 at 09:13:03AM +0000, David Laight wrote: >> From: Phil Sutter >> > Sent: 03 August 2016 22:23 >> > This is required to correctly interpret INET_DIAG_INFO messages exported >> > by sctp_diag module. >> ... >> > diff --git a/include/linux/sctp.h b/include/linux/sctp.h >> > index de1f64318fc4e..fcb4c36461732 100644 >> > --- a/include/linux/sctp.h >> > +++ b/include/linux/sctp.h >> > @@ -705,70 +705,6 @@ typedef struct sctp_auth_chunk { >> > sctp_authhdr_t auth_hdr; >> > } __packed sctp_auth_chunk_t; >> > >> > -struct sctp_info { >> > - __u32 sctpi_tag; >> ... >> > - __u32 __reserved3; >> > -}; >> > - >> > struct sctp_infox { >> > struct sctp_info *sctpinfo; >> > struct sctp_association *asoc; >> > diff --git a/include/uapi/linux/sctp.h b/include/uapi/linux/sctp.h >> > index d304f4c9792c4..a406adcc0793e 100644 >> > --- a/include/uapi/linux/sctp.h >> > +++ b/include/uapi/linux/sctp.h >> > @@ -944,4 +944,68 @@ struct sctp_default_prinfo { >> > __u16 pr_policy; >> > }; >> > >> > +struct sctp_info { >> > + __u32 sctpi_tag; >> >> Should these be uint32_t (etc) for userspace? > > Grepping through include/uapi in my clone of net-next, I see 271 results > for uint32_t but 4595 ones for __u32. So while not necessarily correct, > it seems to be the far more popular choice. Do you see any benefit in > using the uint*_t typedefs instead? > >> > + __u32 sctpi_state; >> ... >> > + __u16 __reserved1; >> >> Is it worth adding some extra pad here in case anything extra needs >> to be added to this set of data? >> >> ... >> > + __u32 __reserved3; >> >> Think I'd definitely add a few words of pad here. >> Or at least make absolutely sure the interface passes the buffer length and >> allows for kernels that report different length buffers. > > I merely copy and pasted the struct from include/linux/sctp.h without > thinking about it's layout. Xin, what are your thoughts about this? You can see all the __reserved* fields here are to fill the memory holes. As sctp's asoc is a quite big structure, now we cannot know exactly all we really should dump. It's very possible to add more fields here in the future. Think about not to break the compatibility with iproute at that time, we will use __reserved* fields firstly, secondly put it to the end of this structure. by now we're not sure what they will be. > > Thanks, Phil
diff --git a/include/linux/sctp.h b/include/linux/sctp.h index de1f64318fc4e..fcb4c36461732 100644 --- a/include/linux/sctp.h +++ b/include/linux/sctp.h @@ -705,70 +705,6 @@ typedef struct sctp_auth_chunk { sctp_authhdr_t auth_hdr; } __packed sctp_auth_chunk_t; -struct sctp_info { - __u32 sctpi_tag; - __u32 sctpi_state; - __u32 sctpi_rwnd; - __u16 sctpi_unackdata; - __u16 sctpi_penddata; - __u16 sctpi_instrms; - __u16 sctpi_outstrms; - __u32 sctpi_fragmentation_point; - __u32 sctpi_inqueue; - __u32 sctpi_outqueue; - __u32 sctpi_overall_error; - __u32 sctpi_max_burst; - __u32 sctpi_maxseg; - __u32 sctpi_peer_rwnd; - __u32 sctpi_peer_tag; - __u8 sctpi_peer_capable; - __u8 sctpi_peer_sack; - __u16 __reserved1; - - /* assoc status info */ - __u64 sctpi_isacks; - __u64 sctpi_osacks; - __u64 sctpi_opackets; - __u64 sctpi_ipackets; - __u64 sctpi_rtxchunks; - __u64 sctpi_outofseqtsns; - __u64 sctpi_idupchunks; - __u64 sctpi_gapcnt; - __u64 sctpi_ouodchunks; - __u64 sctpi_iuodchunks; - __u64 sctpi_oodchunks; - __u64 sctpi_iodchunks; - __u64 sctpi_octrlchunks; - __u64 sctpi_ictrlchunks; - - /* primary transport info */ - struct sockaddr_storage sctpi_p_address; - __s32 sctpi_p_state; - __u32 sctpi_p_cwnd; - __u32 sctpi_p_srtt; - __u32 sctpi_p_rto; - __u32 sctpi_p_hbinterval; - __u32 sctpi_p_pathmaxrxt; - __u32 sctpi_p_sackdelay; - __u32 sctpi_p_sackfreq; - __u32 sctpi_p_ssthresh; - __u32 sctpi_p_partial_bytes_acked; - __u32 sctpi_p_flight_size; - __u16 sctpi_p_error; - __u16 __reserved2; - - /* sctp sock info */ - __u32 sctpi_s_autoclose; - __u32 sctpi_s_adaptation_ind; - __u32 sctpi_s_pd_point; - __u8 sctpi_s_nodelay; - __u8 sctpi_s_disable_fragments; - __u8 sctpi_s_v4mapped; - __u8 sctpi_s_frag_interleave; - __u32 sctpi_s_type; - __u32 __reserved3; -}; - struct sctp_infox { struct sctp_info *sctpinfo; struct sctp_association *asoc; diff --git a/include/uapi/linux/sctp.h b/include/uapi/linux/sctp.h index d304f4c9792c4..a406adcc0793e 100644 --- a/include/uapi/linux/sctp.h +++ b/include/uapi/linux/sctp.h @@ -944,4 +944,68 @@ struct sctp_default_prinfo { __u16 pr_policy; }; +struct sctp_info { + __u32 sctpi_tag; + __u32 sctpi_state; + __u32 sctpi_rwnd; + __u16 sctpi_unackdata; + __u16 sctpi_penddata; + __u16 sctpi_instrms; + __u16 sctpi_outstrms; + __u32 sctpi_fragmentation_point; + __u32 sctpi_inqueue; + __u32 sctpi_outqueue; + __u32 sctpi_overall_error; + __u32 sctpi_max_burst; + __u32 sctpi_maxseg; + __u32 sctpi_peer_rwnd; + __u32 sctpi_peer_tag; + __u8 sctpi_peer_capable; + __u8 sctpi_peer_sack; + __u16 __reserved1; + + /* assoc status info */ + __u64 sctpi_isacks; + __u64 sctpi_osacks; + __u64 sctpi_opackets; + __u64 sctpi_ipackets; + __u64 sctpi_rtxchunks; + __u64 sctpi_outofseqtsns; + __u64 sctpi_idupchunks; + __u64 sctpi_gapcnt; + __u64 sctpi_ouodchunks; + __u64 sctpi_iuodchunks; + __u64 sctpi_oodchunks; + __u64 sctpi_iodchunks; + __u64 sctpi_octrlchunks; + __u64 sctpi_ictrlchunks; + + /* primary transport info */ + struct sockaddr_storage sctpi_p_address; + __s32 sctpi_p_state; + __u32 sctpi_p_cwnd; + __u32 sctpi_p_srtt; + __u32 sctpi_p_rto; + __u32 sctpi_p_hbinterval; + __u32 sctpi_p_pathmaxrxt; + __u32 sctpi_p_sackdelay; + __u32 sctpi_p_sackfreq; + __u32 sctpi_p_ssthresh; + __u32 sctpi_p_partial_bytes_acked; + __u32 sctpi_p_flight_size; + __u16 sctpi_p_error; + __u16 __reserved2; + + /* sctp sock info */ + __u32 sctpi_s_autoclose; + __u32 sctpi_s_adaptation_ind; + __u32 sctpi_s_pd_point; + __u8 sctpi_s_nodelay; + __u8 sctpi_s_disable_fragments; + __u8 sctpi_s_v4mapped; + __u8 sctpi_s_frag_interleave; + __u32 sctpi_s_type; + __u32 __reserved3; +}; + #endif /* _UAPI_SCTP_H */
This is required to correctly interpret INET_DIAG_INFO messages exported by sctp_diag module. Signed-off-by: Phil Sutter <phil@nwl.cc> --- include/linux/sctp.h | 64 ----------------------------------------------- include/uapi/linux/sctp.h | 64 +++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 64 insertions(+), 64 deletions(-)