netdev - Re: [PATCH net] sctp: not allow to set rto

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: <CACT4Y+YozRSfcoUoKHOWy5wujhVdks38vcfNGhwNj-REWcd-hw@mail.gmail.com>
Date:   Sat, 26 May 2018 17:50:39 +0200
From:   Dmitry Vyukov <dvyukov@...gle.com>
To:     Michael Tuexen <michael.tuexen@...chi.franken.de>
Cc:     Neil Horman <nhorman@...driver.com>,
        Xin Long <lucien.xin@...il.com>,
        network dev <netdev@...r.kernel.org>,
        linux-sctp@...r.kernel.org, David Miller <davem@...emloft.net>,
        David Ahern <dsa@...ulusnetworks.com>,
        Eric Dumazet <edumazet@...gle.com>,
        Marcelo Ricardo Leitner <marcelo.leitner@...il.com>,
        syzkaller <syzkaller@...glegroups.com>
Subject: Re: [PATCH net] sctp: not allow to set rto_min with a value below 200 msecs

On Sat, May 26, 2018 at 5:42 PM, Michael Tuexen
<michael.tuexen@...chi.franken.de> wrote:
>> On 25. May 2018, at 21:13, Neil Horman <nhorman@...driver.com> wrote:
>>
>> On Sat, May 26, 2018 at 01:41:02AM +0800, Xin Long wrote:
>>> syzbot reported a rcu_sched self-detected stall on CPU which is caused
>>> by too small value set on rto_min with SCTP_RTOINFO sockopt. With this
>>> value, hb_timer will get stuck there, as in its timer handler it starts
>>> this timer again with this value, then goes to the timer handler again.
>>>
>>> This problem is there since very beginning, and thanks to Eric for the
>>> reproducer shared from a syzbot mail.
>>>
>>> This patch fixes it by not allowing to set rto_min with a value below
>>> 200 msecs, which is based on TCP's, by either setsockopt or sysctl.
>>>
>>> Reported-by: syzbot+3dcd59a1f907245f891f@...kaller.appspotmail.com
>>> Suggested-by: Marcelo Ricardo Leitner <marcelo.leitner@...il.com>
>>> Signed-off-by: Xin Long <lucien.xin@...il.com>
>>> ---
>>> include/net/sctp/constants.h |  1 +
>>> net/sctp/socket.c            | 10 +++++++---
>>> net/sctp/sysctl.c            |  3 ++-
>>> 3 files changed, 10 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/include/net/sctp/constants.h b/include/net/sctp/constants.h
>>> index 20ff237..2ee7a7b 100644
>>> --- a/include/net/sctp/constants.h
>>> +++ b/include/net/sctp/constants.h
>>> @@ -277,6 +277,7 @@ enum { SCTP_MAX_GABS = 16 };
>>> #define SCTP_RTO_INITIAL     (3 * 1000)
>>> #define SCTP_RTO_MIN         (1 * 1000)
>>> #define SCTP_RTO_MAX         (60 * 1000)
>>> +#define SCTP_RTO_HARD_MIN   200
>>>
>>> #define SCTP_RTO_ALPHA          3   /* 1/8 when converted to right shifts. */
>>> #define SCTP_RTO_BETA           2   /* 1/4 when converted to right shifts. */
>>> diff --git a/net/sctp/socket.c b/net/sctp/socket.c
>>> index ae7e7c6..6ef12c7 100644
>>> --- a/net/sctp/socket.c
>>> +++ b/net/sctp/socket.c
>>> @@ -3029,7 +3029,8 @@ static int sctp_setsockopt_nodelay(struct sock *sk, char __user *optval,
>>>  * be changed.
>>>  *
>>>  */
>>> -static int sctp_setsockopt_rtoinfo(struct sock *sk, char __user *optval, unsigned int optlen)
>>> +static int sctp_setsockopt_rtoinfo(struct sock *sk, char __user *optval,
>>> +                               unsigned int optlen)
>>> {
>>>      struct sctp_rtoinfo rtoinfo;
>>>      struct sctp_association *asoc;
>>> @@ -3056,10 +3057,13 @@ static int sctp_setsockopt_rtoinfo(struct sock *sk, char __user *optval, unsigne
>>>      else
>>>              rto_max = asoc ? asoc->rto_max : sp->rtoinfo.srto_max;
>>>
>>> -    if (rto_min)
>>> +    if (rto_min) {
>>> +            if (rto_min < SCTP_RTO_HARD_MIN)
>>> +                    return -EINVAL;
>>>              rto_min = asoc ? msecs_to_jiffies(rto_min) : rto_min;
>>> -    else
>>> +    } else {
>>>              rto_min = asoc ? asoc->rto_min : sp->rtoinfo.srto_min;
>>> +    }
>>>
>>>      if (rto_min > rto_max)
>>>              return -EINVAL;
>>> diff --git a/net/sctp/sysctl.c b/net/sctp/sysctl.c
>>> index 33ca5b7..7ec854a 100644
>>> --- a/net/sctp/sysctl.c
>>> +++ b/net/sctp/sysctl.c
>>> @@ -52,6 +52,7 @@ static int rto_alpha_min = 0;
>>> static int rto_beta_min = 0;
>>> static int rto_alpha_max = 1000;
>>> static int rto_beta_max = 1000;
>>> +static int rto_hard_min = SCTP_RTO_HARD_MIN;
>>>
>>> static unsigned long max_autoclose_min = 0;
>>> static unsigned long max_autoclose_max =
>>> @@ -116,7 +117,7 @@ static struct ctl_table sctp_net_table[] = {
>>>              .maxlen         = sizeof(unsigned int),
>>>              .mode           = 0644,
>>>              .proc_handler   = proc_sctp_do_rto_min,
>>> -            .extra1         = &one,
>>> +            .extra1         = &rto_hard_min,
>>>              .extra2         = &init_net.sctp.rto_max
>>>      },
>>>      {
>>> --
>>> 2.1.0
>>>
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe linux-sctp" in
>>> the body of a message to majordomo@...r.kernel.org
>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>>
>> Patch looks fine, you probably want to note this hard minimum in man(7) sctp as
>> well
>>
> I'm aware of some signalling networks which use RTO.min of smaller values than 200ms.
> So could this be reduced?

Hi Michael,

What value do they use?

Xin, Neil, is there more principled way of ensuring that a timer won't
cause a hard CPU stall? There are slow machines and there are slow
kernels (in particular syzbot kernel has tons of debug configs
enabled). 200ms _should_ not cause problems because we did not see
them with tcp. But it's hard to say what's the low limit as we are
trying to put a hard upper bound on execution time of a complex
section of code. Is there something like cond_resched for timers?