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

Numan Siddique numans at ovn.org
Fri Sep 18 13:55:23 UTC 2020


On Fri, Sep 18, 2020 at 6:13 PM Dumitru Ceara <dceara at redhat.com> wrote:

> 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)
>

Hi Dumitru,

This won't work. Since  smap_get_ullong() returns unsigned long long. And
the packet marker
field is 32-bit.

If we use smap_get_ullong() a user can set a value greater than 42949672965
and we will still accept it
and that won't work.

Thanks
Numan



> >          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
> >
> >
>
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
>


More information about the dev mailing list