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

[Ed Swierk]

On Fri, Jan 5, 2018 at 10:14 AM, Ed Swierk <eswierk at skyportsystems.com>
wrote:
> On Thu, Jan 4, 2018 at 7:36 PM, Pravin Shelar <pshelar at ovn.org> wrote:
>> 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?

Something like this for verifying the IP header checksum (not tested):

--- a/net/openvswitch/flow.c
+++ b/net/openvswitch/flow.c
@@ -203,6 +203,7 @@ static int check_iphdr(struct sk_buff *skb)
 {
  unsigned int nh_ofs = skb_network_offset(skb);
  unsigned int ip_len;
+ const struct iphdr *nh;
  int err;

  err = check_header(skb, nh_ofs + sizeof(struct iphdr));
@@ -214,6 +215,13 @@ static int check_iphdr(struct sk_buff *skb)
       skb->len < nh_ofs + ip_len))
  return -EINVAL;

+ if (unlikely(!pskb_may_pull(skb, nh_ofs + ip_len)))
+ return -ENOMEM;
+
+ nh = ip_hdr(skb);
+ if (unlikely(ip_fast_csum((u8 *)nh, nh->ihl)))
+ return -EINVAL;
+
  skb_set_transport_header(skb, nh_ofs + ip_len);
  return 0;
 }

> 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.

And something like this for trimming the padding (also not tested):

--- a/net/openvswitch/conntrack.c
+++ b/net/openvswitch/conntrack.c
@@ -1112,6 +1112,14 @@ int ovs_ct_execute(struct net *net, struct sk_buff
*skb,
  nh_ofs = skb_network_offset(skb);
  skb_pull_rcsum(skb, nh_ofs);

+ if (unlikely(OVS_CB(skb)->padlen)) {
+ err = pskb_trim_rcsum(skb, skb->len - OVS_CB(skb)->padlen);
+ if (err) {
+ kfree_skb(skb);
+ return err;
+ }
+ }
+
  if (key->ip.frag != OVS_FRAG_TYPE_NONE) {
  err = handle_fragments(net, key, info->zone.id, skb);
  if (err)
diff --git a/net/openvswitch/datapath.h b/net/openvswitch/datapath.h
index 523d655..eab29fa 100644
--- a/net/openvswitch/datapath.h
+++ b/net/openvswitch/datapath.h
@@ -106,12 +106,14 @@ struct datapath {
  * fragmented.
  * @acts_origlen: The netlink size of the flow actions applied to this skb.
  * @cutlen: The number of bytes from the packet end to be removed.
+ * @padlen: The number of padding bytes to be ignored in L3+ processing.
  */
 struct ovs_skb_cb {
  struct vport *input_vport;
  u16 mru;
  u16 acts_origlen;
  u32 cutlen;
+ u32                     padlen;
 };
 #define OVS_CB(skb) ((struct ovs_skb_cb *)(skb)->cb)

diff --git a/net/openvswitch/flow.c b/net/openvswitch/flow.c
index 987a97f..c749dfd 100644
--- a/net/openvswitch/flow.c
+++ b/net/openvswitch/flow.c
@@ -223,6 +223,7 @@ static int check_iphdr(struct sk_buff *skb)
  return -EINVAL;

  skb_set_transport_header(skb, nh_ofs + ip_len);
+ OVS_CB(skb)->padlen = skb->len - nh_ofs - ntohs(nh->tot_len);
  return 0;
 }

@@ -305,6 +306,7 @@ static int parse_ipv6hdr(struct sk_buff *skb, struct
sw_flow_key *key)

  nh_len = payload_ofs - nh_ofs;
  skb_set_transport_header(skb, nh_ofs + nh_len);
+ OVS_CB(skb)->padlen = skb->len - payload_ofs - ntohs(nh->payload_len);
  key->ip.proto = nexthdr;
  return nh_len;
 }