On 8/9/23 05:02, Peng He wrote:
> 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 <hepeng.0...@bytedance.com>
> ---
> lib/dpif-netdev.c | 45 ++++++++++++++++++++++++++++++---------------
> tests/pmd.at | 47 +++++++++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 77 insertions(+), 15 deletions(-)
>
> diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
> index 70b953ae6..8479630b8 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,35 @@ 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) {
> + dp_netdev_flow_add(pmd, match, 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 (!put->ufid && !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 +4258,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..b03cd831e 100644
> --- a/tests/pmd.at
> +++ b/tests/pmd.at
> @@ -1300,3 +1300,50 @@ 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 bridge br0 datapath-type=dummy \
> + -- set interface p1 type=dummy-pmd \
> + -- 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)'])
> +
> +OVS_WAIT_UNTIL_EQUAL([ovs-appctl dpctl/dump-flows | sed 's/.*core:
> [[0-9]]*//'], [
> +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])
For some reason this check keeps failing on FreeBSD. Investigating...
Best regards, Ilya Maximets.
> +
> +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)'])
> +OVS_WAIT_UNTIL_EQUAL([ovs-appctl dpctl/dump-flows | sed 's/.*core:
> [[0-9]]*//' | strip_xout_keep_actions], [
> +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 revalidator/wait])
> +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
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
