On Thu, Jan 4, 2018 at 7:23 PM, Marcelo Ricardo Leitner
<marcelo.leitner@xxxxxxxxx> 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@xxxxxxxxx> 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@xxxxxxxxx>
>> > ---
>> > 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@xxxxxxxxxx>
>> 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@xxxxxxxxxxxxxx>
> 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.
--
To unsubscribe from this list: send the line "unsubscribe linux-sctp" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
