From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from relay5-d.mail.gandi.net ([217.70.183.197]:44819 "EHLO relay5-d.mail.gandi.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751344AbeACGVT (ORCPT ); Wed, 3 Jan 2018 01:21:19 -0500 Received: from mail-wm0-f51.google.com (mail-wm0-f51.google.com [74.125.82.51]) (Authenticated sender: pshelar@ovn.org) by relay5-d.mail.gandi.net (Postfix) with ESMTPSA id 60DAD41C07E for ; Wed, 3 Jan 2018 07:21:18 +0100 (CET) Received: by mail-wm0-f51.google.com with SMTP id b141so820493wme.1 for ; Tue, 02 Jan 2018 22:21:18 -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: Tue, 2 Jan 2018 22:21:17 -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 Fri, Dec 22, 2017 at 4:39 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. > > I'll add unlikely(). > >>> + kfree_skb(skb); >>> + >>> + return err; >>> +} >>> + >> This looks like a generic function, it probably does not belong to OVS >> code base. > > Indeed. I'll move it to skbuff.c, unless you have a better idea. > >>> #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. > > I see. Doesn't that mean that skb_pull_rcsum() is also unnecessary in > ovs_ct_nat_execute(), as ovs_ct_execute() has already pulled the skb > to the L3 header? > Yes, It looks redundant. but lets address it in separate patch.