[ovs-dev] [PATCH 3/3] datapath: Don't drop packets with partial vlan tags.

Ben Pfaff blp at nicira.com
Wed Nov 2 17:36:02 UTC 2011

On Tue, Nov 01, 2011 at 10:50:51PM -0700, Jesse Gross wrote:
> In the future it is likely that our vlan support will expand to
> include multiply tagged packets.  When this happens, we would
> ideally like for it to be consistent with our current tagging.
> Currently, if we receive a packet with a partial VLAN tag we will
> automatically drop it in the kernel, which is unique among the
> protocols we support.  The only other reason to drop a packet is
> a memory allocation error.  For a doubly tagged packet, we will
> parse the first tag and indicate that another tag was present but
> do not drop if the second tag is incorrect as we do not parse it.
> This changes the behavior of the vlan parser to match other protocols
> and also deeper tags by indicating the presence of a broken tag with
> the 802.1Q EtherType but no vlan information.  This shifts the policy
> decision to userspace on whether to drop broken tags and allows us to
> uniformly add new levels of tag parsing.
> Although additional levels of control are provided to userspace, this
> maintains the current behavior of dropping packets with a broken
> tag when using the NORMAL action because that is the correct behavior
> for an 802.1Q-aware switch.  The userspace flow parser actually
> already had the new behavior so this corrects an inconsistency.
> Signed-off-by: Jesse Gross <jesse at nicira.com>

It took me a minute to realize that by "partial VLAN tag" you mean a
frame that is truncated after the tpid.

A frame with a partial VLAN tag can't have anything after the partial
tag, but the kernel flow_from_nlattrs() appears to allow arbitrary
additional flow data, e.g. it will accept a partial tag followed by a
valid Ethertype, IP, etc.

Representing a partial VLAN tag as a special case of
OVS_KEY_ATTR_8021Q seems to work, but I'm inclined to think that just
using OVS_KEY_ATTR_ETHERTYPE would work just as well.  After all, a
VLAN tag without a TCI isn't really a VLAN tag at all, so I don't know
that it necessarily makes sense to represent it as one, but it's
otherwise a perfectly good Ethertype.  I think that just using
OVS_KEY_ATTR_ETHERTYPE would introduce fewer special cases elsewhere
in the tree, too.

It's probably a good idea to distinguish these special non-tags in
format_odp_key_attr(), otherwise someday I'll be very confused
debugging.  And we'd want to adapt parse_odp_key_attr() also to be
able to parse them.

I think that the changes to odp_flow_key_from_flow() are unnecessary.
As posted, I think that they won't have the desired effect of creating
partial VLAN tags, but I believe that the original code would have.

Do you want the {} around the statement below?

> @@ -949,9 +953,13 @@ int flow_from_nlattrs(struct sw_flow_key *swkey, int *key_lenp,
>                       /* Only standard 0x8100 VLANs currently supported. */
>                       if (q_key->q_tpid != htons(ETH_P_8021Q))
>                               goto invalid;
> -                     if (q_key->q_tci & htons(VLAN_TAG_PRESENT))
> -                             goto invalid;
> -                     swkey->eth.tci = q_key->q_tci | htons(VLAN_TAG_PRESENT);
> +                     if (q_key->q_tci & htons(VLAN_TAG_PRESENT)) {
> +                             swkey->eth.tci = q_key->q_tci;
> +                     } else {

More information about the dev mailing list