From 5a2e524418154deab5cac0e65d48a8391c860eea Mon Sep 17 00:00:00 2001 From: dimitri staessens Date: Sun, 23 Oct 2016 16:00:33 +0200 Subject: ipcpd: Improve locking in ipcp-data This solves some potential data races in the shims. --- src/ipcpd/ipcp-data.c | 112 +++++++++++++++++++++++++++++++++----------------- src/ipcpd/ipcp-data.h | 6 +-- 2 files changed, 76 insertions(+), 42 deletions(-) diff --git a/src/ipcpd/ipcp-data.c b/src/ipcpd/ipcp-data.c index c4838d3a..ed7e578d 100644 --- a/src/ipcpd/ipcp-data.c +++ b/src/ipcpd/ipcp-data.c @@ -32,6 +32,7 @@ #include #include +#include struct reg_entry { struct list_head list; @@ -50,6 +51,8 @@ static struct reg_entry * reg_entry_create(char * name) if (entry == NULL) return NULL; + assert(name); + entry->name = name; if (entry->name == NULL) return NULL; @@ -59,11 +62,11 @@ static struct reg_entry * reg_entry_create(char * name) static void reg_entry_destroy(struct reg_entry * entry) { - if (entry == NULL) - return; + assert(entry); if (entry->name != NULL) free(entry->name); + free(entry); } @@ -74,6 +77,8 @@ static struct dir_entry * dir_entry_create(char * ap_name, if (entry == NULL) return NULL; + assert(ap_name); + entry->addr = addr; entry->ap_name = ap_name; if (entry->ap_name == NULL) @@ -84,11 +89,11 @@ static struct dir_entry * dir_entry_create(char * ap_name, static void dir_entry_destroy(struct dir_entry * entry) { - if (entry == NULL) - return; + assert(entry); if (entry->ap_name != NULL) free(entry->ap_name); + free(entry); } @@ -116,9 +121,9 @@ struct ipcp_data * ipcp_data_init(struct ipcp_data * dst, INIT_LIST_HEAD(&dst->registry); INIT_LIST_HEAD(&dst->directory); - /* init the mutexes */ - pthread_mutex_init(&dst->reg_lock, NULL); - pthread_mutex_init(&dst->dir_lock, NULL); + /* init the locks */ + pthread_rwlock_init(&dst->reg_lock, NULL); + pthread_rwlock_init(&dst->dir_lock, NULL); return dst; } @@ -127,6 +132,9 @@ static void clear_registry(struct ipcp_data * data) { struct list_head * h; struct list_head * t; + + assert(data); + list_for_each_safe(h, t, &data->registry) { struct reg_entry * e = list_entry(h, struct reg_entry, list); list_del(&e->list); @@ -138,6 +146,9 @@ static void clear_directory(struct ipcp_data * data) { struct list_head * h; struct list_head * t; + + assert(data); + list_for_each_safe(h, t, &data->directory) { struct dir_entry * e = list_entry(h, struct dir_entry, list); list_del(&e->list); @@ -150,21 +161,23 @@ void ipcp_data_destroy(struct ipcp_data * data) if (data == NULL) return; - pthread_mutex_lock(&data->reg_lock); - pthread_mutex_lock(&data->dir_lock); - /* clear the lists */ + pthread_rwlock_wrlock(&data->reg_lock); + clear_registry(data); + + pthread_rwlock_unlock(&data->reg_lock); + pthread_rwlock_wrlock(&data->dir_lock); + clear_directory(data); + pthread_rwlock_unlock(&data->dir_lock); + if (data->dif_name != NULL) free(data->dif_name); - pthread_mutex_unlock(&data->dir_lock); - pthread_mutex_unlock(&data->reg_lock); - - pthread_mutex_destroy(&data->dir_lock); - pthread_mutex_destroy(&data->reg_lock); + pthread_rwlock_destroy(&data->dir_lock); + pthread_rwlock_destroy(&data->reg_lock); free(data); } @@ -175,6 +188,10 @@ static struct reg_entry * find_reg_entry_by_name(struct ipcp_data * data, const char * name) { struct list_head * h; + + assert(data); + assert(name); + list_for_each(h, &data->registry) { struct reg_entry * e = list_entry(h, struct reg_entry, list); if (!strcmp(e->name, name)) @@ -214,7 +231,15 @@ static struct dir_entry * find_dir_entry_any(struct ipcp_data * data, bool ipcp_data_is_in_directory(struct ipcp_data * data, const char * ap_name) { - return find_dir_entry_any(data, ap_name) != NULL; + bool ret = false; + + pthread_rwlock_rdlock(&data->dir_lock); + + ret = (find_dir_entry_any(data, ap_name) != NULL); + + pthread_rwlock_unlock(&data->dir_lock); + + return ret; } int ipcp_data_del_reg_entry(struct ipcp_data * data, @@ -224,19 +249,19 @@ int ipcp_data_del_reg_entry(struct ipcp_data * data, if (data == NULL) return -1; - pthread_mutex_lock(&data->reg_lock); + pthread_rwlock_wrlock(&data->reg_lock); e = find_reg_entry_by_name(data, name); if (e == NULL) { - pthread_mutex_unlock(&data->reg_lock); + pthread_rwlock_unlock(&data->reg_lock); return 0; /* nothing to do */ } list_del(&e->list); - reg_entry_destroy(e); + pthread_rwlock_unlock(&data->reg_lock); - pthread_mutex_unlock(&data->reg_lock); + reg_entry_destroy(e); return 0; } @@ -249,19 +274,19 @@ int ipcp_data_del_dir_entry(struct ipcp_data * data, if (data == NULL) return -1; - pthread_mutex_lock(&data->dir_lock); + pthread_rwlock_wrlock(&data->dir_lock); e = find_dir_entry(data, ap_name, addr); if (e == NULL) { - pthread_mutex_unlock(&data->dir_lock); + pthread_rwlock_unlock(&data->dir_lock); return 0; /* nothing to do */ } list_del(&e->list); - dir_entry_destroy(e); + pthread_rwlock_unlock(&data->dir_lock); - pthread_mutex_unlock(&data->dir_lock); + dir_entry_destroy(e); return 0; } @@ -274,22 +299,22 @@ int ipcp_data_add_reg_entry(struct ipcp_data * data, if (data == NULL || name == NULL) return -1; - pthread_mutex_lock(&data->reg_lock); + pthread_rwlock_wrlock(&data->reg_lock); if (find_reg_entry_by_name(data, name)) { - pthread_mutex_unlock(&data->reg_lock); - return -2; + pthread_rwlock_unlock(&data->reg_lock); + return -1; } entry = reg_entry_create(name); if (entry == NULL) { - pthread_mutex_unlock(&data->reg_lock); + pthread_rwlock_unlock(&data->reg_lock); return -1; } list_add(&entry->list, &data->registry); - pthread_mutex_unlock(&data->reg_lock); + pthread_rwlock_unlock(&data->reg_lock); return 0; } @@ -303,22 +328,22 @@ int ipcp_data_add_dir_entry(struct ipcp_data * data, if (data == NULL || ap_name == NULL) return -1; - pthread_mutex_lock(&data->dir_lock); + pthread_rwlock_wrlock(&data->dir_lock); if (find_dir_entry(data, ap_name, addr) != NULL) { - pthread_mutex_unlock(&data->dir_lock); - return -2; + pthread_rwlock_unlock(&data->dir_lock); + return -1; } entry = dir_entry_create(ap_name, addr); if (entry == NULL) { - pthread_mutex_unlock(&data->dir_lock); + pthread_rwlock_unlock(&data->dir_lock); return -1; } list_add(&entry->list,&data->directory); - pthread_mutex_unlock(&data->dir_lock); + pthread_rwlock_unlock(&data->dir_lock); return 0; } @@ -326,7 +351,18 @@ int ipcp_data_add_dir_entry(struct ipcp_data * data, bool ipcp_data_is_in_registry(struct ipcp_data * data, const char * ap_name) { - return find_reg_entry_by_name(data, ap_name) != NULL; + bool ret = false; + + if (data == NULL || ap_name == NULL) + return false; + + pthread_rwlock_rdlock(&data->reg_lock); + + ret = (find_reg_entry_by_name(data, ap_name) != NULL); + + pthread_rwlock_unlock(&data->reg_lock); + + return ret; } uint64_t ipcp_data_get_addr(struct ipcp_data * data, @@ -335,18 +371,18 @@ uint64_t ipcp_data_get_addr(struct ipcp_data * data, struct dir_entry * entry; uint64_t addr; - pthread_mutex_lock(&data->dir_lock); + pthread_rwlock_rdlock(&data->dir_lock); entry = find_dir_entry_any(data, ap_name); if (entry == NULL) { - pthread_mutex_unlock(&data->dir_lock); + pthread_rwlock_unlock(&data->dir_lock); return 0; /* undefined behaviour, 0 may be a valid address */ } addr = entry->addr; - pthread_mutex_unlock(&data->dir_lock); + pthread_rwlock_unlock(&data->dir_lock); return addr; } diff --git a/src/ipcpd/ipcp-data.h b/src/ipcpd/ipcp-data.h index a9a637f0..4472a1d7 100644 --- a/src/ipcpd/ipcp-data.h +++ b/src/ipcpd/ipcp-data.h @@ -36,12 +36,10 @@ struct ipcp_data { char * dif_name; struct list_head registry; - pthread_mutex_t reg_lock; + pthread_rwlock_t reg_lock; struct list_head directory; - pthread_mutex_t dir_lock; - - pthread_mutex_t lock; + pthread_rwlock_t dir_lock; }; struct ipcp_data * ipcp_data_create(void); -- cgit v1.2.3 From 15bc37f3cbfc8df224854f5d988e0689ec7c94e0 Mon Sep 17 00:00:00 2001 From: dimitri staessens Date: Sun, 23 Oct 2016 16:34:54 +0200 Subject: ipcpd: Fix log message and lock Remote requests for a flow that's locally deallocated should not generate an error. Sending messages should not be under lock. --- src/ipcpd/shim-eth-llc/main.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/ipcpd/shim-eth-llc/main.c b/src/ipcpd/shim-eth-llc/main.c index 3fc7eda5..6046d939 100644 --- a/src/ipcpd/shim-eth-llc/main.c +++ b/src/ipcpd/shim-eth-llc/main.c @@ -426,7 +426,7 @@ static int eth_llc_ipcp_flow_dealloc_req(uint8_t ssap) if (fd < 0) { pthread_rwlock_unlock(ð_llc_data.flows_lock); pthread_rwlock_unlock(&ipcpi.state_lock); - LOG_ERR("No flow found for remote deallocation request."); + LOG_DBG("Flow already deallocated."); return 0; } @@ -1009,10 +1009,9 @@ static int eth_llc_ipcp_flow_dealloc(int fd) eth_llc_data.ef_to_fd[sap] = -1; pthread_rwlock_unlock(ð_llc_data.flows_lock); - - ret = eth_llc_ipcp_sap_dealloc(addr, r_sap); pthread_rwlock_unlock(&ipcpi.state_lock); + ret = eth_llc_ipcp_sap_dealloc(addr, r_sap); if (ret < 0) LOG_DBG("Could not notify remote."); -- cgit v1.2.3