From 7b7ec67680415c22773ebb2a5daacf298b6b1e06 Mon Sep 17 00:00:00 2001 From: Xavi Hernandez Date: Sat, 13 Feb 2021 18:37:32 +0100 Subject: [PATCH 537/538] cluster/afr: Fix race in lockinfo (f)getxattr A shared dictionary was updated outside the lock after having updated the number of remaining answers. This means that one thread may be processing the last answer and unwinding the request before another thread completes updating the dict. Thread 1 Thread 2 LOCK() call_cnt-- (=1) UNLOCK() LOCK() call_cnt-- (=0) UNLOCK() update_dict(dict) if (call_cnt == 0) { STACK_UNWIND(dict); } update_dict(dict) if (call_cnt == 0) { STACK_UNWIND(dict); } The updates from thread 1 are lost. This patch also reduces the work done inside the locked region and reduces code duplication. Upstream-patch: > Upstream-patch-link: https://github.com/gluster/glusterfs/pull/2162 > Fixes: #2161 > Change-Id: Idc0d34ab19ea6031de0641f7b05c624d90fac8fa > Signed-off-by: Xavi Hernandez BUG: 1911292 Change-Id: Idc0d34ab19ea6031de0641f7b05c624d90fac8fa Signed-off-by: Xavi Hernandez Reviewed-on: https://code.engineering.redhat.com/gerrit/228924 Tested-by: RHGS Build Bot Reviewed-by: Sunil Kumar Heggodu Gopala Acharya --- xlators/cluster/afr/src/afr-inode-read.c | 254 ++++++++++++++----------------- 1 file changed, 112 insertions(+), 142 deletions(-) diff --git a/xlators/cluster/afr/src/afr-inode-read.c b/xlators/cluster/afr/src/afr-inode-read.c index cf305af..98e195a 100644 --- a/xlators/cluster/afr/src/afr-inode-read.c +++ b/xlators/cluster/afr/src/afr-inode-read.c @@ -15,6 +15,8 @@ #include #include +#include + #include #include "afr.h" #include @@ -868,188 +870,121 @@ afr_getxattr_quota_size_cbk(call_frame_t *frame, void *cookie, xlator_t *this, return 0; } -int32_t -afr_getxattr_lockinfo_cbk(call_frame_t *frame, void *cookie, xlator_t *this, - int32_t op_ret, int32_t op_errno, dict_t *dict, - dict_t *xdata) +static int32_t +afr_update_local_dicts(call_frame_t *frame, dict_t *dict, dict_t *xdata) { - int call_cnt = 0, len = 0; - char *lockinfo_buf = NULL; - dict_t *lockinfo = NULL, *newdict = NULL; - afr_local_t *local = NULL; + afr_local_t *local; + dict_t *local_dict; + dict_t *local_xdata; + int32_t ret; - LOCK(&frame->lock); - { - local = frame->local; + local = frame->local; + local_dict = NULL; + local_xdata = NULL; - call_cnt = --local->call_count; + ret = -ENOMEM; - if ((op_ret < 0) || (!dict && !xdata)) { - goto unlock; - } - - if (xdata) { - if (!local->xdata_rsp) { - local->xdata_rsp = dict_new(); - if (!local->xdata_rsp) { - local->op_ret = -1; - local->op_errno = ENOMEM; - goto unlock; - } - } + if ((dict != NULL) && (local->dict == NULL)) { + local_dict = dict_new(); + if (local_dict == NULL) { + goto done; } + } - if (!dict) { - goto unlock; + if ((xdata != NULL) && (local->xdata_rsp == NULL)) { + local_xdata = dict_new(); + if (local_xdata == NULL) { + goto done; } + } - op_ret = dict_get_ptr_and_len(dict, GF_XATTR_LOCKINFO_KEY, - (void **)&lockinfo_buf, &len); + if ((local_dict != NULL) || (local_xdata != NULL)) { + /* TODO: Maybe it would be better to preallocate both dicts before + * sending the requests. This way we don't need to use a LOCK() + * here. */ + LOCK(&frame->lock); - if (!lockinfo_buf) { - goto unlock; + if ((local_dict != NULL) && (local->dict == NULL)) { + local->dict = local_dict; + local_dict = NULL; } - if (!local->dict) { - local->dict = dict_new(); - if (!local->dict) { - local->op_ret = -1; - local->op_errno = ENOMEM; - goto unlock; - } + if ((local_xdata != NULL) && (local->xdata_rsp == NULL)) { + local->xdata_rsp = local_xdata; + local_xdata = NULL; } - } -unlock: - UNLOCK(&frame->lock); - if (lockinfo_buf != NULL) { - lockinfo = dict_new(); - if (lockinfo == NULL) { - local->op_ret = -1; - local->op_errno = ENOMEM; - } else { - op_ret = dict_unserialize(lockinfo_buf, len, &lockinfo); - - if (lockinfo && local->dict) { - dict_copy(lockinfo, local->dict); - } - } - } - - if (xdata && local->xdata_rsp) { - dict_copy(xdata, local->xdata_rsp); + UNLOCK(&frame->lock); } - if (!call_cnt) { - newdict = dict_new(); - if (!newdict) { - local->op_ret = -1; - local->op_errno = ENOMEM; - goto unwind; + if (dict != NULL) { + if (dict_copy(dict, local->dict) < 0) { + goto done; } + } - op_ret = dict_allocate_and_serialize( - local->dict, (char **)&lockinfo_buf, (unsigned int *)&len); - if (op_ret != 0) { - local->op_ret = -1; - goto unwind; + if (xdata != NULL) { + if (dict_copy(xdata, local->xdata_rsp) < 0) { + goto done; } + } - op_ret = dict_set_dynptr(newdict, GF_XATTR_LOCKINFO_KEY, - (void *)lockinfo_buf, len); - if (op_ret < 0) { - local->op_ret = -1; - local->op_errno = -op_ret; - goto unwind; - } + ret = 0; - unwind: - AFR_STACK_UNWIND(getxattr, frame, op_ret, op_errno, newdict, - local->xdata_rsp); +done: + if (local_dict != NULL) { + dict_unref(local_dict); } - dict_unref(lockinfo); + if (local_xdata != NULL) { + dict_unref(local_xdata); + } - return 0; + return ret; } -int32_t -afr_fgetxattr_lockinfo_cbk(call_frame_t *frame, void *cookie, xlator_t *this, - int32_t op_ret, int32_t op_errno, dict_t *dict, - dict_t *xdata) +static void +afr_getxattr_lockinfo_cbk_common(call_frame_t *frame, int32_t op_ret, + int32_t op_errno, dict_t *dict, dict_t *xdata, + bool is_fgetxattr) { - int call_cnt = 0, len = 0; + int len = 0; char *lockinfo_buf = NULL; dict_t *lockinfo = NULL, *newdict = NULL; afr_local_t *local = NULL; - LOCK(&frame->lock); - { - local = frame->local; - - call_cnt = --local->call_count; - - if ((op_ret < 0) || (!dict && !xdata)) { - goto unlock; - } - - if (xdata) { - if (!local->xdata_rsp) { - local->xdata_rsp = dict_new(); - if (!local->xdata_rsp) { - local->op_ret = -1; - local->op_errno = ENOMEM; - goto unlock; - } - } - } - - if (!dict) { - goto unlock; - } + local = frame->local; + if ((op_ret >= 0) && (dict != NULL)) { op_ret = dict_get_ptr_and_len(dict, GF_XATTR_LOCKINFO_KEY, (void **)&lockinfo_buf, &len); - - if (!lockinfo_buf) { - goto unlock; - } - - if (!local->dict) { - local->dict = dict_new(); - if (!local->dict) { - local->op_ret = -1; - local->op_errno = ENOMEM; - goto unlock; + if (lockinfo_buf != NULL) { + lockinfo = dict_new(); + if (lockinfo == NULL) { + op_ret = -1; + } else { + op_ret = dict_unserialize(lockinfo_buf, len, &lockinfo); } } } -unlock: - UNLOCK(&frame->lock); - if (lockinfo_buf != NULL) { - lockinfo = dict_new(); - if (lockinfo == NULL) { - local->op_ret = -1; - local->op_errno = ENOMEM; - } else { - op_ret = dict_unserialize(lockinfo_buf, len, &lockinfo); - - if (lockinfo && local->dict) { - dict_copy(lockinfo, local->dict); - } + if ((op_ret >= 0) && ((lockinfo != NULL) || (xdata != NULL))) { + op_ret = afr_update_local_dicts(frame, lockinfo, xdata); + if (lockinfo != NULL) { + dict_unref(lockinfo); } } - if (xdata && local->xdata_rsp) { - dict_copy(xdata, local->xdata_rsp); + if (op_ret < 0) { + local->op_ret = -1; + local->op_errno = ENOMEM; } - if (!call_cnt) { + if (uatomic_sub_return(&local->call_count, 1) == 0) { newdict = dict_new(); if (!newdict) { local->op_ret = -1; - local->op_errno = ENOMEM; + local->op_errno = op_errno = ENOMEM; goto unwind; } @@ -1057,23 +992,58 @@ unlock: local->dict, (char **)&lockinfo_buf, (unsigned int *)&len); if (op_ret != 0) { local->op_ret = -1; + local->op_errno = op_errno = ENOMEM; goto unwind; } op_ret = dict_set_dynptr(newdict, GF_XATTR_LOCKINFO_KEY, (void *)lockinfo_buf, len); if (op_ret < 0) { - local->op_ret = -1; - local->op_errno = -op_ret; + GF_FREE(lockinfo_buf); + local->op_ret = op_ret = -1; + local->op_errno = op_errno = -op_ret; goto unwind; } unwind: - AFR_STACK_UNWIND(fgetxattr, frame, op_ret, op_errno, newdict, - local->xdata_rsp); + /* TODO: These unwinds use op_ret and op_errno instead of local->op_ret + * and local->op_errno. This doesn't seem right because any + * failure during processing of each answer could be silently + * ignored. This is kept this was the old behavior and because + * local->op_ret is initialized as -1 and local->op_errno is + * initialized as EUCLEAN, which makes these values useless. */ + if (is_fgetxattr) { + AFR_STACK_UNWIND(fgetxattr, frame, op_ret, op_errno, newdict, + local->xdata_rsp); + } else { + AFR_STACK_UNWIND(getxattr, frame, op_ret, op_errno, newdict, + local->xdata_rsp); + } + + if (newdict != NULL) { + dict_unref(newdict); + } } +} + +static int32_t +afr_getxattr_lockinfo_cbk(call_frame_t *frame, void *cookie, xlator_t *this, + int32_t op_ret, int32_t op_errno, dict_t *dict, + dict_t *xdata) +{ + afr_getxattr_lockinfo_cbk_common(frame, op_ret, op_errno, dict, xdata, + false); - dict_unref(lockinfo); + return 0; +} + +static int32_t +afr_fgetxattr_lockinfo_cbk(call_frame_t *frame, void *cookie, xlator_t *this, + int32_t op_ret, int32_t op_errno, dict_t *dict, + dict_t *xdata) +{ + afr_getxattr_lockinfo_cbk_common(frame, op_ret, op_errno, dict, xdata, + true); return 0; } -- 1.8.3.1