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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <ffdc7f0d-2c98-d63a-725a-01f738689907@gmail.com>
Date:	Sat, 30 Jul 2016 10:33:48 -0300
From:	Marcelo Ricardo Leitner <marcelo.leitner@...il.com>
To:	Xin Long <lucien.xin@...il.com>
Cc:	Phil Sutter <phil@....cc>, David Miller <davem@...emloft.net>,
	network dev <netdev@...r.kernel.org>
Subject: Re: [PATCH 2/3] sctp_diag: export timer value only if it is active



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?