From patchwork Thu Jun 17 00:09:50 2021 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: =?utf-8?q?Maciej_=C5=BBenczykowski?= X-Patchwork-Id: 12326301 X-Patchwork-Delegate: bpf@iogearbox.net Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-15.8 required=3.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,FREEMAIL_FORGED_FROMDOMAIN,FREEMAIL_FROM, HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_CR_TRAILER,INCLUDES_PATCH, MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,USER_AGENT_GIT autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id E4ED7C48BE5 for ; Thu, 17 Jun 2021 00:10:09 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id BC9B5613BD for ; Thu, 17 Jun 2021 00:10:09 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S231686AbhFQAMP (ORCPT ); Wed, 16 Jun 2021 20:12:15 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:41974 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S230481AbhFQAMP (ORCPT ); Wed, 16 Jun 2021 20:12:15 -0400 Received: from mail-pl1-x630.google.com (mail-pl1-x630.google.com [IPv6:2607:f8b0:4864:20::630]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 36A79C061574; Wed, 16 Jun 2021 17:10:07 -0700 (PDT) Received: by mail-pl1-x630.google.com with SMTP id e7so1990202plj.7; Wed, 16 Jun 2021 17:10:07 -0700 (PDT) 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 :mime-version:content-transfer-encoding; bh=UHZ5YMekjSRT+s0xrshOz5js1a8Rx9jxwpgPSLh3c50=; b=VcPw2lDwWr8gXttl6/n11H2bPuN7FSdlgQb0nNY2CZT4rrtr62H5uf0D3JNbyUYN9v gsmSS0OHfQ7c9KUdEXSNfEfvkTZLtkX9+bN/rLrl0g+934gYyKR1svtXcU19qoQv3YNP QhmI4XgFWin3PmmAv1BoROOMLi3vTtj8xwabXCwsmcZcaHFJSoMha5Z704wIlJR1T82H JWw/XS6qbHWYDm6c+wGKPeT2XeEwFZkaYC0dNpu559fwAXBSIdlaTkBIZlcG+cRJ2LAt YVu1rPMcbuNQmWcxTrv7+TylTGnhgw/9qhwqkFLpebAeSc/yMtR5gCin/ZUWjSVJcSus 84tw== 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:mime-version:content-transfer-encoding; bh=UHZ5YMekjSRT+s0xrshOz5js1a8Rx9jxwpgPSLh3c50=; b=FT4zKuljCvd7K69zFgH3d35DuTyi7xPL3SWOvCrO8oYIolGzzWj9RTEusGqVdCKedk UNbHQ+QzzRg2TzFPvTu84010XK61uuggS+iuF2ydI5umkkweUjrXyG+W4IcwRA55iKG/ r1ZpmFXgtQtH5nnoNErI9FN1PqDXrtNqePsOW4ci6V498NCRMofP0FsB5tnxDoZVwR2R gpw1QGwKvAB8XKffxmf7n4/fWrpyeh+vPgqgeBb0FciMPXd2/5YMgjthSuKXYvyN76a5 TLEr23lXExLZHV5osRrZJov5Om6mKr6Fy42o4sMCNofhmPnC5bIHaABOwc+3m+d279+R kqoQ== X-Gm-Message-State: AOAM5314HPFU9two+Vhlv8YmrgbdvAUkO3B9wsY1638DViQGgtbOVamN Cf0FpDr8cqIFu1Lm9gi9Gi0= X-Google-Smtp-Source: ABdhPJwzbbuVxuncCTkspM/BldbgMP+QMLtdFrOs8MgJF15b0MYiY/1TxZyqoEp1K+K6XFBYKVUnIA== X-Received: by 2002:a17:90a:5907:: with SMTP id k7mr2474463pji.46.1623888606695; Wed, 16 Jun 2021 17:10:06 -0700 (PDT) Received: from athina.mtv.corp.google.com ([2620:15c:211:200:926a:e8dd:9095:3ddd]) by smtp.gmail.com with ESMTPSA id r92sm6599633pja.6.2021.06.16.17.10.05 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 16 Jun 2021 17:10:06 -0700 (PDT) From: =?utf-8?q?Maciej_=C5=BBenczykowski?= To: =?utf-8?q?Maciej_=C5=BBenczykowski?= , Alexei Starovoitov , Daniel Borkmann Cc: Linux Network Development Mailing List , Linux Kernel Mailing List , BPF Mailing List , "David S . Miller" , Dongseok Yi , Willem de Bruijn Subject: [PATCH bpf-next v2 1/4] Revert "bpf: Check for BPF_F_ADJ_ROOM_FIXED_GSO when bpf_skb_change_proto" Date: Wed, 16 Jun 2021 17:09:50 -0700 Message-Id: <20210617000953.2787453-1-zenczykowski@gmail.com> X-Mailer: git-send-email 2.32.0.272.g935e593368-goog In-Reply-To: References: MIME-Version: 1.0 Precedence: bulk List-ID: X-Mailing-List: bpf@vger.kernel.org X-Patchwork-Delegate: bpf@iogearbox.net From: Maciej Żenczykowski This reverts commit fa7b83bf3b156c767f3e4a25bbf3817b08f3ff8e. See the followup commit for the reasoning why I believe the appropriate approach is to simply make this change without a flag, but it can basically be summarized as using this helper without the flag is bug-prone or outright buggy, and thus the default should be this new behaviour. As this commit has only made it into net-next/master, but not into any real release, such a backwards incompatible change is still ok. Cc: Dongseok Yi Cc: Daniel Borkmann Cc: Willem de Bruijn Signed-off-by: Maciej Żenczykowski --- net/core/filter.c | 22 +++++++++------------- 1 file changed, 9 insertions(+), 13 deletions(-) diff --git a/net/core/filter.c b/net/core/filter.c index 239de1306de9..65ab4e21c087 100644 --- a/net/core/filter.c +++ b/net/core/filter.c @@ -3235,7 +3235,7 @@ static int bpf_skb_net_hdr_pop(struct sk_buff *skb, u32 off, u32 len) return ret; } -static int bpf_skb_proto_4_to_6(struct sk_buff *skb, u64 flags) +static int bpf_skb_proto_4_to_6(struct sk_buff *skb) { const u32 len_diff = sizeof(struct ipv6hdr) - sizeof(struct iphdr); u32 off = skb_mac_header_len(skb); @@ -3264,9 +3264,7 @@ static int bpf_skb_proto_4_to_6(struct sk_buff *skb, u64 flags) } /* Due to IPv6 header, MSS needs to be downgraded. */ - if (!(flags & BPF_F_ADJ_ROOM_FIXED_GSO)) - skb_decrease_gso_size(shinfo, len_diff); - + skb_decrease_gso_size(shinfo, len_diff); /* Header must be checked, and gso_segs recomputed. */ shinfo->gso_type |= SKB_GSO_DODGY; shinfo->gso_segs = 0; @@ -3278,7 +3276,7 @@ static int bpf_skb_proto_4_to_6(struct sk_buff *skb, u64 flags) return 0; } -static int bpf_skb_proto_6_to_4(struct sk_buff *skb, u64 flags) +static int bpf_skb_proto_6_to_4(struct sk_buff *skb) { const u32 len_diff = sizeof(struct ipv6hdr) - sizeof(struct iphdr); u32 off = skb_mac_header_len(skb); @@ -3307,9 +3305,7 @@ static int bpf_skb_proto_6_to_4(struct sk_buff *skb, u64 flags) } /* Due to IPv4 header, MSS can be upgraded. */ - if (!(flags & BPF_F_ADJ_ROOM_FIXED_GSO)) - skb_increase_gso_size(shinfo, len_diff); - + skb_increase_gso_size(shinfo, len_diff); /* Header must be checked, and gso_segs recomputed. */ shinfo->gso_type |= SKB_GSO_DODGY; shinfo->gso_segs = 0; @@ -3321,17 +3317,17 @@ static int bpf_skb_proto_6_to_4(struct sk_buff *skb, u64 flags) return 0; } -static int bpf_skb_proto_xlat(struct sk_buff *skb, __be16 to_proto, u64 flags) +static int bpf_skb_proto_xlat(struct sk_buff *skb, __be16 to_proto) { __be16 from_proto = skb->protocol; if (from_proto == htons(ETH_P_IP) && to_proto == htons(ETH_P_IPV6)) - return bpf_skb_proto_4_to_6(skb, flags); + return bpf_skb_proto_4_to_6(skb); if (from_proto == htons(ETH_P_IPV6) && to_proto == htons(ETH_P_IP)) - return bpf_skb_proto_6_to_4(skb, flags); + return bpf_skb_proto_6_to_4(skb); return -ENOTSUPP; } @@ -3341,7 +3337,7 @@ BPF_CALL_3(bpf_skb_change_proto, struct sk_buff *, skb, __be16, proto, { int ret; - if (unlikely(flags & ~(BPF_F_ADJ_ROOM_FIXED_GSO))) + if (unlikely(flags)) return -EINVAL; /* General idea is that this helper does the basic groundwork @@ -3361,7 +3357,7 @@ BPF_CALL_3(bpf_skb_change_proto, struct sk_buff *, skb, __be16, proto, * that. For offloads, we mark packet as dodgy, so that headers * need to be verified first. */ - ret = bpf_skb_proto_xlat(skb, proto, flags); + ret = bpf_skb_proto_xlat(skb, proto); bpf_compute_data_pointers(skb); return ret; } From patchwork Thu Jun 17 00:09:51 2021 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: =?utf-8?q?Maciej_=C5=BBenczykowski?= X-Patchwork-Id: 12326305 X-Patchwork-Delegate: bpf@iogearbox.net Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-15.7 required=3.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,FREEMAIL_FORGED_FROMDOMAIN,FREEMAIL_FROM, HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_CR_TRAILER,INCLUDES_PATCH, MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED,USER_AGENT_GIT autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 00B04C48BE8 for ; Thu, 17 Jun 2021 00:10:16 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id D536E61359 for ; Thu, 17 Jun 2021 00:10:15 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S230481AbhFQAMS (ORCPT ); Wed, 16 Jun 2021 20:12:18 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:41978 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S234890AbhFQAMQ (ORCPT ); Wed, 16 Jun 2021 20:12:16 -0400 Received: from mail-pj1-x102a.google.com (mail-pj1-x102a.google.com [IPv6:2607:f8b0:4864:20::102a]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 9FB8AC06175F; Wed, 16 Jun 2021 17:10:08 -0700 (PDT) Received: by mail-pj1-x102a.google.com with SMTP id fy24-20020a17090b0218b029016c5a59021fso5065521pjb.0; Wed, 16 Jun 2021 17:10:08 -0700 (PDT) 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 :mime-version:content-transfer-encoding; bh=Gd7tgXejXrm1qsZ0ihSiw33btJPV8Qm19OUG2FFawb4=; b=ummzKY2M8rXbQsXsRCtiSk0ANtqCRga9cTlcrLkRsAJNbl+uwl5LYTX0BaeWH7/Ajw DX5+sqX7zDjcjjNfkliSBefie7garr2y+Uc11e4nyP7NxQMUGbv0KaiJlM+3s8IMmcEP TVg8eHkrriEVGoLFMKL0lqSD6Lc5/d6gPxduPxPT2Gsqv2T1FhHXsp5lZXmkQcUXqcJ7 bxv4woChPAkx3VDV8noEo34xMTkmFARUlKQTgyeGwAkyC/aFRa5kirWodbBhkt3lA0BN OG30CyIeyUJDHrN/7k5fVJm/aw1hgz15q474HEZk4z6U9bann4E0/E5R7Rcmvzq6qptP Lp4Q== 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:mime-version:content-transfer-encoding; bh=Gd7tgXejXrm1qsZ0ihSiw33btJPV8Qm19OUG2FFawb4=; b=Xwb0n1bm/nncWorMh2lWvu+moyXVAtm6Dkp4jQPV985UlBHpe8diWGgxpaux8wmKDS 95wZajzne2eVw4L/uNtVjVT/gh0qin60g/eVaRozOqshopp/m2MHVe5j9FAqGbDsd2Na ilhC/ISM/dgRK/qk3LS/3B4IGvfYdzskdKq4TYeXW3ZktQOTOaEhTvTh1IvUuO63icHy Rr7ax+fn5VI62Lusvra3xwkNy8FTRinsSnKNG/0g7DSAmFSRCC9u8QeOmwNqBSPn3hBT zIuVKckzd2faB8qr3bytFrSQa4lO8HvbzAJSM8sWpArSuSPr0z3T8jaf6sSoDbBIlR1i b8ug== X-Gm-Message-State: AOAM532fEpAEXCdUVRnR0wVTvfVmHu6tO9VlDsz6Y4R8+isQ3fw8XgUO X2Ql/N4+PnJOstSiQlu4RM0= X-Google-Smtp-Source: ABdhPJygz3iu2QqYbwszrNnTwJ1YJm3ew7OJwE7plZGffIrVbwL3x6v6UGfB6g9JVBaoKmJrhIzWGA== X-Received: by 2002:a17:90b:45d5:: with SMTP id jt21mr1073404pjb.75.1623888608134; Wed, 16 Jun 2021 17:10:08 -0700 (PDT) Received: from athina.mtv.corp.google.com ([2620:15c:211:200:926a:e8dd:9095:3ddd]) by smtp.gmail.com with ESMTPSA id r92sm6599633pja.6.2021.06.16.17.10.06 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 16 Jun 2021 17:10:07 -0700 (PDT) From: =?utf-8?q?Maciej_=C5=BBenczykowski?= To: =?utf-8?q?Maciej_=C5=BBenczykowski?= , Alexei Starovoitov , Daniel Borkmann Cc: Linux Network Development Mailing List , Linux Kernel Mailing List , BPF Mailing List , "David S . Miller" , Dongseok Yi , Willem de Bruijn Subject: [PATCH bpf-next v2 2/4] bpf: do not change gso_size during bpf_skb_change_proto() Date: Wed, 16 Jun 2021 17:09:51 -0700 Message-Id: <20210617000953.2787453-2-zenczykowski@gmail.com> X-Mailer: git-send-email 2.32.0.272.g935e593368-goog In-Reply-To: <20210617000953.2787453-1-zenczykowski@gmail.com> References: <20210617000953.2787453-1-zenczykowski@gmail.com> MIME-Version: 1.0 Precedence: bulk List-ID: X-Mailing-List: bpf@vger.kernel.org X-Patchwork-Delegate: bpf@iogearbox.net From: Maciej Żenczykowski This is technically a backwards incompatible change in behaviour, but I'm going to argue that it is very unlikely to break things, and likely to fix *far* more then it breaks. In no particular order, various reasons follow: (a) I've long had a bug assigned to myself to debug a super rare kernel crash on Android Pixel phones which can (per stacktrace) be traced back to bpf clat ipv6 to ipv4 protocol conversion causing some sort of ugly failure much later on during transmit deep in the GSO engine, AFAICT precisely because of this change to gso_size, though I've never been able to manually reproduce it. I believe it may be related to the particular network offload support of attached usb ethernet dongle being used for tethering off of an IPv6-only cellular connection. The reason might be we end up with more segments than max permitted, or with a gso packet with only one segment... (either way we break some assumption and hit a BUG_ON) (b) There is no check that the gso_size is > 20 when reducing it by 20, so we might end up with a negative (or underflowing) gso_size or a gso_size of 0. This can't possibly be good. Indeed this is probably somehow exploitable (or at least can result in a kernel crash) by delivering crafted packets and perhaps triggering an infinite loop or a divide by zero... As a reminder: gso_size (mss) is related to mtu, but not directly derived from it: gso_size/mss may be significantly smaller then one would get by deriving from local mtu. And on some nics (which do loose mtu checking on receive, it may even potentially be larger, for example my work pc with 1500 mtu can receive 1520 byte frames [and sometimes does due to bugs in a vendor plat46 implementation]). Indeed even just going from 21 to 1 is potentially problematic because it increases the number of segments by a factor of 21 (think DoS, or some other crash due to too many segments). (c) It's always safe to not increase the gso_size, because it doesn't result in the max packet size increasing. So the skb_increase_gso_size() call was always unnecessary for correctness (and outright undesirable, see later). As such the only part which is potentially dangerous (ie. could cause backwards compatibility issues) is the removal of the skb_decrease_gso_size() call. (d) If the packets are ultimately destined to the local device, then there is absolutely no benefit to playing around with gso_size. It only matters if the packets will egress the device. ie. we're either forwarding, or transmitting from the device. (e) This logic only triggers for packets which are GSO. It does not trigger for skbs which are not GSO. It will not convert a non-GSO mtu sized packet into a GSO packet (and you don't even know what the mtu is, so you can't even fix it). As such your transmit path must *already* be able to handle an mtu 20 bytes larger then your receive path (for ipv4 to ipv6 translation) - and indeed 28 bytes larger due to ipv4 fragments. Thus removing the skb_decrease_gso_size() call doesn't actually increase the size of the packets your transmit side must be able to handle. ie. to handle non-GSO max-mtu packets, the ipv4/ipv6 device/route mtus must already be set correctly. Since for example with an ipv4 egress mtu of 1500, ipv4 to ipv6 translation will already build 1520 byte ipv6 frames, so you need a 1520 byte device mtu. This means if your ipv6 device's egress mtu is 1280, your ipv4 route must be 1260 (and actually 1252, because of the need to handle fragments). This is to handle normal non-GSO packets. Thus the reduction is simply not needed for GSO packets, because when they're correctly built, they will already be the right size. (f) TSO/GSO should be able to exactly undo GRO: the number of packets (TCP segments) should not be modified, so that TCP's mss counting works correctly (this matters for congestion control). If protocol conversion changes the gso_size, then the number of TCP segments may increase or decrease. Packet loss after protocol conversion can result in partial loss of mss segments that the sender sent. How's the sending TCP stack going to react to receiving ACKs/SACKs in the middle of the segments it sent? (g) skb_{decrease,increase}_gso_size() are already no-ops for GSO_BY_FRAGS case (besides triggering WARN_ON_ONCE). This means you already cannot guarantee that gso_size (and thus resulting packet mtu) is changed. ie. you must assume it won't be changed. (h) changing gso_size is outright buggy for UDP GSO packets, where framing matters (I believe that's also the case for SCTP, but it's already excluded by [g]). So the only remaining case is TCP, which also doesn't want it (see [f]). (i) see also the reasoning on the previous attempt at fixing this (commit fa7b83bf3b156c767f3e4a25bbf3817b08f3ff8e) which shows that the current behaviour causes TCP packet loss: In the forwarding path GRO -> BPF 6 to 4 -> GSO for TCP traffic, the coalesced packet payload can be > MSS, but < MSS + 20. bpf_skb_proto_6_to_4() will upgrade the MSS and it can be > the payload length. After then tcp_gso_segment checks for the payload length if it is <= MSS. The condition is causing the packet to be dropped. tcp_gso_segment(): [...] mss = skb_shinfo(skb)->gso_size; if (unlikely(skb->len <= mss)) goto out; [...] Thus changing the gso_size is simply a very bad idea. Increasing is unnecessary and buggy, and decreasing can go negative. Cc: Dongseok Yi Cc: Daniel Borkmann Cc: Willem de Bruijn Fixes: 6578171a7ff0 ("bpf: add bpf_skb_change_proto helper") Signed-off-by: Maciej Żenczykowski --- net/core/filter.c | 4 ---- 1 file changed, 4 deletions(-) diff --git a/net/core/filter.c b/net/core/filter.c index 65ab4e21c087..6541358a770b 100644 --- a/net/core/filter.c +++ b/net/core/filter.c @@ -3263,8 +3263,6 @@ static int bpf_skb_proto_4_to_6(struct sk_buff *skb) shinfo->gso_type |= SKB_GSO_TCPV6; } - /* Due to IPv6 header, MSS needs to be downgraded. */ - skb_decrease_gso_size(shinfo, len_diff); /* Header must be checked, and gso_segs recomputed. */ shinfo->gso_type |= SKB_GSO_DODGY; shinfo->gso_segs = 0; @@ -3304,8 +3302,6 @@ static int bpf_skb_proto_6_to_4(struct sk_buff *skb) shinfo->gso_type |= SKB_GSO_TCPV4; } - /* Due to IPv4 header, MSS can be upgraded. */ - skb_increase_gso_size(shinfo, len_diff); /* Header must be checked, and gso_segs recomputed. */ shinfo->gso_type |= SKB_GSO_DODGY; shinfo->gso_segs = 0; From patchwork Thu Jun 17 00:09:52 2021 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: =?utf-8?q?Maciej_=C5=BBenczykowski?= X-Patchwork-Id: 12326303 X-Patchwork-Delegate: bpf@iogearbox.net Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-15.8 required=3.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,FREEMAIL_FORGED_FROMDOMAIN,FREEMAIL_FROM, HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_CR_TRAILER,INCLUDES_PATCH, MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,USER_AGENT_GIT autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 0A206C49361 for ; Thu, 17 Jun 2021 00:10:16 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id E34CC613BD for ; Thu, 17 Jun 2021 00:10:15 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S234915AbhFQAMU (ORCPT ); Wed, 16 Jun 2021 20:12:20 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:41984 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S234897AbhFQAMR (ORCPT ); Wed, 16 Jun 2021 20:12:17 -0400 Received: from mail-pj1-x102e.google.com (mail-pj1-x102e.google.com [IPv6:2607:f8b0:4864:20::102e]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id DDD4BC061574; Wed, 16 Jun 2021 17:10:09 -0700 (PDT) Received: by mail-pj1-x102e.google.com with SMTP id x21-20020a17090aa395b029016e25313bfcso2800385pjp.2; Wed, 16 Jun 2021 17:10:09 -0700 (PDT) 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 :mime-version:content-transfer-encoding; bh=zf+Y9BbICR7uXIOjfsxq5dY5y3BEhQJ7HuObnjMRR8I=; b=afjGMf66GoysKxQYFQlbzLJML35lVDHt/lALOF4BdW4RO5GxSnBRzMFWtv/2m0odbk +facFjSuY43dJ8z9tnNmfR4GfLF6nD3wHjG7kOeaFeLTFQ7W0QREbV1GneSxoH0j+hdS GpHhpuxuIatPpPm98IPP3dzs/YC0SYxKAqfLMrx/aZLuWSvR8v+BUbuJP3lH9ROkdunC E8ovt1BWfYzzaXEqOWLWSo2nEcsw5oIW84BOiD/vQEIJhPYzen6gt4h3VsRUONMQM30E XfNSREarCSKyFApzKck70RYr8YNP/leNRBDrYVAB07Xzb6JoL+DPHTybA0XiTKv5HiHt 1sLQ== 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:mime-version:content-transfer-encoding; bh=zf+Y9BbICR7uXIOjfsxq5dY5y3BEhQJ7HuObnjMRR8I=; b=RpAILF5Y2emAnU1OYen8PQeV50zcIhfESRc5gtbHeW7qKRhOWqBkhsoL+0WXf3+SJL zpB7vIK9ZuoxggYs6qFxCWzZiGIzDglgjoxSApm4spv6E6LctKhi93UkYgvRsvPZPvQM XSS6VVxoFPLSmCa9ih+IlGero2+dE2MDvq1mxLlKPGFqnfsos94ImLvQBe38uG80OyYY 1Qjjil22Dvx7ugtpq5ADf9G8PQUEx3yhZuLmDIq8YtQ1O2SY9ar5Cgf3W8woU7IB3JH7 lb4/9I/l+NHGaNrgKfk0WPQfAA4NKipWPaxqeZAWJHMecMv2anHVc616DIJtD1qYPXrm bXbw== X-Gm-Message-State: AOAM530hkttp2GfhNIrpZyyG5joBjG7MZW3kMLCXCIHwWUDVoNxthKC1 uUgox/AEBuPBDHNG+jwPfBI= X-Google-Smtp-Source: ABdhPJypFSqVrF7rFrHTbLNSGjci+IHB63c3QoCV4WSLAzUlfJ5KYPcssu3QkAQUo0jdlQATptafTA== X-Received: by 2002:a17:90b:188c:: with SMTP id mn12mr6588649pjb.122.1623888609432; Wed, 16 Jun 2021 17:10:09 -0700 (PDT) Received: from athina.mtv.corp.google.com ([2620:15c:211:200:926a:e8dd:9095:3ddd]) by smtp.gmail.com with ESMTPSA id r92sm6599633pja.6.2021.06.16.17.10.08 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 16 Jun 2021 17:10:09 -0700 (PDT) From: =?utf-8?q?Maciej_=C5=BBenczykowski?= To: =?utf-8?q?Maciej_=C5=BBenczykowski?= , Alexei Starovoitov , Daniel Borkmann Cc: Linux Network Development Mailing List , Linux Kernel Mailing List , BPF Mailing List , "David S . Miller" , Dongseok Yi , Willem de Bruijn Subject: [PATCH bpf-next v2 3/4] bpf: support all gso types in bpf_skb_change_proto() Date: Wed, 16 Jun 2021 17:09:52 -0700 Message-Id: <20210617000953.2787453-3-zenczykowski@gmail.com> X-Mailer: git-send-email 2.32.0.272.g935e593368-goog In-Reply-To: <20210617000953.2787453-1-zenczykowski@gmail.com> References: <20210617000953.2787453-1-zenczykowski@gmail.com> MIME-Version: 1.0 Precedence: bulk List-ID: X-Mailing-List: bpf@vger.kernel.org X-Patchwork-Delegate: bpf@iogearbox.net From: Maciej Żenczykowski Since we no longer modify gso_size, it is now theoretically safe to not set SKB_GSO_DODGY and reset gso_segs to zero. This also means the skb_is_gso_tcp() check should no longer be necessary. Unfortunately we cannot remove the skb_{decrease,increase}_gso_size() helpers, as they are still used elsewhere: bpf_skb_net_grow() without BPF_F_ADJ_ROOM_FIXED_GSO bpf_skb_net_shrink() without BPF_F_ADJ_ROOM_FIXED_GSO and net/core/lwt_bpf.c's handle_gso_type() Cc: Dongseok Yi Cc: Daniel Borkmann Cc: Willem de Bruijn Signed-off-by: Maciej Żenczykowski --- net/core/filter.c | 22 ++-------------------- 1 file changed, 2 insertions(+), 20 deletions(-) diff --git a/net/core/filter.c b/net/core/filter.c index 6541358a770b..8f05498f497e 100644 --- a/net/core/filter.c +++ b/net/core/filter.c @@ -3241,9 +3241,6 @@ static int bpf_skb_proto_4_to_6(struct sk_buff *skb) u32 off = skb_mac_header_len(skb); int ret; - if (skb_is_gso(skb) && !skb_is_gso_tcp(skb)) - return -ENOTSUPP; - ret = skb_cow(skb, len_diff); if (unlikely(ret < 0)) return ret; @@ -3255,17 +3252,11 @@ static int bpf_skb_proto_4_to_6(struct sk_buff *skb) if (skb_is_gso(skb)) { struct skb_shared_info *shinfo = skb_shinfo(skb); - /* SKB_GSO_TCPV4 needs to be changed into - * SKB_GSO_TCPV6. - */ + /* SKB_GSO_TCPV4 needs to be changed into SKB_GSO_TCPV6. */ if (shinfo->gso_type & SKB_GSO_TCPV4) { shinfo->gso_type &= ~SKB_GSO_TCPV4; shinfo->gso_type |= SKB_GSO_TCPV6; } - - /* Header must be checked, and gso_segs recomputed. */ - shinfo->gso_type |= SKB_GSO_DODGY; - shinfo->gso_segs = 0; } skb->protocol = htons(ETH_P_IPV6); @@ -3280,9 +3271,6 @@ static int bpf_skb_proto_6_to_4(struct sk_buff *skb) u32 off = skb_mac_header_len(skb); int ret; - if (skb_is_gso(skb) && !skb_is_gso_tcp(skb)) - return -ENOTSUPP; - ret = skb_unclone(skb, GFP_ATOMIC); if (unlikely(ret < 0)) return ret; @@ -3294,17 +3282,11 @@ static int bpf_skb_proto_6_to_4(struct sk_buff *skb) if (skb_is_gso(skb)) { struct skb_shared_info *shinfo = skb_shinfo(skb); - /* SKB_GSO_TCPV6 needs to be changed into - * SKB_GSO_TCPV4. - */ + /* SKB_GSO_TCPV6 needs to be changed into SKB_GSO_TCPV4. */ if (shinfo->gso_type & SKB_GSO_TCPV6) { shinfo->gso_type &= ~SKB_GSO_TCPV6; shinfo->gso_type |= SKB_GSO_TCPV4; } - - /* Header must be checked, and gso_segs recomputed. */ - shinfo->gso_type |= SKB_GSO_DODGY; - shinfo->gso_segs = 0; } skb->protocol = htons(ETH_P_IP); From patchwork Thu Jun 17 00:09:53 2021 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: =?utf-8?q?Maciej_=C5=BBenczykowski?= X-Patchwork-Id: 12326307 X-Patchwork-Delegate: bpf@iogearbox.net Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-15.8 required=3.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,FREEMAIL_FORGED_FROMDOMAIN,FREEMAIL_FROM, HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_CR_TRAILER,INCLUDES_PATCH, MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,USER_AGENT_GIT autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id A9D72C49EA3 for ; Thu, 17 Jun 2021 00:10:17 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 953B2613BF for ; Thu, 17 Jun 2021 00:10:17 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S234872AbhFQAMW (ORCPT ); Wed, 16 Jun 2021 20:12:22 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:41990 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S234905AbhFQAMT (ORCPT ); Wed, 16 Jun 2021 20:12:19 -0400 Received: from mail-pl1-x635.google.com (mail-pl1-x635.google.com [IPv6:2607:f8b0:4864:20::635]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 30FF7C06175F; Wed, 16 Jun 2021 17:10:11 -0700 (PDT) Received: by mail-pl1-x635.google.com with SMTP id e7so1990278plj.7; Wed, 16 Jun 2021 17:10:11 -0700 (PDT) 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 :mime-version:content-transfer-encoding; bh=2EVe6yWId4rmhH7SxvVNrCWJZunis2SdOr3XkJ2G69A=; b=foX74GqvdnCt/QsbrX2s6sJE/O6n8eGQ/zCW0eXZX4e2RmlMHmPXbWU//EXUnt9MbI U3jqGNyGv7bj7AdqK6jS78S5KfOI3GkGPLK1FikXKnDlq5an/s4bZgKPnMpo2c7YBnE3 4scItAkG/6WKsbOUPb5XwceEOgilSIQQfHpMCj5DQHjn8afEeEe/Ick8m9rSPNJ7aaRZ YpQ9CGyhpife+LqMCZtqd7XhXM57MhxpJDm3ekD4oBkSuGgvejvX8kDw6Zl77SSDQ8FA zOq6mIXhTCbaqY9DSANRycRRPJBKY6R0RnpoHpgJhnlCqChZfLCaw78DbTykio0+2sH1 un0A== 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:mime-version:content-transfer-encoding; bh=2EVe6yWId4rmhH7SxvVNrCWJZunis2SdOr3XkJ2G69A=; b=U+A5JxrhNrwtL/RNuQYlJdabkSLwHDbE9goZnRx/cMn9vLPqAUMp/GNGDwuITXnN96 zmQTnR7DkttKXIFW5gIxoPiNFDF2tfDShKc2ykVpnRC+aS7QMdVQQCW4ik+FHIWdztd4 0NK9awRt0BaIoAqeP24AH6FfhdDD8LJWe2HdI/9Dd+XI0f7Nyp1Oqx7Qr1N+/ODjMC1W 4KEko/P0Sx9XUAH8GjLGSFD32K5vyDQO7yJquC2SENXPLirKJ2OE6oc9HGVs9l+/tLg/ IVDpx7YjML19TxLbCH5KJuIGdnp+j+jSBY1Ivcq9FE2yvUoyFOYqY96zRjPlDzbL0LYC HUUw== X-Gm-Message-State: AOAM530QrOPahmEPm7cVcZM8ktBnmFQ0xBSYB/UMxG/LjDn9wQqYaR8y js8T44OA0+Nt0KLY8I6fwdU= X-Google-Smtp-Source: ABdhPJwtMFezZ2ja2Z+GrWh+C8/6DGBa3R1dsFmSpZ02TVPG5lzSFdH5/ESoJ/7klenA0M0gpEgf+A== X-Received: by 2002:a17:90b:3004:: with SMTP id hg4mr13712647pjb.12.1623888610716; Wed, 16 Jun 2021 17:10:10 -0700 (PDT) Received: from athina.mtv.corp.google.com ([2620:15c:211:200:926a:e8dd:9095:3ddd]) by smtp.gmail.com with ESMTPSA id r92sm6599633pja.6.2021.06.16.17.10.09 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 16 Jun 2021 17:10:10 -0700 (PDT) From: =?utf-8?q?Maciej_=C5=BBenczykowski?= To: =?utf-8?q?Maciej_=C5=BBenczykowski?= , Alexei Starovoitov , Daniel Borkmann Cc: Linux Network Development Mailing List , Linux Kernel Mailing List , BPF Mailing List , "David S . Miller" , Willem de Bruijn Subject: [PATCH bpf-next v2 4/4] bpf: more lenient bpf_skb_net_shrink() with BPF_F_ADJ_ROOM_FIXED_GSO Date: Wed, 16 Jun 2021 17:09:53 -0700 Message-Id: <20210617000953.2787453-4-zenczykowski@gmail.com> X-Mailer: git-send-email 2.32.0.272.g935e593368-goog In-Reply-To: <20210617000953.2787453-1-zenczykowski@gmail.com> References: <20210617000953.2787453-1-zenczykowski@gmail.com> MIME-Version: 1.0 Precedence: bulk List-ID: X-Mailing-List: bpf@vger.kernel.org X-Patchwork-Delegate: bpf@iogearbox.net From: Maciej Żenczykowski This is to more closely match behaviour of bpf_skb_change_proto() which now does not adjust gso_size, and thus thoretically supports all gso types, and does not need to set SKB_GSO_DODGY nor reset gso_segs to zero. Something similar should probably be done with bpf_skb_net_grow(), but that code scares me. Cc: Daniel Borkmann Cc: Willem de Bruijn Signed-off-by: Maciej Żenczykowski --- net/core/filter.c | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-) diff --git a/net/core/filter.c b/net/core/filter.c index 8f05498f497e..faf2bae0309b 100644 --- a/net/core/filter.c +++ b/net/core/filter.c @@ -3506,11 +3506,10 @@ static int bpf_skb_net_shrink(struct sk_buff *skb, u32 off, u32 len_diff, BPF_F_ADJ_ROOM_NO_CSUM_RESET))) return -EINVAL; - if (skb_is_gso(skb) && !skb_is_gso_tcp(skb)) { - /* udp gso_size delineates datagrams, only allow if fixed */ - if (!(skb_shinfo(skb)->gso_type & SKB_GSO_UDP_L4) || - !(flags & BPF_F_ADJ_ROOM_FIXED_GSO)) - return -ENOTSUPP; + if (skb_is_gso(skb) && + !skb_is_gso_tcp(skb) && + !(flags & BPF_F_ADJ_ROOM_FIXED_GSO)) { + return -ENOTSUPP; } ret = skb_unclone(skb, GFP_ATOMIC); @@ -3521,12 +3520,11 @@ static int bpf_skb_net_shrink(struct sk_buff *skb, u32 off, u32 len_diff, if (unlikely(ret < 0)) return ret; - if (skb_is_gso(skb)) { + if (skb_is_gso(skb) && !(flags & BPF_F_ADJ_ROOM_FIXED_GSO)) { struct skb_shared_info *shinfo = skb_shinfo(skb); /* Due to header shrink, MSS can be upgraded. */ - if (!(flags & BPF_F_ADJ_ROOM_FIXED_GSO)) - skb_increase_gso_size(shinfo, len_diff); + skb_increase_gso_size(shinfo, len_diff); /* Header must be checked, and gso_segs recomputed. */ shinfo->gso_type |= SKB_GSO_DODGY;