[ovs-dev] [PATCH v2] Tunnel: Cleanup old tunnel infrastructure.

Jesse Gross jesse at nicira.com
Mon Feb 25 23:47:50 UTC 2013


On Thu, Feb 21, 2013 at 8:52 AM, Pravin B Shelar <pshelar at nicira.com> wrote:
> diff --git a/datapath/tunnel.c b/datapath/tunnel.c
> index 3964208..4ff8448 100644
> --- a/datapath/tunnel.c
> +++ b/datapath/tunnel.c
> @@ -551,74 +300,22 @@ int ovs_tnl_send(struct vport *vport, struct sk_buff *skb)
>         }
>  #endif
>
> -       /* If OVS_CB(skb)->tun_key is NULL, point it at the local tun_key here,
> -        * and zero it out.
> -        */
> -       if (!OVS_CB(skb)->tun_key) {
> -               memset(&tun_key, 0, sizeof(tun_key));
> -               OVS_CB(skb)->tun_key = &tun_key;
> -       }

I think we still need a check for tun_key being NULL, however, in this
case we can just drop the packet.

> -       tunnel_hlen = tnl_vport->tnl_ops->hdr_len(mutable, OVS_CB(skb)->tun_key);
> +       tunnel_hlen = tnl_vport->tnl_ops->hdr_len(OVS_CB(skb)->tun_key);
>         if (unlikely(tunnel_hlen < 0)) {
>                 err = VPORT_E_TX_DROPPED;
>                 goto error_free;
>         }

I don't believe that anything returns an error for hdr_len any more,
so I would assume that it isn't possible until we need it.

> @@ -661,6 +351,9 @@ int ovs_tnl_send(struct vport *vport, struct sk_buff *skb)
>                 else
>                         skb_dst_set(skb, &rt_dst(rt));
>
> +               /* Push Tunnel header. */
> +               tnl_vport->tnl_ops->build_header(vport, skb, tunnel_hlen);
> +
>                 /* Push IP header. */
>                 iph = ip_hdr(skb);
>                 iph->version    = 4;

The order that we push headers on isn't completely consistent - the
documentation in tunnel.h says that we push the IP header on first but
here we do the tunnel header and then IP.  Pushing the tunnel header
on first makes sense to me so I would just update the documentation.
However, the calculation of frag_len doesn't look right to me because
we subtract tunnel_hlen even though we haven't pushed the tunnel
header on yet.

Also, I think that we could cleanup the comments in tunnel.h about
update_header potentially returning a linked list and stop using
ovs_tnl_free_linked_skbs().  Both of those are left over from CAPWAP.

> diff --git a/datapath/tunnel.h b/datapath/tunnel.h
> index 54c34ef..8b8b035 100644
> --- a/datapath/tunnel.h
> +++ b/datapath/tunnel.h
>  struct tnl_vport {
>         struct rcu_head rcu;
>         struct hlist_node hash_node;

Is hash_node still used anywhere?

> diff --git a/datapath/vport-gre.c b/datapath/vport-gre.c
> index 680e7b3..bdd7edd 100644
> --- a/datapath/vport-gre.c
> +++ b/datapath/vport-gre.c
>  static struct vport *gre_create(const struct vport_parms *parms)
>  {
> -       return ovs_tnl_create(parms, &ovs_gre_vport_ops, &gre_tnl_ops);
> +       struct net *net = ovs_dp_get_net(parms->dp);
> +       struct ovs_net *ovs_net;
> +       struct vport *vport;
> +
> +       ovs_net = net_generic(net, ovs_net_id);
> +       vport = ovs_tnl_create(parms, &ovs_gre_vport_ops, &gre_tnl_ops);
> +
> +       ovs_net->vport_net.gre_vport = vport;

We should probably check whether a GRE port already exists and if so
reject creating a new one.  Otherwise, we can leak ports.

Shouldn't this also use RCU?  Otherwise, on receive we could find the
new port before memory has made it over to the other CPU.  I think all
we need is the rcu_assign_pointer()/rcu_dereference() pieces since
we're already handling the cleanup through the vport layer.

> +static void gre_tnl_destroy(struct vport *vport)
> +{
> +       struct net *net = ovs_dp_get_net(vport->dp);
> +       struct ovs_net *ovs_net;
> +
> +       ovs_net = net_generic(net, ovs_net_id);
> +
> +       ovs_tnl_destroy(vport);
> +       ovs_net->vport_net.gre_vport = NULL;

I think we need to NULL out the port before we destroy, otherwise we
could start processing with a port that has been freed.



More information about the dev mailing list