From 20ba93a7c5f9e1b1c1e988a9910ca7d0534d4774 Mon Sep 17 00:00:00 2001 From: Giuseppe Lettieri Date: Thu, 15 Aug 2013 23:08:40 +0200 Subject: [PATCH] thread safety for netdev_pltap and netdev_tunnel --- lib/netdev-pltap.c | 125 +++++++++++++++++++++++++++++++++++--------- lib/netdev-tunnel.c | 100 +++++++++++++++++++++++++++++------ 2 files changed, 186 insertions(+), 39 deletions(-) diff --git a/lib/netdev-pltap.c b/lib/netdev-pltap.c index 9d93b9e96..c8c79896a 100644 --- a/lib/netdev-pltap.c +++ b/lib/netdev-pltap.c @@ -45,8 +45,21 @@ VLOG_DEFINE_THIS_MODULE(netdev_pltap); +/* Protects 'sync_list'. */ +static struct ovs_mutex sync_list_mutex = OVS_MUTEX_INITIALIZER; + +static struct list sync_list OVS_GUARDED_BY(sync_list_mutex) + = LIST_INITIALIZER(&sync_list); + struct netdev_pltap { struct netdev up; + + /* In sync_list. */ + struct list sync_list OVS_GUARDED_BY(sync_list_mutex); + + /* Protects all members below. */ + struct ovs_mutex mutex OVS_ACQ_AFTER(sync_list_mutex); + char *real_name; struct netdev_stats stats; enum netdev_flags new_flags; @@ -57,11 +70,9 @@ struct netdev_pltap { bool valid_local_ip; bool valid_local_netmask; bool sync_flags_needed; - struct list sync_list; unsigned int change_seq; }; -static struct list sync_list; struct netdev_rx_pltap { struct netdev_rx up; @@ -70,15 +81,21 @@ struct netdev_rx_pltap { static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 20); -static struct shash pltap_netdevs = SHASH_INITIALIZER(&pltap_netdevs); +/* Protects 'pltap_netdevs' */ +static struct ovs_mutex pltap_netdevs_mutex = OVS_MUTEX_INITIALIZER; +static struct shash pltap_netdevs OVS_GUARDED_BY(pltap_netdevs_mutex) + = SHASH_INITIALIZER(&pltap_netdevs); static int netdev_pltap_construct(struct netdev *netdev_); -static void netdev_pltap_update_seq(struct netdev_pltap *); -static int get_flags(struct netdev_pltap *dev, enum netdev_flags *flags); +static void netdev_pltap_update_seq(struct netdev_pltap *) + OVS_REQUIRES(dev->mutex); +static int get_flags(struct netdev_pltap *dev, enum netdev_flags *flags) + OVS_REQUIRES(dev->mutex); static bool netdev_pltap_finalized(struct netdev_pltap *dev) + OVS_REQUIRES(dev->mutex) { return dev->valid_local_ip && dev->valid_local_netmask; } @@ -104,16 +121,17 @@ netdev_rx_pltap_cast(const struct netdev_rx *rx) } static void sync_needed(struct netdev_pltap *dev) + OVS_REQUIRES(dev->mutex, sync_list_mutex) { if (dev->sync_flags_needed) return; dev->sync_flags_needed = true; list_insert(&sync_list, &dev->sync_list); - } static void sync_done(struct netdev_pltap *dev) + OVS_REQUIRES(dev->mutex, sync_list_mutex) { if (!dev->sync_flags_needed) return; @@ -135,6 +153,7 @@ netdev_pltap_construct(struct netdev *netdev_) struct netdev_pltap *netdev = netdev_pltap_cast(netdev_); int error; + ovs_mutex_init(&netdev->mutex, PTHREAD_MUTEX_NORMAL); netdev->real_name = xzalloc(IFNAMSIZ + 1); memset(&netdev->local_addr, 0, sizeof(netdev->local_addr)); netdev->valid_local_ip = false; @@ -142,7 +161,6 @@ netdev_pltap_construct(struct netdev *netdev_) netdev->flags = 0; netdev->sync_flags_needed = false; netdev->change_seq = 1; - list_init(&netdev->sync_list); /* Open tap device. */ @@ -161,7 +179,9 @@ netdev_pltap_construct(struct netdev *netdev_) return error; } + ovs_mutex_lock(&pltap_netdevs_mutex); shash_add(&pltap_netdevs, netdev_get_name(netdev_), netdev); + ovs_mutex_unlock(&pltap_netdevs_mutex); return 0; } @@ -170,13 +190,20 @@ netdev_pltap_destruct(struct netdev *netdev_) { struct netdev_pltap *netdev = netdev_pltap_cast(netdev_); + ovs_mutex_lock(&pltap_netdevs_mutex); if (netdev->fd != -1) close(netdev->fd); - sync_done(netdev); + if (netdev->sync_flags_needed) { + ovs_mutex_lock(&sync_list_mutex); + (void) list_remove(&netdev->sync_list); + ovs_mutex_unlock(&sync_list_mutex); + } shash_find_and_delete(&pltap_netdevs, netdev_get_name(netdev_)); + ovs_mutex_unlock(&pltap_netdevs_mutex); + ovs_mutex_destroy(&netdev->mutex); } static void @@ -186,7 +213,7 @@ netdev_pltap_dealloc(struct netdev *netdev_) free(netdev); } -static int netdev_pltap_up(struct netdev_pltap *dev); +static int netdev_pltap_up(struct netdev_pltap *dev) OVS_REQUIRES(dev->mutex); static struct netdev_rx * netdev_pltap_rx_alloc(void) @@ -202,16 +229,19 @@ netdev_pltap_rx_construct(struct netdev_rx *rx_) struct netdev *netdev_ = rx->up.netdev; struct netdev_pltap *netdev = netdev_pltap_cast(netdev_); - int error; + int error = 0; + ovs_mutex_lock(&netdev->mutex); rx->fd = netdev->fd; if (!netdev_pltap_finalized(netdev)) - return 0; + goto out; error = netdev_pltap_up(netdev); if (error) { - return error; + goto out; } - return 0; +out: + ovs_mutex_unlock(&netdev->mutex); + return error; } static void @@ -351,6 +381,7 @@ cleanup: static int netdev_pltap_up(struct netdev_pltap *dev) + OVS_REQUIRES(dev->mutex) { if (!netdev_pltap_finalized(dev)) { return 0; @@ -364,6 +395,7 @@ netdev_pltap_up(struct netdev_pltap *dev) static int netdev_pltap_down(struct netdev_pltap *dev) + OVS_REQUIRES(dev->mutex) { if (!netdev_pltap_finalized(dev)) { return 0; @@ -374,6 +406,7 @@ netdev_pltap_down(struct netdev_pltap *dev) static int netdev_pltap_promisc(struct netdev_pltap *dev, bool promisc) + OVS_REQUIRES(dev-mutex) { if (!netdev_pltap_finalized(dev)) { return 0; @@ -386,11 +419,13 @@ netdev_pltap_promisc(struct netdev_pltap *dev, bool promisc) static void netdev_pltap_sync_flags(struct netdev_pltap *dev) + OVS_REQUIRES(sync_list_mutex) { + ovs_mutex_lock(&dev->mutex); + if (dev->fd < 0 || !netdev_pltap_finalized(dev)) { - sync_done(dev); - return; + goto out; } VLOG_DBG("sync_flags(%s): current: %s %s target: %s %s", @@ -411,7 +446,10 @@ netdev_pltap_sync_flags(struct netdev_pltap *dev) } netdev_pltap_update_seq(dev); + +out: sync_done(dev); + ovs_mutex_unlock(&dev->mutex); } @@ -420,12 +458,14 @@ netdev_pltap_get_config(const struct netdev *dev_, struct smap *args) { struct netdev_pltap *netdev = netdev_pltap_cast(dev_); + ovs_mutex_lock(&netdev->mutex); if (netdev->valid_local_ip) smap_add_format(args, "local_ip", IP_FMT, IP_ARGS(netdev->local_addr.sin_addr.s_addr)); if (netdev->valid_local_netmask) smap_add_format(args, "local_netmask", "%"PRIu32, ntohs(netdev->local_netmask)); + ovs_mutex_unlock(&netdev->mutex); return 0; } @@ -435,6 +475,8 @@ netdev_pltap_set_config(struct netdev *dev_, const struct smap *args) struct netdev_pltap *netdev = netdev_pltap_cast(dev_); struct shash_node *node; + ovs_mutex_lock(&sync_list_mutex); + ovs_mutex_lock(&netdev->mutex); VLOG_DBG("pltap_set_config(%s)", netdev_get_name(dev_)); SMAP_FOR_EACH(node, args) { VLOG_DBG("arg: %s->%s", node->name, (char*)node->data); @@ -459,6 +501,8 @@ netdev_pltap_set_config(struct netdev *dev_, const struct smap *args) netdev->new_flags |= NETDEV_UP; sync_needed(netdev); } + ovs_mutex_unlock(&netdev->mutex); + ovs_mutex_unlock(&sync_list_mutex); return 0; } @@ -574,6 +618,7 @@ netdev_pltap_set_etheraddr(struct netdev *netdevi OVS_UNUSED, // XXX from netdev-linux.c static int get_etheraddr(struct netdev_pltap *dev, uint8_t ea[ETH_ADDR_LEN]) + OVS_REQUIRES(dev->mutex) { struct ifreq ifr; int hwaddr_family; @@ -584,7 +629,7 @@ get_etheraddr(struct netdev_pltap *dev, uint8_t ea[ETH_ADDR_LEN]) error = af_inet_ifreq_ioctl(dev->real_name, &ifr, SIOCGIFHWADDR, "SIOCGIFHWADDR"); if (error) { - return errno; + return error; } hwaddr_family = ifr.ifr_hwaddr.sa_family; if (hwaddr_family != AF_UNSPEC && hwaddr_family != ARPHRD_ETHER) { @@ -597,6 +642,7 @@ get_etheraddr(struct netdev_pltap *dev, uint8_t ea[ETH_ADDR_LEN]) static int get_flags(struct netdev_pltap *dev, enum netdev_flags *flags) + OVS_REQUIRES(dev->mutex) { struct ifreq ifr; int error; @@ -620,9 +666,18 @@ netdev_pltap_get_etheraddr(const struct netdev *netdev, { struct netdev_pltap *dev = netdev_pltap_cast(netdev); - if (dev->fd < 0) - return EAGAIN; - return get_etheraddr(dev, mac); + int error = 0; + + ovs_mutex_lock(&dev->mutex); + if (dev->fd < 0) { + error = EAGAIN; + goto out; + } + error = get_etheraddr(dev, mac); + +out: + ovs_mutex_unlock(&dev->mutex); + return error; } @@ -649,8 +704,11 @@ netdev_pltap_update_flags(struct netdev *dev_, netdev_pltap_cast(dev_); int error = 0; + ovs_mutex_lock(&sync_list_mutex); + ovs_mutex_lock(&netdev->mutex); if ((off | on) & ~(NETDEV_UP | NETDEV_PROMISC)) { - return EINVAL; + error = EINVAL; + goto out; } if (netdev_pltap_finalized(netdev)) { @@ -664,19 +722,31 @@ netdev_pltap_update_flags(struct netdev *dev_, sync_needed(netdev); } +out: + ovs_mutex_unlock(&netdev->mutex); + ovs_mutex_unlock(&sync_list_mutex); return error; } static unsigned int netdev_pltap_change_seq(const struct netdev *netdev) { - return netdev_pltap_cast(netdev)->change_seq; + struct netdev_pltap *dev = + netdev_pltap_cast(netdev); + unsigned int change_seq; + + ovs_mutex_lock(&dev->mutex); + change_seq = dev->change_seq; + ovs_mutex_unlock(&dev->mutex); + + return change_seq; } /* Helper functions. */ static void netdev_pltap_update_seq(struct netdev_pltap *dev) + OVS_REQUIRES(dev->mutex) { dev->change_seq++; if (!dev->change_seq) { @@ -690,23 +760,26 @@ netdev_pltap_get_real_name(struct unixctl_conn *conn, { struct netdev_pltap *pltap_dev; + ovs_mutex_lock(&pltap_netdevs_mutex); pltap_dev = shash_find_data(&pltap_netdevs, argv[1]); if (!pltap_dev) { unixctl_command_reply_error(conn, "no such pltap netdev"); - return; + goto out; } if (pltap_dev->fd < 0) { unixctl_command_reply_error(conn, "no real device attached"); - return; + goto out; } unixctl_command_reply(conn, pltap_dev->real_name); + +out: + ovs_mutex_unlock(&pltap_netdevs_mutex); } static int netdev_pltap_init(void) { - list_init(&sync_list); unixctl_command_register("netdev-pltap/get-tapname", "port", 1, 1, netdev_pltap_get_real_name, NULL); return 0; @@ -716,18 +789,22 @@ static void netdev_pltap_run(void) { struct netdev_pltap *iter, *next; + ovs_mutex_lock(&sync_list_mutex); LIST_FOR_EACH_SAFE(iter, next, sync_list, &sync_list) { netdev_pltap_sync_flags(iter); } + ovs_mutex_unlock(&sync_list_mutex); } static void netdev_pltap_wait(void) { + ovs_mutex_lock(&sync_list_mutex); if (!list_is_empty(&sync_list)) { VLOG_DBG("netdev_pltap: scheduling sync"); poll_immediate_wake(); } + ovs_mutex_unlock(&sync_list_mutex); } const struct netdev_class netdev_pltap_class = { diff --git a/lib/netdev-tunnel.c b/lib/netdev-tunnel.c index d9c113668..f8eadabc8 100644 --- a/lib/netdev-tunnel.c +++ b/lib/netdev-tunnel.c @@ -40,6 +40,10 @@ VLOG_DEFINE_THIS_MODULE(netdev_tunnel); struct netdev_tunnel { struct netdev up; + + /* Protects all members below. */ + struct ovs_mutex mutex; + uint8_t hwaddr[ETH_ADDR_LEN]; struct netdev_stats stats; enum netdev_flags flags; @@ -59,7 +63,9 @@ struct netdev_rx_tunnel { static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 20); -static struct shash tunnel_netdevs = SHASH_INITIALIZER(&tunnel_netdevs); +static struct ovs_mutex tunnel_netdevs_mutex = OVS_MUTEX_INITIALIZER; +static struct shash tunnel_netdevs OVS_GUARDED_BY(tunnel_netdevs_mutex) + = SHASH_INITIALIZER(&tunnel_netdevs); static int netdev_tunnel_construct(struct netdev *netdevp_); static void netdev_tunnel_update_seq(struct netdev_tunnel *); @@ -94,9 +100,13 @@ netdev_tunnel_alloc(void) static int netdev_tunnel_construct(struct netdev *netdev_) { - static unsigned int n = 0; + static atomic_uint next_n = ATOMIC_VAR_INIT(0); struct netdev_tunnel *netdev = netdev_tunnel_cast(netdev_); + unsigned int n; + + atomic_add(&next_n, 1, &n); + ovs_mutex_init(&netdev->mutex, PTHREAD_MUTEX_NORMAL); netdev->hwaddr[0] = 0xfe; netdev->hwaddr[1] = 0xff; netdev->hwaddr[2] = 0xff; @@ -133,11 +143,16 @@ netdev_tunnel_destruct(struct netdev *netdev_) { struct netdev_tunnel *netdev = netdev_tunnel_cast(netdev_); + ovs_mutex_lock(&tunnel_netdevs_mutex); + if (netdev->sockfd != -1) close(netdev->sockfd); shash_find_and_delete(&tunnel_netdevs, netdev_get_name(netdev_)); + + ovs_mutex_destroy(&netdev->mutex); + ovs_mutex_unlock(&tunnel_netdevs_mutex); } static void @@ -152,17 +167,20 @@ netdev_tunnel_get_config(const struct netdev *dev_, struct smap *args) { struct netdev_tunnel *netdev = netdev_tunnel_cast(dev_); + ovs_mutex_lock(&netdev->mutex); if (netdev->valid_remote_ip) smap_add_format(args, "remote_ip", IP_FMT, IP_ARGS(netdev->remote_addr.sin_addr.s_addr)); if (netdev->valid_remote_port) smap_add_format(args, "remote_port", "%"PRIu16, ntohs(netdev->remote_addr.sin_port)); + ovs_mutex_unlock(&netdev->mutex); return 0; } static int netdev_tunnel_connect(struct netdev_tunnel *dev) + OVS_REQUIRES(dev->mutex) { char buf[1024]; if (dev->sockfd < 0) @@ -185,7 +203,9 @@ netdev_tunnel_set_config(struct netdev *dev_, const struct smap *args) { struct netdev_tunnel *netdev = netdev_tunnel_cast(dev_); struct shash_node *node; + int error; + ovs_mutex_lock(&netdev->mutex); VLOG_DBG("tunnel_set_config(%s)", netdev_get_name(dev_)); SMAP_FOR_EACH(node, args) { VLOG_DBG("arg: %s->%s", node->name, (char*)node->data); @@ -205,7 +225,9 @@ netdev_tunnel_set_config(struct netdev *dev_, const struct smap *args) netdev_get_name(dev_), node->name); } } - return netdev_tunnel_connect(netdev); + error = netdev_tunnel_connect(netdev); + ovs_mutex_unlock(&netdev->mutex); + return error; } static struct netdev_rx * @@ -220,9 +242,11 @@ netdev_tunnel_rx_construct(struct netdev_rx *rx_) { struct netdev_rx_tunnel *rx = netdev_rx_tunnel_cast(rx_); struct netdev *netdev_ = rx->up.netdev; - struct netdev_tunnel *netdev = - netdev_tunnel_cast(netdev_); + struct netdev_tunnel *netdev = netdev_tunnel_cast(netdev_); + + ovs_mutex_lock(&netdev->mutex); rx->fd = netdev->sockfd; + ovs_mutex_unlock(&netdev->mutex); return 0; } @@ -354,10 +378,12 @@ netdev_tunnel_set_etheraddr(struct netdev *netdev, { struct netdev_tunnel *dev = netdev_tunnel_cast(netdev); + ovs_mutex_lock(&dev->mutex); if (!eth_addr_equals(dev->hwaddr, mac)) { memcpy(dev->hwaddr, mac, ETH_ADDR_LEN); netdev_tunnel_update_seq(dev); } + ovs_mutex_unlock(&dev->mutex); return 0; } @@ -368,7 +394,9 @@ netdev_tunnel_get_etheraddr(const struct netdev *netdev, { const struct netdev_tunnel *dev = netdev_tunnel_cast(netdev); + ovs_mutex_lock(&dev->mutex); memcpy(mac, dev->hwaddr, ETH_ADDR_LEN); + ovs_mutex_unlock(&dev->mutex); return 0; } @@ -378,7 +406,9 @@ netdev_tunnel_get_stats(const struct netdev *netdev, struct netdev_stats *stats) { const struct netdev_tunnel *dev = netdev_tunnel_cast(netdev); + ovs_mutex_lock(&dev->mutex); *stats = dev->stats; + ovs_mutex_unlock(&dev->mutex); return 0; } @@ -387,7 +417,9 @@ netdev_tunnel_set_stats(struct netdev *netdev, const struct netdev_stats *stats) { struct netdev_tunnel *dev = netdev_tunnel_cast(netdev); + ovs_mutex_lock(&dev->mutex); dev->stats = *stats; + ovs_mutex_unlock(&dev->mutex); return 0; } @@ -398,9 +430,12 @@ netdev_tunnel_update_flags(struct netdev *dev_, { struct netdev_tunnel *netdev = netdev_tunnel_cast(dev_); + int error = 0; + ovs_mutex_lock(&netdev->mutex); if ((off | on) & ~(NETDEV_UP | NETDEV_PROMISC)) { - return EINVAL; + error = EINVAL; + goto out; } // XXX should we actually do something with these flags? @@ -410,19 +445,30 @@ netdev_tunnel_update_flags(struct netdev *dev_, if (*old_flagsp != netdev->flags) { netdev_tunnel_update_seq(netdev); } - return 0; + +out: + ovs_mutex_unlock(&netdev->mutex); + return error; } static unsigned int -netdev_tunnel_change_seq(const struct netdev *netdev) +netdev_tunnel_change_seq(const struct netdev *netdev_) { - return netdev_tunnel_cast(netdev)->change_seq; + struct netdev_tunnel *netdev = netdev_tunnel_cast(netdev_); + unsigned int change_seq; + + + ovs_mutex_lock(&netdev->mutex); + change_seq = netdev->change_seq; + ovs_mutex_unlock(&netdev->mutex); + return change_seq; } /* Helper functions. */ static void netdev_tunnel_update_seq(struct netdev_tunnel *dev) + OVS_REQUIRES(dev->mutex) { dev->change_seq++; if (!dev->change_seq) { @@ -437,14 +483,20 @@ netdev_tunnel_get_port(struct unixctl_conn *conn, struct netdev_tunnel *tunnel_dev; char buf[6]; + ovs_mutex_lock(&tunnel_netdevs_mutex); tunnel_dev = shash_find_data(&tunnel_netdevs, argv[1]); if (!tunnel_dev) { unixctl_command_reply_error(conn, "no such tunnel netdev"); - return; + goto out; } + ovs_mutex_lock(&tunnel_dev->mutex); sprintf(buf, "%d", ntohs(tunnel_dev->local_addr.sin_port)); + ovs_mutex_unlock(&tunnel_dev->mutex); + unixctl_command_reply(conn, buf); +out: + ovs_mutex_unlock(&tunnel_netdevs_mutex); } static void @@ -454,14 +506,19 @@ netdev_tunnel_get_tx_bytes(struct unixctl_conn *conn, struct netdev_tunnel *tunnel_dev; char buf[128]; + ovs_mutex_lock(&tunnel_netdevs_mutex); tunnel_dev = shash_find_data(&tunnel_netdevs, argv[1]); if (!tunnel_dev) { unixctl_command_reply_error(conn, "no such tunnel netdev"); - return; + goto out; } + ovs_mutex_lock(&tunnel_dev->mutex); sprintf(buf, "%"PRIu64, tunnel_dev->stats.tx_bytes); + ovs_mutex_unlock(&tunnel_dev->mutex); unixctl_command_reply(conn, buf); +out: + ovs_mutex_unlock(&tunnel_netdevs_mutex); } static void @@ -471,14 +528,17 @@ netdev_tunnel_get_rx_bytes(struct unixctl_conn *conn, struct netdev_tunnel *tunnel_dev; char buf[128]; + ovs_mutex_lock(&tunnel_netdevs_mutex); tunnel_dev = shash_find_data(&tunnel_netdevs, argv[1]); if (!tunnel_dev) { unixctl_command_reply_error(conn, "no such tunnel netdev"); - return; + goto out; } sprintf(buf, "%"PRIu64, tunnel_dev->stats.rx_bytes); unixctl_command_reply(conn, buf); +out: + ovs_mutex_unlock(&tunnel_netdevs_mutex); } @@ -494,11 +554,21 @@ netdev_tunnel_init(void) return 0; } +static void +netdev_tunnel_run(void) +{ +} + +static void +netdev_tunnel_wait(void) +{ +} + const struct netdev_class netdev_tunnel_class = { "tunnel", - netdev_tunnel_init, /* init */ - NULL, /* run */ - NULL, /* wait */ + netdev_tunnel_init, + netdev_tunnel_run, + netdev_tunnel_wait, netdev_tunnel_alloc, netdev_tunnel_construct, -- 2.43.0