[ovs-dev] [PATCH] tunneling: Don't match on source IP address for native tunnels.

[Pravin Shelar]

On Wed, Jun 24, 2015 at 2:55 PM, Jesse Gross <jesse at nicira.com> wrote:
> When doing native tunneling, we look at packets destined to the
> local port to see if they match tunnel protocols that we should
> intercept. The criteria are IP protocol, destination UDP port, etc.
>
> However, we also look at the source IP address of the packets. This
> should be a function of the port-based tunnel layer and not the
> tunnel receive code itself. For comparison, the kernel tunnel code
> has no idea about the IP addresses of its link partners. If port
> based tunnel is desired, it can be handled using the normal port
> tunnel layer, regardless of whether the packets originally came
> from userspace or the kernel.
>
> For port based tunneling, this bug has no effect - the check is
> simply redundant. However, it breaks flow-based native tunnels
> because the remote IP address is not known at port creation time.
>
> CC: Pravin Shelar <pshelar at nicira.com>
> Reported-by: David Griswold <David.Griswold at overturenetworks.com>
> Signed-off-by: Jesse Gross <jesse at nicira.com>

Can you add this test case?

> ---
>  lib/tnl-ports.c  | 15 +++++----------
>  lib/tnl-ports.h  |  4 ++--
>  ofproto/tunnel.c |  5 ++---
>  3 files changed, 9 insertions(+), 15 deletions(-)
>
> diff --git a/lib/tnl-ports.c b/lib/tnl-ports.c
> index a0a73c8..79c9631 100644
> --- a/lib/tnl-ports.c
> +++ b/lib/tnl-ports.c
> @@ -56,7 +56,7 @@ tnl_port_free(struct tnl_port_in *p)
>  }
>
>  static void
> -tnl_port_init_flow(struct flow *flow, ovs_be32 ip_dst, ovs_be16 udp_port)
> +tnl_port_init_flow(struct flow *flow, ovs_be16 udp_port)
>  {
>      memset(flow, 0, sizeof *flow);
>      flow->dl_type = htons(ETH_TYPE_IP);
> @@ -66,21 +66,17 @@ tnl_port_init_flow(struct flow *flow, ovs_be32 ip_dst, ovs_be16 udp_port)
>          flow->nw_proto = IPPROTO_GRE;
>      }
>      flow->tp_dst = udp_port;
> -    /* When matching on incoming flow from remove tnl end point,
> -     * our dst ip address is source ip for them. */
> -    flow->nw_src = ip_dst;
>  }
>
>  void
> -tnl_port_map_insert(odp_port_t port, ovs_be32 ip_dst, ovs_be16 udp_port,
> -                    const char dev_name[])
> +tnl_port_map_insert(odp_port_t port, ovs_be16 udp_port, const char dev_name[])
>  {
>      const struct cls_rule *cr;
>      struct tnl_port_in *p;
>      struct match match;
>
>      memset(&match, 0, sizeof match);
> -    tnl_port_init_flow(&match.flow, ip_dst, udp_port);
> +    tnl_port_init_flow(&match.flow, udp_port);
>
>      ovs_mutex_lock(&mutex);
>      do {
> @@ -97,7 +93,6 @@ tnl_port_map_insert(odp_port_t port, ovs_be32 ip_dst, ovs_be16 udp_port,
>          match.wc.masks.nw_proto = 0xff;
>          match.wc.masks.nw_frag = 0xff;      /* XXX: No fragments support. */
>          match.wc.masks.tp_dst = OVS_BE16_MAX;
> -        match.wc.masks.nw_src = OVS_BE32_MAX;
>
>          cls_rule_init(&p->cr, &match, 0, CLS_MIN_VERSION); /* Priority == 0. */
>          ovs_refcount_init(&p->ref_cnt);
> @@ -123,12 +118,12 @@ tnl_port_unref(const struct cls_rule *cr)
>  }
>
>  void
> -tnl_port_map_delete(ovs_be32 ip_dst, ovs_be16 udp_port)
> +tnl_port_map_delete(ovs_be16 udp_port)
>  {
>      const struct cls_rule *cr;
>      struct flow flow;
>
> -    tnl_port_init_flow(&flow, ip_dst, udp_port);
> +    tnl_port_init_flow(&flow, udp_port);
>
>      cr = classifier_lookup(&cls, CLS_MAX_VERSION, &flow, NULL);
>      tnl_port_unref(cr);
> diff --git a/lib/tnl-ports.h b/lib/tnl-ports.h
> index 37a689f..8e4911d 100644
> --- a/lib/tnl-ports.h
> +++ b/lib/tnl-ports.h
> @@ -26,10 +26,10 @@
>
>  odp_port_t tnl_port_map_lookup(struct flow *flow, struct flow_wildcards *wc);
>
> -void tnl_port_map_insert(odp_port_t port, ovs_be32 ip_dst, ovs_be16 udp_port,
> +void tnl_port_map_insert(odp_port_t port, ovs_be16 udp_port,
>                           const char dev_name[]);
>
> -void tnl_port_map_delete(ovs_be32 ip_dst,  ovs_be16 udp_port);
> +void tnl_port_map_delete(ovs_be16 udp_port);
>
>  void tnl_port_map_init(void);
>
> diff --git a/ofproto/tunnel.c b/ofproto/tunnel.c
> index d2ac7c6..42f760e 100644
> --- a/ofproto/tunnel.c
> +++ b/ofproto/tunnel.c
> @@ -195,8 +195,7 @@ tnl_port_add__(const struct ofport_dpif *ofport, const struct netdev *netdev,
>      tnl_port_mod_log(tnl_port, "adding");
>
>      if (native_tnl) {
> -        tnl_port_map_insert(odp_port, tnl_port->match.ip_dst,
> -                            cfg->dst_port, name);
> +        tnl_port_map_insert(odp_port, cfg->dst_port, name);
>      }
>      return true;
>  }
> @@ -263,7 +262,7 @@ tnl_port_del__(const struct ofport_dpif *ofport) OVS_REQ_WRLOCK(rwlock)
>              netdev_get_tunnel_config(tnl_port->netdev);
>          struct hmap **map;
>
> -        tnl_port_map_delete(tnl_port->match.ip_dst, cfg->dst_port);
> +        tnl_port_map_delete(cfg->dst_port);
>          tnl_port_mod_log(tnl_port, "removing");
>          map = tnl_match_map(&tnl_port->match);
>          hmap_remove(*map, &tnl_port->match_node);
> --

Patch looks good to me.