[ovs-dev] [PATCH v5 11/13] datapath: Allow masks for set actions.

[Jesse Gross]

On Fri, Sep 5, 2014 at 4:05 PM, Jarno Rajahalme <jrajahalme at nicira.com> wrote:
> Masked set action allows more megaflow wildcarding.  Masked set action
> is now supported for all writeable key types, except for the tunnel
> key.
>
> The set tunnel action is an exception as any input tunnel info is
> cleared before action processing starts, so there is no tunnel info to
> mask.
>
> The kernel module converts all (non-tunnel) set actions to masked set
> actions.  This makes action processing more uniform, and results in
> less branching and duplicating the action processing code.  When
> returning actions to userspace, the fully masked set actions are
> converted to normal set actions.  To make this mapping consistent, we
> reject fully masked set actions at flow set-up time.
>
> Signed-off-by: Jarno Rajahalme <jrajahalme at nicira.com>

Sorry about the extremely long delay, I'm finally coming back to this...

> diff --git a/datapath/actions.c b/datapath/actions.c
> index 43ca2a0..243b672 100644
> --- a/datapath/actions.c
> +++ b/datapath/actions.c
> +static int set_ipv6(struct sk_buff *skb, const struct ovs_key_ipv6 *key,
> +                   const struct ovs_key_ipv6 *mask)
>  {
> @@ -483,38 +525,56 @@ static int set_ipv6(struct sk_buff *skb, const struct ovs_key_ipv6 *ipv6_key)
> -       if (memcmp(ipv6_key->ipv6_dst, daddr, sizeof(ipv6_key->ipv6_dst))) {
> +               mask_ipv6_addr(saddr, key->ipv6_src, mask->ipv6_src, masked);
> +
> +               if (unlikely(memcmp(saddr, masked, sizeof masked))) {
> +                       set_ipv6_addr(skb, key->ipv6_proto, saddr, masked,
> +                                     true);

set_ipv6_addr() does a memcpy as well, I think that's now unnecessary
given that we are masking into the packet directly.

> diff --git a/datapath/flow_netlink.c b/datapath/flow_netlink.c
> index 69d1919..ec32a00 100644
> --- a/datapath/flow_netlink.c
> +++ b/datapath/flow_netlink.c
> @@ -1682,12 +1743,45 @@ static int validate_set(const struct nlattr *a,
> +       /* Convert non-masked non-tunnel set actions to masked set actions. */
> +       if (!masked && key_type != OVS_KEY_ATTR_TUNNEL) {
> +               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 = __add_action(sfa, key_type, NULL, len);
> +               if (IS_ERR(at))
> +                       return PTR_ERR(at);
> +
> +               memcpy(nla_data(at), nla_data(ovs_key), key_len); /* Key. */
> +               /* Even though all-ones mask includes non-writeable fields,
> +                * which we do not allow above, we will not actually set them
> +                * when we execute the masked set action. */
> +               memset(nla_data(at) + key_len, 0xff, key_len);    /* Mask. */

What about IPv6 flow label? I think we need to actually unmask those
bits since we want to retain the original value in the packet instead
of zeroing them.

> +       } else if (masked) {
> +               /* Reject fully masked masked set actions so that the above
> +                * conversion can be unabiguously reverted when sending
> +                * actions back to userspace. */
> +               if (is_all_ones(nla_data(ovs_key) + key_len, key_len))
> +                       return -EINVAL;

It kind of pains me to have these types of exceptions from the
perspective of the new protocol that we are introducing. Is there any
way to achieve this effect without introducing a user-visible
interface quirk?