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.
_______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
