From 79e9b447bf82ec1ce09ed5c0c45dcc5751fa9cf2 Mon Sep 17 00:00:00 2001 From: Xavi Hernandez Date: Wed, 24 Feb 2021 16:44:55 +0100 Subject: cluster/afr: Fix race in lockinfo (f)getxattr * 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. Fixes: #2161 Change-Id: Idc0d34ab19ea6031de0641f7b05c624d90fac8fa Signed-off-by: Xavi Hernandez --- 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 c5521704de..13db3581ec 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 @@ -866,188 +868,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; } @@ -1055,23 +990,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; } -- cgit