In ovs kernel datapath upcall, the *key* and packet are passed to userspace. The key contains the fields/meta extracted from packet. Once the ovs-vswitchd receives the upcall, the packet is extracted again into *flow*. Next, the flow is used to match openflow rules to generate the wildcard(wc). At last, vswitchd installs a mega_flow in datapath(mega_flow = key/wc,action)
We can see that vswitchd generate wc from flow while it installs dp flow with key. If the key is not consistent with the flow [1], we get bad mega_flow. Let's assume we have the flowing rules, means to block tcp port 0-0xf, but allow other ports. "table=0,priority=100,tcp,tp_dst=0x0/0xfff0 actions=drop" "table=0,priority=90,tcp actions=p1" good case: If a packet has tcp dst=0x10, generated `mega_flow=0x10/0xfff0,out:p1`, this is expected. bad case: If a packet has tcp dst=0x10 but not pass tcphdr_ok [1], generated wc and action are `0xfff0,out:p1`. The mega_flow will be `0x0/0xfff0,out:p1`, bacause mega_flow=key/wc,action. This allows packets with tcp port 0-0xf pass by mistake. The following scapy3 script triggers the issue: ```py eth=Ether(src="fa:16:3e:5e:e3:57",dst="be:95:df:40:fb:57") ip=IP(src="10.10.10.10",dst="20.20.20.20") tcp=TCP(sport=100,dport=16,dataofs=1) sendp(eth/ip/tcp) ``` This patch is to prevent from installing datapath flow if the key is not consistant with the flow. [1] https://github.com/openvswitch/ovs/blob/v2.16.1/datapath/flow.c#L601 Signed-off-by: lic121 <lic...@chinatelecom.cn> --- ofproto/ofproto-dpif-upcall.c | 26 +++++++++++++++++++++++--- 1 file changed, 23 insertions(+), 3 deletions(-) diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c index 1c9c720..81f297d 100644 --- a/ofproto/ofproto-dpif-upcall.c +++ b/ofproto/ofproto-dpif-upcall.c @@ -244,6 +244,7 @@ struct upcall { size_t key_len; /* Datapath flow key length. */ const struct nlattr *out_tun_key; /* Datapath output tunnel key. */ + const struct flow *key_as_flow; /* converted from key. */ struct user_action_cookie cookie; uint64_t odp_actions_stub[1024 / 8]; /* Stub for odp_actions. */ @@ -810,6 +811,7 @@ recv_upcalls(struct handler *handler) struct dpif_upcall dupcalls[UPCALL_MAX_BATCH]; struct upcall upcalls[UPCALL_MAX_BATCH]; struct flow flows[UPCALL_MAX_BATCH]; + struct flow key_as_flows[UPCALL_MAX_BATCH]; size_t n_upcalls, i; n_upcalls = 0; @@ -818,6 +820,7 @@ recv_upcalls(struct handler *handler) struct dpif_upcall *dupcall = &dupcalls[n_upcalls]; struct upcall *upcall = &upcalls[n_upcalls]; struct flow *flow = &flows[n_upcalls]; + struct flow *key_as_flow = &key_as_flows[n_upcalls]; unsigned int mru = 0; uint64_t hash = 0; int error; @@ -830,7 +833,7 @@ recv_upcalls(struct handler *handler) } upcall->fitness = odp_flow_key_to_flow(dupcall->key, dupcall->key_len, - flow, NULL); + key_as_flow, NULL); if (upcall->fitness == ODP_FIT_ERROR) { goto free_dupcall; } @@ -843,6 +846,9 @@ recv_upcalls(struct handler *handler) hash = nl_attr_get_u64(dupcall->hash); } + /* Fill flow with key_as_flow as upcall_receive needs + * packet flow info. */ + *flow = *key_as_flow; error = upcall_receive(upcall, udpif->backer, &dupcall->packet, dupcall->type, dupcall->userdata, flow, mru, &dupcall->ufid, PMD_ID_NULL); @@ -856,20 +862,21 @@ recv_upcalls(struct handler *handler) dupcall->key_len, NULL, 0, NULL, 0, &dupcall->ufid, PMD_ID_NULL, NULL); VLOG_INFO_RL(&rl, "received packet on unassociated datapath " - "port %"PRIu32, flow->in_port.odp_port); + "port %"PRIu32, key_as_flow->in_port.odp_port); } goto free_dupcall; } upcall->key = dupcall->key; upcall->key_len = dupcall->key_len; + upcall->key_as_flow = key_as_flow; upcall->ufid = &dupcall->ufid; upcall->hash = hash; upcall->out_tun_key = dupcall->out_tun_key; upcall->actions = dupcall->actions; - pkt_metadata_from_flow(&dupcall->packet.md, flow); + pkt_metadata_from_flow(&dupcall->packet.md, key_as_flow); flow_extract(&dupcall->packet, flow); error = process_upcall(udpif, upcall, @@ -1332,6 +1339,19 @@ should_install_flow(struct udpif *udpif, struct upcall *upcall) return false; } + /* For linux kernel datapath, the "key" extracted by kernel may be + * inconsistent with the flow extracted from packet by ovs. If that + * is the case, twe can't install the datapth flow (key/wc) */ + if (upcall->key_len && !flow_equal_except(upcall->key_as_flow, + upcall->flow, &upcall->wc)) { + VLOG_INFO_RL(&rl, "upcall: inconsistent on datapath key and " + "vswitchd extracted key. Datapath flow will not be " + "installed\n" + "datapath key: %s \nvswitchd extracted key: %s", + flow_to_string(upcall->key_as_flow, NULL), + flow_to_string(upcall->flow, NULL)); + return false; + } return true; } -- 1.8.3.1 _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev