[ovs-dev] [PATCH 2/3] odp-util: Always export the priority and skb_mark netline attributes.

[Jesse Gross]

On Mon, Aug 5, 2013 at 11:06 AM, Ben Pfaff <blp at nicira.com> wrote:
> After some consultation with Andy, this is the commit message I ended
> up with.  Jesse, Andy, is this correct?
>
> --8<--------------------------cut here-------------------------->8--
>
> From: Andy Zhou <azhou at nicira.com>
> Date: Sat, 3 Aug 2013 12:23:15 -0700
> Subject: [PATCH] odp-util: Always export the priority and skb_mark netlink
>  attributes.
>
> Some of the kernel flows have "opaque" values used in special situations:
>
>     - When no in_port is available, the kernel internally uses
>       USHRT_MAX to represent this, but this should not be visible to
>       userspace, that is, it should be possible for the kernel to switch
>       to using some other value without affecting userspace-visible
>       behavior.
>
>     - When no skb_mark is specified, the kernel uses zero.  (It is easy to
>       believe that zero is fixed by the kernel ABI, not opaque, but there
>       is some desire to keep it opaque.)
>
>     - skb_priority, like skb_mark.
>
> If a flow specified via Netlink contained a mask without a value, then the
> kernel would default to matching against the opaque "default" value for that
> field.  For example, a match against in_port that specified no value but a
> mask of 0xf would, in the current kernel implementation, specify a match
> against USHRT_MAX with a mask of 0xf.  If the kernel accepted that value and
> mask as-is, and then the user tried to add another flow with, say, a value
> of 1 and mask of 1, that would overlap with the first flow, causing the
> kernel to reject it.
>
> There are multiple ways that the kernel could solve this problem:
>
>     - Reject flows that do not include a value, but only a mask, for a
>       given field.  This is what the kernel does for most fields,
>       including skb_priority and skb_mask.
>
>     - Always expand all masks for a given field to exact-match, so that
>       there can be no unexpected overlap among flows.  The kernel does
>       this for in_port.
>
> We are somewhat uncertain, however, what the final handling of skb_mark and
> skb_priority should be.  This commit allows us some flexibility in deferring
> the decision on kernel handling by making userspace always specify both a
> value and a mask for the field.
>
> Signed-off-by: Andy Zhou <azhou at nicira.com>
> [blp at nicira.com rewrote the commit message]
> Signed-off-by: Ben Pfaff <blp at nicira.com>

This information is factually correct but it's not really related to
the change in this commit.

The problem here is very simple: the current protocol allows a default
value of zero if either mark or priority is not specified (this
actually is part of the ABI). When userspace serializes either the
value or mask it looks at the value and omits the netlink attribute if
it is zero. This is a bug because an exact match on zero turns into a
wildcard of the field.

These two fields (plus input port and EtherType) are special because
they can be omitted whereas most other values are required to be fully
specified. These protocol variations tend to cause bugs (as above)
when we evolve the protocol because an exception that makes sense in
one context might not be logical in another. Since the default value
for mark and priority are merely shorthands, we can push the protocol
in a more consistent direction by ignoring the shortcut and always
serializing the values.