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: Wed, 3 Jan 2018 19:49:57 -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-f194.google.com ([209.85.223.194]:38049 "EHLO mail-io0-f194.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751813AbeADDuj (ORCPT ); Wed, 3 Jan 2018 22:50:39 -0500 Received: by mail-io0-f194.google.com with SMTP id 87so822902ior.5 for ; Wed, 03 Jan 2018 19:50:39 -0800 (PST) In-Reply-To: Sender: netdev-owner@vger.kernel.org List-ID: 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. What if we turn br_validate_ipv4() and br_validate_ipv6() into generic functions and call them from both br_netfilter and ovs_ct--should there be any fundamental difference between these two receive paths, at least for L3+ conntrack processing? For example, currently br_netfilter updates the IPSTATS_MIB_INTRUNCATEDPKTS and IPSTATS_MIB_INDISCARDS counters. It would be easy to make this conditional in a generic function, if we still don't want ovs_ct to update those counters. >> #ifdef CONFIG_NF_NAT_NEEDED >> /* Modelled after nf_nat_ipv[46]_fn(). >> * range is only used for new, uninitialized NAT state. >> @@ -715,8 +742,12 @@ static int ovs_ct_nat_execute(struct sk_buff *skb, struct nf_conn *ct, >> { >> int hooknum, nh_off, err = NF_ACCEPT; >> >> + /* The nat module expects to be working at L3. */ >> nh_off = skb_network_offset(skb); >> skb_pull_rcsum(skb, nh_off); >> + err = skb_trim_l3(skb); >> + if (err) >> + return err; >> > ct-nat is executed within ct action, so I do not see why you you call > skb-trim again from ovs_ct_nat_execute(). > ovs_ct_execute() trim should take care of the skb. > >> /* See HOOK2MANIP(). */ >> if (maniptype == NF_NAT_MANIP_SRC) >> @@ -1111,6 +1142,9 @@ int ovs_ct_execute(struct net *net, struct sk_buff *skb, >> /* The conntrack module expects to be working at L3. */ >> nh_ofs = skb_network_offset(skb); >> skb_pull_rcsum(skb, nh_ofs); >> + err = skb_trim_l3(skb); >> + if (err) >> + return err; >> >> if (key->ip.frag != OVS_FRAG_TYPE_NONE) { >> err = handle_fragments(net, key, info->zone.id, skb); >> -- >> 1.9.1 >>