netdev - Re: [PATCH net] sctp: fix handling of ICMP Frag Needed for too small MTUs

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] [day] [month] [year] [list]
Message-ID: <CADvbK_fEziqWGay51uTsnB6PLNbO+qguzDO28WN2_t-3thSHBg@mail.gmail.com>
Date:   Wed, 3 Jan 2018 23:31:17 +0800
From:   Xin Long <lucien.xin@...il.com>
To:     Marcelo Ricardo Leitner <marcelo.leitner@...il.com>
Cc:     network dev <netdev@...r.kernel.org>, linux-sctp@...r.kernel.org,
        Vlad Yasevich <vyasevich@...il.com>,
        Neil Horman <nhorman@...driver.com>
Subject: Re: [PATCH net] sctp: fix handling of ICMP Frag Needed for too small MTUs

On Wed, Jan 3, 2018 at 9:35 PM, Marcelo Ricardo Leitner
<marcelo.leitner@...il.com> wrote:
> On Wed, Jan 03, 2018 at 03:31:00PM +0800, Xin Long wrote:
>> On Wed, Jan 3, 2018 at 5:44 AM, Marcelo Ricardo Leitner
>> <marcelo.leitner@...il.com> wrote:
>> > syzbot reported a hang involving SCTP, on which it kept flooding dmesg
>> > with the message:
>> > [  246.742374] sctp: sctp_transport_update_pmtu: Reported pmtu 508 too
>> > low, using default minimum of 512
>> >
>> > That happened because whenever SCTP hits an ICMP Frag Needed, it tries
>> > to adjust to the new MTU and triggers an immediate retransmission. But
>> > it didn't consider the fact that MTUs smaller than the SCTP minimum MTU
>> > allowed (512) would not cause the PMTU to change, and issued the
>> > retransmission anyway (thus leading to another ICMP Frag Needed, and so
>> > on).
>> >
>> > The fix is to disable Path MTU discovery for such transport and to skip
>> > the retransmission in such cases. By doing this, SCTP will do the
>> > backoff retransmissions as needed and will likely switch to another
>> > transport if available.
>> >
>> > See-also: https://lkml.org/lkml/2017/12/22/811
>> > Reported-by: syzbot <syzkaller@...glegroups.com>
>> > Signed-off-by: Marcelo Ricardo Leitner <marcelo.leitner@...il.com>
>> > ---
>> >  net/sctp/input.c     | 5 ++++-
>> >  net/sctp/transport.c | 2 ++
>> >  2 files changed, 6 insertions(+), 1 deletion(-)
>> >
>> > diff --git a/net/sctp/input.c b/net/sctp/input.c
>> > index 621b5ca3fd1c17c3d7ef7bb1c7677ab98cebbe77..a24658c6f181e03d85f12dbe929c8bb4abaefcbd 100644
>> > --- a/net/sctp/input.c
>> > +++ b/net/sctp/input.c
>> > @@ -412,8 +412,11 @@ void sctp_icmp_frag_needed(struct sock *sk, struct sctp_association *asoc,
>> >          * Needed will never be sent, but if a message was sent before
>> >          * PMTU discovery was disabled that was larger than the PMTU, it
>> >          * would not be fragmented, so it must be re-transmitted fragmented.
>> > +        * If the new PMTU is invalid, we will keep getting ICMP Frag
>> > +        * Needed. In this case, simply avoid the retransmit.
>> >          */
>> > -       sctp_retransmit(&asoc->outqueue, t, SCTP_RTXR_PMTUD);
>> > +       if (pmtu >= SCTP_DEFAULT_MINSEGMENT)
>> > +               sctp_retransmit(&asoc->outqueue, t, SCTP_RTXR_PMTUD);
>> >  }
>> >
>> >  void sctp_icmp_redirect(struct sock *sk, struct sctp_transport *t,
>> > diff --git a/net/sctp/transport.c b/net/sctp/transport.c
>> > index 1e5a22430cf56e40a6f323081beb97836b506384..fbd9fe25764d4d98f93c60a48eccefd9cc6b4165 100644
>> > --- a/net/sctp/transport.c
>> > +++ b/net/sctp/transport.c
>> > @@ -259,6 +259,8 @@ void sctp_transport_update_pmtu(struct sctp_transport *t, u32 pmtu)
>> >                  * pmtu discovery on this transport.
>> >                  */
>> >                 t->pathmtu = SCTP_DEFAULT_MINSEGMENT;
>> > +               t->param_flags = (t->param_flags & ~SPP_PMTUD) |
>> > +                                SPP_PMTUD_DISABLE;
>> It seems that once it hits here,  this transport will have the minimum pmtu
>> forever, even after t->dst has expired. It means this tx path will not come
>> back to normal any more even when it gets a needfrag with reasonable
>> pmtu.  is it too harsh to this transport ?
>
> That was the idea. That is what the comment above these lines is
> describing already. Though I missed 06ad391919b2 ("[SCTP] Don't
> disable PMTU discovery when mtu is small") and yes, too harsh.
>
>>
>> Another thing is on sctp_sendmsg, it also checks pmtu_pending that may
>> be set by needfrag, and goes to sctp_assoc_sync_pmtu to trigger this
>> warning again.
>
> That is true but that's not an issue, is it? We are not trying to get
> ride of the warning, instead we want to not cause a flood of
> bogus retransmissions (which led to the flood of warnings).
Right, I guess that the flood of warnings mostly came from that
sctp_retransmit() in sctp_icmp_frag_needed().
Otherwise, that transport should be marked as 'unreachable'
or the asoc should abort after so many times rtx.

>
> By not disabling PMTU discovery (as above) we will have such warning
> every now and then again for the same transport. We may add
> _ratelimited to it, that would help in the case of we have like a
> thousand transports suddenly being affected by such small MTU, but
> won't omit it completely.
If it can't be avoided only with the check 'pmtu >= SCTP_DEFAULT_MINSEGMENT',
yeah, _ratelimited looks good. :-)

>
> I'll spin a v2, thanks.
>
>>
>> >         } else {
>> >                 t->pathmtu = pmtu;
>> >         }
>> > --
>> > 2.14.3
>> >
>> --
>> 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