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.
>From 9a9834cb949abeebc6308cd6d2e92704cbd52af1 Mon Sep 17 00:00:00 2001
From: Jakub Hrozek <jhro...@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 | 68 ++++++++++++++++++++++++++++++++++++++++++++-----
2 files changed, 69 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..4d1f33f14f6dd7a9f295c10b974d7858b38fcaf3
100644
--- a/src/sss_client/common.c
+++ b/src/sss_client/common.c
@@ -976,24 +976,80 @@ 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_mutexattr_t attr;
+ 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)
+{
+ if (pthread_mutexattr_init(&m->attr) != 0) {
+ return;
+ }
+ if (pthread_mutexattr_setrobust(&m->attr, PTHREAD_MUTEX_ROBUST) != 0) {
+ return;
+ }
+ if (pthread_mutex_init(&m->mtx, &m->attr) != 0) {
+ return;
+ }
+}
+
+void sss_mt_lock(struct sss_mutex *m)
+{
+ pthread_once(&m->once, m->init);
+ if (pthread_mutex_lock(&m->mtx) == EOWNERDEAD) {
+ 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
_______________________________________________
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
