Adding support for multi-threaded gssd and allow for parallel upcalls.
For each upcall create a thread, detach to allow for automatic cleanup.
Communicate location of credential cache with gss api function instead
of an environmental variable. Use syscall api instead of libc setuid()
to change user identity.
Previously parent thread after detaching from child thread would call
yield to let the child thread run and consume the event from the fd.
Instead, use synchronization variables to signal parent after reading
from fd and have parent thread wait for the event.
Olga Kornievskaia (3):
gssd: use pthreads to handle upcalls
gssd: using syscalls directly to change thread's identity
gssd: always call gss_krb5_ccache_name
aclocal/libpthread.m4 | 13 ++++++++++
configure.ac | 3 +++
utils/gssd/Makefile.am | 3 ++-
utils/gssd/gssd.c | 44 ++++++++++++++++++++++++++++++---
utils/gssd/gssd.h | 5 ++++
utils/gssd/gssd_proc.c | 66 +++++++++++++++++++++++---------------------------
utils/gssd/krb5_util.c | 59 ++++++++++----------------------------------
utils/gssd/krb5_util.h | 1 -
8 files changed, 106 insertions(+), 88 deletions(-)
create mode 100644 aclocal/libpthread.m4
--
1.8.3.1
Currently, to persevere global data over multiple mounts,
the root process does not fork when handling an upcall.
Instead on not-forking create a pthread to handle the
upcall since global data can be shared among threads.
Signed-off-by: Steve Dickson <[email protected]>
Signed-off-by: Olga Kornievskaia <[email protected]>
---
aclocal/libpthread.m4 | 13 +++++++++++++
configure.ac | 3 +++
utils/gssd/Makefile.am | 3 ++-
utils/gssd/gssd.c | 45 ++++++++++++++++++++++++++++++++++++++++-----
utils/gssd/gssd.h | 5 +++++
utils/gssd/gssd_proc.c | 49 ++++++++++++++++++-------------------------------
utils/gssd/krb5_util.c | 3 +++
7 files changed, 84 insertions(+), 37 deletions(-)
create mode 100644 aclocal/libpthread.m4
diff --git a/aclocal/libpthread.m4 b/aclocal/libpthread.m4
new file mode 100644
index 0000000..e87d2a0
--- /dev/null
+++ b/aclocal/libpthread.m4
@@ -0,0 +1,13 @@
+dnl Checks for pthreads library and headers
+dnl
+AC_DEFUN([AC_LIBPTHREAD], [
+
+ dnl Check for library, but do not add -lpthreads to LIBS
+ AC_CHECK_LIB([pthread], [pthread_create], [LIBPTHREAD=-lpthread],
+ [AC_MSG_ERROR([libpthread not found.])])
+ AC_SUBST(LIBPTHREAD)
+
+ AC_CHECK_HEADERS([pthread.h], ,
+ [AC_MSG_ERROR([libpthread headers not found.])])
+
+])dnl
diff --git a/configure.ac b/configure.ac
index 25d2ba4..e0ecd61 100644
--- a/configure.ac
+++ b/configure.ac
@@ -385,6 +385,9 @@ if test "$enable_gss" = yes; then
dnl Invoked after AC_KERBEROS_V5; AC_LIBRPCSECGSS needs to have KRBLIBS set
AC_LIBRPCSECGSS
+ dnl Check for pthreads
+ AC_LIBPTHREAD
+
dnl librpcsecgss already has a dependency on libgssapi,
dnl but we need to make sure we get the right version
if test "$enable_gss" = yes; then
diff --git a/utils/gssd/Makefile.am b/utils/gssd/Makefile.am
index cb040b3..3f5f59a 100644
--- a/utils/gssd/Makefile.am
+++ b/utils/gssd/Makefile.am
@@ -49,7 +49,8 @@ gssd_LDADD = \
$(RPCSECGSS_LIBS) \
$(KRBLIBS) \
$(GSSAPI_LIBS) \
- $(LIBTIRPC)
+ $(LIBTIRPC) \
+ $(LIBPTHREAD)
gssd_LDFLAGS = \
$(KRBLDFLAGS)
diff --git a/utils/gssd/gssd.c b/utils/gssd/gssd.c
index e7cb07f..0b7ffca 100644
--- a/utils/gssd/gssd.c
+++ b/utils/gssd/gssd.c
@@ -87,7 +87,9 @@ unsigned int rpc_timeout = 5;
char *preferred_realm = NULL;
/* Avoid DNS reverse lookups on server names */
static bool avoid_dns = true;
-
+int thread_started = false;
+pthread_mutex_t pmutex = PTHREAD_MUTEX_INITIALIZER;
+pthread_cond_t pcond = PTHREAD_COND_INITIALIZER;
TAILQ_HEAD(topdir_list_head, topdir) topdir_list;
@@ -361,20 +363,53 @@ gssd_destroy_client(struct clnt_info *clp)
static void gssd_scan(void);
+static void wait_for_child_and_detach(pthread_t th)
+{
+ pthread_mutex_lock(&pmutex);
+ while (!thread_started)
+ pthread_cond_wait(&pcond, &pmutex);
+ thread_started = false;
+ pthread_mutex_unlock(&pmutex);
+ pthread_detach(th);
+}
+
+/* For each upcall create a thread, detach from the main process so that
+ * resources are released back into the system without the need for a join.
+ * We need to wait for the child thread to start and consume the event from
+ * the file descriptor.
+ */
static void
gssd_clnt_gssd_cb(int UNUSED(fd), short UNUSED(which), void *data)
{
struct clnt_info *clp = data;
-
- handle_gssd_upcall(clp);
+ pthread_t th;
+ int ret;
+
+ ret = pthread_create(&th, NULL, (void *)handle_gssd_upcall,
+ (void *)clp);
+ if (ret != 0) {
+ printerr(0, "ERROR: pthread_create failed: ret %d: %s\n",
+ ret, strerror(errno));
+ return;
+ }
+ wait_for_child_and_detach(th);
}
static void
gssd_clnt_krb5_cb(int UNUSED(fd), short UNUSED(which), void *data)
{
struct clnt_info *clp = data;
-
- handle_krb5_upcall(clp);
+ pthread_t th;
+ int ret;
+
+ ret = pthread_create(&th, NULL, (void *)handle_krb5_upcall,
+ (void *)clp);
+ if (ret != 0) {
+ printerr(0, "ERROR: pthread_create failed: ret %d: %s\n",
+ ret, strerror(errno));
+ return;
+ }
+ wait_for_child_and_detach(th);
}
static struct clnt_info *
diff --git a/utils/gssd/gssd.h b/utils/gssd/gssd.h
index c6937c5..565bce3 100644
--- a/utils/gssd/gssd.h
+++ b/utils/gssd/gssd.h
@@ -36,6 +36,7 @@
#include <gssapi/gssapi.h>
#include <event.h>
#include <stdbool.h>
+#include <pthread.h>
#ifndef GSSD_PIPEFS_DIR
#define GSSD_PIPEFS_DIR "/var/lib/nfs/rpc_pipefs"
@@ -61,6 +62,10 @@ extern int root_uses_machine_creds;
extern unsigned int context_timeout;
extern unsigned int rpc_timeout;
extern char *preferred_realm;
+extern pthread_mutex_t ple_lock;
+extern pthread_cond_t pcond;
+extern pthread_mutex_t pmutex;
+extern int thread_started;
struct clnt_info {
TAILQ_ENTRY(clnt_info) list;
diff --git a/utils/gssd/gssd_proc.c b/utils/gssd/gssd_proc.c
index 1ef68d8..e2e95dc 100644
--- a/utils/gssd/gssd_proc.c
+++ b/utils/gssd/gssd_proc.c
@@ -629,36 +629,6 @@ process_krb5_upcall(struct clnt_info *clp, uid_t uid, int fd, char *tgtname,
if (uid != 0 || (uid == 0 && root_uses_machine_creds == 0 &&
service == NULL)) {
- /* already running as uid 0 */
- if (uid == 0)
- goto no_fork;
-
- pid = fork();
- switch(pid) {
- case 0:
- /* Child: fall through to rest of function */
- childpid = getpid();
- unsetenv("KRB5CCNAME");
- printerr(2, "CHILD forked pid %d \n", childpid);
- break;
- case -1:
- /* fork() failed! */
- printerr(0, "WARNING: unable to fork() to handle"
- "upcall: %s\n", strerror(errno));
- return;
- default:
- /* Parent: just wait on child to exit and return */
- do {
- pid = wait(&err);
- } while(pid == -1 && errno != -ECHILD);
-
- if (WIFSIGNALED(err))
- printerr(0, "WARNING: forked child was killed"
- "with signal %d\n", WTERMSIG(err));
- return;
- }
-no_fork:
-
auth = krb5_not_machine_creds(clp, uid, tgtname, &downcall_err,
&err, &rpc_clnt);
if (err)
@@ -735,12 +705,28 @@ out_return_error:
goto out;
}
+/* signal to the parent thread that we have read from the file descriptor.
+ * it should allow the parent to proceed to poll on the descriptor for
+ * the next upcall from the kernel.
+ */
+static void
+signal_parent_event_consumed(void)
+{
+ pthread_mutex_lock(&pmutex);
+ thread_started = true;
+ pthread_cond_signal(&pcond);
+ pthread_mutex_unlock(&pmutex);
+}
+
void
handle_krb5_upcall(struct clnt_info *clp)
{
uid_t uid;
+ int status;
- if (read(clp->krb5_fd, &uid, sizeof(uid)) < (ssize_t)sizeof(uid)) {
+ status = read(clp->krb5_fd, &uid, sizeof(uid)) < (ssize_t)sizeof(uid);
+ signal_parent_event_consumed();
+ if (status) {
printerr(0, "WARNING: failed reading uid from krb5 "
"upcall pipe: %s\n", strerror(errno));
return;
@@ -765,6 +751,7 @@ handle_gssd_upcall(struct clnt_info *clp)
char *enctypes = NULL;
lbuflen = read(clp->gssd_fd, lbuf, sizeof(lbuf));
+ signal_parent_event_consumed();
if (lbuflen <= 0 || lbuf[lbuflen-1] != '\n') {
printerr(0, "WARNING: handle_gssd_upcall: "
"failed reading request\n");
diff --git a/utils/gssd/krb5_util.c b/utils/gssd/krb5_util.c
index 8ef8184..3328696 100644
--- a/utils/gssd/krb5_util.c
+++ b/utils/gssd/krb5_util.c
@@ -128,6 +128,7 @@
/* Global list of principals/cache file names for machine credentials */
struct gssd_k5_kt_princ *gssd_k5_kt_princ_list = NULL;
+pthread_mutex_t ple_lock = PTHREAD_MUTEX_INITIALIZER;
#ifdef HAVE_SET_ALLOWABLE_ENCTYPES
int limit_to_legacy_enctypes = 0;
@@ -586,10 +587,12 @@ get_ple_by_princ(krb5_context context, krb5_principal princ)
/* Need to serialize list if we ever become multi-threaded! */
+ pthread_mutex_lock(&ple_lock);
ple = find_ple_by_princ(context, princ);
if (ple == NULL) {
ple = new_ple(context, princ);
}
+ pthread_mutex_unlock(&ple_lock);
return ple;
}
--
1.8.3.1
Previously the location of the credential cache was passed in either
using environment variable KRB5CCNAME or gss_krb5_ccache_name() if
supported. For threaded-gssd, we can't use an environment variable
as it's shared among all thread. Thus always use the api call.
Signed-off-by: Olga Kornievskaia <[email protected]>
Reviewed-by: Steve Dickson <[email protected]>
---
utils/gssd/gssd_proc.c | 12 +++++++++--
utils/gssd/krb5_util.c | 56 +++++++++-----------------------------------------
utils/gssd/krb5_util.h | 1 -
3 files changed, 20 insertions(+), 49 deletions(-)
diff --git a/utils/gssd/gssd_proc.c b/utils/gssd/gssd_proc.c
index 487a4f5..c9ba399 100644
--- a/utils/gssd/gssd_proc.c
+++ b/utils/gssd/gssd_proc.c
@@ -554,7 +554,15 @@ krb5_use_machine_creds(struct clnt_info *clp, uid_t uid, char *tgtname,
goto out;
}
for (ccname = credlist; ccname && *ccname; ccname++) {
- gssd_setup_krb5_machine_gss_ccache(*ccname);
+ u_int min_stat;
+
+ if (gss_krb5_ccache_name(&min_stat, *ccname, NULL) !=
+ GSS_S_COMPLETE) {
+ printerr(1, "WARNING: gss_krb5_ccache_name "
+ "with name '%s' failed (%s)\n",
+ *ccname, error_message(min_stat));
+ continue;
+ }
if ((create_auth_rpc_client(clp, tgtname, rpc_clnt,
&auth, uid,
AUTHTYPE_KRB5,
@@ -575,7 +583,7 @@ krb5_use_machine_creds(struct clnt_info *clp, uid_t uid, char *tgtname,
"recreate cache for server %s\n",
clp->servername);
} else {
- printerr(1, "WARNING: Failed to create machine"
+ printerr(0, "ERROR: Failed to create machine"
"krb5 context with any credentials"
"cache for server %s\n",
clp->servername);
diff --git a/utils/gssd/krb5_util.c b/utils/gssd/krb5_util.c
index 3328696..7b74ab3 100644
--- a/utils/gssd/krb5_util.c
+++ b/utils/gssd/krb5_util.c
@@ -468,37 +468,6 @@ gssd_get_single_krb5_cred(krb5_context context,
}
/*
- * Depending on the version of Kerberos, we either need to use
- * a private function, or simply set the environment variable.
- */
-static void
-gssd_set_krb5_ccache_name(char *ccname)
-{
-#ifdef USE_GSS_KRB5_CCACHE_NAME
- u_int maj_stat, min_stat;
-
- printerr(3, "using gss_krb5_ccache_name to select krb5 ccache %s\n",
- ccname);
- maj_stat = gss_krb5_ccache_name(&min_stat, ccname, NULL);
- if (maj_stat != GSS_S_COMPLETE) {
- printerr(0, "WARNING: gss_krb5_ccache_name with "
- "name '%s' failed (%s)\n",
- ccname, error_message(min_stat));
- }
-#else
- /*
- * Set the KRB5CCNAME environment variable to tell the krb5 code
- * which credentials cache to use. (Instead of using the private
- * function above for which there is no generic gssapi
- * equivalent.)
- */
- printerr(3, "using environment variable to select krb5 ccache %s\n",
- ccname);
- setenv("KRB5CCNAME", ccname, 1);
-#endif
-}
-
-/*
* Given a principal, find a matching ple structure
*/
static struct gssd_k5_kt_princ *
@@ -1094,6 +1063,7 @@ gssd_setup_krb5_user_gss_ccache(uid_t uid, char *servername, char *dirpattern)
const char *cctype;
struct dirent *d;
int err, i, j;
+ u_int maj_stat, min_stat;
printerr(3, "looking for client creds with uid %u for "
"server %s in %s\n", uid, servername, dirpattern);
@@ -1129,22 +1099,16 @@ gssd_setup_krb5_user_gss_ccache(uid_t uid, char *servername, char *dirpattern)
printerr(2, "using %s as credentials cache for client with "
"uid %u for server %s\n", buf, uid, servername);
- gssd_set_krb5_ccache_name(buf);
- return 0;
-}
-/*
- * Let the gss code know where to find the machine credentials ccache.
- *
- * Returns:
- * void
- */
-void
-gssd_setup_krb5_machine_gss_ccache(char *ccname)
-{
- printerr(2, "using %s as credentials cache for machine creds\n",
- ccname);
- gssd_set_krb5_ccache_name(ccname);
+ printerr(3, "using gss_krb5_ccache_name to select krb5 ccache %s\n",
+ buf);
+ maj_stat = gss_krb5_ccache_name(&min_stat, buf, NULL);
+ if (maj_stat != GSS_S_COMPLETE) {
+ printerr(0, "ERROR: unable to get user cred cache '%s' "
+ "failed (%s)\n", buf, error_message(min_stat));
+ return maj_stat;
+ }
+ return 0;
}
/*
diff --git a/utils/gssd/krb5_util.h b/utils/gssd/krb5_util.h
index a319588..d3b0777 100644
--- a/utils/gssd/krb5_util.h
+++ b/utils/gssd/krb5_util.h
@@ -27,7 +27,6 @@ int gssd_setup_krb5_user_gss_ccache(uid_t uid, char *servername,
char *dirname);
int gssd_get_krb5_machine_cred_list(char ***list);
void gssd_free_krb5_machine_cred_list(char **list);
-void gssd_setup_krb5_machine_gss_ccache(char *servername);
void gssd_destroy_krb5_machine_creds(void);
int gssd_refresh_krb5_machine_credential(char *hostname,
struct gssd_k5_kt_princ *ple,
--
1.8.3.1
For the threaded version we have to set uid,gid per thread instead
of per process. glibc setresuid() when called from a thread, it'll
send a signal to all other threads to synchronize the uid in all
other threads. To bypass this, we have to call syscall() directly.
Signed-off-by: Olga Kornievskaia <[email protected]>
Reviewed-by: Steve Dickson <[email protected]>
---
utils/gssd/gssd_proc.c | 12 +++++++++---
1 file changed, 9 insertions(+), 3 deletions(-)
diff --git a/utils/gssd/gssd_proc.c b/utils/gssd/gssd_proc.c
index e2e95dc..487a4f5 100644
--- a/utils/gssd/gssd_proc.c
+++ b/utils/gssd/gssd_proc.c
@@ -69,6 +69,7 @@
#include <netdb.h>
#include <sys/types.h>
#include <sys/wait.h>
+#include <syscall.h>
#include "gssd.h"
#include "err_util.h"
@@ -436,7 +437,7 @@ change_identity(uid_t uid)
struct passwd *pw;
/* drop list of supplimentary groups first */
- if (setgroups(0, NULL) != 0) {
+ if (syscall(SYS_setgroups, 0) != 0) {
printerr(0, "WARNING: unable to drop supplimentary groups!");
return errno;
}
@@ -457,7 +458,12 @@ change_identity(uid_t uid)
* Switch the GIDs. Note that we leave the saved-set-gid alone in an
* attempt to prevent attacks via ptrace()
*/
- if (setresgid(pw->pw_gid, pw->pw_gid, -1) != 0) {
+ /* For the threaded version we have to set uid,gid per thread instead
+ * of per process. glibc setresuid() when called from a thread, it'll
+ * send a signal to all other threads to synchronize the uid in all
+ * other threads. To bypass this, we have to call syscall() directly.
+ */
+ if (syscall(SYS_setresgid, pw->pw_gid) != 0) {
printerr(0, "WARNING: failed to set gid to %u!\n", pw->pw_gid);
return errno;
}
@@ -466,7 +472,7 @@ change_identity(uid_t uid)
* Switch UIDs, but leave saved-set-uid alone to prevent ptrace() by
* other processes running with this uid.
*/
- if (setresuid(uid, uid, -1) != 0) {
+ if (syscall(SYS_setresuid, uid) != 0) {
printerr(0, "WARNING: Failed to setuid for user with uid %u\n",
uid);
return errno;
--
1.8.3.1
On Mon, 2016-04-25 at 12:58 -0400, Olga Kornievskaia wrote:
> For the threaded version we have to set uid,gid per thread instead
> of per process. glibc setresuid() when called from a thread, it'll
> send a signal to all other threads to synchronize the uid in all
> other threads. To bypass this, we have to call syscall() directly.
>
> Signed-off-by: Olga Kornievskaia <[email protected]>
> Reviewed-by: Steve Dickson <[email protected]>
> ---
> utils/gssd/gssd_proc.c | 12 +++++++++---
> 1 file changed, 9 insertions(+), 3 deletions(-)
>
> diff --git a/utils/gssd/gssd_proc.c b/utils/gssd/gssd_proc.c
> index e2e95dc..487a4f5 100644
> --- a/utils/gssd/gssd_proc.c
> +++ b/utils/gssd/gssd_proc.c
> @@ -69,6 +69,7 @@
> #include
> #include
> #include
> +#include
>
> #include "gssd.h"
> #include "err_util.h"
> @@ -436,7 +437,7 @@ change_identity(uid_t uid)
> struct passwd *pw;
>
> /* drop list of supplimentary groups first */
> - if (setgroups(0, NULL) != 0) {
> + if (syscall(SYS_setgroups, 0) != 0) {
> printerr(0, "WARNING: unable to drop supplimentary groups!");
> return errno;
> }
> @@ -457,7 +458,12 @@ change_identity(uid_t uid)
> * Switch the GIDs. Note that we leave the saved-set-gid alone in an
> * attempt to prevent attacks via ptrace()
> */
> - if (setresgid(pw->pw_gid, pw->pw_gid, -1) != 0) {
> + /* For the threaded version we have to set uid,gid per thread instead
> + * of per process. glibc setresuid() when called from a thread, it'll
> + * send a signal to all other threads to synchronize the uid in all
> + * other threads. To bypass this, we have to call syscall() directly.
> + */
> + if (syscall(SYS_setresgid, pw->pw_gid) != 0) {
> printerr(0, "WARNING: failed to set gid to %u!\n", pw->pw_gid);
> return errno;
> }
> @@ -466,7 +472,7 @@ change_identity(uid_t uid)
> * Switch UIDs, but leave saved-set-uid alone to prevent ptrace() by
> * other processes running with this uid.
> */
> - if (setresuid(uid, uid, -1) != 0) {
> + if (syscall(SYS_setresuid, uid) != 0) {
That looks wrong. setresuid takes 3 arguments:
SYSCALL_DEFINE3(setresuid, uid_t, ruid, uid_t, euid, uid_t, suid)
Ditto for setresgid above. syscall is a varargs function, so you really
_must_ pass in the right number of args or you'll end up feeding it
random junk in registers or off the stack. The compiler won't save you
here...
> printerr(0, "WARNING: Failed to setuid for user with
uid %u\n",
> uid);
> return errno;
--
Jeff Layton <[email protected]>
DQo+IE9uIEFwciAyNSwgMjAxNiwgYXQgNDoyMyBQTSwgSmVmZiBMYXl0b24gPGpsYXl0b25AcG9v
Y2hpZXJlZHMubmV0PiB3cm90ZToNCj4gDQo+IE9uIE1vbiwgMjAxNi0wNC0yNSBhdCAxMjo1OCAt
MDQwMCwgT2xnYSBLb3JuaWV2c2thaWEgd3JvdGU6DQo+PiBGb3IgdGhlIHRocmVhZGVkIHZlcnNp
b24gd2UgaGF2ZSB0byBzZXQgdWlkLGdpZCBwZXIgdGhyZWFkIGluc3RlYWQNCj4+IG9mIHBlciBw
cm9jZXNzLiBnbGliYyBzZXRyZXN1aWQoKSB3aGVuIGNhbGxlZCBmcm9tIGEgdGhyZWFkLCBpdCds
bA0KPj4gc2VuZCBhIHNpZ25hbCB0byBhbGwgb3RoZXIgdGhyZWFkcyB0byBzeW5jaHJvbml6ZSB0
aGUgdWlkIGluIGFsbA0KPj4gb3RoZXIgdGhyZWFkcy4gVG8gYnlwYXNzIHRoaXMsIHdlIGhhdmUg
dG8gY2FsbCBzeXNjYWxsKCkgZGlyZWN0bHkuDQo+PiANCj4+IFNpZ25lZC1vZmYtYnk6IE9sZ2Eg
S29ybmlldnNrYWlhIDxrb2xnYUBuZXRhcHAuY29tPg0KPj4gUmV2aWV3ZWQtYnk6IFN0ZXZlIERp
Y2tzb24gPHN0ZXZlZEByZWRoYXQuY29tPg0KPj4gLS0tDQo+PiAgdXRpbHMvZ3NzZC9nc3NkX3By
b2MuYyB8IDEyICsrKysrKysrKy0tLQ0KPj4gIDEgZmlsZSBjaGFuZ2VkLCA5IGluc2VydGlvbnMo
KyksIDMgZGVsZXRpb25zKC0pDQo+PiANCj4+IGRpZmYgLS1naXQgYS91dGlscy9nc3NkL2dzc2Rf
cHJvYy5jIGIvdXRpbHMvZ3NzZC9nc3NkX3Byb2MuYw0KPj4gaW5kZXggZTJlOTVkYy4uNDg3YTRm
NSAxMDA2NDQNCj4+IC0tLSBhL3V0aWxzL2dzc2QvZ3NzZF9wcm9jLmMNCj4+ICsrKyBiL3V0aWxz
L2dzc2QvZ3NzZF9wcm9jLmMNCj4+IEBAIC02OSw2ICs2OSw3IEBADQo+PiAgI2luY2x1ZGUgDQo+
PiAgI2luY2x1ZGUgDQo+PiAgI2luY2x1ZGUgDQo+PiArI2luY2x1ZGUgDQo+PiAgDQo+PiAgI2lu
Y2x1ZGUgImdzc2QuaCINCj4+ICAjaW5jbHVkZSAiZXJyX3V0aWwuaCINCj4+IEBAIC00MzYsNyAr
NDM3LDcgQEAgY2hhbmdlX2lkZW50aXR5KHVpZF90IHVpZCkNCj4+ICAJc3RydWN0IHBhc3N3ZAkq
cHc7DQo+PiAgDQo+PiAgCS8qIGRyb3AgbGlzdCBvZiBzdXBwbGltZW50YXJ5IGdyb3VwcyBmaXJz
dCAqLw0KPj4gLQlpZiAoc2V0Z3JvdXBzKDAsIE5VTEwpICE9IDApIHsNCj4+ICsJaWYgKHN5c2Nh
bGwoU1lTX3NldGdyb3VwcywgMCkgIT0gMCkgew0KPj4gIAkJcHJpbnRlcnIoMCwgIldBUk5JTkc6
IHVuYWJsZSB0byBkcm9wIHN1cHBsaW1lbnRhcnkgZ3JvdXBzISIpOw0KPj4gIAkJcmV0dXJuIGVy
cm5vOw0KPj4gIAl9DQo+PiBAQCAtNDU3LDcgKzQ1OCwxMiBAQCBjaGFuZ2VfaWRlbnRpdHkodWlk
X3QgdWlkKQ0KPj4gIAkgKiBTd2l0Y2ggdGhlIEdJRHMuIE5vdGUgdGhhdCB3ZSBsZWF2ZSB0aGUg
c2F2ZWQtc2V0LWdpZCBhbG9uZSBpbiBhbg0KPj4gIAkgKiBhdHRlbXB0IHRvIHByZXZlbnQgYXR0
YWNrcyB2aWEgcHRyYWNlKCkNCj4+ICAJICovDQo+PiAtCWlmIChzZXRyZXNnaWQocHctPnB3X2dp
ZCwgcHctPnB3X2dpZCwgLTEpICE9IDApIHsNCj4+ICsJLyogRm9yIHRoZSB0aHJlYWRlZCB2ZXJz
aW9uIHdlIGhhdmUgdG8gc2V0IHVpZCxnaWQgcGVyIHRocmVhZCBpbnN0ZWFkDQo+PiArCSAqIG9m
IHBlciBwcm9jZXNzLiBnbGliYyBzZXRyZXN1aWQoKSB3aGVuIGNhbGxlZCBmcm9tIGEgdGhyZWFk
LCBpdCdsbA0KPj4gKwkgKiBzZW5kIGEgc2lnbmFsIHRvIGFsbCBvdGhlciB0aHJlYWRzIHRvIHN5
bmNocm9uaXplIHRoZSB1aWQgaW4gYWxsDQo+PiArCSAqIG90aGVyIHRocmVhZHMuIFRvIGJ5cGFz
cyB0aGlzLCB3ZSBoYXZlIHRvIGNhbGwgc3lzY2FsbCgpIGRpcmVjdGx5Lg0KPj4gKwkgKi8NCj4+
ICsJaWYgKHN5c2NhbGwoU1lTX3NldHJlc2dpZCwgcHctPnB3X2dpZCkgIT0gMCkgew0KPj4gIAkJ
cHJpbnRlcnIoMCwgIldBUk5JTkc6IGZhaWxlZCB0byBzZXQgZ2lkIHRvICV1IVxuIiwgcHctPnB3
X2dpZCk7DQo+PiAgCQlyZXR1cm4gZXJybm87DQo+PiAgCX0NCj4+IEBAIC00NjYsNyArNDcyLDcg
QEAgY2hhbmdlX2lkZW50aXR5KHVpZF90IHVpZCkNCj4+ICAJICogU3dpdGNoIFVJRHMsIGJ1dCBs
ZWF2ZSBzYXZlZC1zZXQtdWlkIGFsb25lIHRvIHByZXZlbnQgcHRyYWNlKCkgYnkNCj4+ICAJICog
b3RoZXIgcHJvY2Vzc2VzIHJ1bm5pbmcgd2l0aCB0aGlzIHVpZC4NCj4+ICAJICovDQo+PiAtCWlm
IChzZXRyZXN1aWQodWlkLCB1aWQsIC0xKSAhPSAwKSB7DQo+PiArCWlmIChzeXNjYWxsKFNZU19z
ZXRyZXN1aWQsIHVpZCkgIT0gMCkgew0KPiANCj4gVGhhdCBsb29rcyB3cm9uZy4gc2V0cmVzdWlk
IHRha2VzIDMgYXJndW1lbnRzOg0KPiANCj4gICAgU1lTQ0FMTF9ERUZJTkUzKHNldHJlc3VpZCwg
dWlkX3QsIHJ1aWQsIHVpZF90LCBldWlkLCB1aWRfdCwgc3VpZCkNCj4gDQo+IERpdHRvIGZvciBz
ZXRyZXNnaWQgYWJvdmUuIHN5c2NhbGwgaXMgYSB2YXJhcmdzIGZ1bmN0aW9uLCBzbyB5b3UgcmVh
bGx5DQo+IF9tdXN0XyBwYXNzIGluIHRoZSByaWdodCBudW1iZXIgb2YgYXJncyBvciB5b3UnbGwg
ZW5kIHVwIGZlZWRpbmcgaXQNCj4gcmFuZG9tIGp1bmsgaW4gcmVnaXN0ZXJzIG9yIG9mZiB0aGUg
c3RhY2suIFRoZSBjb21waWxlciB3b24ndCBzYXZlIHlvdQ0KPiBoZXJl4oCmDQoNClRoYW5rcyBK
ZWZmLiBXaWxsIGZpeCB0aGF0LiBzZXRncm91cHMgdGFrZSAyIGFyZ3Mgc28gdGhhdCBzaG91bGQg
YmUgZml4ZWQgdG9vLg0KDQoNCj4+ICAJCXByaW50ZXJyKDAsICJXQVJOSU5HOiBGYWlsZWQgdG8g
c2V0dWlkIGZvciB1c2VyIHdpdGgNCj4gdWlkICV1XG4iLA0KPj4gIAkJCQl1aWQpOw0KPj4gIAkJ
cmV0dXJuIGVycm5vOw0KPiAtLSANCj4gSmVmZiBMYXl0b24gPGpsYXl0b25AcG9vY2hpZXJlZHMu
bmV0Pg0KDQo=
On 04/25/2016 12:58 PM, Olga Kornievskaia wrote:
> Currently, to persevere global data over multiple mounts,
> the root process does not fork when handling an upcall.
> Instead on not-forking create a pthread to handle the
> upcall since global data can be shared among threads.
>
> Signed-off-by: Steve Dickson <[email protected]>
> Signed-off-by: Olga Kornievskaia <[email protected]>
> ---
> aclocal/libpthread.m4 | 13 +++++++++++++
> configure.ac | 3 +++
> utils/gssd/Makefile.am | 3 ++-
> utils/gssd/gssd.c | 45 ++++++++++++++++++++++++++++++++++++++++-----
> utils/gssd/gssd.h | 5 +++++
> utils/gssd/gssd_proc.c | 49 ++++++++++++++++++-------------------------------
> utils/gssd/krb5_util.c | 3 +++
> 7 files changed, 84 insertions(+), 37 deletions(-)
> create mode 100644 aclocal/libpthread.m4
>
> diff --git a/aclocal/libpthread.m4 b/aclocal/libpthread.m4
> new file mode 100644
> index 0000000..e87d2a0
> --- /dev/null
> +++ b/aclocal/libpthread.m4
> @@ -0,0 +1,13 @@
> +dnl Checks for pthreads library and headers
> +dnl
> +AC_DEFUN([AC_LIBPTHREAD], [
> +
> + dnl Check for library, but do not add -lpthreads to LIBS
> + AC_CHECK_LIB([pthread], [pthread_create], [LIBPTHREAD=-lpthread],
> + [AC_MSG_ERROR([libpthread not found.])])
> + AC_SUBST(LIBPTHREAD)
> +
> + AC_CHECK_HEADERS([pthread.h], ,
> + [AC_MSG_ERROR([libpthread headers not found.])])
> +
> +])dnl
> diff --git a/configure.ac b/configure.ac
> index 25d2ba4..e0ecd61 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -385,6 +385,9 @@ if test "$enable_gss" = yes; then
> dnl Invoked after AC_KERBEROS_V5; AC_LIBRPCSECGSS needs to have KRBLIBS set
> AC_LIBRPCSECGSS
>
> + dnl Check for pthreads
> + AC_LIBPTHREAD
> +
> dnl librpcsecgss already has a dependency on libgssapi,
> dnl but we need to make sure we get the right version
> if test "$enable_gss" = yes; then
> diff --git a/utils/gssd/Makefile.am b/utils/gssd/Makefile.am
> index cb040b3..3f5f59a 100644
> --- a/utils/gssd/Makefile.am
> +++ b/utils/gssd/Makefile.am
> @@ -49,7 +49,8 @@ gssd_LDADD = \
> $(RPCSECGSS_LIBS) \
> $(KRBLIBS) \
> $(GSSAPI_LIBS) \
> - $(LIBTIRPC)
> + $(LIBTIRPC) \
> + $(LIBPTHREAD)
>
> gssd_LDFLAGS = \
> $(KRBLDFLAGS)
> diff --git a/utils/gssd/gssd.c b/utils/gssd/gssd.c
> index e7cb07f..0b7ffca 100644
> --- a/utils/gssd/gssd.c
> +++ b/utils/gssd/gssd.c
> @@ -87,7 +87,9 @@ unsigned int rpc_timeout = 5;
> char *preferred_realm = NULL;
> /* Avoid DNS reverse lookups on server names */
> static bool avoid_dns = true;
> -
> +int thread_started = false;
> +pthread_mutex_t pmutex = PTHREAD_MUTEX_INITIALIZER;
> +pthread_cond_t pcond = PTHREAD_COND_INITIALIZER;
>
> TAILQ_HEAD(topdir_list_head, topdir) topdir_list;
>
> @@ -361,20 +363,53 @@ gssd_destroy_client(struct clnt_info *clp)
>
> static void gssd_scan(void);
>
> +static void wait_for_child_and_detach(pthread_t th)
> +{
> + pthread_mutex_lock(&pmutex);
> + while (!thread_started)
> + pthread_cond_wait(&pcond, &pmutex);
> + thread_started = false;
> + pthread_mutex_unlock(&pmutex);
> + pthread_detach(th);
> +}
I was just looking at this... does it make more sense to
make these types of routines inline instead of allocating
stack space on every call...
With rpc.gssd, when running, is now involved every mount
(even sec=sys ones) so I'm wondering if inlining 4 new
routines would buy us anything...
One down fall with inline routines it makes more
difficult to debug from gdb breakpoint point of view.
steved.
> +
> +/* For each upcall create a thread, detach from the main process so that
> + * resources are released back into the system without the need for a join.
> + * We need to wait for the child thread to start and consume the event from
> + * the file descriptor.
> + */
> static void
> gssd_clnt_gssd_cb(int UNUSED(fd), short UNUSED(which), void *data)
> {
> struct clnt_info *clp = data;
> -
> - handle_gssd_upcall(clp);
> + pthread_t th;
> + int ret;
> +
> + ret = pthread_create(&th, NULL, (void *)handle_gssd_upcall,
> + (void *)clp);
> + if (ret != 0) {
> + printerr(0, "ERROR: pthread_create failed: ret %d: %s\n",
> + ret, strerror(errno));
> + return;
> + }
> + wait_for_child_and_detach(th);
> }
>
> static void
> gssd_clnt_krb5_cb(int UNUSED(fd), short UNUSED(which), void *data)
> {
> struct clnt_info *clp = data;
> -
> - handle_krb5_upcall(clp);
> + pthread_t th;
> + int ret;
> +
> + ret = pthread_create(&th, NULL, (void *)handle_krb5_upcall,
> + (void *)clp);
> + if (ret != 0) {
> + printerr(0, "ERROR: pthread_create failed: ret %d: %s\n",
> + ret, strerror(errno));
> + return;
> + }
> + wait_for_child_and_detach(th);
> }
>
> static struct clnt_info *
> diff --git a/utils/gssd/gssd.h b/utils/gssd/gssd.h
> index c6937c5..565bce3 100644
> --- a/utils/gssd/gssd.h
> +++ b/utils/gssd/gssd.h
> @@ -36,6 +36,7 @@
> #include <gssapi/gssapi.h>
> #include <event.h>
> #include <stdbool.h>
> +#include <pthread.h>
>
> #ifndef GSSD_PIPEFS_DIR
> #define GSSD_PIPEFS_DIR "/var/lib/nfs/rpc_pipefs"
> @@ -61,6 +62,10 @@ extern int root_uses_machine_creds;
> extern unsigned int context_timeout;
> extern unsigned int rpc_timeout;
> extern char *preferred_realm;
> +extern pthread_mutex_t ple_lock;
> +extern pthread_cond_t pcond;
> +extern pthread_mutex_t pmutex;
> +extern int thread_started;
>
> struct clnt_info {
> TAILQ_ENTRY(clnt_info) list;
> diff --git a/utils/gssd/gssd_proc.c b/utils/gssd/gssd_proc.c
> index 1ef68d8..e2e95dc 100644
> --- a/utils/gssd/gssd_proc.c
> +++ b/utils/gssd/gssd_proc.c
> @@ -629,36 +629,6 @@ process_krb5_upcall(struct clnt_info *clp, uid_t uid, int fd, char *tgtname,
> if (uid != 0 || (uid == 0 && root_uses_machine_creds == 0 &&
> service == NULL)) {
>
> - /* already running as uid 0 */
> - if (uid == 0)
> - goto no_fork;
> -
> - pid = fork();
> - switch(pid) {
> - case 0:
> - /* Child: fall through to rest of function */
> - childpid = getpid();
> - unsetenv("KRB5CCNAME");
> - printerr(2, "CHILD forked pid %d \n", childpid);
> - break;
> - case -1:
> - /* fork() failed! */
> - printerr(0, "WARNING: unable to fork() to handle"
> - "upcall: %s\n", strerror(errno));
> - return;
> - default:
> - /* Parent: just wait on child to exit and return */
> - do {
> - pid = wait(&err);
> - } while(pid == -1 && errno != -ECHILD);
> -
> - if (WIFSIGNALED(err))
> - printerr(0, "WARNING: forked child was killed"
> - "with signal %d\n", WTERMSIG(err));
> - return;
> - }
> -no_fork:
> -
> auth = krb5_not_machine_creds(clp, uid, tgtname, &downcall_err,
> &err, &rpc_clnt);
> if (err)
> @@ -735,12 +705,28 @@ out_return_error:
> goto out;
> }
>
> +/* signal to the parent thread that we have read from the file descriptor.
> + * it should allow the parent to proceed to poll on the descriptor for
> + * the next upcall from the kernel.
> + */
> +static void
> +signal_parent_event_consumed(void)
> +{
> + pthread_mutex_lock(&pmutex);
> + thread_started = true;
> + pthread_cond_signal(&pcond);
> + pthread_mutex_unlock(&pmutex);
> +}
> +
> void
> handle_krb5_upcall(struct clnt_info *clp)
> {
> uid_t uid;
> + int status;
>
> - if (read(clp->krb5_fd, &uid, sizeof(uid)) < (ssize_t)sizeof(uid)) {
> + status = read(clp->krb5_fd, &uid, sizeof(uid)) < (ssize_t)sizeof(uid);
> + signal_parent_event_consumed();
> + if (status) {
> printerr(0, "WARNING: failed reading uid from krb5 "
> "upcall pipe: %s\n", strerror(errno));
> return;
> @@ -765,6 +751,7 @@ handle_gssd_upcall(struct clnt_info *clp)
> char *enctypes = NULL;
>
> lbuflen = read(clp->gssd_fd, lbuf, sizeof(lbuf));
> + signal_parent_event_consumed();
> if (lbuflen <= 0 || lbuf[lbuflen-1] != '\n') {
> printerr(0, "WARNING: handle_gssd_upcall: "
> "failed reading request\n");
> diff --git a/utils/gssd/krb5_util.c b/utils/gssd/krb5_util.c
> index 8ef8184..3328696 100644
> --- a/utils/gssd/krb5_util.c
> +++ b/utils/gssd/krb5_util.c
> @@ -128,6 +128,7 @@
>
> /* Global list of principals/cache file names for machine credentials */
> struct gssd_k5_kt_princ *gssd_k5_kt_princ_list = NULL;
> +pthread_mutex_t ple_lock = PTHREAD_MUTEX_INITIALIZER;
>
> #ifdef HAVE_SET_ALLOWABLE_ENCTYPES
> int limit_to_legacy_enctypes = 0;
> @@ -586,10 +587,12 @@ get_ple_by_princ(krb5_context context, krb5_principal princ)
>
> /* Need to serialize list if we ever become multi-threaded! */
>
> + pthread_mutex_lock(&ple_lock);
> ple = find_ple_by_princ(context, princ);
> if (ple == NULL) {
> ple = new_ple(context, princ);
> }
> + pthread_mutex_unlock(&ple_lock);
>
> return ple;
> }
>
> On Apr 27, 2016, at 10:54 AM, Steve Dickson <[email protected]> wrote:
>
>
>
> On 04/25/2016 12:58 PM, Olga Kornievskaia wrote:
>> Currently, to persevere global data over multiple mounts,
>> the root process does not fork when handling an upcall.
>> Instead on not-forking create a pthread to handle the
>> upcall since global data can be shared among threads.
>>
>> Signed-off-by: Steve Dickson <[email protected]>
>> Signed-off-by: Olga Kornievskaia <[email protected]>
>> ---
>> aclocal/libpthread.m4 | 13 +++++++++++++
>> configure.ac | 3 +++
>> utils/gssd/Makefile.am | 3 ++-
>> utils/gssd/gssd.c | 45 ++++++++++++++++++++++++++++++++++++++++-----
>> utils/gssd/gssd.h | 5 +++++
>> utils/gssd/gssd_proc.c | 49 ++++++++++++++++++-------------------------------
>> utils/gssd/krb5_util.c | 3 +++
>> 7 files changed, 84 insertions(+), 37 deletions(-)
>> create mode 100644 aclocal/libpthread.m4
>>
>> diff --git a/aclocal/libpthread.m4 b/aclocal/libpthread.m4
>> new file mode 100644
>> index 0000000..e87d2a0
>> --- /dev/null
>> +++ b/aclocal/libpthread.m4
>> @@ -0,0 +1,13 @@
>> +dnl Checks for pthreads library and headers
>> +dnl
>> +AC_DEFUN([AC_LIBPTHREAD], [
>> +
>> + dnl Check for library, but do not add -lpthreads to LIBS
>> + AC_CHECK_LIB([pthread], [pthread_create], [LIBPTHREAD=-lpthread],
>> + [AC_MSG_ERROR([libpthread not found.])])
>> + AC_SUBST(LIBPTHREAD)
>> +
>> + AC_CHECK_HEADERS([pthread.h], ,
>> + [AC_MSG_ERROR([libpthread headers not found.])])
>> +
>> +])dnl
>> diff --git a/configure.ac b/configure.ac
>> index 25d2ba4..e0ecd61 100644
>> --- a/configure.ac
>> +++ b/configure.ac
>> @@ -385,6 +385,9 @@ if test "$enable_gss" = yes; then
>> dnl Invoked after AC_KERBEROS_V5; AC_LIBRPCSECGSS needs to have KRBLIBS set
>> AC_LIBRPCSECGSS
>>
>> + dnl Check for pthreads
>> + AC_LIBPTHREAD
>> +
>> dnl librpcsecgss already has a dependency on libgssapi,
>> dnl but we need to make sure we get the right version
>> if test "$enable_gss" = yes; then
>> diff --git a/utils/gssd/Makefile.am b/utils/gssd/Makefile.am
>> index cb040b3..3f5f59a 100644
>> --- a/utils/gssd/Makefile.am
>> +++ b/utils/gssd/Makefile.am
>> @@ -49,7 +49,8 @@ gssd_LDADD = \
>> $(RPCSECGSS_LIBS) \
>> $(KRBLIBS) \
>> $(GSSAPI_LIBS) \
>> - $(LIBTIRPC)
>> + $(LIBTIRPC) \
>> + $(LIBPTHREAD)
>>
>> gssd_LDFLAGS = \
>> $(KRBLDFLAGS)
>> diff --git a/utils/gssd/gssd.c b/utils/gssd/gssd.c
>> index e7cb07f..0b7ffca 100644
>> --- a/utils/gssd/gssd.c
>> +++ b/utils/gssd/gssd.c
>> @@ -87,7 +87,9 @@ unsigned int rpc_timeout = 5;
>> char *preferred_realm = NULL;
>> /* Avoid DNS reverse lookups on server names */
>> static bool avoid_dns = true;
>> -
>> +int thread_started = false;
>> +pthread_mutex_t pmutex = PTHREAD_MUTEX_INITIALIZER;
>> +pthread_cond_t pcond = PTHREAD_COND_INITIALIZER;
>>
>> TAILQ_HEAD(topdir_list_head, topdir) topdir_list;
>>
>> @@ -361,20 +363,53 @@ gssd_destroy_client(struct clnt_info *clp)
>>
>> static void gssd_scan(void);
>>
>> +static void wait_for_child_and_detach(pthread_t th)
>> +{
>> + pthread_mutex_lock(&pmutex);
>> + while (!thread_started)
>> + pthread_cond_wait(&pcond, &pmutex);
>> + thread_started = false;
>> + pthread_mutex_unlock(&pmutex);
>> + pthread_detach(th);
>> +}
> I was just looking at this... does it make more sense to
> make these types of routines inline instead of allocating
> stack space on every call...
>
> With rpc.gssd, when running, is now involved every mount
> (even sec=sys ones) so I'm wondering if inlining 4 new
> routines would buy us anything...
>
> One down fall with inline routines it makes more
> difficult to debug from gdb breakpoint point of view.
This doesn?t look like a function that we really need to step into, does it? I think inlining it would be ok.
I think the biggest saving would be to take the next step and do the pool of threads that are pre-created (and thus not taking time to be allocated for each upcall). Then we need a way to make sure we cleanup any threads that might be hanging for whatever reason.
>
> steved.
>> +
>> +/* For each upcall create a thread, detach from the main process so that
>> + * resources are released back into the system without the need for a join.
>> + * We need to wait for the child thread to start and consume the event from
>> + * the file descriptor.
>> + */
>> static void
>> gssd_clnt_gssd_cb(int UNUSED(fd), short UNUSED(which), void *data)
>> {
>> struct clnt_info *clp = data;
>> -
>> - handle_gssd_upcall(clp);
>> + pthread_t th;
>> + int ret;
>> +
>> + ret = pthread_create(&th, NULL, (void *)handle_gssd_upcall,
>> + (void *)clp);
>> + if (ret != 0) {
>> + printerr(0, "ERROR: pthread_create failed: ret %d: %s\n",
>> + ret, strerror(errno));
>> + return;
>> + }
>> + wait_for_child_and_detach(th);
>> }
>>
>> static void
>> gssd_clnt_krb5_cb(int UNUSED(fd), short UNUSED(which), void *data)
>> {
>> struct clnt_info *clp = data;
>> -
>> - handle_krb5_upcall(clp);
>> + pthread_t th;
>> + int ret;
>> +
>> + ret = pthread_create(&th, NULL, (void *)handle_krb5_upcall,
>> + (void *)clp);
>> + if (ret != 0) {
>> + printerr(0, "ERROR: pthread_create failed: ret %d: %s\n",
>> + ret, strerror(errno));
>> + return;
>> + }
>> + wait_for_child_and_detach(th);
>> }
>>
>> static struct clnt_info *
>> diff --git a/utils/gssd/gssd.h b/utils/gssd/gssd.h
>> index c6937c5..565bce3 100644
>> --- a/utils/gssd/gssd.h
>> +++ b/utils/gssd/gssd.h
>> @@ -36,6 +36,7 @@
>> #include <gssapi/gssapi.h>
>> #include <event.h>
>> #include <stdbool.h>
>> +#include <pthread.h>
>>
>> #ifndef GSSD_PIPEFS_DIR
>> #define GSSD_PIPEFS_DIR "/var/lib/nfs/rpc_pipefs"
>> @@ -61,6 +62,10 @@ extern int root_uses_machine_creds;
>> extern unsigned int context_timeout;
>> extern unsigned int rpc_timeout;
>> extern char *preferred_realm;
>> +extern pthread_mutex_t ple_lock;
>> +extern pthread_cond_t pcond;
>> +extern pthread_mutex_t pmutex;
>> +extern int thread_started;
>>
>> struct clnt_info {
>> TAILQ_ENTRY(clnt_info) list;
>> diff --git a/utils/gssd/gssd_proc.c b/utils/gssd/gssd_proc.c
>> index 1ef68d8..e2e95dc 100644
>> --- a/utils/gssd/gssd_proc.c
>> +++ b/utils/gssd/gssd_proc.c
>> @@ -629,36 +629,6 @@ process_krb5_upcall(struct clnt_info *clp, uid_t uid, int fd, char *tgtname,
>> if (uid != 0 || (uid == 0 && root_uses_machine_creds == 0 &&
>> service == NULL)) {
>>
>> - /* already running as uid 0 */
>> - if (uid == 0)
>> - goto no_fork;
>> -
>> - pid = fork();
>> - switch(pid) {
>> - case 0:
>> - /* Child: fall through to rest of function */
>> - childpid = getpid();
>> - unsetenv("KRB5CCNAME");
>> - printerr(2, "CHILD forked pid %d \n", childpid);
>> - break;
>> - case -1:
>> - /* fork() failed! */
>> - printerr(0, "WARNING: unable to fork() to handle"
>> - "upcall: %s\n", strerror(errno));
>> - return;
>> - default:
>> - /* Parent: just wait on child to exit and return */
>> - do {
>> - pid = wait(&err);
>> - } while(pid == -1 && errno != -ECHILD);
>> -
>> - if (WIFSIGNALED(err))
>> - printerr(0, "WARNING: forked child was killed"
>> - "with signal %d\n", WTERMSIG(err));
>> - return;
>> - }
>> -no_fork:
>> -
>> auth = krb5_not_machine_creds(clp, uid, tgtname, &downcall_err,
>> &err, &rpc_clnt);
>> if (err)
>> @@ -735,12 +705,28 @@ out_return_error:
>> goto out;
>> }
>>
>> +/* signal to the parent thread that we have read from the file descriptor.
>> + * it should allow the parent to proceed to poll on the descriptor for
>> + * the next upcall from the kernel.
>> + */
>> +static void
>> +signal_parent_event_consumed(void)
>> +{
>> + pthread_mutex_lock(&pmutex);
>> + thread_started = true;
>> + pthread_cond_signal(&pcond);
>> + pthread_mutex_unlock(&pmutex);
>> +}
>> +
>> void
>> handle_krb5_upcall(struct clnt_info *clp)
>> {
>> uid_t uid;
>> + int status;
>>
>> - if (read(clp->krb5_fd, &uid, sizeof(uid)) < (ssize_t)sizeof(uid)) {
>> + status = read(clp->krb5_fd, &uid, sizeof(uid)) < (ssize_t)sizeof(uid);
>> + signal_parent_event_consumed();
>> + if (status) {
>> printerr(0, "WARNING: failed reading uid from krb5 "
>> "upcall pipe: %s\n", strerror(errno));
>> return;
>> @@ -765,6 +751,7 @@ handle_gssd_upcall(struct clnt_info *clp)
>> char *enctypes = NULL;
>>
>> lbuflen = read(clp->gssd_fd, lbuf, sizeof(lbuf));
>> + signal_parent_event_consumed();
>> if (lbuflen <= 0 || lbuf[lbuflen-1] != '\n') {
>> printerr(0, "WARNING: handle_gssd_upcall: "
>> "failed reading request\n");
>> diff --git a/utils/gssd/krb5_util.c b/utils/gssd/krb5_util.c
>> index 8ef8184..3328696 100644
>> --- a/utils/gssd/krb5_util.c
>> +++ b/utils/gssd/krb5_util.c
>> @@ -128,6 +128,7 @@
>>
>> /* Global list of principals/cache file names for machine credentials */
>> struct gssd_k5_kt_princ *gssd_k5_kt_princ_list = NULL;
>> +pthread_mutex_t ple_lock = PTHREAD_MUTEX_INITIALIZER;
>>
>> #ifdef HAVE_SET_ALLOWABLE_ENCTYPES
>> int limit_to_legacy_enctypes = 0;
>> @@ -586,10 +587,12 @@ get_ple_by_princ(krb5_context context, krb5_principal princ)
>>
>> /* Need to serialize list if we ever become multi-threaded! */
>>
>> + pthread_mutex_lock(&ple_lock);
>> ple = find_ple_by_princ(context, princ);
>> if (ple == NULL) {
>> ple = new_ple(context, princ);
>> }
>> + pthread_mutex_unlock(&ple_lock);
>>
>> return ple;
>> }
>>
On 04/27/2016 11:16 AM, Kornievskaia, Olga wrote:
>
>> On Apr 27, 2016, at 10:54 AM, Steve Dickson <[email protected]> wrote:
>>
>>
>>
>> On 04/25/2016 12:58 PM, Olga Kornievskaia wrote:
>>> Currently, to persevere global data over multiple mounts,
>>> the root process does not fork when handling an upcall.
>>> Instead on not-forking create a pthread to handle the
>>> upcall since global data can be shared among threads.
>>>
>>> Signed-off-by: Steve Dickson <[email protected]>
>>> Signed-off-by: Olga Kornievskaia <[email protected]>
>>> ---
>>> aclocal/libpthread.m4 | 13 +++++++++++++
>>> configure.ac | 3 +++
>>> utils/gssd/Makefile.am | 3 ++-
>>> utils/gssd/gssd.c | 45 ++++++++++++++++++++++++++++++++++++++++-----
>>> utils/gssd/gssd.h | 5 +++++
>>> utils/gssd/gssd_proc.c | 49 ++++++++++++++++++-------------------------------
>>> utils/gssd/krb5_util.c | 3 +++
>>> 7 files changed, 84 insertions(+), 37 deletions(-)
>>> create mode 100644 aclocal/libpthread.m4
>>>
>>> diff --git a/aclocal/libpthread.m4 b/aclocal/libpthread.m4
>>> new file mode 100644
>>> index 0000000..e87d2a0
>>> --- /dev/null
>>> +++ b/aclocal/libpthread.m4
>>> @@ -0,0 +1,13 @@
>>> +dnl Checks for pthreads library and headers
>>> +dnl
>>> +AC_DEFUN([AC_LIBPTHREAD], [
>>> +
>>> + dnl Check for library, but do not add -lpthreads to LIBS
>>> + AC_CHECK_LIB([pthread], [pthread_create], [LIBPTHREAD=-lpthread],
>>> + [AC_MSG_ERROR([libpthread not found.])])
>>> + AC_SUBST(LIBPTHREAD)
>>> +
>>> + AC_CHECK_HEADERS([pthread.h], ,
>>> + [AC_MSG_ERROR([libpthread headers not found.])])
>>> +
>>> +])dnl
>>> diff --git a/configure.ac b/configure.ac
>>> index 25d2ba4..e0ecd61 100644
>>> --- a/configure.ac
>>> +++ b/configure.ac
>>> @@ -385,6 +385,9 @@ if test "$enable_gss" = yes; then
>>> dnl Invoked after AC_KERBEROS_V5; AC_LIBRPCSECGSS needs to have KRBLIBS set
>>> AC_LIBRPCSECGSS
>>>
>>> + dnl Check for pthreads
>>> + AC_LIBPTHREAD
>>> +
>>> dnl librpcsecgss already has a dependency on libgssapi,
>>> dnl but we need to make sure we get the right version
>>> if test "$enable_gss" = yes; then
>>> diff --git a/utils/gssd/Makefile.am b/utils/gssd/Makefile.am
>>> index cb040b3..3f5f59a 100644
>>> --- a/utils/gssd/Makefile.am
>>> +++ b/utils/gssd/Makefile.am
>>> @@ -49,7 +49,8 @@ gssd_LDADD = \
>>> $(RPCSECGSS_LIBS) \
>>> $(KRBLIBS) \
>>> $(GSSAPI_LIBS) \
>>> - $(LIBTIRPC)
>>> + $(LIBTIRPC) \
>>> + $(LIBPTHREAD)
>>>
>>> gssd_LDFLAGS = \
>>> $(KRBLDFLAGS)
>>> diff --git a/utils/gssd/gssd.c b/utils/gssd/gssd.c
>>> index e7cb07f..0b7ffca 100644
>>> --- a/utils/gssd/gssd.c
>>> +++ b/utils/gssd/gssd.c
>>> @@ -87,7 +87,9 @@ unsigned int rpc_timeout = 5;
>>> char *preferred_realm = NULL;
>>> /* Avoid DNS reverse lookups on server names */
>>> static bool avoid_dns = true;
>>> -
>>> +int thread_started = false;
>>> +pthread_mutex_t pmutex = PTHREAD_MUTEX_INITIALIZER;
>>> +pthread_cond_t pcond = PTHREAD_COND_INITIALIZER;
>>>
>>> TAILQ_HEAD(topdir_list_head, topdir) topdir_list;
>>>
>>> @@ -361,20 +363,53 @@ gssd_destroy_client(struct clnt_info *clp)
>>>
>>> static void gssd_scan(void);
>>>
>>> +static void wait_for_child_and_detach(pthread_t th)
>>> +{
>>> + pthread_mutex_lock(&pmutex);
>>> + while (!thread_started)
>>> + pthread_cond_wait(&pcond, &pmutex);
>>> + thread_started = false;
>>> + pthread_mutex_unlock(&pmutex);
>>> + pthread_detach(th);
>>> +}
>> I was just looking at this... does it make more sense to
>> make these types of routines inline instead of allocating
>> stack space on every call...
>>
>> With rpc.gssd, when running, is now involved every mount
>> (even sec=sys ones) so I'm wondering if inlining 4 new
>> routines would buy us anything...
>>
>> One down fall with inline routines it makes more
>> difficult to debug from gdb breakpoint point of view.
>
> This doesn?t look like a function that we really
> need to step into, does it? I think inlining it would be ok.
No it does not... inlining it is...
>
> I think the biggest saving would be to take the next step
> and do the pool of threads that are pre-created (and thus
> not taking time to be allocated for each upcall). Then we
> need a way to make sure we cleanup any threads that might
> be hanging for whatever reason.
This will be an excellent next step!
steved.
On 04/27/2016 12:33 PM, Kornievskaia, Olga wrote:
>>>> I think the biggest saving would be to take the next step
>>>> and do the pool of threads that are pre-created (and thus
>>>> not taking time to be allocated for each upcall). Then we
>>>> need a way to make sure we cleanup any threads that might
>>>> be hanging for whatever reason.
>>> This will be an excellent next step!
>>
>> This patch series is suppose to show ?proof-of-concept? and
>> functionally equivalent but better to what we have now.
>> Old: upcall -> new process. New: upcall -> new thread.
>> Old: process hangs -> resources not cleaned and no new upcalls.
>> New: thread hangs -> resources not cleaned and new upcalls can proceed.
Well I think you proved it... Nice work!
>>
>> I will submit the new version with:
>> ? make function inline
>> ? change setresuid/setresgid to set all three uids: real, effective, saved. git blame tells me Jeff you wrote the comment about attacks from ptrace().
>
> Sorry I unintentionally hit send. I wanted to say that then I?ll work on
> getting the pool of thread but working on top of these patches.
> Or do you want me to keep working and then submit that version
> as if this one wasn?t written?
Let's get these in now so they will get some real world
testing... So lets build on these...
steved.