From 7b7ec67680415c22773ebb2a5daacf298b6b1e06 Mon Sep 17 00:00:00 2001

From: Xavi Hernandez <xhernandez@redhat.com>

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

BUG: 1911292

Change-Id: Idc0d34ab19ea6031de0641f7b05c624d90fac8fa

Signed-off-by: Xavi Hernandez <xhernandez@redhat.com>

Reviewed-on: https://code.engineering.redhat.com/gerrit/228924

Tested-by: RHGS Build Bot <nigelb@redhat.com>

Reviewed-by: Sunil Kumar Heggodu Gopala Acharya <sheggodu@redhat.com>

---

 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 <stdlib.h>

 #include <signal.h>

+#include <urcu/uatomic.h>

+

 #include <glusterfs/glusterfs.h>

 #include "afr.h"

 #include <glusterfs/dict.h>

@@ -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