nfs: Fix inconsistency between glibc and libtirpc (#1698)

* nfs: Fix inconsistency between glibc and libtirpc

There's a critical difference between RPC implementation in glibc
and libtirpc.

In libtirpc, svc_run() starts a polling loop that listens from all
registered connections and handles requests. When this is done from
multiple threads, a race can happen where both threads are trying
to process the same connection simultaneously. This causes memory
corruption.

However, in glibc, svc_run() only handles the registered connections
on the current thread, so it's necessary to call svc_run() on all
threads that register services.

This patch fixes that problem by calling svc_run() from all threads
only if libtirpc is not used. Otherwise it's called only once.

Change-Id: I97c3c39a9aad90115e7e23c70884a4ee7769a2dd
Updates: #1009
Signed-off-by: Xavi Hernandez <xhernandez@redhat.com>

* nfs: Use an atomic instead of a mutex

Instead of using a mutex to synchronize the call to svc_run(), we
use an atomic operation to update the flag.

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

* nfs: Align fields of struct nfs_state

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

5 files changed, 52 insertions, 18 deletions

diff --git a/configure.ac b/configure.ac
index 2ce2a91137..2ad440335d 100644
--- a/configure.ac
+++ b/configure.ac
@@ -1174,6 +1174,10 @@ if test "x${with_libtirpc}" = "xyes" || test "x${with_ipv6_default}" = "xyes" ;
        [with_libtirpc="missing"; with_ipv6_default="no"])
 fi
 
+if test "x${with_libtirpc}" = "xyes" ; then
+    AC_DEFINE(HAVE_LIBTIRPC, 1, [Using libtirpc])
+fi
+
 if test "x${with_libtirpc}" = "xmissing" ; then
     AC_CHECK_HEADERS([rpc/rpc.h],[
         AC_MSG_WARN([
diff --git a/xlators/nfs/server/src/mount3udp_svc.c b/xlators/nfs/server/src/mount3udp_svc.c
index 1a2b0f8545..391f3e0ab9 100644
--- a/xlators/nfs/server/src/mount3udp_svc.c
+++ b/xlators/nfs/server/src/mount3udp_svc.c
@@ -231,8 +231,7 @@ mount3udp_thread(void *argv)
         return NULL;
     }
 
-    svc_run();
-    gf_msg(GF_MNT, GF_LOG_ERROR, 0, NFS_MSG_SVC_RUN_RETURNED,
-           "svc_run returned");
+    nfs_start_rpc_poller(nfsx->private);
+
     return NULL;
 }
diff --git a/xlators/nfs/server/src/nfs.c b/xlators/nfs/server/src/nfs.c
index 577dd94487..c01e5f01b7 100644
--- a/xlators/nfs/server/src/nfs.c
+++ b/xlators/nfs/server/src/nfs.c
@@ -1668,6 +1668,30 @@ out:
     return ret;
 }
 
+void
+nfs_start_rpc_poller(struct nfs_state *state)
+{
+/* RPC implementation in glibc uses per-thread global variables, while
+ * libtirpc uses global shared variables. This causes a big difference
+ * in svc_run():
+ *
+ *   - In glibc, svc_run() needs to be called in the same thread that
+ *     registered the service.
+ *
+ *   - In libtirpc, only one thread can call svc_run() and will serve
+ *     all registered services, from any thread.
+ */
+#ifdef HAVE_LIBTIRPC
+    if (uatomic_xchg(&state->svc_running, true)) {
+        return;
+    }
+#endif
+
+    svc_run();
+    gf_msg(GF_NLM, GF_LOG_ERROR, 0, NFS_MSG_SVC_RUN_RETURNED,
+           "svc_run returned");
+}
+
 int32_t
 nfs_itable_dump(xlator_t *this)
 {
diff --git a/xlators/nfs/server/src/nfs.h b/xlators/nfs/server/src/nfs.h
index e3daaed17a..46744ddaa4 100644
--- a/xlators/nfs/server/src/nfs.h
+++ b/xlators/nfs/server/src/nfs.h
@@ -60,19 +60,23 @@ struct nfs_initer_list {
 };
 
 struct nfs_state {
-    rpcsvc_t *rpcsvc;
     struct list_head versions;
+    gid_cache_t gid_cache;
+    gf_lock_t svinitlock;
+    rpcsvc_t *rpcsvc;
     struct mount3_state *mstate;
     struct nfs3_state *nfs3state;
     struct nlm4_state *nlm4state;
     struct mem_pool *foppool;
-    unsigned int memfactor;
     xlator_list_t *subvols;
-
-    gf_lock_t svinitlock;
+    xlator_t **initedxl;
+    char *rmtab;
+    struct rpc_clnt *rpc_clnt;
+    char *rpc_statd;
+    char *rpc_statd_pid_file;
+    unsigned int memfactor;
     int allsubvols;
     int upsubvols;
-    xlator_t **initedxl;
     int subvols_started;
     int dynamicvolumes;
     int enable_ino32;
@@ -90,17 +94,17 @@ struct nfs_state {
     unsigned int auth_refresh_time_secs;
     unsigned int auth_cache_ttl_sec;
 
-    char *rmtab;
-    struct rpc_clnt *rpc_clnt;
-    gf_boolean_t server_aux_gids;
     uint32_t server_aux_gids_max_age;
-    gid_cache_t gid_cache;
     uint32_t generation;
+    uint32_t event_threads;
+
+    gf_boolean_t server_aux_gids;
     gf_boolean_t register_portmap;
-    char *rpc_statd;
-    char *rpc_statd_pid_file;
     gf_boolean_t rdirplus;
-    uint32_t event_threads;
+
+#ifdef HAVE_LIBTIRPC
+    bool svc_running;
+#endif
 };
 
 struct nfs_inode_ctx {
@@ -151,4 +155,8 @@ nfs_subvolume_started(struct nfs_state *nfs, xlator_t *xl);
 
 extern void
 nfs_fix_groups(xlator_t *this, call_stack_t *root);
+
+void
+nfs_start_rpc_poller(struct nfs_state *state);
+
 #endif
diff --git a/xlators/nfs/server/src/nlmcbk_svc.c b/xlators/nfs/server/src/nlmcbk_svc.c
index eaa7b91619..9fe3b5001f 100644
--- a/xlators/nfs/server/src/nlmcbk_svc.c
+++ b/xlators/nfs/server/src/nlmcbk_svc.c
@@ -126,9 +126,8 @@ nsm_thread(void *argv)
         return NULL;
     }
 
-    svc_run();
-    gf_msg(GF_NLM, GF_LOG_ERROR, 0, NFS_MSG_SVC_RUN_RETURNED,
-           "svc_run returned");
+    nfs_start_rpc_poller(nfsx->private);
+
     return NULL;
     /* NOTREACHED */
 }