[ovs-dev] [PATCH v3] odp-util: Fix clearing match mask if set action is partially unnecessary.

[Ilya Maximets]

On 7/29/20 3:47 PM, Adrian Moreno wrote:
> Verified that v3 also solves the issue reported.
> 
> Tested-by: Adrián Moreno <amorenoz at redhat.com>
> 
> On 7/29/20 12:37 PM, Eli Britstein wrote:
>> Thanks!
>>
>> Acked-by: Eli Britstein <elibr at mellanox.com>
>>
>> On 7/29/2020 12:13 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 were 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)
>>>
>>> Sending 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 must be dropped.
>>>
>>> Fix that by updating the match mask with only the new bits set by
>>> commit(), but keeping those that were cleared (OR operation).
>>>
>>> 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%3D1854376&data=02%7C01%7Celibr%40mellanox.com%7C7d70b87d6084436b6ff608d8339fb47b%7Ca652971c7d2e4d9ba6a4d149256f461b%7C0%7C0%7C637316108313661481&sdata=pa3tutGAU3%2BRuRy8y2rjb7AdBouIZrbX4iAE87YXrTY%3D&reserved=0
>>>
>>> Signed-off-by: Ilya Maximets <i.maximets at ovn.org>
>>> ---

Thanks!

Applied to master and backported down to 2.12.

Bets regards, Ilya Maximets.