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

[Pravin Shelar]

On Mon, Feb 25, 2013 at 3:47 PM, Jesse Gross <jesse at nicira.com> wrote:
> 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.
>
frag_len calculation is correct since tunnel_hlen includes ip_hdr len.
I agree with rest of comments, will post updated patch.

Thanks.

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