Hi Alex,
On 02/27/2014 08:44 PM, Alex Wang wrote:
diff --git a/datapath/datapath.c b/datapath/datapath.c
index f7c3391..de4b97a 100644
--- a/datapath/datapath.c
+++ b/datapath/datapath.c
@@ -1459,7 +1459,7 @@ static const struct nla_policy
vport_policy[OVS_VPORT_ATTR_MAX + 1] = {
[OVS_VPORT_ATTR_STATS] = { .len = sizeof(struct ovs_vport_stats) },
[OVS_VPORT_ATTR_PORT_NO] = { .type = NLA_U32 },
[OVS_VPORT_ATTR_TYPE] = { .type = NLA_U32 },
- [OVS_VPORT_ATTR_UPCALL_PID] = { .type = NLA_U32 },
+ [OVS_VPORT_ATTR_UPCALL_PIDS] = { .type = NLA_UNSPEC },
[OVS_VPORT_ATTR_OPTIONS] = { .type = NLA_NESTED },
};
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.
@@ -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? :)
+/**
+ * ovs_vport_set_upcall_portids - set upcall portids of @vport.
+ *
+ * @vport: vport to modify.
+ * @ids: new configuration, an array of port ids.
+ *
+ * Sets the vport's upcall_portids to @ids.
+ *
+ * Returns 0 if successful, -EINVAL if @ids is NULL or cannot be parsed as
+ * an array of U32.
+ *
+ * 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.
+ old = ovsl_dereference(vport->upcall_portids);
+
+ vport_portids = kmalloc(sizeof *vport_portids + nla_len(ids),
+ GFP_KERNEL);
+ vport_portids->ids = (void *) vport_portids + sizeof *vport_portids;
+ vport_portids->n_ids = nla_len(ids) / sizeof(u32);
+ 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()?
*/
+int ovs_vport_get_upcall_portids(const struct vport *vport,
+ struct sk_buff *skb)
+{
+ struct vport_portids *ids;
+ int err = 0;
+
+ 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.
+/**
+ * ovs_vport_find_portid - find the upcall portid to send upcall.
+ *
+ * @vport: vport from which the missed packet is received.
+ * @skb: skb that the missed packet was received.
+ *
+ * Uses the skb_get_rxhash() to select the upcall portid to send the
+ * upcall.
+ *
+ * Returns the portid of the target socket. Must be called with rcu_read_lock.
+ */
+u32 ovs_vport_find_portid(const struct vport *p, struct sk_buff *skb)
+{
+ struct vport_portids *ids;
+
+ ids = rcu_dereference_ovsl(p->upcall_portids);
+
+ if (ids->n_ids == 1 && *ids->ids == 0)
+ return 0;
+
+ return ids->ids[skb_get_rxhash(skb) % ids->n_ids];
This is a potential divsion by 0 since n_ids is not validated to be > 0.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
