From patchwork Wed Jan 3 22:59:52 2018 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Marcelo Ricardo Leitner X-Patchwork-Id: 855304 X-Patchwork-Delegate: davem@davemloft.net Return-Path: X-Original-To: patchwork-incoming@ozlabs.org Delivered-To: patchwork-incoming@ozlabs.org Authentication-Results: ozlabs.org; spf=none (mailfrom) smtp.mailfrom=vger.kernel.org (client-ip=209.132.180.67; helo=vger.kernel.org; envelope-from=netdev-owner@vger.kernel.org; receiver=) Authentication-Results: ozlabs.org; dkim=pass (2048-bit key; unprotected) header.d=gmail.com header.i=@gmail.com header.b="nqFcbZ2Q"; dkim-atps=neutral Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by ozlabs.org (Postfix) with ESMTP id 3zBmcG4mgwz9s7G for ; Thu, 4 Jan 2018 10:00:30 +1100 (AEDT) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751441AbeACXA2 (ORCPT ); Wed, 3 Jan 2018 18:00:28 -0500 Received: from mail-qt0-f195.google.com ([209.85.216.195]:45393 "EHLO mail-qt0-f195.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751014AbeACXA0 (ORCPT ); Wed, 3 Jan 2018 18:00:26 -0500 Received: by mail-qt0-f195.google.com with SMTP id g10so4025980qtj.12; Wed, 03 Jan 2018 15:00:25 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=from:to:cc:subject:date:message-id:in-reply-to:references; bh=t16lx0oykZQogCRLCEuto1ifUWVAqwRMhWGPrOGmBg0=; b=nqFcbZ2QenrTKXws1hrG9f0bFA0V4m12gXEbQpLXNEkFl5eDc8q6X9TO/4mzRnRnCt cCmyQVdej7Vxve0hwu/4Z3rsHkRoRgGmaidAP6nT5OvlKnsQRH/zdtzBYfwWleW3dmxb 4PYTirkkTZizj78Koje/14UGbgCOwaH9wIv/mZNmaWi0P0W6cgswFqFQoLe7mjTwNQUt s/6qqwrR/9XB/472sxvKdJtHnd5TA0/PEhcaWYrBGKvzaIbg6Db/GfZyhBD4DPkvsjNA 4nI6iJZhhNxd4XJg3/oLWcWGRGKdPF4tll/3Kecc/kwfg0GRuGdu+YRLF24reUDME8rk LWLA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:from:to:cc:subject:date:message-id:in-reply-to :references; bh=t16lx0oykZQogCRLCEuto1ifUWVAqwRMhWGPrOGmBg0=; b=CRfjzjOE7kFXUkSwcpBQuKGUU6WArW1Q6YSjeoV6oLaYvB5xz6r56lMijLFSC1dk3v /15eZPDlkTiGo4GO5Dufej0gqeHst7HUQqcTRk4w1EIA/rj+fN5A8JuAeAL7ms8azAim MpnFad4L9kdDx4nYBnD20OJqEaeCAH5oOZc2PSWK1OLXwjHmVFtA+tfuUX0dRG9MZmG2 aJIcoE6Lw1CZzYKXIpMYZKvjrKBKXVWpvAAlfm4sNl1uzjewiCiqh/EZQ50c88/vk64Y CeA0gCWOKzCZ3pFjNO9BExNtX7YI48gvLzNhdfOciOai+x4rm9rLLQBiU9mSZjvYDpXv /c6A== X-Gm-Message-State: AKGB3mJWbzrsxcOdnzXUX3wsfmJs9cwPZXYvbBapu45Gqq/vM32mopAY QmI14N6N/JSlrVV+FVlhAAprvw== X-Google-Smtp-Source: ACJfBoud6HAxlf1lq6UqaJSqX8faa0IIbn+qgWvhEsdVnT/k6pBYSQiE+z1/4Ewp9KLL471mIDqcGw== X-Received: by 10.200.42.228 with SMTP id c33mr4256181qta.256.1515020424944; Wed, 03 Jan 2018 15:00:24 -0800 (PST) Received: from localhost.localdomain.com ([2001:1284:f013:23a9:9544:d086:4c8e:2344]) by smtp.gmail.com with ESMTPSA id v50sm1309462qta.0.2018.01.03.15.00.22 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Wed, 03 Jan 2018 15:00:24 -0800 (PST) From: Marcelo Ricardo Leitner To: netdev@vger.kernel.org Cc: linux-sctp@vger.kernel.org, Xin Long , Vlad Yasevich , Neil Horman Subject: [PATCH net v2 2/2] sctp: fix the handling of ICMP Frag Needed for too small MTUs Date: Wed, 3 Jan 2018 20:59:52 -0200 Message-Id: X-Mailer: git-send-email 2.14.3 In-Reply-To: References: Sender: netdev-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: netdev@vger.kernel.org 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). As IPv4 (ip_rt_min_pmtu=556) and IPv6 (IPV6_MIN_MTU=1280) minimum MTU are higher than that, sctp_transport_update_pmtu() is changed to re-fetch the PMTU that got set after our request, and with that, detect if there was an actual change or not. The fix, thus, skips the immediate retransmission if the received ICMP resulted in no change, in the hope that SCTP will select another path. Note: The value being used for the minimum MTU (512, SCTP_DEFAULT_MINSEGMENT) is not right and instead it should be (576, SCTP_MIN_PMTU), but such change belongs to another patch. Changes from v1: - do not disable PMTU discovery, in the light of commit 06ad391919b2 ("[SCTP] Don't disable PMTU discovery when mtu is small") and as suggested by Xin Long. - changed the way to break the rtx loop by detecting if the icmp resulted in a change or not. - change the warn to use pr_warn_ratelimited, to protect in case there are several transports using such path. See-also: https://lkml.org/lkml/2017/12/22/811 Reported-by: syzbot Signed-off-by: Marcelo Ricardo Leitner --- include/net/sctp/structs.h | 2 +- net/sctp/input.c | 8 ++++++-- net/sctp/transport.c | 29 +++++++++++++++++++---------- 3 files changed, 26 insertions(+), 13 deletions(-) diff --git a/include/net/sctp/structs.h b/include/net/sctp/structs.h index 2f8f93da5dc2660f4db37c04f8a434809b3120a1..9a5ccf03a59b1e0a4820e2a978c66f1df8a9a82f 100644 --- a/include/net/sctp/structs.h +++ b/include/net/sctp/structs.h @@ -966,7 +966,7 @@ void sctp_transport_burst_limited(struct sctp_transport *); void sctp_transport_burst_reset(struct sctp_transport *); unsigned long sctp_transport_timeout(struct sctp_transport *); void sctp_transport_reset(struct sctp_transport *t); -void sctp_transport_update_pmtu(struct sctp_transport *t, u32 pmtu); +bool sctp_transport_update_pmtu(struct sctp_transport *t, u32 pmtu); void sctp_transport_immediate_rtx(struct sctp_transport *); void sctp_transport_dst_release(struct sctp_transport *t); void sctp_transport_dst_confirm(struct sctp_transport *t); diff --git a/net/sctp/input.c b/net/sctp/input.c index 4a8e76f4834c90de9398455862423e598b8354a7..3cddbecabaa2db410971c66d3256c466c7f5289f 100644 --- a/net/sctp/input.c +++ b/net/sctp/input.c @@ -406,8 +406,12 @@ void sctp_icmp_frag_needed(struct sock *sk, struct sctp_association *asoc, */ return; - /* Update transports view of the MTU */ - sctp_transport_update_pmtu(t, pmtu); + /* Update transports view of the MTU. Return if no update was needed. + * If an update wasn't needed/possible, it also doesn't make sense to + * try to retransmit now. + */ + if (!sctp_transport_update_pmtu(t, pmtu)) + return; /* Update association pmtu. */ sctp_assoc_sync_pmtu(asoc); diff --git a/net/sctp/transport.c b/net/sctp/transport.c index 1e5a22430cf56e40a6f323081beb97836b506384..47f82bd794d915188bad037463c2aa14175a55ef 100644 --- a/net/sctp/transport.c +++ b/net/sctp/transport.c @@ -248,28 +248,37 @@ void sctp_transport_pmtu(struct sctp_transport *transport, struct sock *sk) transport->pathmtu = SCTP_DEFAULT_MAXSEGMENT; } -void sctp_transport_update_pmtu(struct sctp_transport *t, u32 pmtu) +bool sctp_transport_update_pmtu(struct sctp_transport *t, u32 pmtu) { struct dst_entry *dst = sctp_transport_dst_check(t); + bool change = true; if (unlikely(pmtu < SCTP_DEFAULT_MINSEGMENT)) { - pr_warn("%s: Reported pmtu %d too low, using default minimum of %d\n", - __func__, pmtu, SCTP_DEFAULT_MINSEGMENT); - /* Use default minimum segment size and disable - * pmtu discovery on this transport. - */ - t->pathmtu = SCTP_DEFAULT_MINSEGMENT; - } else { - t->pathmtu = pmtu; + pr_warn_ratelimited("%s: Reported pmtu %d too low, using default minimum of %d\n", + __func__, pmtu, SCTP_DEFAULT_MINSEGMENT); + /* Use default minimum segment instead */ + pmtu = SCTP_DEFAULT_MINSEGMENT; } + pmtu = SCTP_TRUNC4(pmtu); if (dst) { dst->ops->update_pmtu(dst, t->asoc->base.sk, NULL, pmtu); dst = sctp_transport_dst_check(t); } - if (!dst) + if (!dst) { t->af_specific->get_dst(t, &t->saddr, &t->fl, t->asoc->base.sk); + dst = t->dst; + } + + if (dst) { + /* Re-fetch, as under layers may have a higher minimum size */ + pmtu = SCTP_TRUNC4(dst_mtu(dst)); + change = t->pathmtu != pmtu; + } + t->pathmtu = pmtu; + + return change; } /* Caches the dst entry and source address for a transport's destination