| Message ID | 20240219105932.342676-1-lic121@chinatelecom.cn |
|---|---|
| State | Under Review |
| Delegated to: | Ilya Maximets |
| Headers | show |
| Series | [ovs-dev,v2] upcall: Check flow consistant in upcall. | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
Hi Cheng, Cheng Li writes: > Ovs ko passes odp key and packet to userspace. Next packet is > extracted into flow, which is the input for xlate to generate wc. > At last, ukey(= odp_key/wc) is installed into datapath. If the > odp_key is not consistant with packet extracted flow. The ukey > will be wrong. > > commit [1] was created to fix inconsistance caused by bad tcp > header. commit [2] was cretaed to fix inconsistance caused by bad > ip header. There is no guarantee of the consistance of odp_key and > packet flow. So it is necessary to make the check to prevent from > installing wrong ukey. > > [1] 1f5749c790accd98dbcafdaefc40bf5e52d7c672 > [2] 79349cbab0b2a755140eedb91833ad2760520a83 > > Signed-off-by: Cheng Li <lic121@chinatelecom.cn> > --- We don't generally rely on the kernel ODP parsing, and this would create a situation where any kind of kernel / userspace mismatch would end up in incredibly poor performance. The two commits referenced are really parsing bugs in the userspace, and they need fixing regardless of whether userspace ignores or incorporates details from the provided kernel key. Did I miss something here?
diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c index b5cbeed87..bd93f0981 100644 --- a/ofproto/ofproto-dpif-upcall.c +++ b/ofproto/ofproto-dpif-upcall.c @@ -66,6 +66,7 @@ COVERAGE_DEFINE(upcall_flow_limit_reduced); COVERAGE_DEFINE(upcall_flow_limit_scaled); COVERAGE_DEFINE(upcall_ukey_contention); COVERAGE_DEFINE(upcall_ukey_replace); +COVERAGE_DEFINE(upcall_packet_flow_inconsistant); /* A thread that reads upcalls from dpif, forwards each upcall's packet, * and possibly sets up a kernel flow as a cache. */ @@ -840,6 +841,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 odp_key_flow; size_t n_upcalls, i; n_upcalls = 0; @@ -903,6 +905,8 @@ recv_upcalls(struct handler *handler) upcall->out_tun_key = dupcall->out_tun_key; upcall->actions = dupcall->actions; + /* Save odp flow before overwrite. */ + memcpy(&odp_key_flow, flow, sizeof odp_key_flow); pkt_metadata_from_flow(&dupcall->packet.md, flow); flow_extract(&dupcall->packet, flow); @@ -912,6 +916,12 @@ recv_upcalls(struct handler *handler) goto cleanup; } + if (!flow_equal_except(&odp_key_flow, flow, &upcall->wc)) { + /* If odp flow is not consistant with flow extract from packet, + * bad ukey/mask will be installed. */ + COVERAGE_INC(upcall_packet_flow_inconsistant); + upcall->xout.avoid_caching = true; + } n_upcalls++; continue; @@ -1376,6 +1386,10 @@ should_install_flow(struct udpif *udpif, struct upcall *upcall) return false; } + if (upcall->xout.avoid_caching) { + return false; + } + atomic_read_relaxed(&udpif->flow_limit, &flow_limit); if (udpif_get_n_flows(udpif) >= flow_limit) { COVERAGE_INC(upcall_flow_limit_hit);
Ovs ko passes odp key and packet to userspace. Next packet is extracted into flow, which is the input for xlate to generate wc. At last, ukey(= odp_key/wc) is installed into datapath. If the odp_key is not consistant with packet extracted flow. The ukey will be wrong. commit [1] was created to fix inconsistance caused by bad tcp header. commit [2] was cretaed to fix inconsistance caused by bad ip header. There is no guarantee of the consistance of odp_key and packet flow. So it is necessary to make the check to prevent from installing wrong ukey. [1] 1f5749c790accd98dbcafdaefc40bf5e52d7c672 [2] 79349cbab0b2a755140eedb91833ad2760520a83 Signed-off-by: Cheng Li <lic121@chinatelecom.cn> --- Notes: v2: Leverage avoid_caching to avoid ukey install. ofproto/ofproto-dpif-upcall.c | 14 ++++++++++++++ 1 file changed, 14 insertions(+)