[ovs-dev] [PATCH v4 4/4] datapath: Allow masks for set actions.

[Jarno Rajahalme]

On Sep 5, 2014, at 2:12 PM, Jarno Rajahalme <jrajahalme at nicira.com> wrote:

>>> 
>>>       case OVS_KEY_ATTR_IPV4:
>> [...]
>>> -               if (ipv4_key->ipv4_frag != flow_key->ip.frag)
>>> -                       return -EINVAL;
>>> +                       /* Non-writeable fields. */
>>> +                       if (mask->ipv4_proto || mask->ipv4_frag)
>>> +                               return -EINVAL;
>>> +               } else {
>>> +                       if (!flow_key->ip.proto)
>>> +                               return -EINVAL;
>> 
>> I believe that this check on ip.proto is being used to verify that the
>> IP header is actually present, so this would mean that we can write
>> off the end of the packet in the masked case.
> 
> But this is at flow set-up time, and the mask could still wildcard the ip.proto field,

I checked this and flow_key is the masked flow key, so it can not be wildcarded, sorry for the confusion. However, it is conceivable that a userspace app wants to set a flow to match on all IP packets with eth_type(0x0800), and then e.g. set(ipv4(tos=0)). So, I’d like to get rid of the ip.proto check anyway!

Is the make_writeable() check sufficient?

  Jarno

> so it could be zero or missing in an actual packet. Doesn’t the make_writeable take care of the verification?:
> 
> static int set_ipv4(struct sk_buff *skb, const struct ovs_key_ipv4 *key,
> 		    const struct ovs_key_ipv4 *mask)
> {
> 	struct iphdr *nh;
> 	__be32 new_addr;
> 	int err;
> 
> 	err = make_writable(skb, skb_network_offset(skb) +
> 				 sizeof(struct iphdr));
> 	if (unlikely(err))
> 		return err;
> 
> 	nh = ip_hdr(skb);
> 
> 
>> 
>>>       case OVS_KEY_ATTR_IPV6:
>> [...]
>>> -               if (ipv6_key->ipv6_frag != flow_key->ip.frag)
>>> -                       return -EINVAL;
>>> +                       /* Non-writeable fields. */
>>> +                       if (mask->ipv6_proto || mask->ipv6_frag)
>>> +                               return -EINVAL;
>>> +
>>> +                       /* Invalid bits in the flow label mask? */
>>> +                       if (ntohl(mask->ipv6_label) & 0xFFF00000)
>>> +                               return -EINVAL;
>>> +               } else {
>>> +                       if (!flow_key->ip.proto)
>>> +                               return -EINVAL;
>> 
>> Same issue with header validation here.
>> 
>>> @@ -1605,12 +1649,43 @@ static int validate_set(const struct nlattr *a,
>>> +       /* Convert non-masked set actions to masked set actions.
>>> +        * Tunnel set action returns before getting here. */
>>> +       if (!masked) {
>>> +               int start, len = key_len * 2;
>>> +               struct nlattr *at;
>>> +
>>> +               *skip_copy = true;
>>> +
>>> +               start = add_nested_action_start(sfa,
>>> +                                               OVS_ACTION_ATTR_SET_MASKED);
>>> +               if (start < 0)
>>> +                       return start;
>>> +
>>> +               at = reserve_sfa_size(sfa, nla_attr_size(len));
>>> +               if (IS_ERR(at))
>>> +                       return PTR_ERR(at);
>>> +
>>> +               at->nla_type = key_type;
>>> +               at->nla_len = nla_attr_size(len);
>> 
>> I think this can be simplified by using add_action() or __add_action().
>> 
>