On Wed, Nov 2, 2011 at 10:36 AM, Ben Pfaff <b...@nicira.com> wrote:
> 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 <je...@nicira.com>
>
> It took me a minute to realize that by "partial VLAN tag" you mean a
> frame that is truncated after the tpid.
Yes, although the truncation could happen not just immediately after
the TPID but part way through the vlan tag as well (I think this is
actually important to how you interpret it).
> 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.
That's a bug - I originally did that but I thought that the check for
EtherType of 0x8100 and !VLAN_TAG_PRESENT would handle it but I see
now that's not really the case. I applied this patch:
diff --git a/datapath/flow.c b/datapath/flow.c
index f9e8c7b..9c9c330d 100644
--- a/datapath/flow.c
+++ b/datapath/flow.c
@@ -963,6 +963,8 @@ int flow_from_nlattrs(struct sw_flow_key *swkey, int *key_le
break;
case TRANSITION(OVS_KEY_ATTR_8021Q, OVS_KEY_ATTR_ETHERTYPE):
+ if (swkey->eth.type != htons(ETH_P_802_2))
+ goto invalid;
case TRANSITION(OVS_KEY_ATTR_ETHERNET, OVS_KEY_ATTR_ETHERTYPE):
swkey->eth.type = nla_get_be16(nla);
if (ntohs(swkey->eth.type) < 1536)
diff --git a/lib/odp-util.c b/lib/odp-util.c
index 5a4eda4..b2d9ecc 100644
--- a/lib/odp-util.c
+++ b/lib/odp-util.c
@@ -1053,6 +1053,9 @@ odp_flow_key_to_flow(const struct nlattr *key, size_t key_
break;
case TRANSITION(OVS_KEY_ATTR_8021Q, OVS_KEY_ATTR_ETHERTYPE):
+ if (flow->dl_type != htons(FLOW_DL_TYPE_NONE)) {
+ return EINVAL;
+ }
case TRANSITION(OVS_KEY_ATTR_ETHERNET, OVS_KEY_ATTR_ETHERTYPE):
flow->dl_type = nl_attr_get_be16(nla);
if (ntohs(flow->dl_type) < 1536) {
> 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.
That was actually my original plan. However, I now think that it is
conceptually wrong:
* There's no way to distinguish between invalid and not parsed. I
like to be able to tell the difference because over time as we add
support for more types of tagging (not just multiple levels but
different TPIDs as well) I don't really want to accept a flow that has
different meaning based on the kernel version.
* It doesn't really mesh with how 802.1Q defines vlan tags - the
standard treats the tag as something that is inserted between the MAC
addresses and the EtherType, which is how we treat it currently in
normal cases by breaking addresses, vlan, and EtherType into three
separate attributes. However, if we put 0x8100 in the EtherType for
an invalid tag then it's essentially merging them together.
* It seems too fluid - ideally I would like vlan to act like just
another protocol layer in the parsing chain. This means that we parse
layers incrementally and later protocols do not affect earlier ones,
even if they are invalid. As long as we consider the TPID to be part
of the vlan tag this isn't possible to fully implement but I'd like to
keep things from bouncing around as much as possible.
* It's inconsistent with other protocols - other protocols (like IPv6)
hand up an empty header if there is a parse error. I think
consistency is important because there are other areas we might have
similar behavior where headers are inserted in the middle of the parse
chain, like extension headers.
> 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.
That makes sense. I applied this patch:
diff --git a/lib/odp-util.c b/lib/odp-util.c
index b2d9ecc..dc4f2b7 100644
--- a/lib/odp-util.c
+++ b/lib/odp-util.c
@@ -357,9 +357,13 @@ format_odp_key_attr(const struct nlattr *a, struct ds *ds)
if (q_key->q_tpid != htons(ETH_TYPE_VLAN)) {
ds_put_format(ds, "tpid=0x%04"PRIx16",", ntohs(q_key->q_tpid));
}
- ds_put_format(ds, "vid=%"PRIu16",pcp=%d)",
- vlan_tci_to_vid(q_key->q_tci),
- vlan_tci_to_pcp(q_key->q_tci));
+ if (q_key->q_tci & htons(VLAN_CFI)) {
+ ds_put_format(ds, "vid=%"PRIu16",pcp=%d)",
+ vlan_tci_to_vid(q_key->q_tci),
+ vlan_tci_to_pcp(q_key->q_tci));
+ } else {
+ ds_put_cstr(ds, "<incomplete>)");
+ }
break;
case OVS_KEY_ATTR_ETHERTYPE:
@@ -573,20 +577,32 @@ parse_odp_key_attr(const char *s, struct ofpbuf *key)
{
uint16_t tpid = ETH_TYPE_VLAN;
- uint16_t vid;
- int pcp;
+ uint16_t vid = 0;
+ int pcp = 0;
+ uint16_t cfi = 0;
+ bool parsed = false;
int n = -1;
+ if ((sscanf(s, "vlan(<incomplete>)%n",
+ &n) >= 0 && n > 0) ||
+ (sscanf(s, "vlan(tpid=%"SCNi16",<incomplete>)%n",
+ &tpid, &n) > 0 && n > 0)) {
+ parsed = true;
+ }
if ((sscanf(s, "vlan(vid=%"SCNi16",pcp=%i)%n",
&vid, &pcp, &n) > 0 && n > 0) ||
(sscanf(s, "vlan(tpid=%"SCNi16",vid=%"SCNi16",pcp=%i)%n",
&tpid, &vid, &pcp, &n) > 0 && n > 0)) {
+ parsed = true;
+ cfi = VLAN_CFI;
+ }
+ if (parsed) {
struct ovs_key_8021q q_key;
q_key.q_tpid = htons(tpid);
q_key.q_tci = htons((vid << VLAN_VID_SHIFT) |
(pcp << VLAN_PCP_SHIFT) |
- VLAN_CFI);
+ cfi);
nl_msg_put_unspec(key, OVS_KEY_ATTR_8021Q, &q_key, sizeof q_key);
return n;
}
> 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.
When there is a partial vlan tag, struct flow will have an EtherType
of 0x8100 and no CFI bit set. The previous version of
odp_flow_key_from_flow() will generate matching Netlink attributes
i.e. an EtherType but not vlan but I want vlan but no EtherType.
> Do you want the {} around the statement below?
Yes, why?
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
