Re: [PATCH net] sctp: not allow to set rto_min with a value below 200 msecs

[Dmitry Vyukov <dvyukov@google.com>]

On Sat, May 26, 2018 at 5:42 PM, Michael Tuexen
<michael.tuexen@lurchi.franken.de> wrote:
>> On 25. May 2018, at 21:13, Neil Horman <nhorman@tuxdriver.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@syzkaller.appspotmail.com
>>> Suggested-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>
>>> Signed-off-by: Xin Long <lucien.xin@gmail.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@vger.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?