On 7/29/20 3:47 PM, Adrian Moreno wrote: > Verified that v3 also solves the issue reported. > > Tested-by: Adrián Moreno <amore...@redhat.com> > > On 7/29/20 12:37 PM, Eli Britstein wrote: >> Thanks! >> >> Acked-by: Eli Britstein <el...@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.maxim...@ovn.org> >>> ---
Thanks! Applied to master and backported down to 2.12. Bets regards, Ilya Maximets. _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev