From 157e55fe43ba13f04452aa11f42200b279fb4f7a Mon Sep 17 00:00:00 2001 From: Xavi Hernandez Date: Fri, 9 Mar 2018 22:48:33 +0100 Subject: protocol/client: fix memory corruption There was an issue when some accesses to saved_fds list were protected by the wrong mutex (lock instead of fd_lock). Additionally, the retrieval of fdctx from fd's context and any checks done on it have also been protected by fd_lock to avoid fdctx to become outdated just after retrieving it. Change-Id: If2910508bcb7d1ff23debb30291391f00903a6fe BUG: 1553129 Signed-off-by: Xavi Hernandez --- xlators/protocol/client/src/client-handshake.c | 9 ++- xlators/protocol/client/src/client-helpers.c | 34 +++++---- xlators/protocol/client/src/client-lk.c | 94 +++++++++--------------- xlators/protocol/client/src/client-rpc-fops.c | 24 +++--- xlators/protocol/client/src/client-rpc-fops_v2.c | 8 +- xlators/protocol/client/src/client.h | 1 - 6 files changed, 78 insertions(+), 92 deletions(-) (limited to 'xlators/protocol') diff --git a/xlators/protocol/client/src/client-handshake.c b/xlators/protocol/client/src/client-handshake.c index 74c601bbcbd..5c0b4750e2e 100644 --- a/xlators/protocol/client/src/client-handshake.c +++ b/xlators/protocol/client/src/client-handshake.c @@ -926,11 +926,14 @@ client_attempt_reopen (fd_t *fd, xlator_t *this) conf = this->private; - fdctx = this_fd_get_ctx (fd, this); - if (!fdctx) - goto out; pthread_spin_lock (&conf->fd_lock); { + fdctx = this_fd_get_ctx (fd, this); + if (!fdctx) { + pthread_spin_unlock(&conf->fd_lock); + goto out; + } + if (__is_fd_reopen_in_progress (fdctx)) goto unlock; if (fdctx->remote_fd != -1) diff --git a/xlators/protocol/client/src/client-helpers.c b/xlators/protocol/client/src/client-helpers.c index 465de2a52d4..d0ccca73fe5 100644 --- a/xlators/protocol/client/src/client-helpers.c +++ b/xlators/protocol/client/src/client-helpers.c @@ -424,20 +424,20 @@ client_get_remote_fd (xlator_t *this, fd_t *fd, int flags, int64_t *remote_fd) GF_VALIDATE_OR_GOTO (this->name, fd, out); GF_VALIDATE_OR_GOTO (this->name, remote_fd, out); - fdctx = this_fd_get_ctx (fd, this); - if (!fdctx) { - *remote_fd = GF_ANON_FD_NO; - } else { - conf = this->private; - pthread_spin_lock (&conf->fd_lock); - { + conf = this->private; + pthread_spin_lock (&conf->fd_lock); + { + fdctx = this_fd_get_ctx (fd, this); + if (!fdctx) { + *remote_fd = GF_ANON_FD_NO; + } else { if (__is_fd_reopen_in_progress (fdctx)) *remote_fd = -1; else *remote_fd = fdctx->remote_fd; } - pthread_spin_unlock (&conf->fd_lock); } + pthread_spin_unlock (&conf->fd_lock); if ((flags & FALLBACK_TO_ANON_FD) && (*remote_fd == -1)) *remote_fd = GF_ANON_FD_NO; @@ -450,13 +450,21 @@ out: gf_boolean_t client_is_reopen_needed (fd_t *fd, xlator_t *this, int64_t remote_fd) { + clnt_conf_t *conf = NULL; clnt_fd_ctx_t *fdctx = NULL; + gf_boolean_t res = _gf_false; + + conf = this->private; + pthread_spin_lock(&conf->fd_lock); + { + fdctx = this_fd_get_ctx (fd, this); + if (fdctx && (fdctx->remote_fd == -1) && + (remote_fd == GF_ANON_FD_NO)) + res = _gf_true; + } + pthread_spin_unlock(&conf->fd_lock); - fdctx = this_fd_get_ctx (fd, this); - if (fdctx && (fdctx->remote_fd == -1) && - (remote_fd == GF_ANON_FD_NO)) - return _gf_true; - return _gf_false; + return res; } int diff --git a/xlators/protocol/client/src/client-lk.c b/xlators/protocol/client/src/client-lk.c index 6468fb4511e..b5e11aa07fd 100644 --- a/xlators/protocol/client/src/client-lk.c +++ b/xlators/protocol/client/src/client-lk.c @@ -43,14 +43,10 @@ dump_client_locks_fd (clnt_fd_ctx_t *fdctx) client_posix_lock_t *lock = NULL; int count = 0; - pthread_mutex_lock (&fdctx->mutex); - { - list_for_each_entry (lock, &fdctx->lock_list, list) { - __dump_client_lock (lock); - count++; - } + list_for_each_entry (lock, &fdctx->lock_list, list) { + __dump_client_lock (lock); + count++; } - pthread_mutex_unlock (&fdctx->mutex); return count; @@ -59,23 +55,27 @@ dump_client_locks_fd (clnt_fd_ctx_t *fdctx) int dump_client_locks (inode_t *inode) { - fd_t *fd = NULL; - xlator_t *this = NULL; - clnt_fd_ctx_t *fdctx = NULL; + fd_t *fd = NULL; + xlator_t *this = NULL; + clnt_fd_ctx_t *fdctx = NULL; + clnt_conf_t *conf = NULL; int total_count = 0; int locks_fd_count = 0; this = THIS; + conf = this->private; LOCK (&inode->lock); { list_for_each_entry (fd, &inode->fd_list, inode_list) { locks_fd_count = 0; + pthread_spin_lock(&conf->fd_lock); fdctx = this_fd_get_ctx (fd, this); if (fdctx) locks_fd_count = dump_client_locks_fd (fdctx); + pthread_spin_unlock(&conf->fd_lock); total_count += locks_fd_count; } @@ -327,13 +327,7 @@ __insert_and_merge (clnt_fd_ctx_t *fdctx, client_posix_lock_t *lock) static void client_setlk (clnt_fd_ctx_t *fdctx, client_posix_lock_t *lock) { - pthread_mutex_lock (&fdctx->mutex); - { - __insert_and_merge (fdctx, lock); - } - pthread_mutex_unlock (&fdctx->mutex); - - return; + __insert_and_merge (fdctx, lock); } static void @@ -349,6 +343,7 @@ delete_granted_locks_owner (fd_t *fd, gf_lkowner_t *owner) client_posix_lock_t *lock = NULL; client_posix_lock_t *tmp = NULL; xlator_t *this = NULL; + clnt_conf_t *conf = NULL; struct list_head delete_list; int ret = 0; @@ -356,25 +351,29 @@ delete_granted_locks_owner (fd_t *fd, gf_lkowner_t *owner) INIT_LIST_HEAD (&delete_list); this = THIS; + conf = this->private; + + pthread_spin_lock(&conf->fd_lock); + fdctx = this_fd_get_ctx (fd, this); if (!fdctx) { + pthread_spin_unlock(&conf->fd_lock); + gf_msg (this->name, GF_LOG_WARNING, EINVAL, PC_MSG_FD_CTX_INVALID, "fdctx not valid"); ret = -1; goto out; } - pthread_mutex_lock (&fdctx->mutex); - { - list_for_each_entry_safe (lock, tmp, &fdctx->lock_list, list) { - if (!is_same_lkowner (&lock->owner, owner)) { - list_del_init (&lock->list); - list_add_tail (&lock->list, &delete_list); - count++; - } + list_for_each_entry_safe (lock, tmp, &fdctx->lock_list, list) { + if (!is_same_lkowner (&lock->owner, owner)) { + list_del_init (&lock->list); + list_add_tail (&lock->list, &delete_list); + count++; } } - pthread_mutex_unlock (&fdctx->mutex); + + pthread_spin_unlock(&conf->fd_lock); list_for_each_entry_safe (lock, tmp, &delete_list, list) { list_del_init (&lock->list); @@ -389,39 +388,6 @@ out: return ret; } -int32_t -delete_granted_locks_fd (clnt_fd_ctx_t *fdctx) -{ - client_posix_lock_t *lock = NULL; - client_posix_lock_t *tmp = NULL; - xlator_t *this = NULL; - - struct list_head delete_list; - int ret = 0; - int count = 0; - - INIT_LIST_HEAD (&delete_list); - this = THIS; - - pthread_mutex_lock (&fdctx->mutex); - { - list_splice_init (&fdctx->lock_list, &delete_list); - } - pthread_mutex_unlock (&fdctx->mutex); - - list_for_each_entry_safe (lock, tmp, &delete_list, list) { - list_del_init (&lock->list); - count++; - destroy_client_lock (lock); - } - - /* FIXME: Need to actually print the locks instead of count */ - gf_msg_trace (this->name, 0, - "Number of locks cleared=%d", count); - - return ret; -} - int32_t client_cmd_to_gf_cmd (int32_t cmd, int32_t *gf_cmd) { @@ -497,13 +463,19 @@ client_add_lock_for_recovery (fd_t *fd, struct gf_flock *flock, clnt_fd_ctx_t *fdctx = NULL; xlator_t *this = NULL; client_posix_lock_t *lock = NULL; + clnt_conf_t *conf = NULL; int ret = 0; this = THIS; + conf = this->private; + + pthread_spin_lock(&conf->fd_lock); fdctx = this_fd_get_ctx (fd, this); if (!fdctx) { + pthread_spin_unlock(&conf->fd_lock); + gf_msg (this->name, GF_LOG_WARNING, 0, PC_MSG_FD_GET_FAIL, "failed to get fd context. sending EBADFD"); ret = -EBADFD; @@ -512,12 +484,16 @@ client_add_lock_for_recovery (fd_t *fd, struct gf_flock *flock, lock = new_client_lock (flock, owner, cmd, fd); if (!lock) { + pthread_spin_unlock(&conf->fd_lock); + ret = -ENOMEM; goto out; } client_setlk (fdctx, lock); + pthread_spin_unlock(&conf->fd_lock); + out: return ret; diff --git a/xlators/protocol/client/src/client-rpc-fops.c b/xlators/protocol/client/src/client-rpc-fops.c index 03ca2172692..94fe4ea5ad2 100644 --- a/xlators/protocol/client/src/client-rpc-fops.c +++ b/xlators/protocol/client/src/client-rpc-fops.c @@ -368,10 +368,10 @@ client_add_fd_to_saved_fds (xlator_t *this, fd_t *fd, loc_t *loc, int32_t flags, INIT_LIST_HEAD (&fdctx->sfd_pos); INIT_LIST_HEAD (&fdctx->lock_list); - this_fd_set_ctx (fd, this, loc, fdctx); - pthread_spin_lock (&conf->fd_lock); { + this_fd_set_ctx (fd, this, loc, fdctx); + list_add_tail (&fdctx->sfd_pos, &conf->saved_fds); } pthread_spin_unlock (&conf->fd_lock); @@ -3225,10 +3225,10 @@ client3_3_releasedir (call_frame_t *frame, xlator_t *this, args = data; conf = this->private; - fdctx = this_fd_del_ctx (args->fd, this); - if (fdctx != NULL) { - pthread_spin_lock (&conf->fd_lock); - { + pthread_spin_lock (&conf->fd_lock); + { + fdctx = this_fd_del_ctx (args->fd, this); + if (fdctx != NULL) { remote_fd = fdctx->remote_fd; /* fdctx->remote_fd == -1 indicates a reopen attempt @@ -3243,8 +3243,8 @@ client3_3_releasedir (call_frame_t *frame, xlator_t *this, destroy = _gf_true; } } - pthread_spin_unlock (&conf->fd_lock); } + pthread_spin_unlock (&conf->fd_lock); if (destroy) client_fdctx_destroy (this, fdctx); @@ -3270,10 +3270,10 @@ client3_3_release (call_frame_t *frame, xlator_t *this, args = data; conf = this->private; - fdctx = this_fd_del_ctx (args->fd, this); - if (fdctx != NULL) { - pthread_spin_lock (&conf->fd_lock); - { + pthread_spin_lock (&conf->fd_lock); + { + fdctx = this_fd_del_ctx (args->fd, this); + if (fdctx != NULL) { remote_fd = fdctx->remote_fd; /* fdctx->remote_fd == -1 indicates a reopen attempt @@ -3287,8 +3287,8 @@ client3_3_release (call_frame_t *frame, xlator_t *this, destroy = _gf_true; } } - pthread_spin_unlock (&conf->fd_lock); } + pthread_spin_unlock (&conf->fd_lock); if (destroy) client_fdctx_destroy (this, fdctx); diff --git a/xlators/protocol/client/src/client-rpc-fops_v2.c b/xlators/protocol/client/src/client-rpc-fops_v2.c index aad1666096b..15f1c956101 100644 --- a/xlators/protocol/client/src/client-rpc-fops_v2.c +++ b/xlators/protocol/client/src/client-rpc-fops_v2.c @@ -2758,7 +2758,7 @@ client4_0_releasedir (call_frame_t *frame, xlator_t *this, args = data; conf = this->private; - pthread_mutex_lock (&conf->lock); + pthread_spin_lock (&conf->fd_lock); { fdctx = this_fd_del_ctx (args->fd, this); if (fdctx != NULL) { @@ -2777,7 +2777,7 @@ client4_0_releasedir (call_frame_t *frame, xlator_t *this, } } } - pthread_mutex_unlock (&conf->lock); + pthread_spin_unlock (&conf->fd_lock); if (destroy) client_fdctx_destroy (this, fdctx); @@ -2803,7 +2803,7 @@ client4_0_release (call_frame_t *frame, xlator_t *this, args = data; conf = this->private; - pthread_mutex_lock (&conf->lock); + pthread_spin_lock (&conf->fd_lock); { fdctx = this_fd_del_ctx (args->fd, this); if (fdctx != NULL) { @@ -2821,7 +2821,7 @@ client4_0_release (call_frame_t *frame, xlator_t *this, } } } - pthread_mutex_unlock (&conf->lock); + pthread_spin_unlock (&conf->fd_lock); if (destroy) client_fdctx_destroy (this, fdctx); diff --git a/xlators/protocol/client/src/client.h b/xlators/protocol/client/src/client.h index b51607fdc45..c3c8aaec0dc 100644 --- a/xlators/protocol/client/src/client.h +++ b/xlators/protocol/client/src/client.h @@ -246,7 +246,6 @@ typedef struct _client_fd_ctx { char released; int32_t flags; fd_lk_ctx_t *lk_ctx; - pthread_mutex_t mutex; uuid_t gfid; void (*reopen_done)(struct _client_fd_ctx*, int64_t rfd, xlator_t *); struct list_head lock_list; /* List of all granted locks on this fd */ -- cgit