[ovs-dev] [PATCH v2] openvswitch: Trim off padding before L3+ netfilter processing

[Ed Swierk]

On Fri, Dec 22, 2017 at 3:31 PM, Pravin Shelar <pshelar at ovn.org> wrote:
> On Thu, Dec 21, 2017 at 7:17 AM, Ed Swierk <eswierk at skyportsystems.com> 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 <eswierk at skyportsystems.com>
>> ---
>> 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
>>