Hi, Ilya, are there any more comments beside the Fixes tag?
Peng He <[email protected]> 于2023年7月31日周一 20:53写道： > just found that I miss the Fixes tag, will add it in the next post. > > Peng He <[email protected]> 于2023年7月31日周一 14:44写道： > >> OVS allows overlapping megaflows, as long as the actions of these >> megaflows are equal. However, the current implementation of action >> modification relies on flow_lookup instead of ufid, this could result >> in looking up a wrong megaflow and make the ukeys and megaflows >> inconsistent >> >> Just like the test case in the patch, at first we have a rule with the >> prefix: >> >> 10.1.2.0/24 >> >> and we will get a megaflow with prefixes 10.1.2.2/24 when a packet with >> IP >> 10.1.2.2 is received. >> >> Then suppose we change the rule into 10.1.0.0/16. OVS prefers to keep the >> 10.1.2.2/24 megaflow and just changes its action instead of extending >> the prefix into 10.1.2.2/16. >> >> then suppose we have a 10.1.0.2 packet, since it misses the megaflow, >> this time, we will have an overlapping megaflow with the right prefix: >> 10.1.0.2/16 >> >> now we have two megaflows: >> 10.1.2.2/24 >> 10.1.0.2/16 >> >> last, suppose we have changed the ruleset again. The revalidator this >> time still decides to change the actions of both megaflows instead of >> deleting them. >> >> The dpif_netdev_flow_put will search the megaflow to modify with unmasked >> keys, however it might lookup the wrong megaflow as the key 10.1.2.2 >> matches >> both 10.1.2.2/24 and 10.1.0.2/16! >> >> This patch changes the megaflow lookup code in modification path into >> relying the ufid to find the correct megaflow instead of key lookup. >> >> Signed-off-by: Peng He <[email protected]> >> --- >> lib/dpif-netdev.c | 49 ++++++++++++++++++++++++++++++++--------------- >> tests/pmd.at | 48 ++++++++++++++++++++++++++++++++++++++++++++++ >> 2 files changed, 82 insertions(+), 15 deletions(-) >> >> diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c >> index 70b953ae6..8c88a5dc0 100644 >> --- a/lib/dpif-netdev.c >> +++ b/lib/dpif-netdev.c >> @@ -4191,7 +4191,7 @@ flow_put_on_pmd(struct dp_netdev_pmd_thread *pmd, >> const struct dpif_flow_put *put, >> struct dpif_flow_stats *stats) >> { >> - struct dp_netdev_flow *netdev_flow; >> + struct dp_netdev_flow *netdev_flow = NULL; >> int error = 0; >> >> if (stats) { >> @@ -4199,16 +4199,39 @@ flow_put_on_pmd(struct dp_netdev_pmd_thread *pmd, >> } >> >> ovs_mutex_lock(&pmd->flow_mutex); >> - netdev_flow = dp_netdev_pmd_lookup_flow(pmd, key, NULL); >> - if (!netdev_flow) { >> - if (put->flags & DPIF_FP_CREATE) { >> - dp_netdev_flow_add(pmd, match, ufid, put->actions, >> - put->actions_len, ODPP_NONE); >> + if (put->ufid) { >> + netdev_flow = dp_netdev_pmd_find_flow(pmd, put->ufid, >> + put->key, put->key_len); >> + } else { >> + /* Use key instead of the locally generated ufid >> + * to search netdev_flow. >> + */ >> + netdev_flow = dp_netdev_pmd_lookup_flow(pmd, key, NULL); >> + } >> + >> + if (put->flags & DPIF_FP_CREATE) { >> + if (!netdev_flow) { >> + /* If ufid is provided, use provided ufid, otherwise >> + * use locally generated ufid. >> + */ >> + dp_netdev_flow_add(pmd, match, put->ufid ? put->ufid : ufid, >> + put->actions, put->actions_len, >> ODPP_NONE); >> } else { >> - error = ENOENT; >> + error = EEXIST; >> } >> - } else { >> - if (put->flags & DPIF_FP_MODIFY) { >> + goto exit; >> + } >> + >> + if (put->flags & DPIF_FP_MODIFY) { >> + if (!netdev_flow) { >> + error = ENOENT; >> + } else { >> + if (!flow_equal(&match->flow, &netdev_flow->flow)) { >> + /* Overlapping flow. */ >> + error = EINVAL; >> + goto exit; >> + } >> + >> struct dp_netdev_actions *new_actions; >> struct dp_netdev_actions *old_actions; >> >> @@ -4239,15 +4262,11 @@ flow_put_on_pmd(struct dp_netdev_pmd_thread *pmd, >> * counter, and subtracting it before outputting the >> stats */ >> error = EOPNOTSUPP; >> } >> - >> ovsrcu_postpone(dp_netdev_actions_free, old_actions); >> - } else if (put->flags & DPIF_FP_CREATE) { >> - error = EEXIST; >> - } else { >> - /* Overlapping flow. */ >> - error = EINVAL; >> } >> } >> + >> +exit: >> ovs_mutex_unlock(&pmd->flow_mutex); >> return error; >> } >> diff --git a/tests/pmd.at b/tests/pmd.at >> index 48f3d432d..f399294d2 100644 >> --- a/tests/pmd.at >> +++ b/tests/pmd.at >> @@ -1300,3 +1300,51 @@ OVS_WAIT_UNTIL([tail -n +$LINENUM ovs-vswitchd.log >> | grep "PMD load based sleeps >> >> OVS_VSWITCHD_STOP >> AT_CLEANUP >> + >> +AT_SETUP([PMD - revalidator wrongly modify userspace megaflows]) >> + >> +OVS_VSWITCHD_START( >> +[add-port br0 p1 \ >> + -- set interface p1 type=dummy-pmd \ >> + -- set bridge br0 datapath-type=dummy \ >> + -- add-port br0 p2 \ >> + -- set interface p2 type=dummy-pmd -- >> +]) >> + >> +dnl Add one openflow rule and generate a megaflow. >> +AT_CHECK([ovs-ofctl add-flow br0 'table=0,in_port=p1,ip,nw_dst= >> 10.1.2.0/24,actions=p2']) >> +AT_CHECK([ovs-appctl netdev-dummy/receive p1 >> 'ipv4(src=10.0.0.1,dst=10.1.2.2),tcp(src=1,dst=2)']) >> + >> +AT_CHECK([ovs-appctl dpctl/dump-flows | sed 's/.*core: [[0-9]]*//'], >> [0], [ >> +recirc_id(0),in_port(1),packet_type(ns=0,id=0),eth_type(0x0800),ipv4(dst= >> 10.1.2.2/255.255.255.0,frag=no), packets:0, bytes:0, used:never, >> actions:2 >> +]) >> + >> +AT_CHECK([ovs-appctl netdev-dummy/receive p1 >> 'ipv4(src=10.0.0.1,dst=10.1.2.2),tcp(src=1,dst=2)']) >> +dnl Replace openflow rules, trigger the revalidation. >> +AT_CHECK([echo 'table=0,in_port=p1,ip,nw_dst=10.1.0.0/16 >> actions=ct(commit)' | dnl >> +ovs-ofctl --bundle replace-flows br0 -]) >> +AT_CHECK([ovs-appctl revalidator/wait]) >> + >> +AT_CHECK([ovs-appctl netdev-dummy/receive p1 >> 'ipv4(src=10.0.0.1,dst=10.1.0.2),tcp(src=1,dst=2)']) >> +AT_CHECK([ovs-appctl dpctl/dump-flows | sed 's/.*core: [[0-9]]*//' | >> strip_xout_keep_actions], [0], [ >> +recirc_id(0),in_port(1),packet_type(ns=0,id=0),eth_type(0x0800),ipv4(dst= >> 10.1.0.2/255.255.0.0,frag=no), packets:0, bytes:0, used:never, >> actions:ct(commit) >> +recirc_id(0),in_port(1),packet_type(ns=0,id=0),eth_type(0x0800),ipv4(dst= >> 10.1.2.2/255.255.255.0,frag=no), packets:0, bytes:0, used:0.0s, >> actions:ct(commit) >> +]) >> + >> +dnl Hold the prefix 10.1.2.2/24 by another 10s. >> +AT_CHECK([ovs-appctl netdev-dummy/receive p1 >> 'ipv4(src=10.0.0.1,dst=10.1.2.2),tcp(src=1,dst=2)']) >> +dnl Send more 10.1.0.2 to make 10.1.0.0/16 tuple preprend 10.1.2.0/24 >> tuple in the pvector of subtables. >> +for i in `seq 0 256`;do >> +AT_CHECK([ovs-appctl netdev-dummy/receive p1 >> 'ipv4(src=10.0.0.1,dst=10.1.0.2),tcp(src=1,dst=2)']) >> +done >> + >> +AT_CHECK([echo 'table=0,in_port=p1,ip,nw_dst=10.1.0.0/16 actions=p2' | >> dnl >> +ovs-ofctl --bundle replace-flows br0 -]) >> + >> +AT_CHECK([ovs-appctl dpctl/dump-flows | sed 's/.*core: [[0-9]]*//' | >> strip_xout_keep_actions], [0], [ >> +recirc_id(0),in_port(1),packet_type(ns=0,id=0),eth_type(0x0800),ipv4(dst= >> 10.1.0.2/255.255.0.0,frag=no), packets:0, bytes:0, used:0.0s, actions:2 >> +recirc_id(0),in_port(1),packet_type(ns=0,id=0),eth_type(0x0800),ipv4(dst= >> 10.1.2.2/255.255.255.0,frag=no), packets:0, bytes:0, used:0.0s, actions:2 >> +]) >> + >> +OVS_VSWITCHD_STOP >> +AT_CLEANUP >> -- >> 2.25.1 >> >> > > -- > hepeng > -- hepeng _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
