[ovs-dev] [PATCH 09/10] datapath: Allow masks for set actions.

Thomas Graf tgraf at redhat.com
Wed Apr 9 09:53:40 UTC 2014


On 04/09/2014 01:38 AM, Jarno Rajahalme wrote:
> Masked set actions allow more megaflow wildcarding.  All other key
> types than the tunnel key that can be set, can now be set with a mask.
>
> It is not clear wether masked set is useful for skb_priority.
> However, we already use the LSB of pkt_mark for IPSec in tunnels, so
> it might be useful to be able to set individual bits on pkt_mark.

This looks great!

> +	if (mask) {
> +		ether_addr_copy_masked(eth_hdr(skb)->h_source, key->eth_src,
> +				       mask->eth_src);
> +		ether_addr_copy_masked(eth_hdr(skb)->h_dest, key->eth_dst,
> +				       mask->eth_dst);
> +	} else {
> +		ether_addr_copy(eth_hdr(skb)->h_source, key->eth_src);
> +		ether_addr_copy(eth_hdr(skb)->h_dest, key->eth_dst);
> +	}

Quite a few of these branches are introduced in the fast path. Might
be worth checking 'perf stat' quickly to see if this generates a lot
of branch misses for either the wildcards enabled or the wildcards
disabled case. It might in fact be faster to always assume a mask if
the compiler cannot predict this correctly.

> @@ -1281,13 +1294,17 @@ static int validate_set(const struct nlattr *a,
>   {
>   	const struct nlattr *ovs_key = nla_data(a);
>   	int key_type = nla_type(ovs_key);
> +	bool have_mask;
>
>   	/* There can be only one key in a action */
>   	if (nla_total_size(nla_len(ovs_key)) != nla_len(a))
>   		return -EINVAL;
>
> +	have_mask = (ovs_key_lens[key_type] * 2 == nla_len(ovs_key));
> +

key_type is not validated at this point and might be > OVS_KEY_ATTR_MAX
and thus access ovs_key_lens[] out of bound.

>   	if (key_type > OVS_KEY_ATTR_MAX ||
>   	    (ovs_key_lens[key_type] != nla_len(ovs_key) &&
> +	     (!have_mask || !validate_masked(nla_data(ovs_key), nla_len(ovs_key))) &&
>   	     ovs_key_lens[key_type] != -1))
>   		return -EINVAL;
>



More information about the dev mailing list