From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ed Swierk Subject: Re: [PATCH v2] openvswitch: Trim off padding before L3+ netfilter processing Date: Fri, 5 Jan 2018 10:14:05 -0800 Message-ID: References: <1513095439-128864-1-git-send-email-eswierk@skyportsystems.com> <1513869437-20059-1-git-send-email-eswierk@skyportsystems.com> Mime-Version: 1.0 Content-Type: text/plain; charset="UTF-8" Cc: ovs-dev , Linux Kernel Network Developers , Benjamin Warren , Keith Holleman To: Pravin Shelar Return-path: Received: from mail-io0-f195.google.com ([209.85.223.195]:40612 "EHLO mail-io0-f195.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752121AbeAESOq (ORCPT ); Fri, 5 Jan 2018 13:14:46 -0500 Received: by mail-io0-f195.google.com with SMTP id v30so6616927iov.7 for ; Fri, 05 Jan 2018 10:14:46 -0800 (PST) In-Reply-To: Sender: netdev-owner@vger.kernel.org List-ID: On Thu, Jan 4, 2018 at 7:36 PM, Pravin Shelar wrote: > 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. Shouldn't the IP header checksum be verified even earlier, like in key_extract(), before actually using any of the fields in the IP header? And since key_extract() is already inspecting the IP/IPv6 header, it would be a convenient spot to check whether the skb->len matches. If there's a difference, it could record the number of bytes to trim in an ovs_skb_cb field. Then ovs_ct_execute() would look at this field and trim the skb only if necessary.