I guess that this will need reconsideration after the "encap" series,
so I guess there's no point in a detailed response right now--let me
know if you disagree.
On Wed, Nov 02, 2011 at 03:08:08PM -0700, Jesse Gross wrote:
> 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
