[ovs-dev] [PATCH ovn] ovn-northd: Fix router policy pkt mark over flow if the value is greater than signed int.

Dumitru Ceara dceara at redhat.com
Fri Sep 18 12:43:07 UTC 2020


On 9/17/20 5:49 PM, numans at ovn.org wrote:
> From: Numan Siddique <numans at ovn.org>
> 
> If the value of pkt_mark in the router policy options is greater than 2147483647, we
> are ignoring it. This is because we use smap_get_int(). This patch fixes this issue
> by using str_to_uint().
> 
> Fixes: a123ef0fb8fd("Support packet metadata marking for logical router policies.")
> Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=1878248
> Reported-by: Alexander Constantinescu <aconstan at redhat.com>
> Signed-off-by: Numan Siddique <numans at ovn.org>
> ---
>  northd/ovn-northd.c | 16 ++++++++++++++--
>  tests/ovn.at        | 37 +++++++++++++++++++++++++++++++------
>  2 files changed, 45 insertions(+), 8 deletions(-)
> 
> diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c
> index cfec6a2c86..df2e39450c 100644
> --- a/northd/ovn-northd.c
> +++ b/northd/ovn-northd.c
> @@ -7523,6 +7523,18 @@ get_outport_for_routing_policy_nexthop(struct ovn_datapath *od,
>      return NULL;
>  }
>  
> +static uint32_t
> +get_lr_policy_pkt_mark(const struct nbrec_logical_router_policy *rule)
> +{
> +    const char *str_pkt_mark = smap_get(&rule->options, "pkt_mark");
> +    uint32_t pkt_mark = 0;
> +    if (str_pkt_mark) {
> +        str_to_uint(str_pkt_mark, 10, &pkt_mark);
> +    }
> +
> +    return pkt_mark;
> +}
> +
>  static void
>  build_routing_policy_flow(struct hmap *lflows, struct ovn_datapath *od,
>                            struct hmap *ports,
> @@ -7547,7 +7559,7 @@ build_routing_policy_flow(struct hmap *lflows, struct ovn_datapath *od,
>                           rule->priority, rule->nexthop);
>              return;
>          }
> -        uint32_t pkt_mark = smap_get_int(&rule->options, "pkt_mark", 0);
> +        uint32_t pkt_mark = get_lr_policy_pkt_mark(rule);

Hi Numan,

I think it should be enough to just:

smap_get_ullong(&rule->options, "pkt_mark", 0)

>          if (pkt_mark) {
>              ds_put_format(&actions, "pkt.mark = %u; ", pkt_mark);
>          }
> @@ -7568,7 +7580,7 @@ build_routing_policy_flow(struct hmap *lflows, struct ovn_datapath *od,
>      } else if (!strcmp(rule->action, "drop")) {
>          ds_put_cstr(&actions, "drop;");
>      } else if (!strcmp(rule->action, "allow")) {
> -        uint32_t pkt_mark = smap_get_int(&rule->options, "pkt_mark", 0);
> +        uint32_t pkt_mark = get_lr_policy_pkt_mark(rule);

Same here.

Thanks,
Dumitru

>          if (pkt_mark) {
>              ds_put_format(&actions, "pkt.mark = %u; ", pkt_mark);
>          }
> diff --git a/tests/ovn.at b/tests/ovn.at
> index a6f1fb58f8..753b9079fb 100644
> --- a/tests/ovn.at
> +++ b/tests/ovn.at
> @@ -20589,7 +20589,7 @@ pol4=$(ovn-nbctl --bare --columns _uuid find logical_router_policy priority=2001
>  pol5=$(ovn-nbctl --bare --columns _uuid find logical_router_policy priority=1001)
>  
>  ovn-nbctl set logical_router_policy $pol4 options:pkt_mark=4
> -ovn-nbctl set logical_router_policy $pol5 options:pkt_mark=5
> +ovn-nbctl set logical_router_policy $pol5 options:pkt_mark=4294967295
>  ovn-nbctl --wait=hv sync
>  
>  OVS_WAIT_UNTIL([
> @@ -20607,9 +20607,11 @@ OVS_WAIT_UNTIL([
>      grep "load:0x4->NXM_NX_PKT_MARK" -c)
>  ])
>  
> +as hv1 ovs-ofctl dump-flows br-int table=20 | grep NXM_NX_PKT_MARK
> +
>  OVS_WAIT_UNTIL([
>      test 1 -eq $(as hv1 ovs-ofctl dump-flows br-int table=20 | \
> -    grep "load:0x5->NXM_NX_PKT_MARK" -c)
> +    grep "load:0xffffffff->NXM_NX_PKT_MARK" -c)
>  ])
>  
>  AT_CHECK([as hv1 ovs-ofctl del-flows br-phys])
> @@ -20620,7 +20622,7 @@ table=0, priority=100, pkt_mark=0x64 actions=drop
>  table=0, priority=100, pkt_mark=0x2 actions=drop
>  table=0, priority=100, pkt_mark=0x3 actions=drop
>  table=0, priority=100, pkt_mark=0x4 actions=drop
> -table=0, priority=100, pkt_mark=0x5 actions=drop
> +table=0, priority=100, pkt_mark=0xffffffff actions=drop
>  ])
>  
>  AT_CHECK([as hv1 ovs-ofctl --protocols=OpenFlow13 add-flows br-phys flows.txt])
> @@ -20690,7 +20692,7 @@ AT_CHECK([
>  
>  AT_CHECK([
>      test 1 -eq $(as hv1 ovs-ofctl dump-flows br-phys table=0 | \
> -    grep "priority=100,pkt_mark=0x5" | \
> +    grep "priority=100,pkt_mark=0xffffffff" | \
>      grep "n_packets=0" -c)
>  ])
>  
> @@ -20745,7 +20747,7 @@ send_icmp6_packet hv1 hv1-vif2 505400000004 00000000ff01 ${src_ip6} ${dst_ip6}
>  
>  OVS_WAIT_UNTIL([
>      test 1 -eq $(as hv1 ovs-ofctl dump-flows br-phys table=0 | \
> -    grep "priority=100,pkt_mark=0x5" | \
> +    grep "priority=100,pkt_mark=0xffffffff" | \
>      grep "n_packets=1" -c)
>  ])
>  
> @@ -20771,10 +20773,33 @@ OVS_WAIT_UNTIL([
>  
>  AT_CHECK([
>      test 1 -eq $(as hv1 ovs-ofctl dump-flows br-phys table=0 | \
> -    grep "priority=100,pkt_mark=0x5" | \
> +    grep "priority=100,pkt_mark=0xffffffff" | \
>      grep "n_packets=1" -c)
>  ])
>  
> +AT_CHECK([ovn-sbctl lflow-list | grep -E "lr_in_policy.*priority=1001" | sort], [0], [dnl
> +  table=12(lr_in_policy       ), priority=1001 , dnl
> +match=(ip6), action=(pkt.mark = 4294967295; next;)
> +])
> +
> +ovn-nbctl --wait=hv set logical_router_policy $pol5 options:pkt_mark=-1
> +AT_CHECK([ovn-sbctl lflow-list | grep -E "lr_in_policy.*priority=1001" | sort], [0], [dnl
> +  table=12(lr_in_policy       ), priority=1001 , dnl
> +match=(ip6), action=(next;)
> +])
> +
> +ovn-nbctl --wait=hv set logical_router_policy $pol5 options:pkt_mark=2147483648
> +AT_CHECK([ovn-sbctl lflow-list | grep -E "lr_in_policy.*priority=1001" | sort], [0], [dnl
> +  table=12(lr_in_policy       ), priority=1001 , dnl
> +match=(ip6), action=(pkt.mark = 2147483648; next;)
> +])
> +
> +ovn-nbctl --wait=hv set logical_router_policy $pol5 options:pkt_mark=foo
> +AT_CHECK([ovn-sbctl lflow-list | grep -E "lr_in_policy.*priority=1001" | sort], [0], [dnl
> +  table=12(lr_in_policy       ), priority=1001 , dnl
> +match=(ip6), action=(next;)
> +])
> +
>  OVN_CLEANUP([hv1])
>  AT_CLEANUP
>  
> 



More information about the dev mailing list