[ovs-dev] [cleanups 02/12] dpif-linux.c: Let the kernel pick a port number if one not requested.

Gurucharan Shetty shettyg at nicira.com
Wed Jan 16 00:01:01 UTC 2013


On Fri, Nov 16, 2012 at 12:02 AM, Justin Pettit <jpettit at nicira.com> wrote:
> With the single datapath change, we no longer depend on the kernel to
> make sure that we don't reuse OpenFlow port numbers, since the ofproto
> library now picks them.  Remove the code that contained that logic.
>
> Suggested-by: Ben Pfaff <blp at nicira.com>
> Signed-off-by: Justin Pettit <jpettit at nicira.com>
> ---
>  lib/dpif-linux.c |   47 +++++++++++++++++------------------------------
>  1 files changed, 17 insertions(+), 30 deletions(-)
>
> diff --git a/lib/dpif-linux.c b/lib/dpif-linux.c
> index 75bfc45..67ff805 100644
> --- a/lib/dpif-linux.c
> +++ b/lib/dpif-linux.c
> @@ -198,9 +198,6 @@ struct dpif_linux {
>      struct sset changed_ports;  /* Ports that have changed. */
>      struct nln_notifier *port_notifier;
>      bool change_error;
> -
> -    /* Port number allocation. */
> -    uint32_t alloc_port_no;
>  };
>
>  static struct vlog_rate_limit error_rl = VLOG_RATE_LIMIT_INIT(9999, 5);
> @@ -399,8 +396,9 @@ dpif_linux_port_add(struct dpif *dpif_, struct netdev *netdev,
>      const char *type = netdev_get_type(netdev);
>      struct dpif_linux_vport request, reply;
>      const struct ofpbuf *options;
> +    uint32_t upcall_pid;
>      struct ofpbuf *buf;
> -    int error, i = 0, max_ports = MAX_PORTS;
> +    int error;
>
>      dpif_linux_vport_init(&request);
>      request.cmd = OVS_VPORT_CMD_NEW;
> @@ -424,33 +422,22 @@ dpif_linux_port_add(struct dpif *dpif_, struct netdev *netdev,
>          netdev_linux_ethtool_set_flag(netdev, ETH_FLAG_LRO, "LRO", false);
>      }
>
> -    /* Unless a specific port was requested, loop until we find a port
> -     * that isn't used. */
> -    do {
> -        uint32_t upcall_pid;
> +    request.port_no = *port_nop;
> +    upcall_pid = dpif_linux_port_get_pid(dpif_, request.port_no);
> +    request.upcall_pid = &upcall_pid;
>
> -        request.port_no = *port_nop != UINT32_MAX ? *port_nop
> -                          : ++dpif->alloc_port_no;
> -        upcall_pid = dpif_linux_port_get_pid(dpif_, request.port_no);
> -        request.upcall_pid = &upcall_pid;
> -        error = dpif_linux_vport_transact(&request, &reply, &buf);
> +    error = dpif_linux_vport_transact(&request, &reply, &buf);
>
> -        if (!error) {
> -            *port_nop = reply.port_no;
> -            VLOG_DBG("%s: assigning port %"PRIu32" to netlink pid %"PRIu32,
> -                     dpif_name(dpif_), request.port_no, upcall_pid);
> -        } else if (error == EFBIG) {
> -            /* Older datapath has lower limit. */
> -            max_ports = dpif->alloc_port_no;
> -            dpif->alloc_port_no = 0;
> -        } else if (error == EBUSY && *port_nop != UINT32_MAX) {
> -            VLOG_INFO("%s: requested port %"PRIu32" is in use",
> -                     dpif_name(dpif_), *port_nop);
> -        }
> +    if (!error) {
> +        *port_nop = reply.port_no;
> +        VLOG_DBG("%s: assigning port %"PRIu32" to netlink pid %"PRIu32,
> +                 dpif_name(dpif_), request.port_no, upcall_pid);
> +    } else if (error == EBUSY && *port_nop != UINT32_MAX) {
> +        VLOG_INFO("%s: requested port %"PRIu32" is in use",
> +                 dpif_name(dpif_), *port_nop);
> +    }
If I run the following 3 commands, vswitchd segfaults.
ovs-vsctl add-br br1
ovs-vsctl add-port br1 gre1 -- set interface gre1 type=gre
options:remote_ip=192.168.1.1
ovs-vsctl add-port br1 gre2 -- set interface gre2 type=gre
options:remote_ip=192.168.1.1

This is because the third command will cause the kernel to return an
error but since *port_nop=UINT32_MAX, we don't return an error.

This causes the following piece of code in function add_channel to
overflow causing the segfault.
    if (port_no >= dpif->uc_array_size) {
        int new_size = port_no + 1;


I can get rid of the " && *port_nop != UINT32_MAX" part to fix this,
But it may be there for a reason.

Thanks,
Guru



>
> -        ofpbuf_delete(buf);
> -    } while ((*port_nop == UINT32_MAX) && (i++ < max_ports)
> -             && (error == EBUSY || error == EFBIG));
> +    ofpbuf_delete(buf);
>
>      return error;
>  }
> --
> 1.7.5.4
>
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev



More information about the dev mailing list