[ovs-dev] [PATCH] dpif-netdev: Use unmasked key when adding datapath flows.
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 userspace datapath.
Signed-off-by: Eelco Chaudron
---
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..daa00aa2f 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 'dst' 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
Re: [ovs-dev] [PATCH] dpif-netdev: Use unmasked key when adding datapath flows.
On 20 Oct 2022, at 9:43, Eelco Chaudron wrote:
> On 19 Oct 2022, at 11:31, Eelco Chaudron wrote:
>
>> On 19 Oct 2022, at 1:08, Ilya Maximets wrote:
>>
>>> On 10/19/22 00:07, Jan Scheurich wrote:
Hi guys,
I am afraid that commit is too long ago that would remember any details
that caused us to change the code in beb75a40fdc2 ("userspace: Switching
of L3 packets in L2 pipeline"). What I vaguely remember was that I
couldn’t comprehend the original code and it was not working correctly in
some of the cases we needed/tested. But perhaps the changes we introduced
also had corner cases we didn't consider.
>>
>> First of all, thanks Ilya, for following this up while I was asleep and
>> digging into who made the change :) I was trying to get the patch out
>> quickly for some feedback and forgot this step :(
>>
>> My change does not seem to cause any failures in the test cases you changed.
>> But it looks like you did not have any specific test cases for the problem
>> you were trying to solve.
>
> I took another look at the original code before Jan’s patch and replaced
> netdev_flow_key_init() with the original function,
> netdev_flow_key_from_flow(), from before commit beb75a40fdc2 I see two tunnel
> cases fail:
>
> 794: tunnel_push_pop - actionFAILED
> (tunnel-push-pop.at:660)
> 803: tunnel_push_pop_ipv6 - action FAILED
> (tunnel-push-pop-ipv6.at:528)
>
> This is due to the odd way of transforming the key to a packet and back to a
> flow, where my implementation uses the key as supplied to create the
> miniflow, which seems the right approach to me.
>
Question though:
>> 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.
That sounds strange to me. I always believed the datapath only supports
non-overlapping flows, i.e. a packet can match at most one datapath flow.
Only with that pre-requisite the dpcls classifier can work without
priorities. Have I been wrong in this? What would be the semantics of
adding a wider flow to the datapath? To my knowledge there is no guarantee
that the dpcls subtables are visited in any specific order that would
honor the mask width. And the first match will win.
>>>
>>> Yes, you're right that classifier is always choosing the first match,
>>> so it can not work correctly in general if there are multiple flows
>>> that match the same packet.
>>>
>>> What this change is trying to achieve, IIUC, is a bit of an odd case
>>> where we have multiple flows that match on the same packet, but one
>>> of them is a superset of another. And these flows should have identical
>>> actions, of course, if we want the system to still function correctly.
>>> (Should we actually compare actions to be identical and matches to
>>> be a superset? If actions are not correct, the flow must be soon
>>> revalidated, so that might not be a big concern. If the mask is not
>>> a superset, the flow_wildcards_has_extra() check should catch that
>>> and revalidate one of them, so should not be a problem either, I guess.
>>> But we need to check that flow mods are always using ufid generated
>>> from a full key and not updating actions on an incorrect flow. Same
>>> for flow deletions, we need to be sure that while removing flows we're
>>> actually finding and removing the one that we need.)
>>
>> Let me take another look at this delete code, as the mod code uses the same
>> path as add, so that should be taken care of.
>
> I did some experiments and reviewed the code, and we should be good with the
> delete part in both userspace and the kernel (with both the ufid and key
> delete methods).
>
>>> We should actually re-check the classier and the caches to make sure
>>> that there will not be any surprises and they are capable of dealing
>>> with possible duplicates...
>>
>> I did take a brief look at it and could not find any red flags, but if you
>> have something specific in mind please let me know and I recheck.
>>
>> Also as the current PMD datapath code already uses it in this way (see
>> below), we might have caught issues earlier if the exist.
>>
Please clarify this. And in which sense OVS relies on this behavior?
>>>
>>> The problem is a revalidation process. 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 dataplane
>>> 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 too narrow flow installed and now
Re: [ovs-dev] [PATCH] dpif-netdev: Use unmasked key when adding datapath flows.
On 10/18/22 18:42, Eelco Chaudron wrote:
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 userspace datapath.
Signed-off-by: Eelco Chaudron
---
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..daa00aa2f 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 'dst' as a copy of 'flow'. */
nit: s/dst/key/
+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
--
Adrián Moreno
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH] dpif-netdev: Use unmasked key when adding datapath flows.
On 25 Nov 2022, at 17:05, Adrian Moreno wrote:
> On 10/18/22 18:42, Eelco Chaudron wrote:
>> 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 userspace datapath.
>>
>> Signed-off-by: Eelco Chaudron
>> ---
>> 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..daa00aa2f 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 'dst' as a copy of 'flow'. */
>
> nit: s/dst/key/
Hi Adrian,
Thanks for the review. There was already a v2 with an updated commit message,
however, I just sent out a v3 with this change included.
//Eelco
>> +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"])
>
Re: [ovs-dev] [PATCH] dpif-netdev: Use unmasked key when adding datapath flows.
On 10/18/22 18:42, Eelco Chaudron wrote:
> 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 userspace datapath.
>
> Signed-off-by: Eelco Chaudron
> ---
> 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..daa00aa2f 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 'dst' 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);
Hi, Eelco. Thanks for the patch.
It is indeed an issue that we can lookup the incorrect flow
due to new mask being wider, but I'm wondering if that will
cause any other issues for L3 traffic as you seem to mainly
revert part of this previous commit:
beb75a40fdc2 ("userspace: Switching of L3 packets in L2 pipeline")
The commit message was tlking about something not being
properly masked...
CC: Jan, maybe you remember some details about that?
Also, the "Use the same method" doesn't make a lot of sense
with the change.
Best regards, Ilya Maximets.
>
> 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),ipv
Re: [ovs-dev] [PATCH] dpif-netdev: Use unmasked key when adding datapath flows.
Hi guys,
I am afraid that commit is too long ago that would remember any details that
caused us to change the code in beb75a40fdc2 ("userspace: Switching of L3
packets in L2 pipeline"). What I vaguely remember was that I couldn’t
comprehend the original code and it was not working correctly in some of the
cases we needed/tested. But perhaps the changes we introduced also had corner
cases we didn't consider.
Question though:
> > 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.
That sounds strange to me. I always believed the datapath only supports
non-overlapping flows, i.e. a packet can match at most one datapath flow. Only
with that pre-requisite the dpcls classifier can work without priorities. Have
I been wrong in this? What would be the semantics of adding a wider flow to the
datapath? To my knowledge there is no guarantee that the dpcls subtables are
visited in any specific order that would honor the mask width. And the first
match will win.
Please clarify this. And in which sense OVS relies on this behavior?
BR, Jan
> -Original Message-
> From: Ilya Maximets
> Sent: Tuesday, 18 October 2022 21:40
> To: Eelco Chaudron ; d...@openvswitch.org
> Cc: i.maxim...@ovn.org; Jan Scheurich
> Subject: Re: [ovs-dev] [PATCH] dpif-netdev: Use unmasked key when adding
> datapath flows.
>
> On 10/18/22 18:42, Eelco Chaudron wrote:
> > 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 userspace datapath.
> >
> > Signed-off-by: Eelco Chaudron
> > ---
> > 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..daa00aa2f 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 'dst' 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
Re: [ovs-dev] [PATCH] dpif-netdev: Use unmasked key when adding datapath flows.
On 10/19/22 00:07, Jan Scheurich wrote:
> Hi guys,
>
> I am afraid that commit is too long ago that would remember any details that
> caused us to change the code in beb75a40fdc2 ("userspace: Switching of L3
> packets in L2 pipeline"). What I vaguely remember was that I couldn’t
> comprehend the original code and it was not working correctly in some of the
> cases we needed/tested. But perhaps the changes we introduced also had corner
> cases we didn't consider.
>
> Question though:
>
>>> 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.
>
> That sounds strange to me. I always believed the datapath only supports
> non-overlapping flows, i.e. a packet can match at most one datapath flow.
> Only with that pre-requisite the dpcls classifier can work without
> priorities. Have I been wrong in this? What would be the semantics of adding
> a wider flow to the datapath? To my knowledge there is no guarantee that the
> dpcls subtables are visited in any specific order that would honor the mask
> width. And the first match will win.
Yes, you're right that classifier is always choosing the first match,
so it can not work correctly in general if there are multiple flows
that match the same packet.
What this change is trying to achieve, IIUC, is a bit of an odd case
where we have multiple flows that match on the same packet, but one
of them is a superset of another. And these flows should have identical
actions, of course, if we want the system to still function correctly.
(Should we actually compare actions to be identical and matches to
be a superset? If actions are not correct, the flow must be soon
revalidated, so that might not be a big concern. If the mask is not
a superset, the flow_wildcards_has_extra() check should catch that
and revalidate one of them, so should not be a problem either, I guess.
But we need to check that flow mods are always using ufid generated
from a full key and not updating actions on an incorrect flow. Same
for flow deletions, we need to be sure that while removing flows we're
actually finding and removing the one that we need.)
We should actually re-check the classier and the caches to make sure
that there will not be any surprises and they are capable of dealing
with possible duplicates...
> Please clarify this. And in which sense OVS relies on this behavior?
The problem is a revalidation process. 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 dataplane
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 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, 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.
This particular scenario is probably not an actual problem for the
userspace datapath, because upcalls are handled in a different code
path, so the wider flow should be installed just fine alongside with
the too narrow flow (We still need to check that all parts of the
code can handle this). So, it is only for a "slow" path here where
upper layers may decide to add a wider flow via dpif_flow_put(), but
will fail.
It's more of an issue for a kernel datapath where we seem to have
a similar code inside of a kernel.
In any case, if the "fast" upcall path is able to add such flows
(I didn't verify that), the "slow" dpif_flow_put() case should work
the same way to avoid confusion and potential more serious issues in
the future.
Best regards, Ilya Maximets.
>
> BR, Jan
>
>> -Original Message-
>> From: Ilya Maximets
>> Sent: Tuesday, 18 October 2022 21:40
>> To: Eelco Chaudron ; d...@openvswitch.org
>> Cc: i.maxim...@ovn.org; Jan Scheurich
>> Subject: Re: [ovs-dev] [PATCH] dpif-netdev: Use unmasked key when adding
>> datapath flows.
>>
>> On 10/18/22 18:42, Eelco Chaudron wrote:
>>> 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
Re: [ovs-dev] [PATCH] dpif-netdev: Use unmasked key when adding datapath flows.
gt; upper layers may decide to add a wider flow via dpif_flow_put(), but
> will fail.
Thats why I had the remark in the “the same way this is handled the userspace
datapath.” in the commit message.
So the dpif_netdev_flow_put() seems to be only used in the dummy datapath used
by all the test cases, but the actual dpif-netdev path seems to use the
handle_packet_upcall().
Where in handle_packet_upcall() it will do a dp_netdev_pmd_lookup_flow() using
the unmasked packet key (like my change below), and then does the
dp_netdev_flow_add() using the masked ukey.
Jan, I’m wondering why you only changed the dpif_netdev_flow_put() and not the
PMD path, as this is probably the path used in real life. Or is there some odd
kernel tunnel combination that is causing this path to be utilized? If so, do
you have a test case you can run on this change? The default dp make checks all
pass.
> It's more of an issue for a kernel datapath where we seem to have
> a similar code inside of a kernel.
Yes, I do have a kernel patch ready also for this. But I waited because I
wanted to get the userspace patch in/discussed first (before someone in the
kernel does a quick ack and commit ;)
> In any case, if the "fast" upcall path is able to add such flows
> (I didn't verify that), the "slow" dpif_flow_put() case should work
> the same way to avoid confusion and potential more serious issues in
> the future.
See above, I can confirm the fast path works similar to this change, i.e. using
the packet ukey.
> Best regards, Ilya Maximets.
>
>>
>> BR, Jan
>>
>>> -Original Message-
>>> From: Ilya Maximets
>>> Sent: Tuesday, 18 October 2022 21:40
>>> To: Eelco Chaudron ; d...@openvswitch.org
>>> Cc: i.maxim...@ovn.org; Jan Scheurich
>>> Subject: Re: [ovs-dev] [PATCH] dpif-netdev: Use unmasked key when adding
>>> datapath flows.
>>>
>>> On 10/18/22 18:42, Eelco Chaudron wrote:
>>>> 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 userspace datapath.
>>>>
>>>> Signed-off-by: Eelco Chaudron
>>>> ---
>>>> 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..daa00aa2f 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 'dst' 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);
>
Re: [ovs-dev] [PATCH] dpif-netdev: Use unmasked key when adding datapath flows.
On 19 Oct 2022, at 11:31, Eelco Chaudron wrote:
> On 19 Oct 2022, at 1:08, Ilya Maximets wrote:
>
>> On 10/19/22 00:07, Jan Scheurich wrote:
>>> Hi guys,
>>>
>>> I am afraid that commit is too long ago that would remember any details
>>> that caused us to change the code in beb75a40fdc2 ("userspace: Switching of
>>> L3 packets in L2 pipeline"). What I vaguely remember was that I couldn’t
>>> comprehend the original code and it was not working correctly in some of
>>> the cases we needed/tested. But perhaps the changes we introduced also had
>>> corner cases we didn't consider.
>
> First of all, thanks Ilya, for following this up while I was asleep and
> digging into who made the change :) I was trying to get the patch out quickly
> for some feedback and forgot this step :(
>
> My change does not seem to cause any failures in the test cases you changed.
> But it looks like you did not have any specific test cases for the problem
> you were trying to solve.
I took another look at the original code before Jan’s patch and replaced
netdev_flow_key_init() with the original function, netdev_flow_key_from_flow(),
from before commit beb75a40fdc2 I see two tunnel cases fail:
794: tunnel_push_pop - actionFAILED
(tunnel-push-pop.at:660)
803: tunnel_push_pop_ipv6 - action FAILED
(tunnel-push-pop-ipv6.at:528)
This is due to the odd way of transforming the key to a packet and back to a
flow, where my implementation uses the key as supplied to create the miniflow,
which seems the right approach to me.
>>> Question though:
>>>
> 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.
>>>
>>> That sounds strange to me. I always believed the datapath only supports
>>> non-overlapping flows, i.e. a packet can match at most one datapath flow.
>>> Only with that pre-requisite the dpcls classifier can work without
>>> priorities. Have I been wrong in this? What would be the semantics of
>>> adding a wider flow to the datapath? To my knowledge there is no guarantee
>>> that the dpcls subtables are visited in any specific order that would honor
>>> the mask width. And the first match will win.
>>
>> Yes, you're right that classifier is always choosing the first match,
>> so it can not work correctly in general if there are multiple flows
>> that match the same packet.
>>
>> What this change is trying to achieve, IIUC, is a bit of an odd case
>> where we have multiple flows that match on the same packet, but one
>> of them is a superset of another. And these flows should have identical
>> actions, of course, if we want the system to still function correctly.
>> (Should we actually compare actions to be identical and matches to
>> be a superset? If actions are not correct, the flow must be soon
>> revalidated, so that might not be a big concern. If the mask is not
>> a superset, the flow_wildcards_has_extra() check should catch that
>> and revalidate one of them, so should not be a problem either, I guess.
>> But we need to check that flow mods are always using ufid generated
>> from a full key and not updating actions on an incorrect flow. Same
>> for flow deletions, we need to be sure that while removing flows we're
>> actually finding and removing the one that we need.)
>
> Let me take another look at this delete code, as the mod code uses the same
> path as add, so that should be taken care of.
I did some experiments and reviewed the code, and we should be good with the
delete part in both userspace and the kernel (with both the ufid and key delete
methods).
>> We should actually re-check the classier and the caches to make sure
>> that there will not be any surprises and they are capable of dealing
>> with possible duplicates...
>
> I did take a brief look at it and could not find any red flags, but if you
> have something specific in mind please let me know and I recheck.
>
> Also as the current PMD datapath code already uses it in this way (see
> below), we might have caught issues earlier if the exist.
>
>>> Please clarify this. And in which sense OVS relies on this behavior?
>>
>> The problem is a revalidation process. 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 dataplane
>> 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 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, we will eventually get
>> an upcall on
