[PATCH v2 0/3] sctp_diag: A bunch of fixes for upcoming 'ss' support

[PATCH v2 0/3] sctp_diag: A bunch of fixes for upcoming 'ss' support

[Phil Sutter]

The following series contains a number of fixes necessary to make my yet
unpublished 'ss' support patch functional.

Changes since v1:
- Fixed patch 2/3
- Rebased whole series onto current net-next/master

Phil Sutter (3):
  sctp: Export struct sctp_info to userspace
  sctp_diag: export timer value only if it is active
  sctp_diag: Respect ss adding TCPF_CLOSE to idiag_states

 include/linux/sctp.h      | 64 -----------------------------------------------
 include/uapi/linux/sctp.h | 64 +++++++++++++++++++++++++++++++++++++++++++++++
 net/sctp/sctp_diag.c      | 18 ++++++++-----
 3 files changed, 76 insertions(+), 70 deletions(-)

-- 
2.8.2

[PATCH v2 1/3] sctp: Export struct sctp_info to userspace

[Phil Sutter]

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(-)

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 */
-- 
2.8.2

[PATCH v2 2/3] sctp_diag: export timer value only if it is active

[Phil Sutter]

Since it is exported as unsigned value, userspace has no way detecting
whether it is negative or just very large. Therefore do this in kernel
space where it is a simple comparison.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
Changes since v1:
- Introduce local variable to shorten long lines.
- Use timer_pending() to decide whether to export the timer value.
- Export the primary path's value instead of garbage.
- If not exporting, zero fields to not confuse userspace.
---
 net/sctp/sctp_diag.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/net/sctp/sctp_diag.c b/net/sctp/sctp_diag.c
index f69edcf219e51..f728ef04a7b2d 100644
--- a/net/sctp/sctp_diag.c
+++ b/net/sctp/sctp_diag.c
@@ -13,6 +13,7 @@ static void inet_diag_msg_sctpasoc_fill(struct inet_diag_msg *r,
 {
 	union sctp_addr laddr, paddr;
 	struct dst_entry *dst;
+	struct timer_list *t3_rtx = &asoc->peer.primary_path->T3_rtx_timer;
 
 	laddr = list_entry(asoc->base.bind_addr.address_list.next,
 			   struct sctp_sockaddr_entry, list)->a;
@@ -40,10 +41,15 @@ static void inet_diag_msg_sctpasoc_fill(struct inet_diag_msg *r,
 	}
 
 	r->idiag_state = asoc->state;
-	r->idiag_timer = SCTP_EVENT_TIMEOUT_T3_RTX;
-	r->idiag_retrans = asoc->rtx_data_chunks;
-	r->idiag_expires = jiffies_to_msecs(
-		asoc->timeouts[SCTP_EVENT_TIMEOUT_T3_RTX] - jiffies);
+	if (timer_pending(t3_rtx)) {
+		r->idiag_timer = SCTP_EVENT_TIMEOUT_T3_RTX;
+		r->idiag_retrans = asoc->rtx_data_chunks;
+		r->idiag_expires = jiffies_to_msecs(t3_rtx->expires - jiffies);
+	} else {
+		r->idiag_timer = 0;
+		r->idiag_retrans = 0;
+		r->idiag_expires = 0;
+	}
 }
 
 static int inet_diag_msg_sctpladdrs_fill(struct sk_buff *skb,
-- 
2.8.2

[PATCH v2 3/3] sctp_diag: Respect ss adding TCPF_CLOSE to idiag_states

[Phil Sutter]

Since 'ss' always adds TCPF_CLOSE to idiag_states flags, sctp_diag can't
rely upon TCPF_LISTEN flag solely being present when listening sockets
are requested.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 net/sctp/sctp_diag.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/net/sctp/sctp_diag.c b/net/sctp/sctp_diag.c
index f728ef04a7b2d..bb691538adc8e 100644
--- a/net/sctp/sctp_diag.c
+++ b/net/sctp/sctp_diag.c
@@ -356,7 +356,7 @@ static int sctp_ep_dump(struct sctp_endpoint *ep, void *p)
 	if (cb->args[4] < cb->args[1])
 		goto next;
 
-	if ((r->idiag_states & ~TCPF_LISTEN) && !list_empty(&ep->asocs))
+	if (!(r->idiag_states & TCPF_LISTEN) && !list_empty(&ep->asocs))
 		goto next;
 
 	if (r->sdiag_family != AF_UNSPEC &&
@@ -471,7 +471,7 @@ skip:
 	 * 3 : to mark if we have dumped the ep info of the current asoc
 	 * 4 : to work as a temporary variable to traversal list
 	 */
-	if (!(idiag_states & ~TCPF_LISTEN))
+	if (!(idiag_states & ~(TCPF_LISTEN | TCPF_CLOSE)))
 		goto done;
 	sctp_for_each_transport(sctp_tsp_dump, net, cb->args[2], &commp);
 done:
-- 
2.8.2

Re: [PATCH v2 2/3] sctp_diag: export timer value only if it is active

[Marcelo Ricardo Leitner]

On Wed, Aug 03, 2016 at 11:23:12PM +0200, Phil Sutter wrote:
> Since it is exported as unsigned value, userspace has no way detecting
> whether it is negative or just very large. Therefore do this in kernel
> space where it is a simple comparison.
> 
> Signed-off-by: Phil Sutter <phil@nwl.cc>
> ---
> Changes since v1:
> - Introduce local variable to shorten long lines.
> - Use timer_pending() to decide whether to export the timer value.
> - Export the primary path's value instead of garbage.

This is now the most important change on the patch IMO. Can we mention
it on changelog itself?

A Fixes tag is welcomed too, as previous values were incorrect.

> - If not exporting, zero fields to not confuse userspace.
> ---
>  net/sctp/sctp_diag.c | 14 ++++++++++----
>  1 file changed, 10 insertions(+), 4 deletions(-)
> 
> diff --git a/net/sctp/sctp_diag.c b/net/sctp/sctp_diag.c
> index f69edcf219e51..f728ef04a7b2d 100644
> --- a/net/sctp/sctp_diag.c
> +++ b/net/sctp/sctp_diag.c
> @@ -13,6 +13,7 @@ static void inet_diag_msg_sctpasoc_fill(struct inet_diag_msg *r,
>  {
>  	union sctp_addr laddr, paddr;
>  	struct dst_entry *dst;
> +	struct timer_list *t3_rtx = &asoc->peer.primary_path->T3_rtx_timer;
>  
>  	laddr = list_entry(asoc->base.bind_addr.address_list.next,
>  			   struct sctp_sockaddr_entry, list)->a;
> @@ -40,10 +41,15 @@ static void inet_diag_msg_sctpasoc_fill(struct inet_diag_msg *r,
>  	}
>  
>  	r->idiag_state = asoc->state;
> -	r->idiag_timer = SCTP_EVENT_TIMEOUT_T3_RTX;
> -	r->idiag_retrans = asoc->rtx_data_chunks;
> -	r->idiag_expires = jiffies_to_msecs(
> -		asoc->timeouts[SCTP_EVENT_TIMEOUT_T3_RTX] - jiffies);
> +	if (timer_pending(t3_rtx)) {
> +		r->idiag_timer = SCTP_EVENT_TIMEOUT_T3_RTX;
> +		r->idiag_retrans = asoc->rtx_data_chunks;
> +		r->idiag_expires = jiffies_to_msecs(t3_rtx->expires - jiffies);
> +	} else {
> +		r->idiag_timer = 0;
> +		r->idiag_retrans = 0;
> +		r->idiag_expires = 0;
> +	}
>  }
>  
>  static int inet_diag_msg_sctpladdrs_fill(struct sk_buff *skb,
> -- 
> 2.8.2
> 

Re: [PATCH v2 2/3] sctp_diag: export timer value only if it is active

[Phil Sutter]

On Wed, Aug 03, 2016 at 06:51:15PM -0300, Marcelo Ricardo Leitner wrote:
> On Wed, Aug 03, 2016 at 11:23:12PM +0200, Phil Sutter wrote:
> > Since it is exported as unsigned value, userspace has no way detecting
> > whether it is negative or just very large. Therefore do this in kernel
> > space where it is a simple comparison.
> > 
> > Signed-off-by: Phil Sutter <phil@nwl.cc>
> > ---
> > Changes since v1:
> > - Introduce local variable to shorten long lines.
> > - Use timer_pending() to decide whether to export the timer value.
> > - Export the primary path's value instead of garbage.
> 
> This is now the most important change on the patch IMO. Can we mention
> it on changelog itself?
> 
> A Fixes tag is welcomed too, as previous values were incorrect.

Oh right, thanks for pointing this out. I will follow-up with a fixed
v3.

Thanks, Phil

RE: [PATCH v2 1/3] sctp: Export struct sctp_info to userspace

[David Laight]

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

Re: [PATCH v2 1/3] sctp: Export struct sctp_info to userspace

[Phil Sutter]

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

Re: [PATCH v2 1/3] sctp: Export struct sctp_info to userspace

[Xin Long]

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