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.
>From 70101d80fd55ff54c381e204f1cc951dc671e94c 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 | 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
_______________________________________________
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
