netdev - Re: [PATCH net v2 1/2] sctp: do not retransmit upon FragNeeded if PMTU discovery is disabled

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]

Date:   Thu, 4 Jan 2018 21:29:33 +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 v2 1/2] sctp: do not retransmit upon FragNeeded if
 PMTU discovery is disabled

On Thu, Jan 4, 2018 at 7:23 PM, Marcelo Ricardo Leitner
<marcelo.leitner@...il.com> wrote:
> On Thu, Jan 04, 2018 at 12:52:32PM +0800, Xin Long wrote:
>> On Thu, Jan 4, 2018 at 6:59 AM, Marcelo Ricardo Leitner
>> <marcelo.leitner@...il.com> wrote:
>> > Currently, if PMTU discovery is disabled on a given transport, but the
>> > configured value is higher than the actual PMTU, it is likely that we
>> > will get some icmp Frag Needed. The issue is, if PMTU discovery is
>> > disabled, we won't update the information and will issue a
>> > retransmission immediately, which may very well trigger another ICMP,
>> > and another retransmission, leading to a loop.
>> >
>> > The fix is to simply not trigger immediate retransmissions if PMTU
>> > discovery is disabled on the given transport.
>> >
>> > Signed-off-by: Marcelo Ricardo Leitner <marcelo.leitner@...il.com>
>> > ---
>> >  net/sctp/input.c | 17 +++++++++++------
>> >  1 file changed, 11 insertions(+), 6 deletions(-)
>> >
>> > diff --git a/net/sctp/input.c b/net/sctp/input.c
>> > index 621b5ca3fd1c17c3d7ef7bb1c7677ab98cebbe77..4a8e76f4834c90de9398455862423e598b8354a7 100644
>> > --- a/net/sctp/input.c
>> > +++ b/net/sctp/input.c
>> > @@ -399,13 +399,18 @@ void sctp_icmp_frag_needed(struct sock *sk, struct sctp_association *asoc,
>> >                 return;
>> >         }
>> >
>> > -       if (t->param_flags & SPP_PMTUD_ENABLE) {
>> > -               /* Update transports view of the MTU */
>> > -               sctp_transport_update_pmtu(t, pmtu);
>> > +       if (!(t->param_flags & SPP_PMTUD_ENABLE))
>> > +               /* We can't allow retransmitting in such case, as the
>> > +                * retransmission would be sized just as before, and thus we
>> > +                * would get another icmp, and retransmit again.
>> > +                */
>> > +               return;
>> >
>> > -               /* Update association pmtu. */
>> > -               sctp_assoc_sync_pmtu(asoc);
>> > -       }
>> > +       /* Update transports view of the MTU */
>> > +       sctp_transport_update_pmtu(t, pmtu);
>> > +
>> > +       /* Update association pmtu. */
>> > +       sctp_assoc_sync_pmtu(asoc);
>> >
>> >         /* Retransmit with the new pmtu setting.
>> >          * Normally, if PMTU discovery is disabled, an ICMP Fragmentation
>> > --
>> > 2.14.3
>> >
>>
>> commit 52ccb8e90c0ace233b8b740f2fc5de0dbd706b27
>> Author: Frank Filz <ffilz@...ibm.com>
>> Date:   Thu Dec 22 11:36:46 2005 -0800
>>
>>     [SCTP]: Update SCTP_PEER_ADDR_PARAMS socket option to the latest api draft.
>>
>> It seemed intended to move sctp_retransmit out of 'if (SPP_PMTUD_ENABLE) {}'
>> on the above commit with some notes:
>
> Good point.
>
>>
>>         /* Retransmit with the new pmtu setting.
>>          * Normally, if PMTU discovery is disabled, an ICMP Fragmentation
>>          * 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.
>>          */
>>
>> But this patch is equivalent to move it back into 'if (SPP_PMTUD_ENABLE) {}'.
>> will there be no regression caused?
>
> I don't think this comment has been effective because the function
> starts with:
>
> void sctp_icmp_frag_needed(struct sock *sk, struct sctp_association *asoc,
>                            struct sctp_transport *t, __u32 pmtu)
> {
>         if (!t || (t->pathmtu <= pmtu))
>                 return;
>
> So if the application managed to adjust pmtu after sending some data,
> t->pathmtu will fit this check and nothing would be done anyway.
>
> commit 91bd6b1e030266cf87d3f567b49f0fa60a7318ba
> Author: Wei Yongjun <yjwei@...fujitsu.com>
> Date:   Thu Oct 23 00:59:52 2008 -0700
>
>     sctp: Drop ICMP packet too big message with MTU larger than
>     current PMTU
>
> I guess I should have removed this comment too. WDYT?
Yes, I agree. Next time the retransmit could also fragment it, and
it actually rarely happens, so no need to do it that immediately.

> I'll prepare a v3 meanwhile.