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

[Numan Siddique]

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

> On 9/18/20 3:55 PM, Numan Siddique wrote:
> >
> >
> > On Fri, Sep 18, 2020 at 6:13 PM Dumitru Ceara <dceara at redhat.com
> > <mailto:dceara at redhat.com>> wrote:
> >
> >     On 9/17/20 5:49 PM, numans at ovn.org <mailto:numans at ovn.org> wrote:
> >     > From: Numan Siddique <numans at ovn.org <mailto: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
> >     <mailto:aconstan at redhat.com>>
> >     > Signed-off-by: Numan Siddique <numans at ovn.org <mailto:
> numans at ovn.org>>
> >     > ---
> >     >  northd/ovn-northd.c | 16 ++++++++++++++--
> >     >  tests/ovn.at <http://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.
> >
>
> You're right.
>
> It seems to me like we're missing an API in smap.h to access uint
> fields. I think we should add one there.
>
>
Thanks Dumitru. You're right. I submitted the patch for ovs to add a helper
function.
Meanwhile to fix this issue in OVN, I have added a wrapper function in ovn
util and I will
submit another patch to remove this function once the ovs patch is merged.

Thanks
Numan


> Thanks,
> Dumitru
>
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>