Re: [ovs-dev] [PATCH v2] odp-util: Fix clearing match mask if set action is partially unnecessary.
On 7/29/20 6:55 AM, Eli Britstein wrote:
>
> On 7/28/2020 7:32 PM, Ilya Maximets wrote:
>> On 7/28/20 4:16 PM, Eli Britstein wrote:
>>> it is not "wildcarded". "wildcarded" means it had a match and it was
>>> removed. the case here it was only not "expanded".
>>>
It actually had a match on a filed and that match was removed from
the original flow while committing set() action. That is the bug that
this patch intended to fix. See the example below. There was a match
on source MAC, but it was removed by commit_set_ether_action().
>>> Right. My mistake before.
>> match key and will never be matched, i.e. flows with any destination
>>
>> I'm a bit warried about this solution since we're not clearing out all
>> the
>> masks, so memory sanitizers might bite us on that in case where will be
>> some
>> holes in the datastructures even though we're ORing them, but not
>> actually
>> using these uninitialized bytes. To not use the unconditional OR we will
>> need to introduce new functions like 'or_ethernet_mask()' and this will
>> grow
>> the code size, which doesn't look very pretty.
>>
>> What do you think?
>>> That was my thought too when I did that work. For that, I introduced the
>>> generic 'struct offsetof_sizeof' array, and wildcarding the mask code is
>>> mutual for all attributes (ETH, IPv4,...). Maybe (I haven't thought it
>>> through) we can leverage the generic "commit" function that already gets
>>> that array to do the "ORs". This way we will do only for the fields and not
>>> tough the "holes".
>> Yes, that a good point. What about incremental change like this
>> (incremental to the previous diff I sent):
>>
>> ---
>> diff --git a/lib/odp-util.c b/lib/odp-util.c
>> index 364a6fbe1..e54a78b43 100644
>> --- a/lib/odp-util.c
>> +++ b/lib/odp-util.c
>> @@ -7701,6 +7701,28 @@ struct offsetof_sizeof {
>> int size;
>> };
>> +
>> +/* Performs bitwise OR over the fields in 'dst_' and 'src_' specified in
>> + * 'offsetof_sizeof_arr' array. Result is stored in 'dst_'. */
>> +static void
>> +or_masks(void *dst_, const void *src_,
>> + struct offsetof_sizeof *offsetof_sizeof_arr)
>> +{
>> + int field, size, offset;
>> + const uint8_t *src = src_;
>> + uint8_t *dst = dst_;
>> +
>> + for (field = 0; ; field++) {
>> + size = offsetof_sizeof_arr[field].size;
>> + offset = offsetof_sizeof_arr[field].offset;
>> +
>> + if (!size) {
>> + return;
>> + }
>> + or_bytes(dst + offset, src + offset, size);
>> + }
>> +}
>> +
>> /* Compares each of the fields in 'key0' and 'key1'. The fields are
>> specified
>> * in 'offsetof_sizeof_arr', which is an array terminated by a 0-size
>> field.
>> * Returns true if all of the fields are equal, false if at least one
>> differs.
>> @@ -7796,7 +7818,7 @@ commit_set_ether_action(const struct flow *flow,
>> struct flow *base_flow,
>> , , , sizeof key,
>> ovs_key_ethernet_offsetof_sizeof_arr, odp_actions)) {
>> put_ethernet_key(, base_flow);
>> - or_bytes(, _mask, sizeof mask);
>> + or_masks(, _mask, ovs_key_ethernet_offsetof_sizeof_arr);
>> put_ethernet_key(, >masks);
>> }
>> }
>> ---
>>
>> And the same for all other callers of or_bytes().
>>
>> What do you think?
> Seems OK. Can you please post v3?
Sure. Sent.
>>
>> Best regards, Ilya Maximets.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v2] odp-util: Fix clearing match mask if set action is partially unnecessary.
On 7/28/2020 7:32 PM, Ilya Maximets wrote:
On 7/28/20 4:16 PM, Eli Britstein wrote:
it is not "wildcarded". "wildcarded" means it had a match and it was removed. the case
here it was only not "expanded".
It actually had a match on a filed and that match was removed from
the original flow while committing set() action. That is the bug that
this patch intended to fix. See the example below. There was a match
on source MAC, but it was removed by commit_set_ether_action().
Right. My mistake before.
match key and will never be matched, i.e. flows with any destination
I'm a bit warried about this solution since we're not clearing out all the
masks, so memory sanitizers might bite us on that in case where will be some
holes in the datastructures even though we're ORing them, but not actually
using these uninitialized bytes. To not use the unconditional OR we will
need to introduce new functions like 'or_ethernet_mask()' and this will grow
the code size, which doesn't look very pretty.
What do you think?
That was my thought too when I did that work. For that, I introduced the generic 'struct offsetof_sizeof'
array, and wildcarding the mask code is mutual for all attributes (ETH, IPv4,...). Maybe (I haven't thought
it through) we can leverage the generic "commit" function that already gets that array to do the
"ORs". This way we will do only for the fields and not tough the "holes".
Yes, that a good point. What about incremental change like this
(incremental to the previous diff I sent):
---
diff --git a/lib/odp-util.c b/lib/odp-util.c
index 364a6fbe1..e54a78b43 100644
--- a/lib/odp-util.c
+++ b/lib/odp-util.c
@@ -7701,6 +7701,28 @@ struct offsetof_sizeof {
int size;
};
+
+/* Performs bitwise OR over the fields in 'dst_' and 'src_' specified in
+ * 'offsetof_sizeof_arr' array. Result is stored in 'dst_'. */
+static void
+or_masks(void *dst_, const void *src_,
+ struct offsetof_sizeof *offsetof_sizeof_arr)
+{
+int field, size, offset;
+const uint8_t *src = src_;
+uint8_t *dst = dst_;
+
+for (field = 0; ; field++) {
+size = offsetof_sizeof_arr[field].size;
+offset = offsetof_sizeof_arr[field].offset;
+
+if (!size) {
+return;
+}
+or_bytes(dst + offset, src + offset, size);
+}
+}
+
/* Compares each of the fields in 'key0' and 'key1'. The fields are specified
* in 'offsetof_sizeof_arr', which is an array terminated by a 0-size field.
* Returns true if all of the fields are equal, false if at least one differs.
@@ -7796,7 +7818,7 @@ commit_set_ether_action(const struct flow *flow, struct
flow *base_flow,
, , , sizeof key,
ovs_key_ethernet_offsetof_sizeof_arr, odp_actions)) {
put_ethernet_key(, base_flow);
-or_bytes(, _mask, sizeof mask);
+or_masks(, _mask, ovs_key_ethernet_offsetof_sizeof_arr);
put_ethernet_key(, >masks);
}
}
---
And the same for all other callers of or_bytes().
What do you think?
Seems OK. Can you please post v3?
Best regards, Ilya Maximets.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v2] odp-util: Fix clearing match mask if set action is partially unnecessary.
On 7/28/20 4:16 PM, Eli Britstein wrote:
>
> it is not "wildcarded". "wildcarded" means it had a match and it was removed.
> the case here it was only not "expanded".
>
>> It actually had a match on a filed and that match was removed from
>> the original flow while committing set() action. That is the bug that
>> this patch intended to fix. See the example below. There was a match
>> on source MAC, but it was removed by commit_set_ether_action().
> Right. My mistake before.
>>
match key and will never be matched, i.e. flows with any destination
I'm a bit warried about this solution since we're not clearing out all the
masks, so memory sanitizers might bite us on that in case where will be
some
holes in the datastructures even though we're ORing them, but not actually
using these uninitialized bytes. To not use the unconditional OR we will
need to introduce new functions like 'or_ethernet_mask()' and this will
grow
the code size, which doesn't look very pretty.
What do you think?
> That was my thought too when I did that work. For that, I introduced the
> generic 'struct offsetof_sizeof' array, and wildcarding the mask code is
> mutual for all attributes (ETH, IPv4,...). Maybe (I haven't thought it
> through) we can leverage the generic "commit" function that already gets that
> array to do the "ORs". This way we will do only for the fields and not tough
> the "holes".
Yes, that a good point. What about incremental change like this
(incremental to the previous diff I sent):
---
diff --git a/lib/odp-util.c b/lib/odp-util.c
index 364a6fbe1..e54a78b43 100644
--- a/lib/odp-util.c
+++ b/lib/odp-util.c
@@ -7701,6 +7701,28 @@ struct offsetof_sizeof {
int size;
};
+
+/* Performs bitwise OR over the fields in 'dst_' and 'src_' specified in
+ * 'offsetof_sizeof_arr' array. Result is stored in 'dst_'. */
+static void
+or_masks(void *dst_, const void *src_,
+ struct offsetof_sizeof *offsetof_sizeof_arr)
+{
+int field, size, offset;
+const uint8_t *src = src_;
+uint8_t *dst = dst_;
+
+for (field = 0; ; field++) {
+size = offsetof_sizeof_arr[field].size;
+offset = offsetof_sizeof_arr[field].offset;
+
+if (!size) {
+return;
+}
+or_bytes(dst + offset, src + offset, size);
+}
+}
+
/* Compares each of the fields in 'key0' and 'key1'. The fields are specified
* in 'offsetof_sizeof_arr', which is an array terminated by a 0-size field.
* Returns true if all of the fields are equal, false if at least one differs.
@@ -7796,7 +7818,7 @@ commit_set_ether_action(const struct flow *flow, struct
flow *base_flow,
, , , sizeof key,
ovs_key_ethernet_offsetof_sizeof_arr, odp_actions)) {
put_ethernet_key(, base_flow);
-or_bytes(, _mask, sizeof mask);
+or_masks(, _mask, ovs_key_ethernet_offsetof_sizeof_arr);
put_ethernet_key(, >masks);
}
}
---
And the same for all other callers of or_bytes().
What do you think?
Best regards, Ilya Maximets.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v2] odp-util: Fix clearing match mask if set action is partially unnecessary.
it is not "wildcarded". "wildcarded" means it had a match and it was removed. the case here it was only not "expanded". It actually had a match on a filed and that match was removed from the original flow while committing set() action. That is the bug that this patch intended to fix. See the example below. There was a match on source MAC, but it was removed by commit_set_ether_action(). Right. My mistake before. match key and will never be matched, i.e. flows with any destination I'm a bit warried about this solution since we're not clearing out all the masks, so memory sanitizers might bite us on that in case where will be some holes in the datastructures even though we're ORing them, but not actually using these uninitialized bytes. To not use the unconditional OR we will need to introduce new functions like 'or_ethernet_mask()' and this will grow the code size, which doesn't look very pretty. What do you think? That was my thought too when I did that work. For that, I introduced the generic 'struct offsetof_sizeof' array, and wildcarding the mask code is mutual for all attributes (ETH, IPv4,...). Maybe (I haven't thought it through) we can leverage the generic "commit" function that already gets that array to do the "ORs". This way we will do only for the fields and not tough the "holes". Best regards, Ilya Maximets. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v2] odp-util: Fix clearing match mask if set action is partially unnecessary.
On 7/28/20 12:58 PM, Eli Britstein wrote:
>
> On 7/27/2020 9:58 PM, Ilya Maximets wrote:
>> While committing set() actions, commit() could wildcard all the fields
>> that are same in match key and in the set action. This leads to
>> situation where mask after commit could actually contain less bits
>> than it was before. And if set action was partially committed, all
>> the fields that was the same will be cleared out from the matching key
> typo 'was' -> 'were'
OK.
>> resulting in the incorrect (too wide) flow.
>>
>> For example, for the flow that matches on both src and dst mac
>> addresses, if the dst mac is the same and only src should be changed
>> by the set() action, destination address will be wildcarded in the
> it is not "wildcarded". "wildcarded" means it had a match and it was removed.
> the case here it was only not "expanded".
It actually had a match on a filed and that match was removed from
the original flow while committing set() action. That is the bug that
this patch intended to fix. See the example below. There was a match
on source MAC, but it was removed by commit_set_ether_action().
>> match key and will never be matched, i.e. flows with any destination
>> mac will match, which is not correct.
>>
>> Setting OF rule:
>>
>> in_port=1,dl_src=50:54:00:00:00:09
>> actions=mod_dl_dst(50:54:00:00:00:0a),output(2)
>>
>> Sendidng following packets on port 1:
>>
>> 1. eth(src=50:54:00:00:00:09,dst=50:54:00:00:00:0a),eth_type(0x0800)
>> 2. eth(src=50:54:00:00:00:09,dst=50:54:00:00:00:0c),eth_type(0x0800)
>> 3. eth(src=50:54:00:00:00:0b,dst=50:54:00:00:00:0c),eth_type(0x0800)
>>
>> Resulted datapath flows:
>> <...> eth(dst=50:54:00:00:00:0c),<...>,
>> actions:set(eth(dst=50:54:00:00:00:0a)),2
>> <...> eth(src=50:54:00:00:00:09,dst=50:54:00:00:00:0a),<...>, actions:2
>>
>> The first flow doesn't have any match on source MAC address and the
>> third packet successfully matched on it while it mast be dropped.
> typo 'mast' -> 'must'.
OK.
>>
>> Fix that by updating matching mask only if mask was expanded, but
>> not in other cases.
>>
>> With fix applied, resulted correct flows are:
>> eth(src=50:54:00:00:00:09,dst=50:54:00:00:00:0a),<...>, actions:2
>> eth(src=50:54:00:00:00:09,dst=50:54:00:00:00:0c),<...>,
>>
>> actions:set(eth(dst=50:54:00:00:00:0a)),2
>> eth(src=50:54:00:00:00:0b),<...>, actions:drop
>>
>>
>> The code before commit dbf4a92800d0 was not able to reduce the mask,
>> it was only possible to expand it to exact match, so it was OK to
>> update original matching mask with the new value in all cases.
>>
>> Fixes: dbf4a92800d0 ("odp-util: Do not rewrite fields with the same values
>> as matched")
>> Reported-at:
>> https://eur03.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla.redhat.com%2Fshow_bug.cgi%3Fid%3D1854376data=02%7C01%7Celibr%40mellanox.com%7C3714762423634df2a85808d8325f1e73%7Ca652971c7d2e4d9ba6a4d149256f461b%7C0%7C0%7C637314731402645471sdata=elvKx9rFPu8Z0b%2FVS1Rdtdq1e8B%2BJr%2FN%2BhcngcrVSxA%3Dreserved=0
>> Tested-by: Adrián Moreno
>> Signed-off-by: Ilya Maximets
>> ---
>>
>> Version 2:
>> - Added simple unit test for this issue.
>> - Added 'Tested-by' tag.
>> - Refined comments.
>>
>> lib/odp-util.c | 82 ---
>> tests/ofproto-dpif.at | 23
>> 2 files changed, 84 insertions(+), 21 deletions(-)
>>
>> diff --git a/lib/odp-util.c b/lib/odp-util.c
>> index 011db9ebb..4c001c75b 100644
>> --- a/lib/odp-util.c
>> +++ b/lib/odp-util.c
>> @@ -7736,8 +7736,9 @@ static bool
>> commit(enum ovs_key_attr attr, bool use_masked_set,
>> const void *key, void *base, void *mask, size_t size,
>> struct offsetof_sizeof *offsetof_sizeof_arr,
>> - struct ofpbuf *odp_actions)
>> + struct ofpbuf *odp_actions, bool *mask_expanded)
> maybe we can change the return value of "commit" to enum of 3 values instead
> of 2 bools (one in the return value and another in "mask_expanded")?
Adding a new argument just because it's the simplest and less intrusive
solution. If we want to fix the issue in a completely right way we need
to have a bitwise OR between the old mask and the mew one after the
commit(). This way we will not need to have an extra argument for
commit() function. Something like this:
--
diff --git a/lib/odp-util.c b/lib/odp-util.c
index 011db9ebb..364a6fbe1 100644
--- a/lib/odp-util.c
+++ b/lib/odp-util.c
@@ -7779,9 +7779,10 @@ commit_set_ether_action(const struct flow *flow, struct
flow *base_flow,
struct flow_wildcards *wc,
bool use_masked)
{
-struct ovs_key_ethernet key, base, mask;
+struct ovs_key_ethernet key, base, mask, orig_mask;
struct offsetof_sizeof ovs_key_ethernet_offsetof_sizeof_arr[] =
OVS_KEY_ETHERNET_OFFSETOF_SIZEOF_ARR;
+
if (flow->packet_type != htonl(PT_ETH)) {
return;
}
@@
Re: [ovs-dev] [PATCH v2] odp-util: Fix clearing match mask if set action is partially unnecessary.
On 7/27/2020 9:58 PM, Ilya Maximets wrote:
While committing set() actions, commit() could wildcard all the fields
that are same in match key and in the set action. This leads to
situation where mask after commit could actually contain less bits
than it was before. And if set action was partially committed, all
the fields that was the same will be cleared out from the matching key
typo 'was' -> 'were'
resulting in the incorrect (too wide) flow.
For example, for the flow that matches on both src and dst mac
addresses, if the dst mac is the same and only src should be changed
by the set() action, destination address will be wildcarded in the
it is not "wildcarded". "wildcarded" means it had a match and it was
removed. the case here it was only not "expanded".
match key and will never be matched, i.e. flows with any destination
mac will match, which is not correct.
Setting OF rule:
in_port=1,dl_src=50:54:00:00:00:09
actions=mod_dl_dst(50:54:00:00:00:0a),output(2)
Sendidng following packets on port 1:
1. eth(src=50:54:00:00:00:09,dst=50:54:00:00:00:0a),eth_type(0x0800)
2. eth(src=50:54:00:00:00:09,dst=50:54:00:00:00:0c),eth_type(0x0800)
3. eth(src=50:54:00:00:00:0b,dst=50:54:00:00:00:0c),eth_type(0x0800)
Resulted datapath flows:
<...> eth(dst=50:54:00:00:00:0c),<...>,
actions:set(eth(dst=50:54:00:00:00:0a)),2
<...> eth(src=50:54:00:00:00:09,dst=50:54:00:00:00:0a),<...>, actions:2
The first flow doesn't have any match on source MAC address and the
third packet successfully matched on it while it mast be dropped.
typo 'mast' -> 'must'.
Fix that by updating matching mask only if mask was expanded, but
not in other cases.
With fix applied, resulted correct flows are:
eth(src=50:54:00:00:00:09,dst=50:54:00:00:00:0a),<...>, actions:2
eth(src=50:54:00:00:00:09,dst=50:54:00:00:00:0c),<...>,
actions:set(eth(dst=50:54:00:00:00:0a)),2
eth(src=50:54:00:00:00:0b),<...>, actions:drop
The code before commit dbf4a92800d0 was not able to reduce the mask,
it was only possible to expand it to exact match, so it was OK to
update original matching mask with the new value in all cases.
Fixes: dbf4a92800d0 ("odp-util: Do not rewrite fields with the same values as
matched")
Reported-at:
https://eur03.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla.redhat.com%2Fshow_bug.cgi%3Fid%3D1854376data=02%7C01%7Celibr%40mellanox.com%7C3714762423634df2a85808d8325f1e73%7Ca652971c7d2e4d9ba6a4d149256f461b%7C0%7C0%7C637314731402645471sdata=elvKx9rFPu8Z0b%2FVS1Rdtdq1e8B%2BJr%2FN%2BhcngcrVSxA%3Dreserved=0
Tested-by: Adrián Moreno
Signed-off-by: Ilya Maximets
---
Version 2:
- Added simple unit test for this issue.
- Added 'Tested-by' tag.
- Refined comments.
lib/odp-util.c| 82 ---
tests/ofproto-dpif.at | 23
2 files changed, 84 insertions(+), 21 deletions(-)
diff --git a/lib/odp-util.c b/lib/odp-util.c
index 011db9ebb..4c001c75b 100644
--- a/lib/odp-util.c
+++ b/lib/odp-util.c
@@ -7736,8 +7736,9 @@ static bool
commit(enum ovs_key_attr attr, bool use_masked_set,
const void *key, void *base, void *mask, size_t size,
struct offsetof_sizeof *offsetof_sizeof_arr,
- struct ofpbuf *odp_actions)
+ struct ofpbuf *odp_actions, bool *mask_expanded)
maybe we can change the return value of "commit" to enum of 3 values
instead of 2 bools (one in the return value and another in "mask_expanded")?
{
+*mask_expanded = false;
if (keycmp_mask(key, base, offsetof_sizeof_arr, mask)) {
bool fully_masked = odp_mask_is_exact(attr, mask, size);
@@ -7746,6 +7747,7 @@ commit(enum ovs_key_attr attr, bool use_masked_set,
} else {
if (!fully_masked) {
memset(mask, 0xff, size);
+*mask_expanded = true;
}
commit_set_action(odp_actions, attr, key, size);
}
@@ -7782,6 +7784,8 @@ commit_set_ether_action(const struct flow *flow, struct
flow *base_flow,
struct ovs_key_ethernet key, base, mask;
struct offsetof_sizeof ovs_key_ethernet_offsetof_sizeof_arr[] =
OVS_KEY_ETHERNET_OFFSETOF_SIZEOF_ARR;
+bool expanded;
+
if (flow->packet_type != htonl(PT_ETH)) {
return;
}
@@ -7792,9 +7796,12 @@ commit_set_ether_action(const struct flow *flow, struct
flow *base_flow,
if (commit(OVS_KEY_ATTR_ETHERNET, use_masked,
, , , sizeof key,
- ovs_key_ethernet_offsetof_sizeof_arr, odp_actions)) {
+ ovs_key_ethernet_offsetof_sizeof_arr, odp_actions, )) {
put_ethernet_key(, base_flow);
-put_ethernet_key(, >masks);
+if (expanded) {
+/* Mask expanded by commit(), need to match new fields. */
+put_ethernet_key(, >masks);
+}
}
}
@@ -7920,6 +7927,7 @@ commit_set_ipv4_action(const struct flow *flow, struct flow
[ovs-dev] [PATCH v2] odp-util: Fix clearing match mask if set action is partially unnecessary.
While committing set() actions, commit() could wildcard all the fields
that are same in match key and in the set action. This leads to
situation where mask after commit could actually contain less bits
than it was before. And if set action was partially committed, all
the fields that was the same will be cleared out from the matching key
resulting in the incorrect (too wide) flow.
For example, for the flow that matches on both src and dst mac
addresses, if the dst mac is the same and only src should be changed
by the set() action, destination address will be wildcarded in the
match key and will never be matched, i.e. flows with any destination
mac will match, which is not correct.
Setting OF rule:
in_port=1,dl_src=50:54:00:00:00:09
actions=mod_dl_dst(50:54:00:00:00:0a),output(2)
Sendidng following packets on port 1:
1. eth(src=50:54:00:00:00:09,dst=50:54:00:00:00:0a),eth_type(0x0800)
2. eth(src=50:54:00:00:00:09,dst=50:54:00:00:00:0c),eth_type(0x0800)
3. eth(src=50:54:00:00:00:0b,dst=50:54:00:00:00:0c),eth_type(0x0800)
Resulted datapath flows:
<...> eth(dst=50:54:00:00:00:0c),<...>,
actions:set(eth(dst=50:54:00:00:00:0a)),2
<...> eth(src=50:54:00:00:00:09,dst=50:54:00:00:00:0a),<...>, actions:2
The first flow doesn't have any match on source MAC address and the
third packet successfully matched on it while it mast be dropped.
Fix that by updating matching mask only if mask was expanded, but
not in other cases.
With fix applied, resulted correct flows are:
eth(src=50:54:00:00:00:09,dst=50:54:00:00:00:0a),<...>, actions:2
eth(src=50:54:00:00:00:09,dst=50:54:00:00:00:0c),<...>,
actions:set(eth(dst=50:54:00:00:00:0a)),2
eth(src=50:54:00:00:00:0b),<...>, actions:drop
The code before commit dbf4a92800d0 was not able to reduce the mask,
it was only possible to expand it to exact match, so it was OK to
update original matching mask with the new value in all cases.
Fixes: dbf4a92800d0 ("odp-util: Do not rewrite fields with the same values as
matched")
Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=1854376
Tested-by: Adrián Moreno
Signed-off-by: Ilya Maximets
---
Version 2:
- Added simple unit test for this issue.
- Added 'Tested-by' tag.
- Refined comments.
lib/odp-util.c| 82 ---
tests/ofproto-dpif.at | 23
2 files changed, 84 insertions(+), 21 deletions(-)
diff --git a/lib/odp-util.c b/lib/odp-util.c
index 011db9ebb..4c001c75b 100644
--- a/lib/odp-util.c
+++ b/lib/odp-util.c
@@ -7736,8 +7736,9 @@ static bool
commit(enum ovs_key_attr attr, bool use_masked_set,
const void *key, void *base, void *mask, size_t size,
struct offsetof_sizeof *offsetof_sizeof_arr,
- struct ofpbuf *odp_actions)
+ struct ofpbuf *odp_actions, bool *mask_expanded)
{
+*mask_expanded = false;
if (keycmp_mask(key, base, offsetof_sizeof_arr, mask)) {
bool fully_masked = odp_mask_is_exact(attr, mask, size);
@@ -7746,6 +7747,7 @@ commit(enum ovs_key_attr attr, bool use_masked_set,
} else {
if (!fully_masked) {
memset(mask, 0xff, size);
+*mask_expanded = true;
}
commit_set_action(odp_actions, attr, key, size);
}
@@ -7782,6 +7784,8 @@ commit_set_ether_action(const struct flow *flow, struct
flow *base_flow,
struct ovs_key_ethernet key, base, mask;
struct offsetof_sizeof ovs_key_ethernet_offsetof_sizeof_arr[] =
OVS_KEY_ETHERNET_OFFSETOF_SIZEOF_ARR;
+bool expanded;
+
if (flow->packet_type != htonl(PT_ETH)) {
return;
}
@@ -7792,9 +7796,12 @@ commit_set_ether_action(const struct flow *flow, struct
flow *base_flow,
if (commit(OVS_KEY_ATTR_ETHERNET, use_masked,
, , , sizeof key,
- ovs_key_ethernet_offsetof_sizeof_arr, odp_actions)) {
+ ovs_key_ethernet_offsetof_sizeof_arr, odp_actions, )) {
put_ethernet_key(, base_flow);
-put_ethernet_key(, >masks);
+if (expanded) {
+/* Mask expanded by commit(), need to match new fields. */
+put_ethernet_key(, >masks);
+}
}
}
@@ -7920,6 +7927,7 @@ commit_set_ipv4_action(const struct flow *flow, struct
flow *base_flow,
struct ovs_key_ipv4 key, mask, base;
struct offsetof_sizeof ovs_key_ipv4_offsetof_sizeof_arr[] =
OVS_KEY_IPV4_OFFSETOF_SIZEOF_ARR;
+bool expanded;
/* Check that nw_proto and nw_frag remain unchanged. */
ovs_assert(flow->nw_proto == base_flow->nw_proto &&
@@ -7937,9 +7945,10 @@ commit_set_ipv4_action(const struct flow *flow, struct
flow *base_flow,
}
if (commit(OVS_KEY_ATTR_IPV4, use_masked, , , , sizeof key,
- ovs_key_ipv4_offsetof_sizeof_arr, odp_actions)) {
+ ovs_key_ipv4_offsetof_sizeof_arr, odp_actions, )) {
put_ipv4_key(, base_flow, false);
-if (mask.ipv4_proto !=
