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

[PATCH 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.

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      | 14 ++++++-----
 3 files changed, 72 insertions(+), 70 deletions(-)

-- 
2.8.2

[PATCH 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

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

[Stephen Hemminger]

On Fri, 29 Jul 2016 18:59:38 +0200
Phil Sutter <phil@nwl.cc> wrote:

> This is required to correctly interpret INET_DIAG_INFO messages exported
> by sctp_diag module.
> 
> Signed-off-by: Phil Sutter <phil@nwl.cc>

You need to add sctp.h to include/linux/Kbuild to get it exported correctly

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

[Phil Sutter]

Hi Stephen,

On Sun, Jul 31, 2016 at 02:18:24PM -0700, Stephen Hemminger wrote:
> On Fri, 29 Jul 2016 18:59:38 +0200
> Phil Sutter <phil@nwl.cc> wrote:
> 
> > This is required to correctly interpret INET_DIAG_INFO messages exported
> > by sctp_diag module.
> > 
> > Signed-off-by: Phil Sutter <phil@nwl.cc>
> 
> You need to add sctp.h to include/linux/Kbuild to get it exported correctly

Thanks for the heads-up! Luckily it is in there already.

Cheers, Phil

[PATCH 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>
---
 net/sctp/sctp_diag.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/net/sctp/sctp_diag.c b/net/sctp/sctp_diag.c
index f69edcf219e51..0ad6033a7330c 100644
--- a/net/sctp/sctp_diag.c
+++ b/net/sctp/sctp_diag.c
@@ -40,10 +40,12 @@ 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 (asoc->timeouts[SCTP_EVENT_TIMEOUT_T3_RTX] > jiffies) {
+		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);
+	}
 }
 
 static int inet_diag_msg_sctpladdrs_fill(struct sk_buff *skb,
-- 
2.8.2

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

[Marcelo Ricardo Leitner]

On Fri, Jul 29, 2016 at 06:59:39PM +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>
> ---
>  net/sctp/sctp_diag.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/net/sctp/sctp_diag.c b/net/sctp/sctp_diag.c
> index f69edcf219e51..0ad6033a7330c 100644
> --- a/net/sctp/sctp_diag.c
> +++ b/net/sctp/sctp_diag.c
> @@ -40,10 +40,12 @@ 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);

I think we have two issues here, prior to your patch, but I noticed
while reviewing it :-)

This array is actually not based on jiffies but on intervals instead, as
per:

sm_sideeffect.c:
                case SCTP_CMD_TIMER_START:                       [1]
                        timer = &asoc->timers[cmd->obj.to];
                        timeout = asoc->timeouts[cmd->obj.to];   <---
                        BUG_ON(!timeout);

                        timer->expires = jiffies + timeout;      <---

But more importantly, this array is actually not used for this timeout
and the timeout is sctp_transport dependant, as per:

/* Schedule retransmission on the given transport */
void sctp_transport_immediate_rtx(struct sctp_transport *t)
{
        /* Stop pending T3_rtx_timer */
        if (del_timer(&t->T3_rtx_timer))
                sctp_transport_put(t);

        sctp_retransmit(&t->asoc->outqueue, t, SCTP_RTXR_T3_RTX);
        if (!timer_pending(&t->T3_rtx_timer)) {
                if (!mod_timer(&t->T3_rtx_timer, jiffies + t->rto))
                                                 ^^^^^^^^^^^^^^^^
                        sctp_transport_hold(t);

Note how on sctp_get_sctp_info() it fetches the RTO (which is T3_RTX)
this way:
	info->sctpi_p_rto = jiffies_to_msecs(prim->rto);
If we want to know how long is left for the timer to expire, we have to
read directly from it.

With git grep -A 1 TIMER_START we can confirm that [1] is never hit for
SCTP_EVENT_TIMEOUT_T3_RTX. Yet, the asoc is allocated with kzalloc(), so
I guess you were just reading -jiffies in there.

Note however that the stats rtx_data_chunks is the accumulated stats,
it's good, and that we may have multiple T3 timers running at once, with
different timeouts.

Xin, ideas on how we can fix this? I'm not sure if we can dump
per-transport info in there. Not as it is now, I guess.

> +	if (asoc->timeouts[SCTP_EVENT_TIMEOUT_T3_RTX] > jiffies) {
> +		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);
> +	}
>  }
>  
>  static int inet_diag_msg_sctpladdrs_fill(struct sk_buff *skb,
> -- 
> 2.8.2
> 

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

[Xin Long]

>> diff --git a/net/sctp/sctp_diag.c b/net/sctp/sctp_diag.c
>> index f69edcf219e51..0ad6033a7330c 100644
>> --- a/net/sctp/sctp_diag.c
>> +++ b/net/sctp/sctp_diag.c
>> @@ -40,10 +40,12 @@ 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);
>
> I think we have two issues here, prior to your patch, but I noticed
> while reviewing it :-)
>
> This array is actually not based on jiffies but on intervals instead, as
> per:
>
> sm_sideeffect.c:
>                 case SCTP_CMD_TIMER_START:                       [1]
>                         timer = &asoc->timers[cmd->obj.to];
>                         timeout = asoc->timeouts[cmd->obj.to];   <---
>                         BUG_ON(!timeout);
>
>                         timer->expires = jiffies + timeout;      <---
understood.

>
> But more importantly, this array is actually not used for this timeout
> and the timeout is sctp_transport dependant, as per:
>
> /* Schedule retransmission on the given transport */
> void sctp_transport_immediate_rtx(struct sctp_transport *t)
> {
>         /* Stop pending T3_rtx_timer */
>         if (del_timer(&t->T3_rtx_timer))
>                 sctp_transport_put(t);
>
>         sctp_retransmit(&t->asoc->outqueue, t, SCTP_RTXR_T3_RTX);
>         if (!timer_pending(&t->T3_rtx_timer)) {
>                 if (!mod_timer(&t->T3_rtx_timer, jiffies + t->rto))
>                                                  ^^^^^^^^^^^^^^^^
>                         sctp_transport_hold(t);
>
> Note how on sctp_get_sctp_info() it fetches the RTO (which is T3_RTX)
> this way:
>         info->sctpi_p_rto = jiffies_to_msecs(prim->rto);
> If we want to know how long is left for the timer to expire, we have to
> read directly from it.
you are right, 3 timers (T3_tx, hb, rtx_data_chunks) are per transport.

>
> With git grep -A 1 TIMER_START we can confirm that [1] is never hit for
> SCTP_EVENT_TIMEOUT_T3_RTX. Yet, the asoc is allocated with kzalloc(), so
> I guess you were just reading -jiffies in there.
>
> Note however that the stats rtx_data_chunks is the accumulated stats,
> it's good, and that we may have multiple T3 timers running at once, with
> different timeouts.
>
> Xin, ideas on how we can fix this? I'm not sure if we can dump
> per-transport info in there. Not as it is now, I guess.
It's not that easy to dump all transports info besed on current sctp_diag codes.

Now for the transport's info,  we only choose primary_path to dump.
It means we should fix this by getting the left time to expire from
primary transport t->T3_rtx_timer. like:

        r->idiag_expires = jiffies_to_msecs(
-               asoc->timeouts[SCTP_EVENT_TIMEOUT_T3_RTX] - jiffies);
+               asoc->peer.primary_path->T3_rtx_timer.expires - jiffies);

but yes, need to check with timer_pending firstly.

what do  you think ?

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

[Marcelo Ricardo Leitner]

Em 30-07-2016 10:25, Xin Long escreveu:
>>> diff --git a/net/sctp/sctp_diag.c b/net/sctp/sctp_diag.c
>>> index f69edcf219e51..0ad6033a7330c 100644
>>> --- a/net/sctp/sctp_diag.c
>>> +++ b/net/sctp/sctp_diag.c
>>> @@ -40,10 +40,12 @@ 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);
>>
>> I think we have two issues here, prior to your patch, but I noticed
>> while reviewing it :-)
>>
>> This array is actually not based on jiffies but on intervals instead, as
>> per:
>>
>> sm_sideeffect.c:
>>                 case SCTP_CMD_TIMER_START:                       [1]
>>                         timer = &asoc->timers[cmd->obj.to];
>>                         timeout = asoc->timeouts[cmd->obj.to];   <---
>>                         BUG_ON(!timeout);
>>
>>                         timer->expires = jiffies + timeout;      <---
> understood.
>
>>
>> But more importantly, this array is actually not used for this timeout
>> and the timeout is sctp_transport dependant, as per:
>>
>> /* Schedule retransmission on the given transport */
>> void sctp_transport_immediate_rtx(struct sctp_transport *t)
>> {
>>         /* Stop pending T3_rtx_timer */
>>         if (del_timer(&t->T3_rtx_timer))
>>                 sctp_transport_put(t);
>>
>>         sctp_retransmit(&t->asoc->outqueue, t, SCTP_RTXR_T3_RTX);
>>         if (!timer_pending(&t->T3_rtx_timer)) {
>>                 if (!mod_timer(&t->T3_rtx_timer, jiffies + t->rto))
>>                                                  ^^^^^^^^^^^^^^^^
>>                         sctp_transport_hold(t);
>>
>> Note how on sctp_get_sctp_info() it fetches the RTO (which is T3_RTX)
>> this way:
>>         info->sctpi_p_rto = jiffies_to_msecs(prim->rto);
>> If we want to know how long is left for the timer to expire, we have to
>> read directly from it.
> you are right, 3 timers (T3_tx, hb, rtx_data_chunks) are per transport.
>
>>
>> With git grep -A 1 TIMER_START we can confirm that [1] is never hit for
>> SCTP_EVENT_TIMEOUT_T3_RTX. Yet, the asoc is allocated with kzalloc(), so
>> I guess you were just reading -jiffies in there.
>>
>> Note however that the stats rtx_data_chunks is the accumulated stats,
>> it's good, and that we may have multiple T3 timers running at once, with
>> different timeouts.
>>
>> Xin, ideas on how we can fix this? I'm not sure if we can dump
>> per-transport info in there. Not as it is now, I guess.
> It's not that easy to dump all transports info besed on current sctp_diag codes.

Okay

>
> Now for the transport's info,  we only choose primary_path to dump.

Okay

> It means we should fix this by getting the left time to expire from
> primary transport t->T3_rtx_timer. like:
>
>         r->idiag_expires = jiffies_to_msecs(
> -               asoc->timeouts[SCTP_EVENT_TIMEOUT_T3_RTX] - jiffies);
> +               asoc->peer.primary_path->T3_rtx_timer.expires - jiffies);
>
> but yes, need to check with timer_pending firstly.

Yes :)

>
> what do  you think ?
>

Makes sense, LGTM.

Phil, not sure how you want to proceed here. Wanna handle the change above?

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

[Phil Sutter]

On Sat, Jul 30, 2016 at 10:33:48AM -0300, Marcelo Ricardo Leitner wrote:
> 
> 
> Em 30-07-2016 10:25, Xin Long escreveu:
> >>> diff --git a/net/sctp/sctp_diag.c b/net/sctp/sctp_diag.c
> >>> index f69edcf219e51..0ad6033a7330c 100644
> >>> --- a/net/sctp/sctp_diag.c
> >>> +++ b/net/sctp/sctp_diag.c
> >>> @@ -40,10 +40,12 @@ 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);
> >>
> >> I think we have two issues here, prior to your patch, but I noticed
> >> while reviewing it :-)
> >>
> >> This array is actually not based on jiffies but on intervals instead, as
> >> per:
> >>
> >> sm_sideeffect.c:
> >>                 case SCTP_CMD_TIMER_START:                       [1]
> >>                         timer = &asoc->timers[cmd->obj.to];
> >>                         timeout = asoc->timeouts[cmd->obj.to];   <---
> >>                         BUG_ON(!timeout);
> >>
> >>                         timer->expires = jiffies + timeout;      <---
> > understood.
> >
> >>
> >> But more importantly, this array is actually not used for this timeout
> >> and the timeout is sctp_transport dependant, as per:
> >>
> >> /* Schedule retransmission on the given transport */
> >> void sctp_transport_immediate_rtx(struct sctp_transport *t)
> >> {
> >>         /* Stop pending T3_rtx_timer */
> >>         if (del_timer(&t->T3_rtx_timer))
> >>                 sctp_transport_put(t);
> >>
> >>         sctp_retransmit(&t->asoc->outqueue, t, SCTP_RTXR_T3_RTX);
> >>         if (!timer_pending(&t->T3_rtx_timer)) {
> >>                 if (!mod_timer(&t->T3_rtx_timer, jiffies + t->rto))
> >>                                                  ^^^^^^^^^^^^^^^^
> >>                         sctp_transport_hold(t);
> >>
> >> Note how on sctp_get_sctp_info() it fetches the RTO (which is T3_RTX)
> >> this way:
> >>         info->sctpi_p_rto = jiffies_to_msecs(prim->rto);
> >> If we want to know how long is left for the timer to expire, we have to
> >> read directly from it.
> > you are right, 3 timers (T3_tx, hb, rtx_data_chunks) are per transport.
> >
> >>
> >> With git grep -A 1 TIMER_START we can confirm that [1] is never hit for
> >> SCTP_EVENT_TIMEOUT_T3_RTX. Yet, the asoc is allocated with kzalloc(), so
> >> I guess you were just reading -jiffies in there.
> >>
> >> Note however that the stats rtx_data_chunks is the accumulated stats,
> >> it's good, and that we may have multiple T3 timers running at once, with
> >> different timeouts.
> >>
> >> Xin, ideas on how we can fix this? I'm not sure if we can dump
> >> per-transport info in there. Not as it is now, I guess.
> > It's not that easy to dump all transports info besed on current sctp_diag codes.
> 
> Okay
> 
> >
> > Now for the transport's info,  we only choose primary_path to dump.
> 
> Okay
> 
> > It means we should fix this by getting the left time to expire from
> > primary transport t->T3_rtx_timer. like:
> >
> >         r->idiag_expires = jiffies_to_msecs(
> > -               asoc->timeouts[SCTP_EVENT_TIMEOUT_T3_RTX] - jiffies);
> > +               asoc->peer.primary_path->T3_rtx_timer.expires - jiffies);
> >
> > but yes, need to check with timer_pending firstly.
> 
> Yes :)
> 
> >
> > what do  you think ?
> >
> 
> Makes sense, LGTM.
> 
> Phil, not sure how you want to proceed here. Wanna handle the change above?

I'll look into this next week. One early question: Does the above mean
we are printing the primary path's timer value for every assoc? If so,
shouldn't we do that for just the EP or the primary path's assoc even?

Thanks, Phil

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

[Xin Long]

>
> I'll look into this next week. One early question: Does the above mean
> we are printing the primary path's timer value for every assoc? If so,
> shouldn't we do that for just the EP or the primary path's assoc even?
>
Nope, we can't say "the primary path's assoc".

Every assoc has their own primary path, even these assocs belong
to one EP.  you can see it from (asoc->peer.primary_path).

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

[Phil Sutter]

Hi,

On Sat, Jul 30, 2016 at 09:25:42PM +0800, Xin Long wrote:
[...]
> Now for the transport's info,  we only choose primary_path to dump.
> It means we should fix this by getting the left time to expire from
> primary transport t->T3_rtx_timer. like:
> 
>         r->idiag_expires = jiffies_to_msecs(
> -               asoc->timeouts[SCTP_EVENT_TIMEOUT_T3_RTX] - jiffies);
> +               asoc->peer.primary_path->T3_rtx_timer.expires - jiffies);
> 
> but yes, need to check with timer_pending firstly.

I have changed the code to this:

| struct timer_list *t3_rtx = &asoc->peer.primary_path->T3_rtx_timer;
| 
| [...]
| 
| 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);
| }

And I'm still getting what appears to be negative values sometimes. Here
are some of the common values in hex when busy looping sctp_diag
requests:

0
7530
1000000
3000000
6000000
14000000
94000000
ed690000
ffffea00

While I wonder a bit about the zero, the last two seem to be unsigned
underruns. Do I still have to check for 't3_rtx->expires > jiffies' or
am I missing something?

Thanks, Phil

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

[Marcelo Ricardo Leitner]

On Wed, Aug 03, 2016 at 09:28:13PM +0200, Phil Sutter wrote:
> Hi,
> 
> On Sat, Jul 30, 2016 at 09:25:42PM +0800, Xin Long wrote:
> [...]
> > Now for the transport's info,  we only choose primary_path to dump.
> > It means we should fix this by getting the left time to expire from
> > primary transport t->T3_rtx_timer. like:
> > 
> >         r->idiag_expires = jiffies_to_msecs(
> > -               asoc->timeouts[SCTP_EVENT_TIMEOUT_T3_RTX] - jiffies);
> > +               asoc->peer.primary_path->T3_rtx_timer.expires - jiffies);
> > 
> > but yes, need to check with timer_pending firstly.
> 
> I have changed the code to this:
> 
> | struct timer_list *t3_rtx = &asoc->peer.primary_path->T3_rtx_timer;
> | 
> | [...]
> | 
> | 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);
> | }
> 
> And I'm still getting what appears to be negative values sometimes. Here
> are some of the common values in hex when busy looping sctp_diag
> requests:
> 
> 0
> 7530
> 1000000
> 3000000
> 6000000
> 14000000
> 94000000
> ed690000
> ffffea00

Are these for the same asoc? I wouldn't expect it to vary that much.
Even the 1000000 it's already just too big to be reasonable. That's
16777 seconds. Only 0x7530 is reasonable, 30 seconds.

> 
> While I wonder a bit about the zero, the last two seem to be unsigned
> underruns. Do I still have to check for 't3_rtx->expires > jiffies' or
> am I missing something?

You shouldn't have to because then the timer wouldn't be pending. 

I don't know what can be wrong in there. Could it be the application not
checking if the timer was exported or not before dumping it? </longshot>

  Marcelo

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

[Phil Sutter]

On Wed, Aug 03, 2016 at 04:46:52PM -0300, Marcelo Ricardo Leitner wrote:
> On Wed, Aug 03, 2016 at 09:28:13PM +0200, Phil Sutter wrote:
> > Hi,
> > 
> > On Sat, Jul 30, 2016 at 09:25:42PM +0800, Xin Long wrote:
> > [...]
> > > Now for the transport's info,  we only choose primary_path to dump.
> > > It means we should fix this by getting the left time to expire from
> > > primary transport t->T3_rtx_timer. like:
> > > 
> > >         r->idiag_expires = jiffies_to_msecs(
> > > -               asoc->timeouts[SCTP_EVENT_TIMEOUT_T3_RTX] - jiffies);
> > > +               asoc->peer.primary_path->T3_rtx_timer.expires - jiffies);
> > > 
> > > but yes, need to check with timer_pending firstly.
> > 
> > I have changed the code to this:
> > 
> > | struct timer_list *t3_rtx = &asoc->peer.primary_path->T3_rtx_timer;
> > | 
> > | [...]
> > | 
> > | 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);
> > | }
> > 
> > And I'm still getting what appears to be negative values sometimes. Here
> > are some of the common values in hex when busy looping sctp_diag
> > requests:
> > 
> > 0
> > 7530
> > 1000000
> > 3000000
> > 6000000
> > 14000000
> > 94000000
> > ed690000
> > ffffea00
> 
> Are these for the same asoc? I wouldn't expect it to vary that much.
> Even the 1000000 it's already just too big to be reasonable. That's
> 16777 seconds. Only 0x7530 is reasonable, 30 seconds.

Nope, those are not for the same asoc. Also, I piped the gathered values
through 'sort -u', so they are not in chronological order.

> > While I wonder a bit about the zero, the last two seem to be unsigned
> > underruns. Do I still have to check for 't3_rtx->expires > jiffies' or
> > am I missing something?
> 
> You shouldn't have to because then the timer wouldn't be pending. 
> 
> I don't know what can be wrong in there. Could it be the application not
> checking if the timer was exported or not before dumping it? </longshot>

Nope, the application is 'ss' in that case (with added debug output to
get the raw values), but your shot wasn't that long after all: I
discovered that in sctp_diag.ko, the inet_diag_msg object to be sent to
userspace is not cleared initially. So by populating r->idiag_timer
conditionally, I managed to leak random data to user space. DOH!

Thanks for the pointer,

Phil

[PATCH 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 0ad6033a7330c..92ae2828189d5 100644
--- a/net/sctp/sctp_diag.c
+++ b/net/sctp/sctp_diag.c
@@ -352,7 +352,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 &&
@@ -467,7 +467,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