On Wed, Jun 24, 2015 at 2:55 PM, Jesse Gross <je...@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 <pshe...@nicira.com>
> Reported-by: David Griswold <david.grisw...@overturenetworks.com>
> Signed-off-by: Jesse Gross <je...@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.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
