Hi, Eelco,
Did this patch only fix the dummy-datapath?
since in the real world scenario, revalidator will only modify or delete
ukey, it will not add a megaflow into the datapath.
If we have a wider new key and narrowed old key, the reavlidator will
modify the action of the old key, and upcall_cb
will install the wider new key (without masking the key with the wider
mask, so in fact, we do not have the problem
you described here?)
Eelco Chaudron <echau...@redhat.com> 于2022年11月28日周一 16:53写道：
> The datapath supports installing wider flows, and OVS relies on
> this behavior. For example if ipv4(src=1.1.1.1/192.0.0.0,
> dst=1.1.1.2/192.0.0.0) exists, a wider flow (smaller mask) of
> ipv4(src=192.1.1.1/128.0.0.0,dst=192.1.1.2/128.0.0.0) is allowed
> to be added.
>
> However, if we try to add a wildcard rule, the installation fails:
>
> # ovs-appctl dpctl/add-flow system@myDP "in_port(1),eth_type(0x0800), \
> ipv4(src=1.1.1.1/192.0.0.0,dst=1.1.1.2/192.0.0.0,frag=no)" 2
> # ovs-appctl dpctl/add-flow system@myDP "in_port(1),eth_type(0x0800), \
> ipv4(src=192.1.1.1/0.0.0.0,dst=49.1.1.2/0.0.0.0,frag=no)" 2
> ovs-vswitchd: updating flow table (File exists)
>
> The reason is that the key used to determine if the flow is already
> present in the system uses the original key ANDed with the mask.
> This results in the IP address not being part of the (miniflow) key,
> i.e., being substituted with an all-zero value. When doing the actual
> lookup, this results in the key wrongfully matching the first flow,
> and therefore the flow does not get installed. The solution is to use
> the unmasked key for the existence check, the same way this is handled
> in the "slow" dpif_flow_put() case.
>
> OVS relies on the fact that overlapping flows can exist if one is a
> superset of the other. Note that this is only true when the same set
> of actions is applied. This is due to how the revalidator process
> works. During revalidation, OVS removes too generic flows from the
> datapath to avoid incorrect matches but allows too narrow flows to
> stay in the datapath to avoid the data plane disruption and also to
> avoid constant flow deletions if the datapath ignores wildcards on
> certain fields/bits. See flow_wildcards_has_extra() check in the
> revalidate_ukey__() function.
>
> The problem here is that we have a too narrow flow installed, and now
> OpenFlow rules got changed, so the actual flow should be more generic.
> Revalidators will not remove the narrow flow, and we will eventually get
> an upcall on the packet that doesn't match the narrow flow, but we will
> not be able to install a more generic flow because after masking with
> the new wider mask, the key matches on the narrow flow, so we get EEXIST.
>
> Fixes: beb75a40fdc2 ("userspace: Switching of L3 packets in L2 pipeline")
> Signed-off-by: Eelco Chaudron <echau...@redhat.com>
>
> ---
> v3:
> - Fixed comment text.
> v2:
> - Updated commit message and added fixes tag.
>
> lib/dpif-netdev.c | 33 +++++++++++++++++++++++++++++----
> tests/dpif-netdev.at | 14 ++++++++++++++
> 2 files changed, 43 insertions(+), 4 deletions(-)
>
> diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
> index a45b46014..be2041ce2 100644
> --- a/lib/dpif-netdev.c
> +++ b/lib/dpif-netdev.c
> @@ -3321,6 +3321,28 @@ netdev_flow_key_init_masked(struct netdev_flow_key
> *dst,
> (dst_u64 - miniflow_get_values(&dst->mf)) *
> 8);
> }
>
> +/* Initializes 'key' as a copy of 'flow'. */
> +static inline void
> +netdev_flow_key_init(struct netdev_flow_key *key,
> + const struct flow *flow)
> +{
> + uint64_t *dst = miniflow_values(&key->mf);
> + uint32_t hash = 0;
> + uint64_t value;
> +
> + miniflow_map_init(&key->mf, flow);
> + miniflow_init(&key->mf, flow);
> +
> + size_t n = dst - miniflow_get_values(&key->mf);
> +
> + FLOW_FOR_EACH_IN_MAPS (value, flow, key->mf.map) {
> + hash = hash_add64(hash, value);
> + }
> +
> + key->hash = hash_finish(hash, n * 8);
> + key->len = netdev_flow_key_size(n);
> +}
> +
> static inline void
> emc_change_entry(struct emc_entry *ce, struct dp_netdev_flow *flow,
> const struct netdev_flow_key *key)
> @@ -4195,7 +4217,7 @@ static int
> dpif_netdev_flow_put(struct dpif *dpif, const struct dpif_flow_put *put)
> {
> struct dp_netdev *dp = get_dp_netdev(dpif);
> - struct netdev_flow_key key, mask;
> + struct netdev_flow_key key;
> struct dp_netdev_pmd_thread *pmd;
> struct match match;
> ovs_u128 ufid;
> @@ -4244,9 +4266,12 @@ dpif_netdev_flow_put(struct dpif *dpif, const
> struct dpif_flow_put *put)
>
> /* Must produce a netdev_flow_key for lookup.
> * Use the same method as employed to create the key when adding
> - * the flow to the dplcs to make sure they match. */
> - netdev_flow_mask_init(&mask, &match);
> - netdev_flow_key_init_masked(&key, &match.flow, &mask);
> + * the flow to the dplcs to make sure they match.
> + * We need to put in the unmasked key as flow_put_on_pmd() will first
> try
> + * to see if an entry exists doing a packet type lookup. As masked-out
> + * fields are interpreted as zeros, they could falsely match a wider
> IP
> + * address mask. Installation of the flow will use the match
> variable. */
> + netdev_flow_key_init(&key, &match.flow);
>
> if (put->pmd_id == PMD_ID_NULL) {
> if (cmap_count(&dp->poll_threads) == 0) {
> diff --git a/tests/dpif-netdev.at b/tests/dpif-netdev.at
> index 3179e1645..32054c52e 100644
> --- a/tests/dpif-netdev.at
> +++ b/tests/dpif-netdev.at
> @@ -636,6 +636,20 @@ OVS_VSWITCHD_STOP(["/flow: in_port is not an exact
> match/d
> /failed to put/d"])
> AT_CLEANUP
>
> +AT_SETUP([dpif-netdev - check dpctl/add-flow wider ip match])
> +OVS_VSWITCHD_START(
> + [add-port br0 p1 \
> + -- set interface p1 type=dummy
> options:pstream=punix:$OVS_RUNDIR/p0.sock \
> + -- set bridge br0 datapath-type=dummy])
> +
> +AT_CHECK([ovs-appctl revalidator/pause])
> +AT_CHECK([ovs-appctl dpctl/add-flow "in_port(1),eth_type(0x0800),ipv4(src=
> 0.0.0.0/192.0.0.0,dst=0.0.0.0/192.0.0.0,frag=no)" "3"])
> +AT_CHECK([ovs-appctl dpctl/add-flow "in_port(1),eth_type(0x0800),ipv4(src=
> 192.1.1.1/0.0.0.0,dst=49.1.1.1/0.0.0.0,frag=no)" "3"])
> +AT_CHECK([ovs-appctl revalidator/resume])
> +
> +OVS_VSWITCHD_STOP
> +AT_CLEANUP
> +
> # SEND_UDP_PKTS([p_name], [p_ofport])
> #
> # Sends 128 packets to port 'p_name' with different UDP destination ports.
>
> _______________________________________________
> dev mailing list
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
--
hepeng
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
