[ovs-dev] [PATCH] datapath: fix bugs in exporting tunnel mask attributes in netlink

[Jesse Gross]

On Wed, Jul 3, 2013 at 9:11 AM, Andy Zhou <azhou at nicira.com> wrote:
> Reported-by: Justin Pettit <jpettit at nicira.com>
> Signed-off-by: Andy Zhou <azhou at nicira.com>
> ---
>  datapath/flow.c |    7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)
>
> diff --git a/datapath/flow.c b/datapath/flow.c
> index 2ac36b6..c598641 100644
> --- a/datapath/flow.c
> +++ b/datapath/flow.c
> @@ -1251,15 +1251,14 @@ int ipv4_tun_to_nlattr(struct sk_buff *skb,
>                 return -EMSGSIZE;
>         if (nla_put_be32(skb, OVS_TUNNEL_KEY_ATTR_IPV4_DST, output->ipv4_dst))
>                 return -EMSGSIZE;
> -       if (tun_key->ipv4_tos &&
> -           nla_put_u8(skb, OVS_TUNNEL_KEY_ATTR_TOS, output->ipv4_tos))
> +       if (nla_put_u8(skb, OVS_TUNNEL_KEY_ATTR_TOS, output->ipv4_tos))
>                 return -EMSGSIZE;
>         if (nla_put_u8(skb, OVS_TUNNEL_KEY_ATTR_TTL, output->ipv4_ttl))
>                 return -EMSGSIZE;
> -       if ((tun_key->tun_flags & TUNNEL_DONT_FRAGMENT) &&
> +       if ((output->tun_flags & TUNNEL_DONT_FRAGMENT) &&
>                 nla_put_flag(skb, OVS_TUNNEL_KEY_ATTR_DONT_FRAGMENT))
>                 return -EMSGSIZE;
> -       if ((tun_key->tun_flags & TUNNEL_CSUM) &&
> +       if ((output->tun_flags & TUNNEL_CSUM) &&
>                 nla_put_flag(skb, OVS_TUNNEL_KEY_ATTR_CSUM))
>                 return -EMSGSIZE;

I think this patch is correct but needs to go farther.

For the masks, at least, we need to always output the attribute. For
example, if the TUNNEL_KEY flag isn't set in the key but it is in the
mask that means that there must not be a tunnel key. However, with the
current logic we wouldn't output the mask, which means that we don't
care.

It's safe to output a key attribute that is zero, even if it isn't
strictly necessary, so we can just error on the side of doing that to
simplify the code. Really the only place where we should look at the
key is when figuring out the prerequisites for later protocols. Can
you look through the other serialization code? I think we have the
same problem other places as well.