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] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 4 Jan 2018 12:52:32 +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 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:

        /* 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?