From 1f82d15cb88333520c367365cf76a7b97bd18e16 Mon Sep 17 00:00:00 2001 From: Numan Siddique Date: Sat, 14 Sep 2019 13:19:17 +0530 Subject: [PATCH] ovn: Exclude inport and outport symbol tables from conjunction If there are multiple ACLs associated with a port group and they match on a range of some field, then ovn-controller doesn't install the flows properly and this results in broken ACL functionality. For example, if there is a port group - pg1 with logical ports - [p1, p2] and if there are below ACLs (only match condition is shown) 1 - outport == @pg1 && ip4 && tcp.dst >= 500 && tcp.dst <= 501 2 - outport == @pg1 && ip4 && tcp.dst >= 600 && tcp.dst <= 601 The first ACL will result in the below OF flows 1. conj_id=1,tcp 2. tcp,reg15=0x11: conjunction(1, 1/2) 3. tcp,reg15=0x12: conjunction(1, 1/2) 5. tcp,tp_dst=500: conjunction(1, 2/2) 6. tcp,tp_dst=501: conjunction(1, 2/2) The second ACL will result in the below OF flows 7. conj_id=2,tcp 8. tcp,reg15=0x11: conjunction(2, 1/2) 9. tcp,reg15=0x12: conjunction(2, 1/2) 11. tcp,tp_dst=600: conjunction(2, 2/2) 12. tcp,tp_dst=601: conjunction(2, 3/2) The OF flows (2) and (8) have the exact match but with different action. This results in only one of the flows getting installed. The same goes for the flows (3) and (9). And this completely breaks the ACL functionality for such scenarios. In order to fix this issue, this patch excludes the 'inport' and 'outport' symbols from conjunction. With this patch we will have the below flows. tcp,reg15=0x11,tp_dst=500 tcp,reg15=0x11,tp_dst=501 tcp,reg15=0x12,tp_dst=500 tcp,reg15=0x12,tp_dst=501 tcp,reg15=0x13,tp_dst=500 tcp,reg15=0x13,tp_dst=501 tcp,reg15=0x11,tp_dst=600 tcp,reg15=0x11,tp_dst=601 tcp,reg15=0x12,tp_dst=600 tcp,reg15=0x12,tp_dst=601 tcp,reg15=0x13,tp_dst=600 tcp,reg15=0x13,tp_dst=601 Acked-by: Mark Michelson Acked-by: Daniel Alvarez Signed-off-by: Numan Siddique (cherry-picked from ovn commit 298701dbc99645700be41680a43d049cb061847a) Signed-off-by: Ben Pfaff --- ovn/lib/expr.c | 2 +- tests/ovn.at | 26 ++++++++++++++++++++++++++ 2 files changed, 27 insertions(+), 1 deletion(-) diff --git a/ovn/lib/expr.c b/ovn/lib/expr.c index e4c650f7c2a..c0871e1e8de 100644 --- a/ovn/lib/expr.c +++ b/ovn/lib/expr.c @@ -1499,7 +1499,7 @@ expr_symtab_add_string(struct shash *symtab, const char *name, const struct mf_field *field = mf_from_id(id); struct expr_symbol *symbol; - symbol = add_symbol(symtab, name, 0, prereqs, EXPR_L_NOMINAL, false, + symbol = add_symbol(symtab, name, 0, prereqs, EXPR_L_NOMINAL, true, field->writable); symbol->field = field; return symbol; diff --git a/tests/ovn.at b/tests/ovn.at index 2361524ff42..54aa19bb293 100644 --- a/tests/ovn.at +++ b/tests/ovn.at @@ -573,6 +573,24 @@ ip,reg14=0x6 ipv6,reg14=0x5 ipv6,reg14=0x6 ]) +AT_CHECK([expr_to_flow 'inport == {"eth0", "eth1", "eth2"} && ip4 && tcp && tcp.dst == {500, 501}'], [0], [dnl +tcp,reg14=0x5,tp_dst=500 +tcp,reg14=0x5,tp_dst=501 +tcp,reg14=0x6,tp_dst=500 +tcp,reg14=0x6,tp_dst=501 +]) +AT_CHECK([expr_to_flow 'outport == {"eth0", "eth1", "eth2"} && ip4 && tcp && tcp.src == {400, 401} && tcp.dst == {500, 501}'], [0], [dnl +conj_id=1,tcp,reg15=0x5 +conj_id=2,tcp,reg15=0x6 +tcp,reg15=0x5,tp_dst=500: conjunction(1, 0/2) +tcp,reg15=0x5,tp_dst=501: conjunction(1, 0/2) +tcp,reg15=0x5,tp_src=400: conjunction(1, 1/2) +tcp,reg15=0x5,tp_src=401: conjunction(1, 1/2) +tcp,reg15=0x6,tp_dst=500: conjunction(2, 0/2) +tcp,reg15=0x6,tp_dst=501: conjunction(2, 0/2) +tcp,reg15=0x6,tp_src=400: conjunction(2, 1/2) +tcp,reg15=0x6,tp_src=401: conjunction(2, 1/2) +]) AT_CHECK([expr_to_flow 'inport == "eth0" && inport == "eth1"'], [0], [dnl (no flows) ]) @@ -677,6 +695,14 @@ reg15=0x11 reg15=0x12 reg15=0x13 ]) +AT_CHECK([expr_to_flow 'outport == @pg1 && ip4.src == {10.0.0.4, 10.0.0.5}'], [0], [dnl +ip,reg15=0x11,nw_src=10.0.0.4 +ip,reg15=0x11,nw_src=10.0.0.5 +ip,reg15=0x12,nw_src=10.0.0.4 +ip,reg15=0x12,nw_src=10.0.0.5 +ip,reg15=0x13,nw_src=10.0.0.4 +ip,reg15=0x13,nw_src=10.0.0.5 +]) AT_CHECK([expr_to_flow 'outport == {@pg_empty}'], [0], [dnl (no flows) ])