[ovs-dev] [PATCH 3/5] datapath: Allow each vport to have an array of 'port_id's.

Alex Wang alexw at nicira.com
Fri Mar 7 23:49:16 UTC 2014


Thanks for the review Thomas~ ;D

Please see my reply inline:

It's nice to see that you preserve backwards compatibility but why
> rename the attribute? It doesn't really serve a purpose except that you
> break the API.
>
> Since OVS_VPORT_ATTR_UPCALL_PID was a single u32 it is already forward
> compatible with the new array you introduce since Netlink on the kernel
> side only enforces a minimal and no maximum attribute payload size.
>
> OTOH, since the OVS user space peer enforces a maximum attribute length
> for NLA_U32 newer kernels will break older user space binaries. It can
> only happen if you downgrade while the DP remains loaded though.



Thanks for pointing this out.  Still new to kernel programming... lot to
learn~

I'll keep the name OVS_VPORT_ATTR_UPCALL_PID.  But, i'd still like to
change the
type to NLA_UNSPEC, since it can now be an array of netlink pids.

I check the code here:
http://www.cs.fsu.edu/~baker/devices/lxr/http/source/linux/lib/nlattr.c#L27
that the min_len for NLA_UNSPEC is 0.  To avoid user passing in
less-than-four-byte
attribute, I'll add the following check:

@@ -376,7 +376,7 @@ int ovs_vport_set_upcall_portids(struct vport *vport,
 struct nlattr *ids)
 {
        struct vport_portids *old, *vport_portids;

-       if (nla_len(ids) % sizeof(u32))
+       if (!nla_len(ids) || nla_len(ids) % sizeof(u32))
                return -EINVAL;



Also I will use the OVS_DP_ATTR_USER_FEATURES to allow datapath to have
knowledge of the userspace implementation.  And will guarantee the
compatibility
in all directions.




>  @@ -162,8 +164,13 @@ struct vport *ovs_vport_alloc(int priv_size, const
>> struct vport_ops *ops,
>>    */
>>   void ovs_vport_free(struct vport *vport)
>>   {
>> +       struct nlattr *a = kzalloc(sizeof *a + sizeof(u32), GFP_KERNEL);
>> +
>> +       a->nla_len = sizeof(u32);
>> +       ovs_vport_set_upcall_portids(vport, a);
>>         free_percpu(vport->percpu_stats);
>>         kfree(vport);
>> +       kfree(a);
>>   }
>>
>
> This is really ugly, any way to avoid this? :)



Yes, i missed the comment which says that the caller guarantees the elapse
of grace
period before calling this function.  I'll use kfree(vport->upcall_portids)
directly.



> + * Must be called with rcu_read_lock.
>> + */
>> +int ovs_vport_set_upcall_portids(struct vport *vport,  struct nlattr
>> *ids)
>> +{
>> +       struct vport_portids *old, *vport_portids;
>> +
>> +       if (nla_len(ids) % sizeof(u32))
>> +               return -EINVAL;
>>
>
> n_ids must be validated to be > 0, otherwise division by 0, see later
> on.



The check I add above will guarantee n_ids > 0.



>
>
> +       memcpy(vport_portids->ids, nla_data(ids), nla_len(ids));
>> +
>> +       rcu_assign_pointer(vport->upcall_portids, vport_portids);
>> +
>> +       if (old)
>> +               call_rcu(&old->rcu, vport_portids_destroy_rcu_cb);
>>
>
> kfree_rcu()?



kfree_rcu() is still a relatively new function, we cannot use it for
compatibility reasons.
we will use it in upstream ovs module



> +       ids = rcu_dereference_ovsl(vport->upcall_portids);
>> +
>> +       if (nla_put(skb, OVS_VPORT_ATTR_UPCALL_PIDS,
>> +                   ids->n_ids * sizeof *ids->ids,
>>
>
> I would hardcode this as n_ids * sizeof(u32), Netlink ABI is set
> in stone.
>
>

Thanks, I'll do that.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openvswitch.org/pipermail/ovs-dev/attachments/20140307/d1f0a0ad/attachment-0005.html>


More information about the dev mailing list