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

[Ansis Atteka]

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?

>
>      tcmsg = tc_make_request(netdev, RTM_NEWTFILTER,
>                              NLM_F_EXCL | NLM_F_CREATE, &request);
> diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c
> index 9338c2e..0c32995 100644
> --- a/vswitchd/bridge.c
> +++ b/vswitchd/bridge.c
> @@ -4445,8 +4445,8 @@ iface_configure_qos(struct iface *iface, const struct ovsrec_qos *qos)
>      }
>
>      netdev_set_policing(iface->netdev,
> -                        iface->cfg->ingress_policing_rate,
> -                        iface->cfg->ingress_policing_burst);
> +                        MIN(UINT32_MAX, iface->cfg->ingress_policing_rate),
> +                        MIN(UINT32_MAX, iface->cfg->ingress_policing_burst));
>
>      ofpbuf_uninit(&queues_buf);
>  }
> --
> 1.7.10.4
>
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev