From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from relay3-d.mail.gandi.net ([217.70.183.195]:52092 "EHLO relay3-d.mail.gandi.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753008AbdLVXbV (ORCPT ); Fri, 22 Dec 2017 18:31:21 -0500 Received: from mail-wm0-f47.google.com (mail-wm0-f47.google.com [74.125.82.47]) (Authenticated sender: pshelar@ovn.org) by relay3-d.mail.gandi.net (Postfix) with ESMTPSA id 17F9CA80CB for ; Sat, 23 Dec 2017 00:31:19 +0100 (CET) Received: by mail-wm0-f47.google.com with SMTP id 64so24100031wme.3 for ; Fri, 22 Dec 2017 15:31:19 -0800 (PST) MIME-Version: 1.0 In-Reply-To: <1513869437-20059-1-git-send-email-eswierk@skyportsystems.com> References: <1513095439-128864-1-git-send-email-eswierk@skyportsystems.com> <1513869437-20059-1-git-send-email-eswierk@skyportsystems.com> From: Pravin Shelar Date: Fri, 22 Dec 2017 15:31:19 -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 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. > #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 >