2016-04-27 16:58:13

by Olga Kornievskaia

[permalink] [raw]
Subject: [RFC PATCH v5 0/3] adding pthread support to gssd

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



2016-04-27 16:58:14

by Olga Kornievskaia

[permalink] [raw]
Subject: [PATCH v5 2/3] gssd: using syscalls directly to change thread's identity

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 | 19 +++++++++----------
1 file changed, 9 insertions(+), 10 deletions(-)

diff --git a/utils/gssd/gssd_proc.c b/utils/gssd/gssd_proc.c
index e2e95dc..de8dc07 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) != 0) {
printerr(0, "WARNING: unable to drop supplimentary groups!");
return errno;
}
@@ -453,20 +454,18 @@ 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()
+ /* Switch the UIDs and GIDs. */
+ /* 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 (setresgid(pw->pw_gid, pw->pw_gid, -1) != 0) {
+ if (syscall(SYS_setresgid, pw->pw_gid, pw->pw_gid, pw->pw_gid) != 0) {
printerr(0, "WARNING: failed to set gid to %u!\n", pw->pw_gid);
return errno;
}

- /*
- * 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, uid, uid) != 0) {
printerr(0, "WARNING: Failed to setuid for user with uid %u\n",
uid);
return errno;
--
1.8.3.1


2016-04-27 16:58:14

by Olga Kornievskaia

[permalink] [raw]
Subject: [PATCH v5 3/3] gssd: always call gss_krb5_ccache_name

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 de8dc07..14298d0 100644
--- a/utils/gssd/gssd_proc.c
+++ b/utils/gssd/gssd_proc.c
@@ -547,7 +547,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,
@@ -568,7 +576,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


2016-04-27 16:58:15

by Olga Kornievskaia

[permalink] [raw]
Subject: [PATCH v5 1/3] gssd: use pthreads to handle upcalls

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..b676341 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 inline 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


2016-04-27 17:39:10

by Steve Dickson

[permalink] [raw]
Subject: Re: [PATCH v5 1/3] gssd: use pthreads to handle upcalls



On 04/27/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..b676341 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 inline 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);
> +}
> +
Small nit... this should be a static inline void
as well... I'll fix it... no need to repost...

steved.

> 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;
> }
>

2016-04-28 13:13:29

by Jeff Layton

[permalink] [raw]
Subject: Re: [PATCH v5 1/3] gssd: use pthreads to handle upcalls

On Wed, 2016-04-27 at 12:58 -0400, 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..b676341 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 inline 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
>  #include
>  #include
> +#include
>  
>  #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;
>  }

Looks good. One thing that might be neat as a follow up is to make the
thread_started variable a per-clnt thing. I don't think there's any
reason to serialize I/O to different fds there.

Reviewed-by: Jeff Layton <[email protected]>

2016-04-28 13:14:32

by Jeff Layton

[permalink] [raw]
Subject: Re: [PATCH v5 2/3] gssd: using syscalls directly to change thread's identity

On Wed, 2016-04-27 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 | 19 +++++++++----------
>  1 file changed, 9 insertions(+), 10 deletions(-)
>
> diff --git a/utils/gssd/gssd_proc.c b/utils/gssd/gssd_proc.c
> index e2e95dc..de8dc07 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) != 0) {
>   printerr(0, "WARNING: unable to drop supplimentary groups!");
>   return errno;
>   }
> @@ -453,20 +454,18 @@ 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()
> + /* Switch the UIDs and GIDs. */
> + /* 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 (setresgid(pw->pw_gid, pw->pw_gid, -1) != 0) {
> + if (syscall(SYS_setresgid, pw->pw_gid, pw->pw_gid, pw->pw_gid) != 0) {
>   printerr(0, "WARNING: failed to set gid to %u!\n", pw->pw_gid);
>   return errno;
>   }
>  
> - /*
> -  * 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, uid, uid) != 0) {
>   printerr(0, "WARNING: Failed to setuid for user with uid %u\n",
>   uid);
>   return errno;

Reviewed-by: Jeff Layton <[email protected]>

2016-04-28 13:19:50

by Jeff Layton

[permalink] [raw]
Subject: Re: [PATCH v5 3/3] gssd: always call gss_krb5_ccache_name

On Wed, 2016-04-27 at 12:58 -0400, Olga Kornievskaia wrote:
> 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 de8dc07..14298d0 100644
> --- a/utils/gssd/gssd_proc.c
> +++ b/utils/gssd/gssd_proc.c
> @@ -547,7 +547,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,
> @@ -568,7 +576,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, 

Looks fine to me, but I wonder why this env var frobbing is there. Did
old MIT krb5 libs lack that function? In any case, they seem to have it
now, so I'm fine with this.

Also, I think you can probably remove USE_GSS_KRB5_CCACHE_NAME
altogether as I don't think it'll be used after this patch.

Reviewed-by: Jeff Layton <[email protected]>

2016-04-28 14:07:17

by Kornievskaia, Olga

[permalink] [raw]
Subject: Re: [PATCH v5 1/3] gssd: use pthreads to handle upcalls

DQo+IE9uIEFwciAyOCwgMjAxNiwgYXQgOToxMyBBTSwgSmVmZiBMYXl0b24gPGpsYXl0b25AcG9v
Y2hpZXJlZHMubmV0PiB3cm90ZToNCj4gDQo+IE9uIFdlZCwgMjAxNi0wNC0yNyBhdCAxMjo1OCAt
MDQwMCwgT2xnYSBLb3JuaWV2c2thaWEgd3JvdGU6DQo+PiBDdXJyZW50bHksIHRvIHBlcnNldmVy
ZSBnbG9iYWwgZGF0YSBvdmVyIG11bHRpcGxlIG1vdW50cywNCj4+IHRoZSByb290IHByb2Nlc3Mg
ZG9lcyBub3QgZm9yayB3aGVuIGhhbmRsaW5nIGFuIHVwY2FsbC4NCj4+IEluc3RlYWQgb24gbm90
LWZvcmtpbmcgY3JlYXRlIGEgcHRocmVhZCB0byBoYW5kbGUgdGhlDQo+PiB1cGNhbGwgc2luY2Ug
Z2xvYmFsIGRhdGEgY2FuIGJlIHNoYXJlZCBhbW9uZyB0aHJlYWRzLg0KPj4gDQo+PiBTaWduZWQt
b2ZmLWJ5OiBTdGV2ZSBEaWNrc29uIDxzdGV2ZWRAcmVkaGF0LmNvbT4NCj4+IFNpZ25lZC1vZmYt
Ynk6IE9sZ2EgS29ybmlldnNrYWlhIDxrb2xnYUBuZXRhcHAuY29tPg0KPj4gLS0tDQo+PiAgYWNs
b2NhbC9saWJwdGhyZWFkLm00ICB8IDEzICsrKysrKysrKysrKysNCj4+ICBjb25maWd1cmUuYWMg
ICAgICAgICAgIHwgIDMgKysrDQo+PiAgdXRpbHMvZ3NzZC9NYWtlZmlsZS5hbSB8ICAzICsrLQ0K
Pj4gIHV0aWxzL2dzc2QvZ3NzZC5jICAgICAgfCA0NSArKysrKysrKysrKysrKysrKysrKysrKysr
KysrKysrKysrKysrKysrLS0tLS0NCj4+ICB1dGlscy9nc3NkL2dzc2QuaCAgICAgIHwgIDUgKysr
KysNCj4+ICB1dGlscy9nc3NkL2dzc2RfcHJvYy5jIHwgNDkgKysrKysrKysrKysrKysrKysrLS0t
LS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLQ0KPj4gIHV0aWxzL2dzc2Qva3JiNV91dGlsLmMg
fCAgMyArKysNCj4+ICA3IGZpbGVzIGNoYW5nZWQsIDg0IGluc2VydGlvbnMoKyksIDM3IGRlbGV0
aW9ucygtKQ0KPj4gIGNyZWF0ZSBtb2RlIDEwMDY0NCBhY2xvY2FsL2xpYnB0aHJlYWQubTQNCj4+
IA0KPj4gZGlmZiAtLWdpdCBhL2FjbG9jYWwvbGlicHRocmVhZC5tNCBiL2FjbG9jYWwvbGlicHRo
cmVhZC5tNA0KPj4gbmV3IGZpbGUgbW9kZSAxMDA2NDQNCj4+IGluZGV4IDAwMDAwMDAuLmU4N2Qy
YTANCj4+IC0tLSAvZGV2L251bGwNCj4+ICsrKyBiL2FjbG9jYWwvbGlicHRocmVhZC5tNA0KPj4g
QEAgLTAsMCArMSwxMyBAQA0KPj4gK2RubCBDaGVja3MgZm9yIHB0aHJlYWRzIGxpYnJhcnkgYW5k
IGhlYWRlcnMNCj4+ICtkbmwNCj4+ICtBQ19ERUZVTihbQUNfTElCUFRIUkVBRF0sIFsNCj4+ICsN
Cj4+ICsgICAgZG5sIENoZWNrIGZvciBsaWJyYXJ5LCBidXQgZG8gbm90IGFkZCAtbHB0aHJlYWRz
IHRvIExJQlMNCj4+ICsgICAgQUNfQ0hFQ0tfTElCKFtwdGhyZWFkXSwgW3B0aHJlYWRfY3JlYXRl
XSwgW0xJQlBUSFJFQUQ9LWxwdGhyZWFkXSwNCj4+ICsgICAgICAgICAgICAgICAgIFtBQ19NU0df
RVJST1IoW2xpYnB0aHJlYWQgbm90IGZvdW5kLl0pXSkNCj4+ICsgIEFDX1NVQlNUKExJQlBUSFJF
QUQpDQo+PiArDQo+PiArICBBQ19DSEVDS19IRUFERVJTKFtwdGhyZWFkLmhdLCAsDQo+PiArICAg
ICAgICAgICAgICAgICAgIFtBQ19NU0dfRVJST1IoW2xpYnB0aHJlYWQgaGVhZGVycyBub3QgZm91
bmQuXSldKQ0KPj4gKw0KPj4gK10pZG5sDQo+PiBkaWZmIC0tZ2l0IGEvY29uZmlndXJlLmFjIGIv
Y29uZmlndXJlLmFjDQo+PiBpbmRleCAyNWQyYmE0Li5lMGVjZDYxIDEwMDY0NA0KPj4gLS0tIGEv
Y29uZmlndXJlLmFjDQo+PiArKysgYi9jb25maWd1cmUuYWMNCj4+IEBAIC0zODUsNiArMzg1LDkg
QEAgaWYgdGVzdCAiJGVuYWJsZV9nc3MiID0geWVzOyB0aGVuDQo+PiAgICBkbmwgSW52b2tlZCBh
ZnRlciBBQ19LRVJCRVJPU19WNTsgQUNfTElCUlBDU0VDR1NTIG5lZWRzIHRvIGhhdmUgS1JCTElC
UyBzZXQNCj4+ICAgIEFDX0xJQlJQQ1NFQ0dTUw0KPj4gIA0KPj4gKyAgZG5sIENoZWNrIGZvciBw
dGhyZWFkcw0KPj4gKyAgQUNfTElCUFRIUkVBRA0KPj4gKw0KPj4gICAgZG5sIGxpYnJwY3NlY2dz
cyBhbHJlYWR5IGhhcyBhIGRlcGVuZGVuY3kgb24gbGliZ3NzYXBpLA0KPj4gICAgZG5sIGJ1dCB3
ZSBuZWVkIHRvIG1ha2Ugc3VyZSB3ZSBnZXQgdGhlIHJpZ2h0IHZlcnNpb24NCj4+ICAgIGlmIHRl
c3QgIiRlbmFibGVfZ3NzIiA9IHllczsgdGhlbg0KPj4gZGlmZiAtLWdpdCBhL3V0aWxzL2dzc2Qv
TWFrZWZpbGUuYW0gYi91dGlscy9nc3NkL01ha2VmaWxlLmFtDQo+PiBpbmRleCBjYjA0MGIzLi4z
ZjVmNTlhIDEwMDY0NA0KPj4gLS0tIGEvdXRpbHMvZ3NzZC9NYWtlZmlsZS5hbQ0KPj4gKysrIGIv
dXRpbHMvZ3NzZC9NYWtlZmlsZS5hbQ0KPj4gQEAgLTQ5LDcgKzQ5LDggQEAgZ3NzZF9MREFERCA9
IFwNCj4+ICAJJChSUENTRUNHU1NfTElCUykgXA0KPj4gIAkkKEtSQkxJQlMpIFwNCj4+ICAJJChH
U1NBUElfTElCUykgXA0KPj4gLQkkKExJQlRJUlBDKQ0KPj4gKwkkKExJQlRJUlBDKSBcDQo+PiAr
CSQoTElCUFRIUkVBRCkNCj4+ICANCj4+ICBnc3NkX0xERkxBR1MgPSBcDQo+PiAgCSQoS1JCTERG
TEFHUykNCj4+IGRpZmYgLS1naXQgYS91dGlscy9nc3NkL2dzc2QuYyBiL3V0aWxzL2dzc2QvZ3Nz
ZC5jDQo+PiBpbmRleCBlN2NiMDdmLi5iNjc2MzQxIDEwMDY0NA0KPj4gLS0tIGEvdXRpbHMvZ3Nz
ZC9nc3NkLmMNCj4+ICsrKyBiL3V0aWxzL2dzc2QvZ3NzZC5jDQo+PiBAQCAtODcsNyArODcsOSBA
QCB1bnNpZ25lZCBpbnQgIHJwY190aW1lb3V0ID0gNTsNCj4+ICBjaGFyICpwcmVmZXJyZWRfcmVh
bG0gPSBOVUxMOw0KPj4gIC8qIEF2b2lkIEROUyByZXZlcnNlIGxvb2t1cHMgb24gc2VydmVyIG5h
bWVzICovDQo+PiAgc3RhdGljIGJvb2wgYXZvaWRfZG5zID0gdHJ1ZTsNCj4+IC0NCj4+ICtpbnQg
dGhyZWFkX3N0YXJ0ZWQgPSBmYWxzZTsNCj4+ICtwdGhyZWFkX211dGV4X3QgcG11dGV4ID0gUFRI
UkVBRF9NVVRFWF9JTklUSUFMSVpFUjsNCj4+ICtwdGhyZWFkX2NvbmRfdCBwY29uZCA9IFBUSFJF
QURfQ09ORF9JTklUSUFMSVpFUjsNCj4+ICANCj4+ICBUQUlMUV9IRUFEKHRvcGRpcl9saXN0X2hl
YWQsIHRvcGRpcikgdG9wZGlyX2xpc3Q7DQo+PiAgDQo+PiBAQCAtMzYxLDIwICszNjMsNTMgQEAg
Z3NzZF9kZXN0cm95X2NsaWVudChzdHJ1Y3QgY2xudF9pbmZvICpjbHApDQo+PiAgDQo+PiAgc3Rh
dGljIHZvaWQgZ3NzZF9zY2FuKHZvaWQpOw0KPj4gIA0KPj4gK3N0YXRpYyBpbmxpbmUgdm9pZCB3
YWl0X2Zvcl9jaGlsZF9hbmRfZGV0YWNoKHB0aHJlYWRfdCB0aCkNCj4+ICt7DQo+PiArCXB0aHJl
YWRfbXV0ZXhfbG9jaygmcG11dGV4KTsNCj4+ICsJd2hpbGUgKCF0aHJlYWRfc3RhcnRlZCkNCj4+
ICsJCXB0aHJlYWRfY29uZF93YWl0KCZwY29uZCwgJnBtdXRleCk7DQo+PiArCXRocmVhZF9zdGFy
dGVkID0gZmFsc2U7DQo+PiArCXB0aHJlYWRfbXV0ZXhfdW5sb2NrKCZwbXV0ZXgpOw0KPj4gKwlw
dGhyZWFkX2RldGFjaCh0aCk7DQo+PiArfQ0KPj4gKw0KPj4gKy8qIEZvciBlYWNoIHVwY2FsbCBj
cmVhdGUgYSB0aHJlYWQsIGRldGFjaCBmcm9tIHRoZSBtYWluIHByb2Nlc3Mgc28gdGhhdA0KPj4g
KyAqIHJlc291cmNlcyBhcmUgcmVsZWFzZWQgYmFjayBpbnRvIHRoZSBzeXN0ZW0gd2l0aG91dCB0
aGUgbmVlZCBmb3IgYSBqb2luLg0KPj4gKyAqIFdlIG5lZWQgdG8gd2FpdCBmb3IgdGhlIGNoaWxk
IHRocmVhZCB0byBzdGFydCBhbmQgY29uc3VtZSB0aGUgZXZlbnQgZnJvbQ0KPj4gKyAqIHRoZSBm
aWxlIGRlc2NyaXB0b3IuDQo+PiArICovDQo+PiAgc3RhdGljIHZvaWQNCj4+ICBnc3NkX2NsbnRf
Z3NzZF9jYihpbnQgVU5VU0VEKGZkKSwgc2hvcnQgVU5VU0VEKHdoaWNoKSwgdm9pZCAqZGF0YSkN
Cj4+ICB7DQo+PiAgCXN0cnVjdCBjbG50X2luZm8gKmNscCA9IGRhdGE7DQo+PiAtDQo+PiAtCWhh
bmRsZV9nc3NkX3VwY2FsbChjbHApOw0KPj4gKwlwdGhyZWFkX3QgdGg7DQo+PiArCWludCByZXQ7
DQo+PiArDQo+PiArCXJldCA9IHB0aHJlYWRfY3JlYXRlKCZ0aCwgTlVMTCwgKHZvaWQgKiloYW5k
bGVfZ3NzZF91cGNhbGwsDQo+PiArCQkJCSh2b2lkICopY2xwKTsNCj4+ICsJaWYgKHJldCAhPSAw
KSB7DQo+PiArCQlwcmludGVycigwLCAiRVJST1I6IHB0aHJlYWRfY3JlYXRlIGZhaWxlZDogcmV0
ICVkOiAlc1xuIiwNCj4+ICsJCQkgcmV0LCBzdHJlcnJvcihlcnJubykpOw0KPj4gKwkJcmV0dXJu
Ow0KPj4gKwl9DQo+PiArCXdhaXRfZm9yX2NoaWxkX2FuZF9kZXRhY2godGgpOw0KPj4gIH0NCj4+
ICANCj4+ICBzdGF0aWMgdm9pZA0KPj4gIGdzc2RfY2xudF9rcmI1X2NiKGludCBVTlVTRUQoZmQp
LCBzaG9ydCBVTlVTRUQod2hpY2gpLCB2b2lkICpkYXRhKQ0KPj4gIHsNCj4+ICAJc3RydWN0IGNs
bnRfaW5mbyAqY2xwID0gZGF0YTsNCj4+IC0NCj4+IC0JaGFuZGxlX2tyYjVfdXBjYWxsKGNscCk7
DQo+PiArCXB0aHJlYWRfdCB0aDsNCj4+ICsJaW50IHJldDsNCj4+ICsNCj4+ICsJcmV0ID0gcHRo
cmVhZF9jcmVhdGUoJnRoLCBOVUxMLCAodm9pZCAqKWhhbmRsZV9rcmI1X3VwY2FsbCwNCj4+ICsJ
CQkJKHZvaWQgKiljbHApOw0KPj4gKwlpZiAocmV0ICE9IDApIHsNCj4+ICsJCXByaW50ZXJyKDAs
ICJFUlJPUjogcHRocmVhZF9jcmVhdGUgZmFpbGVkOiByZXQgJWQ6ICVzXG4iLA0KPj4gKwkJCSBy
ZXQsIHN0cmVycm9yKGVycm5vKSk7DQo+PiArCQlyZXR1cm47DQo+PiArCX0NCj4+ICsJd2FpdF9m
b3JfY2hpbGRfYW5kX2RldGFjaCh0aCk7DQo+PiAgfQ0KPj4gIA0KPj4gIHN0YXRpYyBzdHJ1Y3Qg
Y2xudF9pbmZvICoNCj4+IGRpZmYgLS1naXQgYS91dGlscy9nc3NkL2dzc2QuaCBiL3V0aWxzL2dz
c2QvZ3NzZC5oDQo+PiBpbmRleCBjNjkzN2M1Li41NjViY2UzIDEwMDY0NA0KPj4gLS0tIGEvdXRp
bHMvZ3NzZC9nc3NkLmgNCj4+ICsrKyBiL3V0aWxzL2dzc2QvZ3NzZC5oDQo+PiBAQCAtMzYsNiAr
MzYsNyBAQA0KPj4gICNpbmNsdWRlIA0KPj4gICNpbmNsdWRlIA0KPj4gICNpbmNsdWRlIA0KPj4g
KyNpbmNsdWRlIA0KPj4gIA0KPj4gICNpZm5kZWYgR1NTRF9QSVBFRlNfRElSDQo+PiAgI2RlZmlu
ZSBHU1NEX1BJUEVGU19ESVIJCSIvdmFyL2xpYi9uZnMvcnBjX3BpcGVmcyINCj4+IEBAIC02MSw2
ICs2MiwxMCBAQCBleHRlcm4gaW50CQkJcm9vdF91c2VzX21hY2hpbmVfY3JlZHM7DQo+PiAgZXh0
ZXJuIHVuc2lnbmVkIGludCAJCWNvbnRleHRfdGltZW91dDsNCj4+ICBleHRlcm4gdW5zaWduZWQg
aW50IHJwY190aW1lb3V0Ow0KPj4gIGV4dGVybiBjaGFyCQkJKnByZWZlcnJlZF9yZWFsbTsNCj4+
ICtleHRlcm4gcHRocmVhZF9tdXRleF90IHBsZV9sb2NrOw0KPj4gK2V4dGVybiBwdGhyZWFkX2Nv
bmRfdCBwY29uZDsNCj4+ICtleHRlcm4gcHRocmVhZF9tdXRleF90IHBtdXRleDsNCj4+ICtleHRl
cm4gaW50IHRocmVhZF9zdGFydGVkOw0KPj4gIA0KPj4gIHN0cnVjdCBjbG50X2luZm8gew0KPj4g
IAlUQUlMUV9FTlRSWShjbG50X2luZm8pCWxpc3Q7DQo+PiBkaWZmIC0tZ2l0IGEvdXRpbHMvZ3Nz
ZC9nc3NkX3Byb2MuYyBiL3V0aWxzL2dzc2QvZ3NzZF9wcm9jLmMNCj4+IGluZGV4IDFlZjY4ZDgu
LmUyZTk1ZGMgMTAwNjQ0DQo+PiAtLS0gYS91dGlscy9nc3NkL2dzc2RfcHJvYy5jDQo+PiArKysg
Yi91dGlscy9nc3NkL2dzc2RfcHJvYy5jDQo+PiBAQCAtNjI5LDM2ICs2MjksNiBAQCBwcm9jZXNz
X2tyYjVfdXBjYWxsKHN0cnVjdCBjbG50X2luZm8gKmNscCwgdWlkX3QgdWlkLCBpbnQgZmQsIGNo
YXIgKnRndG5hbWUsDQo+PiAgCWlmICh1aWQgIT0gMCB8fCAodWlkID09IDAgJiYgcm9vdF91c2Vz
X21hY2hpbmVfY3JlZHMgPT0gMCAmJg0KPj4gIAkJCQlzZXJ2aWNlID09IE5VTEwpKSB7DQo+PiAg
DQo+PiAtCQkvKiBhbHJlYWR5IHJ1bm5pbmcgYXMgdWlkIDAgKi8NCj4+IC0JCWlmICh1aWQgPT0g
MCkNCj4+IC0JCQlnb3RvIG5vX2Zvcms7DQo+PiAtDQo+PiAtCQlwaWQgPSBmb3JrKCk7DQo+PiAt
CQlzd2l0Y2gocGlkKSB7DQo+PiAtCQljYXNlIDA6DQo+PiAtCQkJLyogQ2hpbGQ6IGZhbGwgdGhy
b3VnaCB0byByZXN0IG9mIGZ1bmN0aW9uICovDQo+PiAtCQkJY2hpbGRwaWQgPSBnZXRwaWQoKTsN
Cj4+IC0JCQl1bnNldGVudigiS1JCNUNDTkFNRSIpOw0KPj4gLQkJCXByaW50ZXJyKDIsICJDSElM
RCBmb3JrZWQgcGlkICVkIFxuIiwgY2hpbGRwaWQpOw0KPj4gLQkJCWJyZWFrOw0KPj4gLQkJY2Fz
ZSAtMToNCj4+IC0JCQkvKiBmb3JrKCkgZmFpbGVkISAqLw0KPj4gLQkJCXByaW50ZXJyKDAsICJX
QVJOSU5HOiB1bmFibGUgdG8gZm9yaygpIHRvIGhhbmRsZSINCj4+IC0JCQkJInVwY2FsbDogJXNc
biIsIHN0cmVycm9yKGVycm5vKSk7DQo+PiAtCQkJcmV0dXJuOw0KPj4gLQkJZGVmYXVsdDoNCj4+
IC0JCQkvKiBQYXJlbnQ6IGp1c3Qgd2FpdCBvbiBjaGlsZCB0byBleGl0IGFuZCByZXR1cm4gKi8N
Cj4+IC0JCQlkbyB7DQo+PiAtCQkJCXBpZCA9IHdhaXQoJmVycik7DQo+PiAtCQkJfSB3aGlsZShw
aWQgPT0gLTEgJiYgZXJybm8gIT0gLUVDSElMRCk7DQo+PiAtDQo+PiAtCQkJaWYgKFdJRlNJR05B
TEVEKGVycikpDQo+PiAtCQkJCXByaW50ZXJyKDAsICJXQVJOSU5HOiBmb3JrZWQgY2hpbGQgd2Fz
IGtpbGxlZCINCj4+IC0JCQkJCSAid2l0aCBzaWduYWwgJWRcbiIsIFdURVJNU0lHKGVycikpOw0K
Pj4gLQkJCXJldHVybjsNCj4+IC0JCX0NCj4+IC1ub19mb3JrOg0KPj4gLQ0KPj4gIAkJYXV0aCA9
IGtyYjVfbm90X21hY2hpbmVfY3JlZHMoY2xwLCB1aWQsIHRndG5hbWUsICZkb3duY2FsbF9lcnIs
DQo+PiAgCQkJCQkJJmVyciwgJnJwY19jbG50KTsNCj4+ICAJCWlmIChlcnIpDQo+PiBAQCAtNzM1
LDEyICs3MDUsMjggQEAgb3V0X3JldHVybl9lcnJvcjoNCj4+ICAJZ290byBvdXQ7DQo+PiAgfQ0K
Pj4gIA0KPj4gKy8qIHNpZ25hbCB0byB0aGUgcGFyZW50IHRocmVhZCB0aGF0IHdlIGhhdmUgcmVh
ZCBmcm9tIHRoZSBmaWxlIGRlc2NyaXB0b3IuDQo+PiArICogaXQgc2hvdWxkIGFsbG93IHRoZSBw
YXJlbnQgdG8gcHJvY2VlZCB0byBwb2xsIG9uIHRoZSBkZXNjcmlwdG9yIGZvcg0KPj4gKyAqIHRo
ZSBuZXh0IHVwY2FsbCBmcm9tIHRoZSBrZXJuZWwuDQo+PiArICovDQo+PiArc3RhdGljIHZvaWQN
Cj4+ICtzaWduYWxfcGFyZW50X2V2ZW50X2NvbnN1bWVkKHZvaWQpDQo+PiArew0KPj4gKwlwdGhy
ZWFkX211dGV4X2xvY2soJnBtdXRleCk7DQo+PiArCXRocmVhZF9zdGFydGVkID0gdHJ1ZTsNCj4+
ICsJcHRocmVhZF9jb25kX3NpZ25hbCgmcGNvbmQpOw0KPj4gKwlwdGhyZWFkX211dGV4X3VubG9j
aygmcG11dGV4KTsNCj4+ICt9DQo+PiArDQo+PiAgdm9pZA0KPj4gIGhhbmRsZV9rcmI1X3VwY2Fs
bChzdHJ1Y3QgY2xudF9pbmZvICpjbHApDQo+PiAgew0KPj4gIAl1aWRfdAkJCXVpZDsNCj4+ICsJ
aW50IAkJCXN0YXR1czsNCj4+ICANCj4+IC0JaWYgKHJlYWQoY2xwLT5rcmI1X2ZkLCAmdWlkLCBz
aXplb2YodWlkKSkgPCAoc3NpemVfdClzaXplb2YodWlkKSkgew0KPj4gKwlzdGF0dXMgPSByZWFk
KGNscC0+a3JiNV9mZCwgJnVpZCwgc2l6ZW9mKHVpZCkpIDwgKHNzaXplX3Qpc2l6ZW9mKHVpZCk7
DQo+PiArCXNpZ25hbF9wYXJlbnRfZXZlbnRfY29uc3VtZWQoKTsNCj4+ICsJaWYgKHN0YXR1cykg
ew0KPj4gIAkJcHJpbnRlcnIoMCwgIldBUk5JTkc6IGZhaWxlZCByZWFkaW5nIHVpZCBmcm9tIGty
YjUgIg0KPj4gIAkJCSAgICAidXBjYWxsIHBpcGU6ICVzXG4iLCBzdHJlcnJvcihlcnJubykpOw0K
Pj4gIAkJcmV0dXJuOw0KPj4gQEAgLTc2NSw2ICs3NTEsNyBAQCBoYW5kbGVfZ3NzZF91cGNhbGwo
c3RydWN0IGNsbnRfaW5mbyAqY2xwKQ0KPj4gIAljaGFyCQkJKmVuY3R5cGVzID0gTlVMTDsNCj4+
ICANCj4+ICAJbGJ1ZmxlbiA9IHJlYWQoY2xwLT5nc3NkX2ZkLCBsYnVmLCBzaXplb2YobGJ1Zikp
Ow0KPj4gKwlzaWduYWxfcGFyZW50X2V2ZW50X2NvbnN1bWVkKCk7DQo+PiAgCWlmIChsYnVmbGVu
IDw9IDAgfHwgbGJ1ZltsYnVmbGVuLTFdICE9ICdcbicpIHsNCj4+ICAJCXByaW50ZXJyKDAsICJX
QVJOSU5HOiBoYW5kbGVfZ3NzZF91cGNhbGw6ICINCj4+ICAJCQkgICAgImZhaWxlZCByZWFkaW5n
IHJlcXVlc3RcbiIpOw0KPj4gZGlmZiAtLWdpdCBhL3V0aWxzL2dzc2Qva3JiNV91dGlsLmMgYi91
dGlscy9nc3NkL2tyYjVfdXRpbC5jDQo+PiBpbmRleCA4ZWY4MTg0Li4zMzI4Njk2IDEwMDY0NA0K
Pj4gLS0tIGEvdXRpbHMvZ3NzZC9rcmI1X3V0aWwuYw0KPj4gKysrIGIvdXRpbHMvZ3NzZC9rcmI1
X3V0aWwuYw0KPj4gQEAgLTEyOCw2ICsxMjgsNyBAQA0KPj4gIA0KPj4gIC8qIEdsb2JhbCBsaXN0
IG9mIHByaW5jaXBhbHMvY2FjaGUgZmlsZSBuYW1lcyBmb3IgbWFjaGluZSBjcmVkZW50aWFscyAq
Lw0KPj4gIHN0cnVjdCBnc3NkX2s1X2t0X3ByaW5jICpnc3NkX2s1X2t0X3ByaW5jX2xpc3QgPSBO
VUxMOw0KPj4gK3B0aHJlYWRfbXV0ZXhfdCBwbGVfbG9jayA9IFBUSFJFQURfTVVURVhfSU5JVElB
TElaRVI7DQo+PiAgDQo+PiAgI2lmZGVmIEhBVkVfU0VUX0FMTE9XQUJMRV9FTkNUWVBFUw0KPj4g
IGludCBsaW1pdF90b19sZWdhY3lfZW5jdHlwZXMgPSAwOw0KPj4gQEAgLTU4NiwxMCArNTg3LDEy
IEBAIGdldF9wbGVfYnlfcHJpbmMoa3JiNV9jb250ZXh0IGNvbnRleHQsIGtyYjVfcHJpbmNpcGFs
IHByaW5jKQ0KPj4gIA0KPj4gIAkvKiBOZWVkIHRvIHNlcmlhbGl6ZSBsaXN0IGlmIHdlIGV2ZXIg
YmVjb21lIG11bHRpLXRocmVhZGVkISAqLw0KPj4gIA0KPj4gKwlwdGhyZWFkX211dGV4X2xvY2so
JnBsZV9sb2NrKTsNCj4+ICAJcGxlID0gZmluZF9wbGVfYnlfcHJpbmMoY29udGV4dCwgcHJpbmMp
Ow0KPj4gIAlpZiAocGxlID09IE5VTEwpIHsNCj4+ICAJCXBsZSA9IG5ld19wbGUoY29udGV4dCwg
cHJpbmMpOw0KPj4gIAl9DQo+PiArCXB0aHJlYWRfbXV0ZXhfdW5sb2NrKCZwbGVfbG9jayk7DQo+
PiAgDQo+PiAgCXJldHVybiBwbGU7DQo+PiAgfQ0KPiANCj4gTG9va3MgZ29vZC4gT25lIHRoaW5n
IHRoYXQgbWlnaHQgYmUgbmVhdCBhcyBhIGZvbGxvdyB1cCBpcyB0byBtYWtlIHRoZQ0KPiB0aHJl
YWRfc3RhcnRlZCB2YXJpYWJsZSBhIHBlci1jbG50IHRoaW5nLiBJIGRvbid0IHRoaW5rIHRoZXJl
J3MgYW55DQo+IHJlYXNvbiB0byBzZXJpYWxpemUgSS9PIHRvIGRpZmZlcmVudCBmZHMgdGhlcmUu
DQoNClNpbmNlIHRoZXJlIGlzIG9ubHkgMSBtYWluIHRocmVhZCB0aGF0IHJlYWRzL2NyZWF0ZXMg
dGhlIGV2ZW50IGZvciB0aGUgdGhyZWFkIHRvIGNvbnN1bWUgSSBkb27igJl0IHNlZSBob3cgd2Ug
Y2FuIGhhdmUgbXVsdGlwbGUgdGhyZWFkcyBzdGFydGVkIHdpdGggZGlmZmVyZW50IGZkcy4gSnVz
dCB0byBjbGFyaWZ5LCBtYWluIHRocmVhZCBvbmx5IHdhaXRzIGZvciB0aGUgdXBjYWxsIGluZm8g
dG8gYmUgY29uc3VtZWQgZnJvbSB0aGUgZmQgbm90IGZvciB0aGUgcHJvY2VzcyBvZiB0aGUgdXBj
YWxsLiBUaGVuLCBtdWx0aXBsZSB1cGNhbGxzIGNhbiBiZSBwcm9jZXNzaW5nIGF0IHRoZSBzYW1l
IHRpbWUuDQoNCg0KPiANCj4gUmV2aWV3ZWQtYnk6IEplZmYgTGF5dG9uIDxqbGF5dG9uQHBvb2No
aWVyZWRzLm5ldD4NCg0K

2016-04-28 14:11:14

by Kornievskaia, Olga

[permalink] [raw]
Subject: Re: [PATCH v5 3/3] gssd: always call gss_krb5_ccache_name

DQo+IE9uIEFwciAyOCwgMjAxNiwgYXQgOToxOSBBTSwgSmVmZiBMYXl0b24gPGpsYXl0b25AcG9v
Y2hpZXJlZHMubmV0PiB3cm90ZToNCj4gDQo+IE9uIFdlZCwgMjAxNi0wNC0yNyBhdCAxMjo1OCAt
MDQwMCwgT2xnYSBLb3JuaWV2c2thaWEgd3JvdGU6DQo+PiBQcmV2aW91c2x5IHRoZSBsb2NhdGlv
biBvZiB0aGUgY3JlZGVudGlhbCBjYWNoZSB3YXMgcGFzc2VkIGluIGVpdGhlcg0KPj4gdXNpbmcg
ZW52aXJvbm1lbnQgdmFyaWFibGUgS1JCNUNDTkFNRSBvciBnc3Nfa3JiNV9jY2FjaGVfbmFtZSgp
IGlmDQo+PiBzdXBwb3J0ZWQuIEZvciB0aHJlYWRlZC1nc3NkLCB3ZSBjYW4ndCB1c2UgYW4gZW52
aXJvbm1lbnQgdmFyaWFibGUNCj4+IGFzIGl0J3Mgc2hhcmVkIGFtb25nIGFsbCB0aHJlYWQuIFRo
dXMgYWx3YXlzIHVzZSB0aGUgYXBpIGNhbGwuDQo+PiANCj4+IFNpZ25lZC1vZmYtYnk6IE9sZ2Eg
S29ybmlldnNrYWlhIDxrb2xnYUBuZXRhcHAuY29tPg0KPj4gUmV2aWV3ZWQtYnk6IFN0ZXZlIERp
Y2tzb24gPHN0ZXZlZEByZWRoYXQuY29tPg0KPj4gLS0tDQo+PiAgdXRpbHMvZ3NzZC9nc3NkX3By
b2MuYyB8IDEyICsrKysrKysrKy0tDQo+PiAgdXRpbHMvZ3NzZC9rcmI1X3V0aWwuYyB8IDU2ICsr
KysrKysrKy0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tDQo+PiAgdXRp
bHMvZ3NzZC9rcmI1X3V0aWwuaCB8ICAxIC0NCj4+ICAzIGZpbGVzIGNoYW5nZWQsIDIwIGluc2Vy
dGlvbnMoKyksIDQ5IGRlbGV0aW9ucygtKQ0KPj4gDQo+PiBkaWZmIC0tZ2l0IGEvdXRpbHMvZ3Nz
ZC9nc3NkX3Byb2MuYyBiL3V0aWxzL2dzc2QvZ3NzZF9wcm9jLmMNCj4+IGluZGV4IGRlOGRjMDcu
LjE0Mjk4ZDAgMTAwNjQ0DQo+PiAtLS0gYS91dGlscy9nc3NkL2dzc2RfcHJvYy5jDQo+PiArKysg
Yi91dGlscy9nc3NkL2dzc2RfcHJvYy5jDQo+PiBAQCAtNTQ3LDcgKzU0NywxNSBAQCBrcmI1X3Vz
ZV9tYWNoaW5lX2NyZWRzKHN0cnVjdCBjbG50X2luZm8gKmNscCwgdWlkX3QgdWlkLCBjaGFyICp0
Z3RuYW1lLA0KPj4gIAkJCWdvdG8gb3V0Ow0KPj4gIAkJfQ0KPj4gIAkJZm9yIChjY25hbWUgPSBj
cmVkbGlzdDsgY2NuYW1lICYmICpjY25hbWU7IGNjbmFtZSsrKSB7DQo+PiAtCQkJZ3NzZF9zZXR1
cF9rcmI1X21hY2hpbmVfZ3NzX2NjYWNoZSgqY2NuYW1lKTsNCj4+ICsJCQl1X2ludCBtaW5fc3Rh
dDsNCj4+ICsNCj4+ICsJCQlpZiAoZ3NzX2tyYjVfY2NhY2hlX25hbWUoJm1pbl9zdGF0LCAqY2Nu
YW1lLCBOVUxMKSAhPQ0KPj4gKwkJCQkJR1NTX1NfQ09NUExFVEUpIHsNCj4+ICsJCQkJcHJpbnRl
cnIoMSwgIldBUk5JTkc6IGdzc19rcmI1X2NjYWNoZV9uYW1lICINCj4+ICsJCQkJCSAid2l0aCBu
YW1lICclcycgZmFpbGVkICglcylcbiIsDQo+PiArCQkJCQkgKmNjbmFtZSwgZXJyb3JfbWVzc2Fn
ZShtaW5fc3RhdCkpOw0KPj4gKwkJCQljb250aW51ZTsNCj4+ICsJCQl9DQo+PiAgCQkJaWYgKChj
cmVhdGVfYXV0aF9ycGNfY2xpZW50KGNscCwgdGd0bmFtZSwgcnBjX2NsbnQsDQo+PiAgCQkJCQkJ
JmF1dGgsIHVpZCwNCj4+ICAJCQkJCQlBVVRIVFlQRV9LUkI1LA0KPj4gQEAgLTU2OCw3ICs1NzYs
NyBAQCBrcmI1X3VzZV9tYWNoaW5lX2NyZWRzKHN0cnVjdCBjbG50X2luZm8gKmNscCwgdWlkX3Qg
dWlkLCBjaGFyICp0Z3RuYW1lLA0KPj4gIAkJCQkJICJyZWNyZWF0ZSBjYWNoZSBmb3Igc2VydmVy
ICVzXG4iLA0KPj4gIAkJCQkJY2xwLT5zZXJ2ZXJuYW1lKTsNCj4+ICAJCQl9IGVsc2Ugew0KPj4g
LQkJCQlwcmludGVycigxLCAiV0FSTklORzogRmFpbGVkIHRvIGNyZWF0ZSBtYWNoaW5lIg0KPj4g
KwkJCQlwcmludGVycigwLCAiRVJST1I6IEZhaWxlZCB0byBjcmVhdGUgbWFjaGluZSINCj4+ICAJ
CQkJCSAia3JiNSBjb250ZXh0IHdpdGggYW55IGNyZWRlbnRpYWxzIg0KPj4gIAkJCQkJICJjYWNo
ZSBmb3Igc2VydmVyICVzXG4iLA0KPj4gIAkJCQkJY2xwLT5zZXJ2ZXJuYW1lKTsNCj4+IGRpZmYg
LS1naXQgYS91dGlscy9nc3NkL2tyYjVfdXRpbC5jIGIvdXRpbHMvZ3NzZC9rcmI1X3V0aWwuYw0K
Pj4gaW5kZXggMzMyODY5Ni4uN2I3NGFiMyAxMDA2NDQNCj4+IC0tLSBhL3V0aWxzL2dzc2Qva3Ji
NV91dGlsLmMNCj4+ICsrKyBiL3V0aWxzL2dzc2Qva3JiNV91dGlsLmMNCj4+IEBAIC00NjgsMzcg
KzQ2OCw2IEBAIGdzc2RfZ2V0X3NpbmdsZV9rcmI1X2NyZWQoa3JiNV9jb250ZXh0IGNvbnRleHQs
DQo+PiAgfQ0KPj4gIA0KPj4gIC8qDQo+PiAtICogRGVwZW5kaW5nIG9uIHRoZSB2ZXJzaW9uIG9m
IEtlcmJlcm9zLCB3ZSBlaXRoZXIgbmVlZCB0byB1c2UNCj4+IC0gKiBhIHByaXZhdGUgZnVuY3Rp
b24sIG9yIHNpbXBseSBzZXQgdGhlIGVudmlyb25tZW50IHZhcmlhYmxlLg0KPj4gLSAqLw0KPj4g
LXN0YXRpYyB2b2lkDQo+PiAtZ3NzZF9zZXRfa3JiNV9jY2FjaGVfbmFtZShjaGFyICpjY25hbWUp
DQo+PiAtew0KPj4gLSNpZmRlZiBVU0VfR1NTX0tSQjVfQ0NBQ0hFX05BTUUNCj4+IC0JdV9pbnQJ
bWFqX3N0YXQsIG1pbl9zdGF0Ow0KPj4gLQ0KPj4gLQlwcmludGVycigzLCAidXNpbmcgZ3NzX2ty
YjVfY2NhY2hlX25hbWUgdG8gc2VsZWN0IGtyYjUgY2NhY2hlICVzXG4iLA0KPj4gLQkJIGNjbmFt
ZSk7DQo+PiAtCW1hal9zdGF0ID0gZ3NzX2tyYjVfY2NhY2hlX25hbWUoJm1pbl9zdGF0LCBjY25h
bWUsIE5VTEwpOw0KPj4gLQlpZiAobWFqX3N0YXQgIT0gR1NTX1NfQ09NUExFVEUpIHsNCj4+IC0J
CXByaW50ZXJyKDAsICJXQVJOSU5HOiBnc3Nfa3JiNV9jY2FjaGVfbmFtZSB3aXRoICINCj4+IC0J
CQkibmFtZSAnJXMnIGZhaWxlZCAoJXMpXG4iLA0KPj4gLQkJCWNjbmFtZSwgZXJyb3JfbWVzc2Fn
ZShtaW5fc3RhdCkpOw0KPj4gLQl9DQo+PiAtI2Vsc2UNCj4+IC0JLyoNCj4+IC0JICogU2V0IHRo
ZSBLUkI1Q0NOQU1FIGVudmlyb25tZW50IHZhcmlhYmxlIHRvIHRlbGwgdGhlIGtyYjUgY29kZQ0K
Pj4gLQkgKiB3aGljaCBjcmVkZW50aWFscyBjYWNoZSB0byB1c2UuICAoSW5zdGVhZCBvZiB1c2lu
ZyB0aGUgcHJpdmF0ZQ0KPj4gLQkgKiBmdW5jdGlvbiBhYm92ZSBmb3Igd2hpY2ggdGhlcmUgaXMg
bm8gZ2VuZXJpYyBnc3NhcGkNCj4+IC0JICogZXF1aXZhbGVudC4pDQo+PiAtCSAqLw0KPj4gLQlw
cmludGVycigzLCAidXNpbmcgZW52aXJvbm1lbnQgdmFyaWFibGUgdG8gc2VsZWN0IGtyYjUgY2Nh
Y2hlICVzXG4iLA0KPj4gLQkJIGNjbmFtZSk7DQo+PiAtCXNldGVudigiS1JCNUNDTkFNRSIsIGNj
bmFtZSwgMSk7DQo+PiAtI2VuZGlmDQo+PiAtfQ0KPj4gLQ0KPj4gLS8qDQo+PiAgICogR2l2ZW4g
YSBwcmluY2lwYWwsIGZpbmQgYSBtYXRjaGluZyBwbGUgc3RydWN0dXJlDQo+PiAgICovDQo+PiAg
c3RhdGljIHN0cnVjdCBnc3NkX2s1X2t0X3ByaW5jICoNCj4+IEBAIC0xMDk0LDYgKzEwNjMsNyBA
QCBnc3NkX3NldHVwX2tyYjVfdXNlcl9nc3NfY2NhY2hlKHVpZF90IHVpZCwgY2hhciAqc2VydmVy
bmFtZSwgY2hhciAqZGlycGF0dGVybikNCj4+ICAJY29uc3QgY2hhcgkJKmNjdHlwZTsNCj4+ICAJ
c3RydWN0IGRpcmVudAkJKmQ7DQo+PiAgCWludAkJCWVyciwgaSwgajsNCj4+ICsJdV9pbnQJCQlt
YWpfc3RhdCwgbWluX3N0YXQ7DQo+PiAgDQo+PiAgCXByaW50ZXJyKDMsICJsb29raW5nIGZvciBj
bGllbnQgY3JlZHMgd2l0aCB1aWQgJXUgZm9yICINCj4+ICAJCSAgICAic2VydmVyICVzIGluICVz
XG4iLCB1aWQsIHNlcnZlcm5hbWUsIGRpcnBhdHRlcm4pOw0KPj4gQEAgLTExMjksMjIgKzEwOTks
MTYgQEAgZ3NzZF9zZXR1cF9rcmI1X3VzZXJfZ3NzX2NjYWNoZSh1aWRfdCB1aWQsIGNoYXIgKnNl
cnZlcm5hbWUsIGNoYXIgKmRpcnBhdHRlcm4pDQo+PiAgDQo+PiAgCXByaW50ZXJyKDIsICJ1c2lu
ZyAlcyBhcyBjcmVkZW50aWFscyBjYWNoZSBmb3IgY2xpZW50IHdpdGggIg0KPj4gIAkJICAgICJ1
aWQgJXUgZm9yIHNlcnZlciAlc1xuIiwgYnVmLCB1aWQsIHNlcnZlcm5hbWUpOw0KPj4gLQlnc3Nk
X3NldF9rcmI1X2NjYWNoZV9uYW1lKGJ1Zik7DQo+PiAtCXJldHVybiAwOw0KPj4gLX0NCj4+ICAN
Cj4+IC0vKg0KPj4gLSAqIExldCB0aGUgZ3NzIGNvZGUga25vdyB3aGVyZSB0byBmaW5kIHRoZSBt
YWNoaW5lIGNyZWRlbnRpYWxzIGNjYWNoZS4NCj4+IC0gKg0KPj4gLSAqIFJldHVybnM6DQo+PiAt
ICoJdm9pZA0KPj4gLSAqLw0KPj4gLXZvaWQNCj4+IC1nc3NkX3NldHVwX2tyYjVfbWFjaGluZV9n
c3NfY2NhY2hlKGNoYXIgKmNjbmFtZSkNCj4+IC17DQo+PiAtCXByaW50ZXJyKDIsICJ1c2luZyAl
cyBhcyBjcmVkZW50aWFscyBjYWNoZSBmb3IgbWFjaGluZSBjcmVkc1xuIiwNCj4+IC0JCSBjY25h
bWUpOw0KPj4gLQlnc3NkX3NldF9rcmI1X2NjYWNoZV9uYW1lKGNjbmFtZSk7DQo+PiArCXByaW50
ZXJyKDMsICJ1c2luZyBnc3Nfa3JiNV9jY2FjaGVfbmFtZSB0byBzZWxlY3Qga3JiNSBjY2FjaGUg
JXNcbiIsDQo+PiArCQkgYnVmKTsNCj4+ICsJbWFqX3N0YXQgPSBnc3Nfa3JiNV9jY2FjaGVfbmFt
ZSgmbWluX3N0YXQsIGJ1ZiwgTlVMTCk7DQo+PiArCWlmIChtYWpfc3RhdCAhPSBHU1NfU19DT01Q
TEVURSkgew0KPj4gKwkJcHJpbnRlcnIoMCwgIkVSUk9SOiB1bmFibGUgdG8gZ2V0IHVzZXIgY3Jl
ZCBjYWNoZSAnJXMnICINCj4+ICsJCQkgImZhaWxlZCAoJXMpXG4iLCBidWYsIGVycm9yX21lc3Nh
Z2UobWluX3N0YXQpKTsNCj4+ICsJCXJldHVybiBtYWpfc3RhdDsNCj4+ICsJfQ0KPj4gKwlyZXR1
cm4gMDsNCj4+ICB9DQo+PiAgDQo+PiAgLyoNCj4+IGRpZmYgLS1naXQgYS91dGlscy9nc3NkL2ty
YjVfdXRpbC5oIGIvdXRpbHMvZ3NzZC9rcmI1X3V0aWwuaA0KPj4gaW5kZXggYTMxOTU4OC4uZDNi
MDc3NyAxMDA2NDQNCj4+IC0tLSBhL3V0aWxzL2dzc2Qva3JiNV91dGlsLmgNCj4+ICsrKyBiL3V0
aWxzL2dzc2Qva3JiNV91dGlsLmgNCj4+IEBAIC0yNyw3ICsyNyw2IEBAIGludCBnc3NkX3NldHVw
X2tyYjVfdXNlcl9nc3NfY2NhY2hlKHVpZF90IHVpZCwgY2hhciAqc2VydmVybmFtZSwNCj4+ICAJ
CQkJICAgICBjaGFyICpkaXJuYW1lKTsNCj4+ICBpbnQgIGdzc2RfZ2V0X2tyYjVfbWFjaGluZV9j
cmVkX2xpc3QoY2hhciAqKipsaXN0KTsNCj4+ICB2b2lkIGdzc2RfZnJlZV9rcmI1X21hY2hpbmVf
Y3JlZF9saXN0KGNoYXIgKipsaXN0KTsNCj4+IC12b2lkIGdzc2Rfc2V0dXBfa3JiNV9tYWNoaW5l
X2dzc19jY2FjaGUoY2hhciAqc2VydmVybmFtZSk7DQo+PiAgdm9pZCBnc3NkX2Rlc3Ryb3lfa3Ji
NV9tYWNoaW5lX2NyZWRzKHZvaWQpOw0KPj4gIGludCAgZ3NzZF9yZWZyZXNoX2tyYjVfbWFjaGlu
ZV9jcmVkZW50aWFsKGNoYXIgKmhvc3RuYW1lLA0KPj4gIAkJCQkJICBzdHJ1Y3QgZ3NzZF9rNV9r
dF9wcmluYyAqcGxlLCANCj4gDQo+IExvb2tzIGZpbmUgdG8gbWUsIGJ1dCBJIHdvbmRlciB3aHkg
dGhpcyBlbnYgdmFyIGZyb2JiaW5nIGlzIHRoZXJlLiBEaWQNCj4gb2xkIE1JVCBrcmI1IGxpYnMg
bGFjayB0aGF0IGZ1bmN0aW9uPyBJbiBhbnkgY2FzZSwgdGhleSBzZWVtIHRvIGhhdmUgaXQNCj4g
bm93LCBzbyBJJ20gZmluZSB3aXRoIHRoaXMuDQoNCkNvcnJlY3QgcHJpb3IgdG8gc29tZSB2ZXJz
aW9uIHRoZXJlIHdhcyBubyBzdWNoIGZ1bmN0aW9uIHRoZXJlLiANCg0KPiBBbHNvLCBJIHRoaW5r
IHlvdSBjYW4gcHJvYmFibHkgcmVtb3ZlIFVTRV9HU1NfS1JCNV9DQ0FDSEVfTkFNRQ0KPiBhbHRv
Z2V0aGVyIGFzIEkgZG9uJ3QgdGhpbmsgaXQnbGwgYmUgdXNlZCBhZnRlciB0aGlzIHBhdGNoLg0K
DQpJIHRoaW5rIGl0IGlzIHJlbW92ZWTigKYgOi0vIGdyZXAgZm9yIENDTkFNRSBjb21lcyB1cCBl
bXB0eS4NCg0KPiANCj4gUmV2aWV3ZWQtYnk6IEplZmYgTGF5dG9uIDxqbGF5dG9uQHBvb2NoaWVy
ZWRzLm5ldD4NCg0K

2016-04-28 20:43:27

by Steve Dickson

[permalink] [raw]
Subject: Re: [PATCH v5 3/3] gssd: always call gss_krb5_ccache_name



On 04/28/2016 10:11 AM, Kornievskaia, Olga wrote:
>> Also, I think you can probably remove USE_GSS_KRB5_CCACHE_NAME
Good call... Its gone!

>> > altogether as I don't think it'll be used after this patch.
> I think it is removed… :-/ grep for CCNAME comes up empty.
>
Its in the aclocal/kerberos5.m4 file... your favorite files! 8-)

steved.

2016-04-29 14:49:06

by Steve Dickson

[permalink] [raw]
Subject: Re: [RFC PATCH v5 0/3] adding pthread support to gssd



On 04/27/2016 12:58 PM, Olga Kornievskaia wrote:
> 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
>
All three patches have been committed... thanks for doing this work!!

steved.

2016-04-29 14:57:22

by Kornievskaia, Olga

[permalink] [raw]
Subject: Re: [RFC PATCH v5 0/3] adding pthread support to gssd


> On Apr 29, 2016, at 10:49 AM, Steve Dickson <[email protected]> wrote:
>
>
>
> On 04/27/2016 12:58 PM, Olga Kornievskaia wrote:
>> 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
>>
> All three patches have been committed... thanks for doing this work!!

Steve, Jeff thanks for the reviews. I?m onto the next step of making a pool of threads.

>
> steved.


2016-04-30 12:29:11

by Jeff Layton

[permalink] [raw]
Subject: Re: [PATCH v5 1/3] gssd: use pthreads to handle upcalls

On Thu, 2016-04-28 at 14:07 +0000, Kornievskaia, Olga wrote:
> >
> > On Apr 28, 2016, at 9:13 AM, Jeff Layton <[email protected]>
> > wrote:
> >
> > On Wed, 2016-04-27 at 12:58 -0400, 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..b676341 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 inline 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 
> > >  #include 
> > >  #include 
> > > +#include 
> > >  
> > >  #ifndef GSSD_PIPEFS_DIR
> > >  #define GSSD_PIPEFS_DIR "/var/lib/nfs/rpc_pipefs"
> > > @@ -61,6 +62,10 @@ extern int root_uses_ma
> > > chine_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;
> > >  }
> > Looks good. One thing that might be neat as a follow up is to make
> > the
> > thread_started variable a per-clnt thing. I don't think there's any
> > reason to serialize I/O to different fds there.
> Since there is only 1 main thread that reads/creates the event for
> the thread to consume I don’t see how we can have multiple threads
> started with different fds. Just to clarify, main thread only waits
> for the upcall info to be consumed from the fd not for the process of
> the upcall. Then, multiple upcalls can be processing at the same
> time.
>

Ok, yeah you do need to serialize there because of the event handling.
You can't return from the event handler until the read event has been
consumed or or it would get mighty confused.

Still, I wonder -- might it be cleaner to do the read in the context of
the event loop and then just spawn a detached thread to handle the
result?

It seems like that would be less back-and-forth between the parent and
child. You also wouldn't need the pmutex and condition variable with
that. You would have to come up with some way to pass both "clp" and
"uid" to the child however.

Either way, that could be reworked later and this is a clear
improvement. Nice work, btw!
--
Jeff Layton <[email protected]>


2016-04-30 16:52:17

by Olga Kornievskaia

[permalink] [raw]
Subject: Re: [PATCH v5 1/3] gssd: use pthreads to handle upcalls

On Sat, Apr 30, 2016 at 8:29 AM, Jeff Layton <[email protected]> wrote:
> On Thu, 2016-04-28 at 14:07 +0000, Kornievskaia, Olga wrote:
>> >
>> > On Apr 28, 2016, at 9:13 AM, Jeff Layton <[email protected]>
>> > wrote:
>> >
>> > On Wed, 2016-04-27 at 12:58 -0400, 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..b676341 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 inline 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
>> > > #include
>> > > #include
>> > > +#include
>> > >
>> > > #ifndef GSSD_PIPEFS_DIR
>> > > #define GSSD_PIPEFS_DIR "/var/lib/nfs/rpc_pipefs"
>> > > @@ -61,6 +62,10 @@ extern int root_uses_ma
>> > > chine_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;
>> > > }
>> > Looks good. One thing that might be neat as a follow up is to make
>> > the
>> > thread_started variable a per-clnt thing. I don't think there's any
>> > reason to serialize I/O to different fds there.
>> Since there is only 1 main thread that reads/creates the event for
>> the thread to consume I don’t see how we can have multiple threads
>> started with different fds. Just to clarify, main thread only waits
>> for the upcall info to be consumed from the fd not for the process of
>> the upcall. Then, multiple upcalls can be processing at the same
>> time.
>>
>
> Ok, yeah you do need to serialize there because of the event handling.
> You can't return from the event handler until the read event has been
> consumed or or it would get mighty confused.
>
> Still, I wonder -- might it be cleaner to do the read in the context of
> the event loop and then just spawn a detached thread to handle the
> result?
>
> It seems like that would be less back-and-forth between the parent and
> child. You also wouldn't need the pmutex and condition variable with
> that. You would have to come up with some way to pass both "clp" and
> "uid" to the child however.
>
> Either way, that could be reworked later and this is a clear
> improvement. Nice work, btw!

I think that’s a good idea, thanks. Are you suggesting to read and
parse in the main thread (the uid as well as other info is known after
parsing)? Or we can do the read() and pass the buffer for the child to
parse. Instead of doing conditioned wait the main thread need to do a
bit more work --read + memory allocation. Right now child thread has
static memory for the read buffer. With a pool of threads I think we’d
need to do the read in the main thread anyway.

2016-04-30 17:29:43

by Jeff Layton

[permalink] [raw]
Subject: Re: [PATCH v5 1/3] gssd: use pthreads to handle upcalls

On Sat, 2016-04-30 at 15:16 +0000, Kornievskaia, Olga wrote:
>
> > On Apr 30, 2016, at 8:29 AM, Jeff Layton <[email protected]>
> > wrote:
> >
> > On Thu, 2016-04-28 at 14:07 +0000, Kornievskaia, Olga wrote:
> > > >
> > > > On Apr 28, 2016, at 9:13 AM, Jeff Layton <[email protected]
> > > > et>
> > > > wrote:
> > > >
> > > > On Wed, 2016-04-27 at 12:58 -0400, 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..b676341 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 inline 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);
> > > > > +}
> > > > > +

Another relatively minor nit as well: Since you know that you're always
going to detach these threads, it's probably more efficient to just
create them in an already-detached state (a'la
pthread_attr_setdetachstate). AIUI, then the thread spawning code can
skip setting up the stuff to allow joins.


> > > > > +/* 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 
> > > > >  #include 
> > > > >  #include 
> > > > > +#include 
> > > > >  
> > > > >  #ifndef GSSD_PIPEFS_DIR
> > > > >  #define GSSD_PIPEFS_DIR  "/var/lib/nfs/rpc_pipefs"
> > > > > @@ -61,6 +62,10 @@ extern int
> > > > > root_uses_ma
> > > > > chine_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;
> > > > >  }
> > > >  Looks good. One thing that might be neat as a follow up is to
> > > > make
> > > > the
> > > > thread_started variable a per-clnt thing. I don't think there's
> > > > any
> > > > reason to serialize I/O to different fds there.
> > >  Since there is only 1 main thread that reads/creates the event
> > > for
> > > the thread to consume I don’t see how we can have multiple
> > > threads
> > > started with different fds. Just to clarify, main thread only
> > > waits
> > > for the upcall info to be consumed from the fd not for the
> > > process of
> > > the upcall. Then, multiple upcalls can be processing at the same
> > > time.
> > >
> >  
> > Ok, yeah you do need to serialize there because of the event
> > handling.
> > You can't return from the event handler until the read event has
> > been
> > consumed or or it would get mighty confused.
> >
> > Still, I wonder -- might it be cleaner to do the read in the
> > context of
> > the event loop and then just spawn a detached thread to handle the
> > result?
>  
> It seems like that would be less back-and-forth between the parent
> and
> child. You also wouldn't need the pmutex and condition variable with
> that. You would have to come up with some way to pass both "clp" and
> "uid" to the child however.
>
> Either way, that could be reworked later and this is a clear
> improvement. Nice work, btw!
> I think that’s a good idea, thanks.  Are you suggesting to read and
> parse in the main thread (the uid as well as other info is known
> after parsing)? Or we can do the read() and pass the buffer for the
> child to parse. Instead of doing conditioned wait the main thread
> need to do a bit more work --read + memory allocation. Right now
> child thread has static memory for the read buffer. With a pool of
> threads I think we’d need to do the read in the main thread anyway.
>  

Yeah, the usual thing to do is to do the minimum amount that must be
serialized in the context of the event loop (the read in this case) and
then dispatch a thread to do the rest.

I don't think you'd get away without doing an allocation here of some
sort though if you go that route. Create a struct that has the
clnt_info and uid and then pass that in the thread's arg.

--
Jeff Layton <[email protected]>