From: Ben Pfaff Date: Tue, 15 Nov 2011 01:19:41 +0000 (-0800) Subject: datapath: Don't drop packets with partial vlan tags. X-Git-Url: https://pintos-os.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=8ddc056dd1e2c150c3bf8bb16811815736beb554;p=openvswitch datapath: Don't drop packets with partial vlan tags. 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. Reported-by: Jesse Gross Signed-off-by: Ben Pfaff Acked-by: Jesse Gross --- diff --git a/datapath/README b/datapath/README index 2d8a141b..b8a048b8 100644 --- a/datapath/README +++ b/datapath/README @@ -145,6 +145,41 @@ not understand the "vlan" key will not see either of those attributes and therefore will not misinterpret them. (Also, the outer eth_type is still 0x8100, not changed to 0x0800.) +Handling malformed packets +-------------------------- + +Don't drop packets in the kernel for malformed protocol headers, bad +checksums, etc. This would prevent userspace from implementing a +simple Ethernet switch that forwards every packet. + +Instead, in such a case, include an attribute with "empty" content. +It doesn't matter if the empty content could be valid protocol values, +as long as those values are rarely seen in practice, because userspace +can always forward all packets with those values to userspace and +handle them individually. + +For example, consider a packet that contains an IP header that +indicates protocol 6 for TCP, but which is truncated just after the IP +header, so that the TCP header is missing. The flow key for this +packet would include a tcp attribute with all-zero src and dst, like +this: + + eth(...), eth_type(0x0800), ip(proto=6, ...), tcp(src=0, dst=0) + +As another example, consider a packet with an Ethernet type of 0x8100, +indicating that a VLAN TCI should follow, but which is truncated just +after the Ethernet type. The flow key for this packet would include +an all-zero-bits vlan and an empty encap attribute, like this: + + eth(...), eth_type(0x8100), vlan(0), encap() + +Unlike a TCP packet with source and destination ports 0, an +all-zero-bits VLAN TCI is not that rare, so the CFI bit (aka +VLAN_TAG_PRESENT inside the kernel) is ordinarily set in a vlan +attribute expressly to allow this situation to be distinguished. +Thus, the flow key in this second example unambiguously indicates a +missing or malformed VLAN TCI. + Other rules ----------- diff --git a/datapath/actions.c b/datapath/actions.c index 88eca6cd..03fef922 100644 --- a/datapath/actions.c +++ b/datapath/actions.c @@ -112,7 +112,7 @@ static int push_vlan(struct sk_buff *skb, const struct ovs_action_push_vlan *vla + ETH_HLEN, VLAN_HLEN, 0)); } - __vlan_hwaccel_put_tag(skb, ntohs(vlan->vlan_tci)); + __vlan_hwaccel_put_tag(skb, ntohs(vlan->vlan_tci) & ~VLAN_TAG_PRESENT); return 0; } diff --git a/datapath/datapath.c b/datapath/datapath.c index 2d67657d..c7d24129 100644 --- a/datapath/datapath.c +++ b/datapath/datapath.c @@ -657,7 +657,7 @@ static int validate_actions(const struct nlattr *attr, vlan = nla_data(a); if (vlan->vlan_tpid != htons(ETH_P_8021Q)) return -EINVAL; - if (vlan->vlan_tci & htons(VLAN_TAG_PRESENT)) + if (!(vlan->vlan_tci & htons(VLAN_TAG_PRESENT))) return -EINVAL; break; diff --git a/datapath/flow.c b/datapath/flow.c index fded9375..7a136248 100644 --- a/datapath/flow.c +++ b/datapath/flow.c @@ -480,6 +480,9 @@ static int parse_vlan(struct sk_buff *skb, struct sw_flow_key *key) }; struct qtag_prefix *qp; + if (unlikely(skb->len < sizeof(struct qtag_prefix) + sizeof(__be16))) + return 0; + if (unlikely(!pskb_may_pull(skb, sizeof(struct qtag_prefix) + sizeof(__be16)))) return -ENOMEM; @@ -1045,18 +1048,35 @@ int flow_from_nlattrs(struct sw_flow_key *swkey, int *key_lenp, memcpy(swkey->eth.src, eth_key->eth_src, ETH_ALEN); memcpy(swkey->eth.dst, eth_key->eth_dst, ETH_ALEN); - if (attrs == ((1 << OVS_KEY_ATTR_VLAN) | - (1 << OVS_KEY_ATTR_ETHERTYPE) | - (1 << OVS_KEY_ATTR_ENCAP)) && + if (attrs & (1u << OVS_KEY_ATTR_ETHERTYPE) && nla_get_be16(a[OVS_KEY_ATTR_ETHERTYPE]) == htons(ETH_P_8021Q)) { - swkey->eth.tci = nla_get_be16(a[OVS_KEY_ATTR_VLAN]); - if (swkey->eth.tci & htons(VLAN_TAG_PRESENT)) + const struct nlattr *encap; + __be16 tci; + + if (attrs != ((1 << OVS_KEY_ATTR_VLAN) | + (1 << OVS_KEY_ATTR_ETHERTYPE) | + (1 << OVS_KEY_ATTR_ENCAP))) return -EINVAL; - swkey->eth.tci |= htons(VLAN_TAG_PRESENT); - err = parse_flow_nlattrs(a[OVS_KEY_ATTR_ENCAP], a, &attrs); - if (err) - return err; + encap = a[OVS_KEY_ATTR_ENCAP]; + tci = nla_get_be16(a[OVS_KEY_ATTR_VLAN]); + if (tci & htons(VLAN_TAG_PRESENT)) { + swkey->eth.tci = tci; + + err = parse_flow_nlattrs(encap, a, &attrs); + if (err) + return err; + } else if (!tci) { + /* Corner case for truncated 802.1Q header. */ + if (nla_len(encap)) + return -EINVAL; + + swkey->eth.type = htons(ETH_P_8021Q); + *key_lenp = key_len; + return 0; + } else { + return -EINVAL; + } } if (attrs & (1 << OVS_KEY_ATTR_ETHERTYPE)) { @@ -1214,11 +1234,12 @@ int flow_to_nlattrs(const struct sw_flow_key *swkey, struct sk_buff *skb) memcpy(eth_key->eth_src, swkey->eth.src, ETH_ALEN); memcpy(eth_key->eth_dst, swkey->eth.dst, ETH_ALEN); - if (swkey->eth.tci != htons(0)) { + if (swkey->eth.tci || swkey->eth.type == htons(ETH_P_8021Q)) { NLA_PUT_BE16(skb, OVS_KEY_ATTR_ETHERTYPE, htons(ETH_P_8021Q)); - NLA_PUT_BE16(skb, OVS_KEY_ATTR_VLAN, - swkey->eth.tci & ~htons(VLAN_TAG_PRESENT)); + NLA_PUT_BE16(skb, OVS_KEY_ATTR_VLAN, swkey->eth.tci); encap = nla_nest_start(skb, OVS_KEY_ATTR_ENCAP); + if (!swkey->eth.tci) + goto unencap; } else { encap = NULL; } diff --git a/include/linux/openvswitch.h b/include/linux/openvswitch.h index 2d141e0d..4f081369 100644 --- a/include/linux/openvswitch.h +++ b/include/linux/openvswitch.h @@ -439,8 +439,8 @@ enum ovs_userspace_attr { /** * struct ovs_action_push_vlan - %OVS_ACTION_ATTR_PUSH_VLAN action argument. * @vlan_tpid: Tag protocol identifier (TPID) to push. - * @vlan_tci: Tag control identifier (TCI) to push. The CFI bit must not be - * set. + * @vlan_tci: Tag control identifier (TCI) to push. The CFI bit must be set + * (but it will not be set in the 802.1Q header that is pushed). * * The @vlan_tpid value is typically %ETH_P_8021Q. The only acceptable TPID * values are those that the kernel module also parses as 802.1Q headers, to diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c index eb8d66d9..bc93a102 100644 --- a/lib/dpif-netdev.c +++ b/lib/dpif-netdev.c @@ -1313,7 +1313,7 @@ dp_netdev_execute_actions(struct dp_netdev *dp, case OVS_ACTION_ATTR_PUSH_VLAN: vlan = nl_attr_get(a); - eth_push_vlan(packet, vlan->vlan_tci); + eth_push_vlan(packet, vlan->vlan_tci & ~htons(VLAN_CFI)); break; case OVS_ACTION_ATTR_POP_VLAN: diff --git a/lib/odp-util.c b/lib/odp-util.c index b974dcc4..38215538 100644 --- a/lib/odp-util.c +++ b/lib/odp-util.c @@ -197,6 +197,17 @@ format_odp_userspace_action(struct ds *ds, const struct nlattr *attr) ds_put_char(ds, ')'); } +static void +format_vlan_tci(struct ds *ds, ovs_be16 vlan_tci) +{ + ds_put_format(ds, "vid=%"PRIu16",pcp=%d", + vlan_tci_to_vid(vlan_tci), + vlan_tci_to_pcp(vlan_tci)); + if (!(vlan_tci & htons(VLAN_CFI))) { + ds_put_cstr(ds, ",cfi=0"); + } +} + static void format_odp_action(struct ds *ds, const struct nlattr *a) { @@ -230,9 +241,8 @@ format_odp_action(struct ds *ds, const struct nlattr *a) if (vlan->vlan_tpid != htons(ETH_TYPE_VLAN)) { ds_put_format(ds, "tpid=0x%04"PRIx16",", ntohs(vlan->vlan_tpid)); } - ds_put_format(ds, "vid=%"PRIu16",pcp=%d)", - vlan_tci_to_vid(vlan->vlan_tci), - vlan_tci_to_pcp(vlan->vlan_tci)); + format_vlan_tci(ds, vlan->vlan_tci); + ds_put_char(ds, ')'); break; case OVS_ACTION_ATTR_POP_VLAN: ds_put_cstr(ds, "pop_vlan"); @@ -395,9 +405,9 @@ format_odp_key_attr(const struct nlattr *a, struct ds *ds) break; case OVS_KEY_ATTR_VLAN: - ds_put_format(ds, "(vid=%"PRIu16",pcp=%d)", - vlan_tci_to_vid(nl_attr_get_be16(a)), - vlan_tci_to_pcp(nl_attr_get_be16(a))); + ds_put_char(ds, '('); + format_vlan_tci(ds, nl_attr_get_be16(a)); + ds_put_char(ds, ')'); break; case OVS_KEY_ATTR_ETHERTYPE: @@ -616,13 +626,23 @@ parse_odp_key_attr(const char *s, struct ofpbuf *key) { uint16_t vid; int pcp; + int cfi; int n = -1; if ((sscanf(s, "vlan(vid=%"SCNi16",pcp=%i)%n", &vid, &pcp, &n) > 0 && n > 0)) { nl_msg_put_be16(key, OVS_KEY_ATTR_VLAN, htons((vid << VLAN_VID_SHIFT) | - (pcp << VLAN_PCP_SHIFT))); + (pcp << VLAN_PCP_SHIFT) | + VLAN_CFI)); + return n; + } else if ((sscanf(s, "vlan(vid=%"SCNi16",pcp=%i,cfi=%i)%n", + &vid, &pcp, &cfi, &n) > 0 + && n > 0)) { + nl_msg_put_be16(key, OVS_KEY_ATTR_VLAN, + htons((vid << VLAN_VID_SHIFT) | + (pcp << VLAN_PCP_SHIFT) | + (cfi ? VLAN_CFI : 0))); return n; } } @@ -920,11 +940,13 @@ odp_flow_key_from_flow(struct ofpbuf *buf, const struct flow *flow) memcpy(eth_key->eth_src, flow->dl_src, ETH_ADDR_LEN); memcpy(eth_key->eth_dst, flow->dl_dst, ETH_ADDR_LEN); - if (flow->vlan_tci != htons(0)) { + if (flow->vlan_tci != htons(0) || flow->dl_type == htons(ETH_TYPE_VLAN)) { nl_msg_put_be16(buf, OVS_KEY_ATTR_ETHERTYPE, htons(ETH_TYPE_VLAN)); - nl_msg_put_be16(buf, OVS_KEY_ATTR_VLAN, - flow->vlan_tci & ~htons(VLAN_CFI)); + nl_msg_put_be16(buf, OVS_KEY_ATTR_VLAN, flow->vlan_tci); encap = nl_msg_start_nested(buf, OVS_KEY_ATTR_ENCAP); + if (flow->vlan_tci == htons(0)) { + goto unencap; + } } else { encap = 0; } @@ -1198,27 +1220,47 @@ odp_flow_key_to_flow(const struct nlattr *key, size_t key_len, } expected_attrs |= UINT64_C(1) << OVS_KEY_ATTR_ETHERNET; - if ((present_attrs & ~expected_attrs) - == ((UINT64_C(1) << OVS_KEY_ATTR_ETHERTYPE) | - (UINT64_C(1) << OVS_KEY_ATTR_VLAN) | - (UINT64_C(1) << OVS_KEY_ATTR_ENCAP)) + if (present_attrs & (UINT64_C(1) << OVS_KEY_ATTR_ETHERTYPE) && (nl_attr_get_be16(attrs[OVS_KEY_ATTR_ETHERTYPE]) == htons(ETH_TYPE_VLAN))) { - const struct nlattr *encap = attrs[OVS_KEY_ATTR_ENCAP]; - const struct nlattr *vlan = attrs[OVS_KEY_ATTR_VLAN]; - - flow->vlan_tci = nl_attr_get_be16(vlan); - if (flow->vlan_tci & htons(VLAN_CFI)) { - return EINVAL; - } - flow->vlan_tci |= htons(VLAN_CFI); - - error = parse_flow_nlattrs(nl_attr_get(encap), nl_attr_get_size(encap), - attrs, &present_attrs); + /* The Ethernet type is 0x8100 so there must be a VLAN tag + * and encapsulated protocol information. */ + const struct nlattr *encap; + __be16 tci; + int error; + + expected_attrs |= ((UINT64_C(1) << OVS_KEY_ATTR_ETHERTYPE) | + (UINT64_C(1) << OVS_KEY_ATTR_VLAN) | + (UINT64_C(1) << OVS_KEY_ATTR_ENCAP)); + error = check_expectations(present_attrs, expected_attrs, + key, key_len); if (error) { return error; } - expected_attrs = 0; + + encap = attrs[OVS_KEY_ATTR_ENCAP]; + tci = nl_attr_get_be16(attrs[OVS_KEY_ATTR_VLAN]); + if (tci & htons(VLAN_CFI)) { + flow->vlan_tci = tci; + + error = parse_flow_nlattrs(nl_attr_get(encap), + nl_attr_get_size(encap), + attrs, &present_attrs); + if (error) { + return error; + } + expected_attrs = 0; + } else if (tci == htons(0)) { + /* Corner case for a truncated 802.1Q header. */ + if (nl_attr_get_size(encap)) { + return EINVAL; + } + + flow->dl_type = htons(ETH_TYPE_VLAN); + return 0; + } else { + return EINVAL; + } } if (present_attrs & (UINT64_C(1) << OVS_KEY_ATTR_ETHERTYPE)) { diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c index 9356800c..d525d4e7 100644 --- a/ofproto/ofproto-dpif.c +++ b/ofproto/ofproto-dpif.c @@ -3638,7 +3638,7 @@ commit_vlan_action(struct action_xlate_ctx *ctx, ovs_be16 new_tci) struct ovs_action_push_vlan vlan; vlan.vlan_tpid = htons(ETH_TYPE_VLAN); - vlan.vlan_tci = new_tci & ~htons(VLAN_CFI); + vlan.vlan_tci = new_tci; nl_msg_put_unspec(ctx->odp_actions, OVS_ACTION_ATTR_PUSH_VLAN, &vlan, sizeof vlan); } @@ -4719,6 +4719,14 @@ flow_get_vlan(struct ofproto_dpif *ofproto, const struct flow *flow, return -1; } } else { + if (flow->dl_type == htons(ETH_TYPE_VLAN) && + !(flow->vlan_tci & htons(VLAN_CFI))) { + static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5); + VLOG_WARN_RL(&rl, "bridge %s: dropping packet with partial " + "VLAN tag received on port %s", + ofproto->up.name, in_bundle->name); + return -1; + } if (in_bundle->vlan_mode != PORT_VLAN_TRUNK) { return in_bundle->vlan; } else {