[ovs-dev] [PATCH ovn] Exclude inport and outport symbol tables from conjunction

[Mark Michelson]

Acked-by: Mark Michelson

It sucks that we lose the efficiency of the conjunctive match altogether 
on port groups because of this error, but I understand this is a huge 
bug and needs fixing.

Perhaps it would be good to start up a discussion on this list about a 
more longterm solution that would allow for conjunctive matches with no 
ambiguity.

On 9/13/19 4:49 PM, nusiddiq at redhat.com wrote:
> From: Numan Siddique <nusiddiq at redhat.com>
> 
> 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
> 
> Signed-off-by: Numan Siddique <nusiddiq at redhat.com>
> ---
>   lib/expr.c   |  2 +-
>   tests/ovn.at | 26 ++++++++++++++++++++++++++
>   2 files changed, 27 insertions(+), 1 deletion(-)
> 
> diff --git a/lib/expr.c b/lib/expr.c
> index e4c650f7c..c0871e1e8 100644
> --- a/lib/expr.c
> +++ b/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 2a35b4e15..14d9f59b0 100644
> --- a/tests/ovn.at
> +++ b/tests/ovn.at
> @@ -589,6 +589,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)
>   ])
> @@ -693,6 +711,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)
>   ])
>