From: Michael Tuexen <michael.tuexen@lurchi.franken.de> To: Dmitry Vyukov <dvyukov@google.com> Cc: Neil Horman <nhorman@tuxdriver.com>, Xin Long <lucien.xin@gmail.com>, network dev <netdev@vger.kernel.org>, linux-sctp@vger.kernel.org, David Miller <davem@davemloft.net>, David Ahern <dsa@cumulusnetworks.com>, Eric Dumazet <edumazet@google.com>, Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>, syzkaller <syzkaller@googlegroups.com> Subject: Re: [PATCH net] sctp: not allow to set rto_min with a value below 200 msecs Date: Sun, 27 May 2018 10:58:35 +0200 [thread overview] Message-ID: <D8455431-D0D7-4420-BEBF-E0B5BE5DBC97@lurchi.franken.de> (raw) In-Reply-To: <CACT4Y+YozRSfcoUoKHOWy5wujhVdks38vcfNGhwNj-REWcd-hw@mail.gmail.com> > On 26. May 2018, at 17:50, Dmitry Vyukov <dvyukov@google.com> wrote: > > 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? 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?
WARNING: multiple messages have this Message-ID (diff)
From: Michael Tuexen <michael.tuexen@lurchi.franken.de> To: Dmitry Vyukov <dvyukov@google.com> Cc: Neil Horman <nhorman@tuxdriver.com>, Xin Long <lucien.xin@gmail.com>, network dev <netdev@vger.kernel.org>, linux-sctp@vger.kernel.org, David Miller <davem@davemloft.net>, David Ahern <dsa@cumulusnetworks.com>, Eric Dumazet <edumazet@google.com>, Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>, syzkaller <syzkaller@googlegroups.com> Subject: Re: [PATCH net] sctp: not allow to set rto_min with a value below 200 msecs Date: Sun, 27 May 2018 08:58:35 +0000 [thread overview] Message-ID: <D8455431-D0D7-4420-BEBF-E0B5BE5DBC97@lurchi.franken.de> (raw) In-Reply-To: <CACT4Y+YozRSfcoUoKHOWy5wujhVdks38vcfNGhwNj-REWcd-hw@mail.gmail.com> > On 26. May 2018, at 17:50, Dmitry Vyukov <dvyukov@google.com> wrote: > > 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? 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?
next prev parent reply other threads:[~2018-05-27 8:58 UTC|newest] Thread overview: 34+ messages / expand[flat|nested] mbox.gz Atom feed top 2018-05-25 17:41 [PATCH net] sctp: not allow to set rto_min with a value below 200 msecs Xin Long 2018-05-25 17:41 ` Xin Long 2018-05-25 19:13 ` Neil Horman 2018-05-25 19:13 ` Neil Horman 2018-05-26 15:42 ` Michael Tuexen 2018-05-26 15:42 ` Michael Tuexen 2018-05-26 15:50 ` Dmitry Vyukov 2018-05-26 15:50 ` Dmitry Vyukov 2018-05-27 1:01 ` Neil Horman 2018-05-27 1:01 ` Neil Horman 2018-05-28 19:43 ` Marcelo Ricardo Leitner 2018-05-28 19:43 ` Marcelo Ricardo Leitner 2018-05-29 11:41 ` Neil Horman 2018-05-29 11:41 ` Neil Horman 2018-05-29 13:06 ` Michael Tuexen 2018-05-29 13:06 ` Michael Tuexen 2018-05-29 15:45 ` Marcelo Ricardo Leitner 2018-05-29 15:45 ` Marcelo Ricardo Leitner 2018-05-29 16:03 ` Neal Cardwell 2018-05-29 16:03 ` Neal Cardwell 2018-05-29 17:06 ` Marcelo Ricardo Leitner 2018-05-29 17:06 ` Marcelo Ricardo Leitner 2018-05-29 17:45 ` Xin Long 2018-05-29 17:45 ` Xin Long 2018-05-29 18:02 ` Marcelo Ricardo Leitner 2018-05-29 18:02 ` Marcelo Ricardo Leitner 2018-06-04 8:34 ` Dmitry Vyukov 2018-06-04 8:34 ` Dmitry Vyukov 2018-06-04 12:15 ` Xin Long 2018-06-04 12:15 ` Xin Long 2018-05-27 8:58 ` Michael Tuexen [this message] 2018-05-27 8:58 ` Michael Tuexen 2018-05-28 18:56 ` Marcelo Ricardo Leitner 2018-05-28 18:56 ` Marcelo Ricardo Leitner
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=D8455431-D0D7-4420-BEBF-E0B5BE5DBC97@lurchi.franken.de \ --to=michael.tuexen@lurchi.franken.de \ --cc=davem@davemloft.net \ --cc=dsa@cumulusnetworks.com \ --cc=dvyukov@google.com \ --cc=edumazet@google.com \ --cc=linux-sctp@vger.kernel.org \ --cc=lucien.xin@gmail.com \ --cc=marcelo.leitner@gmail.com \ --cc=netdev@vger.kernel.org \ --cc=nhorman@tuxdriver.com \ --cc=syzkaller@googlegroups.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.