[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