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

Jakub Hrozek jhrozek at redhat.com
Mon Aug 13 16:13:24 UTC 2012


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.
-------------- next part --------------
>From 70101d80fd55ff54c381e204f1cc951dc671e94c 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

---
 Makefile.am             |  8 +++++-
 src/sss_client/common.c | 70 ++++++++++++++++++++++++++++++++++++++++++++-----
 2 files changed, 71 insertions(+), 7 deletions(-)

diff --git a/Makefile.am b/Makefile.am
index e89938ecaff3d40faa50ffafcf88ea3dda1bacca..e218b440628d08fa743eeba290b63d6dc9a14a81 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -738,6 +738,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 = -lpthread
 sss_sudo_cli_LDADD = \
     libsss_sudo.la
 endif
@@ -1121,7 +1122,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 -lpthread
 endif
 
 ####################
@@ -1143,6 +1144,7 @@ libnss_sss_la_SOURCES = \
     src/sss_client/nss_mc_group.c \
     src/sss_client/nss_mc.h
 libnss_sss_la_LDFLAGS = \
+    -lpthread \
     -module \
     -version-info 2:0:0 \
     -Wl,--version-script,$(srcdir)/src/sss_client/sss_nss.exports
@@ -1156,6 +1158,7 @@ pam_sss_la_SOURCES = \
     src/sss_client/sss_pam_macros.h
 
 pam_sss_la_LDFLAGS = \
+    -lpthread \
     -lpam \
     -module \
     -avoid-version \
@@ -1171,6 +1174,7 @@ libsss_sudo_la_SOURCES = \
     src/sss_client/sudo/sss_sudo.h \
     src/sss_client/sudo/sss_sudo_private.h
 libsss_sudo_la_LDFLAGS = \
+    -lpthread \
     -Wl,--version-script,$(srcdir)/src/sss_client/sss_sudo.exports \
     -version-info 2:0:1
 
@@ -1190,6 +1194,7 @@ libsss_autofs_la_SOURCES = \
     src/sss_client/autofs/sss_autofs_private.h
 
 libsss_autofs_la_LDFLAGS = \
+    -lpthread \
     -module \
     -avoid-version \
     -Wl,--version-script,$(srcdir)/src/sss_client/autofs/sss_autofs.exports
@@ -1508,6 +1513,7 @@ sssd_pac_plugin_la_CFLAGS = \
     $(AM_CFLAGS) \
     $(KRB5_CFLAGS)
 sssd_pac_plugin_la_LDFLAGS = \
+    -lpthread \
     -lkrb5 \
     -avoid-version \
     -module
diff --git a/src/sss_client/common.c b/src/sss_client/common.c
index 59eae3b1fe4101383de96f680458467a962c2248..357b60aab69f31bea7934ee4349a4e48f42ce477 100644
--- a/src/sss_client/common.c
+++ b/src/sss_client/common.c
@@ -976,24 +976,82 @@ 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 };
+
+/* Generic mutex init, lock, unlock functions */
+void sss_mt_init(struct sss_mutex *m)
+{
+    pthread_mutexattr_t attr;
+
+    if (pthread_mutexattr_init(&attr) != 0) {
+        return;
+    }
+    if (pthread_mutexattr_setrobust(&attr, PTHREAD_MUTEX_ROBUST) != 0) {
+        return;
+    }
+
+    pthread_mutex_init(&m->mtx, &attr);
+    pthread_mutexattr_destroy(&attr);
+}
+
+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();
+        pthread_mutex_consistent(&m->mtx);
+    }
+}
+
+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