[ovs-dev] [PATCH] netdev-linux: Be more careful about integer overflow in policing.

Ben Pfaff blp at nicira.com
Tue Mar 17 19:36:19 UTC 2015


On Mon, Mar 16, 2015 at 10:40:26AM -0700, Ansis Atteka wrote:
> On Fri, Mar 13, 2015 at 11:31 AM, Ben Pfaff <blp at nicira.com> wrote:
> > On Tue, Mar 10, 2015 at 09:28:22PM -0700, Ansis Atteka wrote:
> >> On Tue, Mar 10, 2015 at 1:37 PM, Ben Pfaff <blp at nicira.com> wrote:
> >> > Otherwise the policing limits could make no sense if large rates were
> >> > specified.
> >> >
> >> > Reported-by: Zhangguanghui <zhang.guanghui at h3c.com>
> >> > Signed-off-by: Ben Pfaff <blp at nicira.com>
> >> > ---
> >> >  AUTHORS            |    1 +
> >> >  lib/netdev-linux.c |   13 +++++++------
> >> >  vswitchd/bridge.c  |    4 ++--
> >> >  3 files changed, 10 insertions(+), 8 deletions(-)
> >> >
> >> > diff --git a/AUTHORS b/AUTHORS
> >> > index 6e8b4da..00627ef 100644
> >> > --- a/AUTHORS
> >> > +++ b/AUTHORS
> >> > @@ -344,6 +344,7 @@ Voravit T.              voravit at kth.se
> >> >  Yeming Zhao             zhaoyeming at gmail.com
> >> >  Ying Chen               yingchen at vmware.com
> >> >  Yongqiang Liu           liuyq7809 at gmail.com
> >> > +Zhangguanghui           zhang.guanghui at h3c.com
> >> >  Ziyou Wang              ziyouw at vmware.com
> >> >  Zolt??n Balogh           zoltan.balogh at ericsson.com
> >> >  ankur dwivedi           ankurengg2003 at gmail.com
> >> > diff --git a/lib/netdev-linux.c b/lib/netdev-linux.c
> >> > index 920c884..199e6ac 100644
> >> > --- a/lib/netdev-linux.c
> >> > +++ b/lib/netdev-linux.c
> >> > @@ -1,5 +1,5 @@
> >> >  /*
> >> > - * Copyright (c) 2009, 2010, 2011, 2012, 2013, 2014 Nicira, Inc.
> >> > + * Copyright (c) 2009, 2010, 2011, 2012, 2013, 2014, 2015 Nicira, Inc.
> >> >   *
> >> >   * Licensed under the Apache License, Version 2.0 (the "License");
> >> >   * you may not use this file except in compliance with the License.
> >> > @@ -405,8 +405,8 @@ static struct tcmsg *tc_make_request(const struct netdev *, int type,
> >> >                                       unsigned int flags, struct ofpbuf *);
> >> >  static int tc_transact(struct ofpbuf *request, struct ofpbuf **replyp);
> >> >  static int tc_add_del_ingress_qdisc(struct netdev *netdev, bool add);
> >> > -static int tc_add_policer(struct netdev *netdev, int kbits_rate,
> >> > -                          int kbits_burst);
> >> > +static int tc_add_policer(struct netdev *,
> >> > +                          uint32_t kbits_rate, uint32_t kbits_burst);
> >> >
> >> >  static int tc_parse_qdisc(const struct ofpbuf *, const char **kind,
> >> >                            struct nlattr **options);
> >> > @@ -4525,7 +4525,8 @@ tc_add_del_ingress_qdisc(struct netdev *netdev, bool add)
> >> >   * Returns 0 if successful, otherwise a positive errno value.
> >> >   */
> >> >  static int
> >> > -tc_add_policer(struct netdev *netdev, int kbits_rate, int kbits_burst)
> >> > +tc_add_policer(struct netdev *netdev,
> >> > +               uint32_t kbits_rate, uint32_t kbits_burst)
> >> >  {
> >> >      struct tc_police tc_police;
> >> >      struct ofpbuf request;
> >> > @@ -4539,8 +4540,8 @@ tc_add_policer(struct netdev *netdev, int kbits_rate, int kbits_burst)
> >> >      tc_police.action = TC_POLICE_SHOT;
> >> >      tc_police.mtu = mtu;
> >> >      tc_fill_rate(&tc_police.rate, ((uint64_t) kbits_rate * 1000)/8, mtu);
> >> > -    tc_police.burst = tc_bytes_to_ticks(tc_police.rate.rate,
> >> > -                                        kbits_burst * 1024);
> >> > +    tc_police.burst = tc_bytes_to_ticks(
> >> > +        tc_police.rate.rate, MIN(UINT32_MAX / 1024, kbits_burst) * 1024);
> >>
> >> Maybe silly questions, since I actually did not try out the code, but
> >> 1. is it correct that above kbits_burst is multiplied with 1024, but
> >> in tc_fill_rate()  kbits_rate is multiplied with 1000?
> >> 2. also, tc_fill_rate() above seems to init tc_police.rate in bytes
> >> not bits, but tc_bytes_to_ticks() expects both arguments to be in same
> >> units to cancel out?
> >
> > It's definitely weird, but testing shows that it does the right thing
> > at least from "tc filter show"'s point of view.  How about if I add
> > this comment?
> >
> >     /* The following appears wrong in two ways:
> >      *
> >      * - tc_bytes_to_ticks() should take "bytes" as quantity for both of its
> >      *   arguments (or at least consistently "bytes" as both or "bits" as
> >      *   both), but this supplies bytes for the first argument and bits for the
> >      *   second.
> >      *
> >      * - In networking a kilobit is usually 1000 bits but this uses 1024 bits.
> >      *
> >      * However if you "fix" those problems then "tc filter show ..." shows
> >      * "125000b", meaning 125,000 bits, when OVS configures it for 1000 kbit ==
> >      * 1,000,000 bits, so while this actually ends up doing the right thing
> >      * from tc's point of view.  Whatever. */
> 
> 
> Acked-by: Ansis Atteka <aatteka at nicira.com>

Thanks, applied with that additional comment.



More information about the dev mailing list