From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from relay3-d.mail.gandi.net ([217.70.183.195]:50983 "EHLO relay3-d.mail.gandi.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751250AbeAEDgl (ORCPT ); Thu, 4 Jan 2018 22:36:41 -0500 Received: from mail-wm0-f41.google.com (mail-wm0-f41.google.com [74.125.82.41]) (Authenticated sender: pshelar@ovn.org) by relay3-d.mail.gandi.net (Postfix) with ESMTPSA id 03A3BA80C6 for ; Fri, 5 Jan 2018 04:36:39 +0100 (CET) Received: by mail-wm0-f41.google.com with SMTP id y82so4619984wmg.1 for ; Thu, 04 Jan 2018 19:36:39 -0800 (PST) MIME-Version: 1.0 In-Reply-To: References: <1513095439-128864-1-git-send-email-eswierk@skyportsystems.com> <1513869437-20059-1-git-send-email-eswierk@skyportsystems.com> From: Pravin Shelar Date: Thu, 4 Jan 2018 19:36:38 -0800 Message-ID: Subject: Re: [PATCH v2] openvswitch: Trim off padding before L3+ netfilter processing To: Ed Swierk Cc: ovs-dev , Linux Kernel Network Developers , Benjamin Warren , Keith Holleman Content-Type: text/plain; charset="UTF-8" Sender: netdev-owner@vger.kernel.org List-ID: On Wed, Jan 3, 2018 at 7:49 PM, Ed Swierk wrote: > On Fri, Dec 22, 2017 at 3:31 PM, Pravin Shelar wrote: >> On Thu, Dec 21, 2017 at 7:17 AM, Ed Swierk wrote: >>> IPv4 and IPv6 packets may arrive with lower-layer padding that is not >>> included in the L3 length. For example, a short IPv4 packet may have >>> up to 6 bytes of padding following the IP payload when received on an >>> Ethernet device. In the normal IPv4 receive path, ip_rcv() trims the >>> packet to ip_hdr->tot_len before invoking netfilter hooks (including >>> conntrack and nat). >>> >>> In the IPv6 receive path, ip6_rcv() does the same using >>> ipv6_hdr->payload_len. Similarly in the br_netfilter receive path, >>> br_validate_ipv4() and br_validate_ipv6() trim the packet to the L3 >>> length before invoking NF_INET_PRE_ROUTING hooks. >>> >>> In the OVS conntrack receive path, ovs_ct_execute() pulls the skb to >>> the L3 header but does not trim it to the L3 length before calling >>> nf_conntrack_in(NF_INET_PRE_ROUTING). When nf_conntrack_proto_tcp >>> encounters a packet with lower-layer padding, nf_checksum() fails and >>> logs "nf_ct_tcp: bad TCP checksum". While extra zero bytes don't >>> affect the checksum, the length in the IP pseudoheader does. That >>> length is based on skb->len, and without trimming, it doesn't match >>> the length the sender used when computing the checksum. >>> >>> The assumption throughout nf_conntrack and nf_nat is that skb->len >>> reflects the length of the L3 header and payload, so there is no need >>> to refer back to ip_hdr->tot_len or ipv6_hdr->payload_len. >>> >>> This change brings OVS into line with other netfilter users, trimming >>> IPv4 and IPv6 packets prior to L3+ netfilter processing. >>> >>> Signed-off-by: Ed Swierk >>> --- >>> v2: >>> - Trim packet in nat receive path as well as conntrack >>> - Free skb on error >>> --- >>> net/openvswitch/conntrack.c | 34 ++++++++++++++++++++++++++++++++++ >>> 1 file changed, 34 insertions(+) >>> >>> diff --git a/net/openvswitch/conntrack.c b/net/openvswitch/conntrack.c >>> index b27c5c6..1bdc78f 100644 >>> --- a/net/openvswitch/conntrack.c >>> +++ b/net/openvswitch/conntrack.c >>> @@ -703,6 +703,33 @@ static bool skb_nfct_cached(struct net *net, >>> return ct_executed; >>> } >>> >>> +/* Trim the skb to the L3 length. Assumes the skb is already pulled to >>> + * the L3 header. The skb is freed on error. >>> + */ >>> +static int skb_trim_l3(struct sk_buff *skb) >>> +{ >>> + unsigned int nh_len; >>> + int err; >>> + >>> + switch (skb->protocol) { >>> + case htons(ETH_P_IP): >>> + nh_len = ntohs(ip_hdr(skb)->tot_len); >>> + break; >>> + case htons(ETH_P_IPV6): >>> + nh_len = ntohs(ipv6_hdr(skb)->payload_len) >>> + + sizeof(struct ipv6hdr); >>> + break; >>> + default: >>> + nh_len = skb->len; >>> + } >>> + >>> + err = pskb_trim_rcsum(skb, nh_len); >>> + if (err) >> This should is unlikely. >>> + kfree_skb(skb); >>> + >>> + return err; >>> +} >>> + >> This looks like a generic function, it probably does not belong to OVS >> code base. > > It occurs to me that skb_trim_l3() can't just reach into ip_hdr(skb) > before calling pskb_may_pull(skb, sizeof(struct iphdr)) to make sure > the IP header is actually there; and for IPv4 it should validate the > IP header checksum, including options. Once we add all these steps, > skb_trim_l3() starts to look an awful lot like br_validate_ipv4() and > br_validate_ipv6(). And those in turn are eerily similar to ip_rcv() > and ip6_rcv(). It would be nice to avoid duplicating this logic yet > again. > OVS already pull all required headers in skb linear data, so no need to redo all of it. only check required is the ip-checksum validation. I think we could avoid it in most of cases by checking skb length to ipheader length before verifying the ip header-checksum.