[SSSD] [PATCH] Release mutex if a thread gets cancelled

Jakub Hrozek jhrozek at redhat.com
Tue Aug 21 10:02:43 UTC 2012


On Thu, Aug 16, 2012 at 11:44:35AM +0200, Sumit Bose wrote:
> On Wed, Aug 15, 2012 at 02:07:12PM +0200, Sumit Bose wrote:
> > On Mon, Aug 13, 2012 at 06:13:24PM +0200, Jakub Hrozek wrote:
> > > On Mon, Aug 13, 2012 at 04:40:03PM +0200, Jakub Hrozek wrote:
> > > > On Mon, Aug 13, 2012 at 01:30:21PM +0200, Sumit Bose wrote:
> > > > > On Fri, Aug 10, 2012 at 06:40:35PM +0200, Jakub Hrozek wrote:
> > > > > > https://fedorahosted.org/sssd/ticket/1460
> > > > > > 
> > > > > > Please see the commit. I'm wondering if there is still a (small) race
> > > > > > condition between the call to pthread_cleanup_pop() and unlocking the
> > > > > > mutex. Would it be better to i.e. always call the cleanup handler with
> > > > > > pthread_cleanup_pop(1) and disconnect from the fd based on some other
> > > > > > condition?
> > > > > 
> > > > > this patch works as expected for me on F17. I have some issues on F16,
> > > > > but this might have other reason I still try to investigate. While
> > > > > reading a bit about pthread I can across robust mutexes
> > > > > http://pubs.opengroup.org/onlinepubs/9699919799/functions/pthread_mutexattr_getrobust.html
> > > > > I wonder if we can better avoid races with those and maybe even make the
> > > > > implementation cleaner? E.g. if we get EOWNERDEAD we can call
> > > > > sss_cli_close_socket(); pthread_mutex_consistent(); run the request
> > > > > which finally will call pthread_mutex_unlock().
> > > > > 
> > > > 
> > > > As discussed briefly on IRC, I agree. I like any approach that removes
> > > > the pthread_cleanup_push/pop hack.
> > > > 
> > > > > There are also pthread_mutexattr_getrobust_np() and
> > > > > pthread_mutex_consistent_np() in glibc. I do not have an idea how they
> > > > > differ and which one should be preferred. But the *_np version only need
> > > > > __USE_GNU while the others need __USE_XOPEN2K8.
> > > > > 
> > > > > bye,
> > > > > Sumit
> > > > 
> > > > RHEL5 only contains the _np versions, but they still seem to work. If
> > > > the approach is deemed OK, I'll detect the available versions during
> > > > configure.
> > > > 
> > > > Traditionally the _np suffix means "non-portable GNU extension".
> > > > 
> > > > Please see the attached patch. Would that work for you? If so, I'll
> > > > extend it to work on all supported RHEL releases.
> > > 
> > > Sumit nacked the patch on IRC. I was missing the disconnect logic when
> > > lock returns EOWNERDEAD. I also moved the mutexattr to sss_mt_init, I
> > > don't think there is any reason to keep it separate. It doesn't have to
> > > live as long as the mutex itself.
> > 
> > This patch works as expected on F17, so ACK.
> > 
> > I still have some strange behaviour on F16 and strangely in the case
> > where the fast cache files in the /var/lib/sss/mc directory are
> > available. Here the reproducer get stuck in a futex() call with or
> > without the patch. I looks that glibc itself also uses a mutex here
> > which is neither robust nor unlocked if the tread holding it is killed.
> > I would be nice if someone with a F16 system can verify this.
> > 
> 
> I forgot, just as a reminder, some code must be added to allow building
> on RHEL5.
> 
> bye,
> Sumit

Attached is a patch that uses the _np variants on RHEL5. I've tested it
on F-17, RHEL-5 (where I had to create the SELinux login directory b/c
of https://fedorahosted.org/sssd/ticket/1492) and RHEL-6. All 64bit.

I haven't tested F-16 yet, but I'll spin up a VM.
> _______________________________________________
> sssd-devel mailing list
> sssd-devel at lists.fedorahosted.org
> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
-------------- next part --------------
>From 5d8036eecbe6f4ae15fbcfcbe98247c642682fa3 Mon Sep 17 00:00:00 2001
From: Jakub Hrozek <jhrozek at redhat.com>
Date: Mon, 13 Aug 2012 15:30:39 +0200
Subject: [PATCH] Use PTHREAD_MUTEX_ROBUST to avoid deadlock in the client

https://fedorahosted.org/sssd/ticket/1460
---
 Makefile.am             | 12 ++++++-
 configure.ac            | 15 +++++++-
 src/sss_client/common.c | 96 +++++++++++++++++++++++++++++++++++++++++++++----
 3 files changed, 115 insertions(+), 8 deletions(-)

diff --git a/Makefile.am b/Makefile.am
index 726a8589f11587b2632098561a5396454994136d..dfd9d374d466a45e84214f442a3f871bc0a34b21 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -455,6 +455,10 @@ if BUILD_SUDO
 	SSSD_DOCS += libsss_sudo_doc
 endif
 
+if HAVE_PTHREAD
+CLIENT_LIBS = -lpthread
+endif
+
 #####################
 # Utility libraries #
 #####################
@@ -741,6 +745,7 @@ sss_sudo_cli_SOURCES = \
     src/sss_client/common.c \
     src/sss_client/sudo_testcli/sudo_testcli.c
 sss_sudo_cli_CFLAGS = $(AM_CFLAGS)
+sss_sudo_cli_LDFLAGS = $(CLIENT_LIBS)
 sss_sudo_cli_LDADD = \
     libsss_sudo.la
 endif
@@ -1124,7 +1129,7 @@ autofs_test_client_SOURCES = src/sss_client/autofs/autofs_test_client.c \
 			     src/sss_client/autofs/sss_autofs.c \
 			     src/sss_client/common.c
 autofs_test_client_CFLAGS = $(AM_CFLAGS)
-autofs_test_client_LDFLAGS = -lpopt
+autofs_test_client_LDFLAGS = -lpopt $(CLIENT_LIBS)
 endif
 
 ####################
@@ -1146,6 +1151,7 @@ libnss_sss_la_SOURCES = \
     src/sss_client/nss_mc_group.c \
     src/sss_client/nss_mc.h
 libnss_sss_la_LDFLAGS = \
+    $(CLIENT_LIBS) \
     -module \
     -version-info 2:0:0 \
     -Wl,--version-script,$(srcdir)/src/sss_client/sss_nss.exports
@@ -1159,6 +1165,7 @@ pam_sss_la_SOURCES = \
     src/sss_client/sss_pam_macros.h
 
 pam_sss_la_LDFLAGS = \
+    $(CLIENT_LIBS) \
     -lpam \
     -module \
     -avoid-version \
@@ -1174,6 +1181,7 @@ libsss_sudo_la_SOURCES = \
     src/sss_client/sudo/sss_sudo.h \
     src/sss_client/sudo/sss_sudo_private.h
 libsss_sudo_la_LDFLAGS = \
+    $(CLIENT_LIBS) \
     -Wl,--version-script,$(srcdir)/src/sss_client/sss_sudo.exports \
     -version-info 2:0:1
 
@@ -1193,6 +1201,7 @@ libsss_autofs_la_SOURCES = \
     src/sss_client/autofs/sss_autofs_private.h
 
 libsss_autofs_la_LDFLAGS = \
+    $(CLIENT_LIBS) \
     -module \
     -avoid-version \
     -Wl,--version-script,$(srcdir)/src/sss_client/autofs/sss_autofs.exports
@@ -1511,6 +1520,7 @@ sssd_pac_plugin_la_CFLAGS = \
     $(AM_CFLAGS) \
     $(KRB5_CFLAGS)
 sssd_pac_plugin_la_LDFLAGS = \
+    $(CLIENT_LIBS) \
     -lkrb5 \
     -avoid-version \
     -module
diff --git a/configure.ac b/configure.ac
index 5bb82a77d292c6ee67c12b2ba32c94a6eaa1ffc5..5cd8c723f8b9d24aa00631729c6e9b4f5e5efedd 100644
--- a/configure.ac
+++ b/configure.ac
@@ -47,9 +47,22 @@ BUILD_WITH_SHARED_BUILD_DIR
 AC_COMPILE_IFELSE(
     [AC_LANG_PROGRAM([[#include <pthread.h>]],
         [[pthread_mutex_t m = PTHREAD_MUTEX_INITIALIZER;]])],
-    [AC_DEFINE([HAVE_PTHREAD], [1], [Pthread mutexes available.])],
+    [AC_DEFINE([HAVE_PTHREAD], [1], [Pthread mutexes available.])
+     HAVE_PTHREAD=1
+    ],
     [AC_MSG_WARN([Pthread library not found! Clients will not be thread safe...])])
 
+
+AM_CONDITIONAL([HAVE_PTHREAD], [test x"$HAVE_PTHREAD" != "x"])
+
+SAVE_LIBS=$LIBS
+LIBS="$LIBS -lpthread"
+AC_CHECK_FUNCS([ pthread_mutexattr_setrobust \
+                 pthread_mutex_consistent \
+                 pthread_mutexattr_setrobust_np \
+                 pthread_mutex_consistent_np ])
+LIBS=$SAVE_LIBS
+
 #Check for PAM headers
 AC_CHECK_HEADERS([security/pam_appl.h security/pam_misc.h security/pam_modules.h],
     [AC_CHECK_LIB(pam, pam_get_item, [ PAM_LIBS="-lpam" ], [AC_MSG_ERROR([PAM must support pam_get_item])])],
diff --git a/src/sss_client/common.c b/src/sss_client/common.c
index 59eae3b1fe4101383de96f680458467a962c2248..5e0d67d9b7a683c44705a6e4d1807fb5bfd251da 100644
--- a/src/sss_client/common.c
+++ b/src/sss_client/common.c
@@ -976,24 +976,108 @@ errno_t sss_strnlen(const char *str, size_t maxlen, size_t *len)
 }
 
 #if HAVE_PTHREAD
-static pthread_mutex_t sss_nss_mutex = PTHREAD_MUTEX_INITIALIZER;
-static pthread_mutex_t sss_pam_mutex = PTHREAD_MUTEX_INITIALIZER;
+typedef void (*sss_mutex_init)(void);
 
+struct sss_mutex {
+    pthread_mutex_t mtx;
+
+    pthread_once_t once;
+    sss_mutex_init init;
+};
+
+static void sss_nss_mt_init(void);
+static void sss_pam_mt_init(void);
+
+static struct sss_mutex nss_mtx = { .mtx  = PTHREAD_MUTEX_INITIALIZER,
+                                    .once = PTHREAD_ONCE_INIT,
+                                    .init = sss_nss_mt_init };
+
+static struct sss_mutex pam_mtx = { .mtx  = PTHREAD_MUTEX_INITIALIZER,
+                                    .once = PTHREAD_ONCE_INIT,
+                                    .init = sss_pam_mt_init };
+
+
+/* Wrappers for robust mutex support */
+static int sss_mutexattr_setrobust (pthread_mutexattr_t *attr)
+{
+#ifdef HAVE_PTHREAD_MUTEXATTR_SETROBUST
+    return pthread_mutexattr_setrobust(attr, PTHREAD_MUTEX_ROBUST);
+#elif  HAVE_PTHREAD_MUTEXATTR_SETROBUST_NP
+    return pthread_mutexattr_setrobust_np(attr, PTHREAD_MUTEX_ROBUST_NP);
+#else
+#warning Robust mutexes are not supported on this platform.
+    return 0;
+#endif
+}
+
+static int sss_mutex_consistent(pthread_mutex_t *mtx)
+{
+#ifdef HAVE_PTHREAD_MUTEX_CONSISTENT
+    return pthread_mutex_consistent(mtx);
+#elif HAVE_PTHREAD_MUTEX_CONSISTENT_NP
+    return pthread_mutex_consistent_np(mtx);
+#else
+#warning Robust mutexes are not supported on this platform.
+    return 0;
+#endif
+}
+
+/* Generic mutex init, lock, unlock functions */
+static void sss_mt_init(struct sss_mutex *m)
+{
+    pthread_mutexattr_t attr;
+
+    if (pthread_mutexattr_init(&attr) != 0) {
+        return;
+    }
+    if (sss_mutexattr_setrobust(&attr) != 0) {
+        return;
+    }
+
+    pthread_mutex_init(&m->mtx, &attr);
+    pthread_mutexattr_destroy(&attr);
+}
+
+static void sss_mt_lock(struct sss_mutex *m)
+{
+    pthread_once(&m->once, m->init);
+    if (pthread_mutex_lock(&m->mtx) == EOWNERDEAD) {
+        sss_cli_close_socket();
+        sss_mutex_consistent(&m->mtx);
+    }
+}
+
+static void sss_mt_unlock(struct sss_mutex *m)
+{
+    pthread_mutex_unlock(&m->mtx);
+}
+
+/* NSS mutex wrappers */
+static void sss_nss_mt_init(void)
+{
+    sss_mt_init(&nss_mtx);
+}
 void sss_nss_lock(void)
 {
-    pthread_mutex_lock(&sss_nss_mutex);
+    sss_mt_lock(&nss_mtx);
 }
 void sss_nss_unlock(void)
 {
-    pthread_mutex_unlock(&sss_nss_mutex);
+    sss_mt_unlock(&nss_mtx);
+}
+
+/* NSS mutex wrappers */
+static void sss_pam_mt_init(void)
+{
+    sss_mt_init(&pam_mtx);
 }
 void sss_pam_lock(void)
 {
-    pthread_mutex_lock(&sss_pam_mutex);
+    sss_mt_lock(&pam_mtx);
 }
 void sss_pam_unlock(void)
 {
-    pthread_mutex_unlock(&sss_pam_mutex);
+    sss_mt_unlock(&pam_mtx);
 }
 
 #else
-- 
1.7.11.2



More information about the sssd-devel mailing list