> On 26. May 2018, at 17:50, Dmitry Vyukov <dvyu...@google.com> wrote: > > On Sat, May 26, 2018 at 5:42 PM, Michael Tuexen > <michael.tue...@lurchi.franken.de> wrote: >>> On 25. May 2018, at 21:13, Neil Horman <nhor...@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+3dcd59a1f907245f8...@syzkaller.appspotmail.com >>>> Suggested-by: Marcelo Ricardo Leitner <marcelo.leit...@gmail.com> >>>> Signed-off-by: Xin Long <lucien....@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 majord...@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? I have seen values of RTO.Min = 50ms RTO.Max = 200ms RTO.Initial = 100ms
Best regards Michael > > 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?