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 <xhernandez@redhat.com>

6 files changed, 78 insertions, 92 deletions

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);
@@ -390,39 +389,6 @@ out:
 }
 
 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)
 {
         int ret = 0;
@@ -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 */