From ba57b043db1e19196cf860baeeeb1acfc9985cd2 Mon Sep 17 00:00:00 2001

From: Xavi Hernandez <xhernandez@users.noreply.github.com>

Date: Wed, 24 Feb 2021 15:04:23 +0100

Subject: [PATCH 557/584] cluster/dht: Fix stack overflow in readdir(p)

When parallel-readdir is enabled, readdir(p) requests sent by DHT can be

immediately processed and answered in the same thread before the call to

STACK_WIND_COOKIE() completes.

This means that the readdir(p) cbk is processed synchronously. In some

cases it may decide to send another readdir(p) request, which causes a

recursive call.

When some special conditions happen and the directories are big, it's

possible that the number of nested calls is so high that the process

crashes because of a stack overflow.

This patch fixes this by not allowing nested readdir(p) calls. When a

nested call is detected, it's queued instead of sending it. The queued

request is processed when the current call finishes by the top level

stack function.

Backport of 3 patches:

> Upstream-patch: https://github.com/gluster/glusterfs/pull/2170

> Fixes: #2169

> Change-Id: Id763a8a51fb3c3314588ec7c162f649babf33099

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

> Upstream-patch: https://github.com/gluster/glusterfs/pull/2202

> Updates: #2169

> Change-Id: I97e73c0aae74fc5d80c975f56f2f7a64e3e1ae95

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

> Upstream-patch: https://github.com/gluster/glusterfs/pull/2242

> Fixes: #2239

> Change-Id: I6b2e48e87c85de27fad67a12d97abd91fa27c0c1

> Signed-off-by: Pranith Kumar K <pranith.karampuri@phonepe.com>

BUG: 1798897

Change-Id: Id763a8a51fb3c3314588ec7c162f649babf33099

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

Reviewed-on: https://code.engineering.redhat.com/gerrit/c/rhs-glusterfs/+/244549

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

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

---

 tests/bugs/distribute/issue-2169.t   |  33 +++++++++

 xlators/cluster/dht/src/dht-common.c | 134 ++++++++++++++++++++++++++++++++---

 xlators/cluster/dht/src/dht-common.h |   5 ++

 3 files changed, 162 insertions(+), 10 deletions(-)

 create mode 100755 tests/bugs/distribute/issue-2169.t

diff --git a/tests/bugs/distribute/issue-2169.t b/tests/bugs/distribute/issue-2169.t

new file mode 100755

index 0000000..91fa72a

--- /dev/null

+++ b/tests/bugs/distribute/issue-2169.t

@@ -0,0 +1,33 @@

+#!/bin/bash

+

+. $(dirname $0)/../../include.rc

+. $(dirname $0)/../../volume.rc

+

+cleanup

+

+TEST glusterd

+TEST ${CLI} volume create ${V0} ${H0}:/$B0/${V0}_0

+TEST ${CLI} volume set ${V0} readdir-ahead on

+TEST ${CLI} volume set ${V0} parallel-readdir on

+TEST ${CLI} volume start ${V0}

+

+TEST ${GFS} --volfile-server ${H0} --volfile-id ${V0} ${M0}

+

+TEST mkdir -p ${M0}/d/d.{000..999}

+

+EXPECT_WITHIN ${UMOUNT_TIMEOUT} "Y" force_umount ${M0}

+

+TEST ${CLI} volume add-brick ${V0} ${H0}:${B0}/${V0}_{1..7}

+

+TEST ${GFS} --volfile-server ${H0} --volfile-id ${V0} ${M0}

+

+ls -l ${M0}/d/ | wc -l

+

+EXPECT_WITHIN ${UMOUNT_TIMEOUT} "Y" force_umount ${M0}

+TEST ${GFS} --volfile-server ${H0} --volfile-id ${V0} ${M0}

+

+ls -l ${M0}/d/ | wc -l

+

+TEST ls ${M0}/d

+

+cleanup

diff --git a/xlators/cluster/dht/src/dht-common.c b/xlators/cluster/dht/src/dht-common.c

index 0773092..ce0fbbf 100644

--- a/xlators/cluster/dht/src/dht-common.c

+++ b/xlators/cluster/dht/src/dht-common.c

@@ -24,8 +24,15 @@

 #include <libgen.h>

 #include <signal.h>

+#include <urcu/uatomic.h>

+

 int run_defrag = 0;

+static int

+dht_rmdir_readdirp_cbk(call_frame_t *frame, void *cookie, xlator_t *this,

+                       int op_ret, int op_errno, gf_dirent_t *entries,

+                       dict_t *xdata);

+

 int

 dht_link2(xlator_t *this, xlator_t *dst_node, call_frame_t *frame, int ret);

@@ -6681,6 +6688,94 @@ out:

     return;

 }

+/* Execute a READDIR request if no other request is in progress. Otherwise

+ * queue it to be executed when the current one finishes.

+ *

+ * When parallel-readdir is enabled and directory contents are cached, the

+ * callback of a readdirp will be called before returning from STACK_WIND.

+ * If the returned contents are not useful for DHT, and the buffer is not

+ * yet full, a nested readdirp request will be sent. This means that there

+ * will be many recursive calls. In the worst case there might be a stack

+ * overflow.

+ *

+ * To avoid this, we only wind a request if no other request is being wound.

+ * If there's another request, we simple store the values for the next call.

+ * When the thread processing the current wind completes it, it will take

+ * the new arguments and send the request from the top level stack. */

+static void

+dht_queue_readdir(call_frame_t *frame, xlator_t *xl, off_t offset,

+                  fop_readdir_cbk_t cbk)

+{

+    dht_local_t *local;

+    int32_t queue;

+    xlator_t *this = NULL;

+

+    local = frame->local;

+    this = frame->this;

+

+    local->queue_xl = xl;

+    local->queue_offset = offset;

+

+    if (uatomic_add_return(&local->queue, 1) == 1) {

+        /* If we are here it means that we are the first one to send a

+         * readdir request. Any attempt to send more readdir requests will

+         * find local->queue > 1, so it won't do anything. The needed data

+         * to send the request has been stored into local->queue_*.

+         *

+         * Note: this works because we will only have 1 additional request

+         *       at most (the one called by the cbk function) while we are

+         *       processing another readdir. */

+        do {

+            STACK_WIND_COOKIE(frame, cbk, local->queue_xl, local->queue_xl,

+                              local->queue_xl->fops->readdir, local->fd,

+                              local->size, local->queue_offset, local->xattr);

+

+            /* If a new readdirp request has been added before returning

+             * from winding, we process it. */

+        } while ((queue = uatomic_sub_return(&local->queue, 1)) > 0);

+

+        if (queue < 0) {

+            /* A negative value means that an unwind has been called before

+             * returning from the previous wind. This means that 'local' is

+             * not needed anymore and must be destroyed. */

+            dht_local_wipe(this, local);

+        }

+    }

+}

+

+/* Execute a READDIRP request if no other request is in progress. Otherwise

+ * queue it to be executed when the current one finishes. */

+static void

+dht_queue_readdirp(call_frame_t *frame, xlator_t *xl, off_t offset,

+                   fop_readdirp_cbk_t cbk)

+{

+    dht_local_t *local;

+    int32_t queue;

+    xlator_t *this = NULL;

+

+    local = frame->local;

+    this = frame->this;

+

+    local->queue_xl = xl;

+    local->queue_offset = offset;

+

+    /* Check dht_queue_readdir() comments for an explanation of this. */

+    if (uatomic_add_return(&local->queue, 1) == 1) {

+        do {

+            STACK_WIND_COOKIE(frame, cbk, local->queue_xl, local->queue_xl,

+                              local->queue_xl->fops->readdirp, local->fd,

+                              local->size, local->queue_offset, local->xattr);

+        } while ((queue = uatomic_sub_return(&local->queue, 1)) > 0);

+

+        if (queue < 0) {

+            /* A negative value means that an unwind has been called before

+             * returning from the previous wind. This means that 'local' is

+             * not needed anymore and must be destroyed. */

+            dht_local_wipe(this, local);

+        }

+    }

+}

+

 /* Posix returns op_errno = ENOENT to indicate that there are no more

  * entries

  */

@@ -6950,9 +7045,8 @@ done:

             }

         }

-        STACK_WIND_COOKIE(frame, dht_readdirp_cbk, next_subvol, next_subvol,

-                          next_subvol->fops->readdirp, local->fd, local->size,

-                          next_offset, local->xattr);

+        dht_queue_readdirp(frame, next_subvol, next_offset, dht_readdirp_cbk);

+

         return 0;

     }

@@ -6970,6 +7064,17 @@ unwind:

     if (prev != dht_last_up_subvol(this))

         op_errno = 0;

+    /* If we are inside a recursive call (or not inside a recursive call but

+     * the cbk is completed before the wind returns), local->queue will be 1.

+     * In this case we cannot destroy 'local' because it will be needed by

+     * the caller of STACK_WIND. In this case, we decrease the value to let

+     * the caller know that the operation has terminated and it must destroy

+     * 'local'. If local->queue 0, we can destroy it here because there are

+     * no other users. */

+    if (uatomic_sub_return(&local->queue, 1) >= 0) {

+        frame->local = NULL;

+    }

+

     DHT_STACK_UNWIND(readdirp, frame, op_ret, op_errno, &entries, NULL);

     gf_dirent_free(&entries);

@@ -7071,9 +7176,8 @@ done:

             goto unwind;

         }

-        STACK_WIND_COOKIE(frame, dht_readdir_cbk, next_subvol, next_subvol,

-                          next_subvol->fops->readdir, local->fd, local->size,

-                          next_offset, NULL);

+        dht_queue_readdir(frame, next_subvol, next_offset, dht_readdir_cbk);

+

         return 0;

     }

@@ -7089,6 +7193,17 @@ unwind:

     if (prev != dht_last_up_subvol(this))

         op_errno = 0;

+    /* If we are inside a recursive call (or not inside a recursive call but

+     * the cbk is completed before the wind returns), local->queue will be 1.

+     * In this case we cannot destroy 'local' because it will be needed by

+     * the caller of STACK_WIND. In this case, we decrease the value to let

+     * the caller know that the operation has terminated and it must destroy

+     * 'local'. If local->queue 0, we can destroy it here because there are

+     * no other users. */

+    if (uatomic_sub_return(&local->queue, 1) >= 0) {

+        frame->local = NULL;

+    }

+

     if (!skip_hashed_check) {

         DHT_STACK_UNWIND(readdir, frame, op_ret, op_errno, &entries, NULL);

         gf_dirent_free(&entries);

@@ -7096,6 +7211,7 @@ unwind:

     } else {

         DHT_STACK_UNWIND(readdir, frame, op_ret, op_errno, orig_entries, NULL);

     }

+

     return 0;

 }

@@ -7172,11 +7288,9 @@ dht_do_readdir(call_frame_t *frame, xlator_t *this, fd_t *fd, size_t size,

             }

         }

-        STACK_WIND_COOKIE(frame, dht_readdirp_cbk, xvol, xvol,

-                          xvol->fops->readdirp, fd, size, yoff, local->xattr);

+        dht_queue_readdirp(frame, xvol, yoff, dht_readdirp_cbk);

     } else {

-        STACK_WIND_COOKIE(frame, dht_readdir_cbk, xvol, xvol,

-                          xvol->fops->readdir, fd, size, yoff, local->xattr);

+        dht_queue_readdir(frame, xvol, yoff, dht_readdir_cbk);

     }

     return 0;

diff --git a/xlators/cluster/dht/src/dht-common.h b/xlators/cluster/dht/src/dht-common.h

index 92f1b89..132b3b3 100644

--- a/xlators/cluster/dht/src/dht-common.h

+++ b/xlators/cluster/dht/src/dht-common.h

@@ -369,6 +369,11 @@ struct dht_local {

     dht_dir_transaction_t lock[2], *current;

+    /* for nested readdirs */

+    xlator_t *queue_xl;

+    off_t queue_offset;

+    int32_t queue;

+

     /* inodelks during filerename for backward compatibility */

     dht_lock_t **rename_inodelk_backward_compatible;

     int rename_inodelk_bc_count;

-- 

1.8.3.1