[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 15:02:22 UTC 2020


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



More information about the dev mailing list