[ovs-dev] [PATCH V3 3/5] datapath: Allow each vport to have an array of 'port_id's.
Thomas Graf
tgraf at redhat.com
Thu Mar 13 12:53:47 UTC 2014
This looks great already. Some minor stuff.
On 03/08/2014 03:04 AM, Alex Wang wrote:
> @@ -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_PID] = { .type = NLA_UNSPEC },
> [OVS_VPORT_ATTR_OPTIONS] = { .type = NLA_NESTED },
> };
I can understand that you want to change the policy to reflect
the change in the extended format. Technically, staying at
NLA_U32 would result in the same as you have now: enforce a
minimal payload size of 4 bytes.
This change might raise eyebrows though in the push to netdev
as it looks like an ABI breakage without closer inspection.
> +int ovs_vport_set_upcall_portids(struct vport *vport, struct nlattr *ids)
> +{
> + struct vport_portids *old, *vport_portids;
> +
> + if (!nla_len(ids) || nla_len(ids) % sizeof(u32))
> + return -EINVAL;
This may crash if OVS_VPORT_ATTR_UPCALL_PID is not provided and ids
becomes NULL since you removed the if (a[OVS_VPORT_ATTR_UPCALL_PID])
in ovs_vport_cmd_set().
> +/**
> + * ovs_vport_get_upcall_portids - get the upcall_portids of @vport.
> + *
> + * @vport: vport from which to retrieve the portids.
> + * @skb: sk_buff where portids should be appended.
> + *
> + * Retrieves the configuration of the given vport, appending the
> + * %OVS_VPORT_ATTR_UPCALL_PID attribute which is the array of upcall
> + * portids to @skb.
> + *
> + * Returns 0 if successful, -EMSGSIZE if @skb has insufficient room.
> + * If an error occurs, @skb is left unmodified. Must be called with
> + * rcu_read_lock.
> + */
> +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 (vport->dp->user_features & OVS_DP_F_VPORT_PIDS) {
> + if (nla_put(skb, OVS_VPORT_ATTR_UPCALL_PID,
> + ids->n_ids * sizeof(u32), (void *) ids->ids))
> + err = -EMSGSIZE;
> +
> + } else {
> + if (nla_put_u32(skb, OVS_VPORT_ATTR_UPCALL_PID,
> + *ids->ids))
> + err = -EMSGSIZE;
> + }
> +
> + return err;
If you want to reduce code size here, you can simply return nla_put*()
and get rid of the additional branching since these functions already
return -EMSGSIZE.
> +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];
I have some dislike for the division here. I realize this is the
slow path but no point in adding it if it can be avoided.
Worst case, this is exploitable in a DoS scenario.
One idea here might be to look at RPS and see how it avoided
this using flow masks, see store_rps_dev_flow_table_cnt().
More information about the dev
mailing list