| Message ID | 20240218154024.373642-1-lic121@chinatelecom.cn |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [ovs-dev] upcall: Check flow consistant in upcall. | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | fail | github build: failed |
| ovsrobot/intel-ovs-compilation | success | test: success |
On 2/18/24 16:40, Cheng Li wrote: > 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> Hi Cheng, Do you have a concrete example of a key / kernel version / OVS version that would generate an inconsistency that is not caught by the call to odp_flow_key_to_flow()? Thanks. Adrián > --- > ofproto/ofproto-dpif-upcall.c | 12 ++++++++++++ > 1 file changed, 12 insertions(+) > > diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c > index b5cbeed87..6e46e5a5a 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,8 @@ 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_flow; > + struct flow_wildcards flow_wc; > size_t n_upcalls, i; > > n_upcalls = 0; > @@ -903,8 +906,17 @@ recv_upcalls(struct handler *handler) > upcall->out_tun_key = dupcall->out_tun_key; > upcall->actions = dupcall->actions; > > + /* Save odp flow before overwrite. */ > + memcpy(&odp_flow, flow, sizeof flow); > pkt_metadata_from_flow(&dupcall->packet.md, flow); > flow_extract(&dupcall->packet, flow); > + flow_wildcards_init_for_packet(&flow_wc, &flow); > + if (!flow_equal_except(&odp_flow, flow, &flow_wc)) { > + /* If odp flow is not consistant with flow extract from packet, > + * bad ukey/mask will be installed. */ > + COVERAGE_INC(upcall_packet_flow_inconsistant); > + goto cleanup; > + } > > error = process_upcall(udpif, upcall, > &upcall->odp_actions, &upcall->wc);
diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c index b5cbeed87..6e46e5a5a 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,8 @@ 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_flow; + struct flow_wildcards flow_wc; size_t n_upcalls, i; n_upcalls = 0; @@ -903,8 +906,17 @@ recv_upcalls(struct handler *handler) upcall->out_tun_key = dupcall->out_tun_key; upcall->actions = dupcall->actions; + /* Save odp flow before overwrite. */ + memcpy(&odp_flow, flow, sizeof flow); pkt_metadata_from_flow(&dupcall->packet.md, flow); flow_extract(&dupcall->packet, flow); + flow_wildcards_init_for_packet(&flow_wc, &flow); + if (!flow_equal_except(&odp_flow, flow, &flow_wc)) { + /* If odp flow is not consistant with flow extract from packet, + * bad ukey/mask will be installed. */ + COVERAGE_INC(upcall_packet_flow_inconsistant); + goto cleanup; + } error = process_upcall(udpif, upcall, &upcall->odp_actions, &upcall->wc);
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> --- ofproto/ofproto-dpif-upcall.c | 12 ++++++++++++ 1 file changed, 12 insertions(+)