Hello,
On 04/20/2016 05:11 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 <steved@xxxxxxxxxx>
> Signed-off-by: Olga Kornievskaia <kolga@xxxxxxxxxx>
> ---
> 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 | 44 ++++++++++++--------------------------------
> utils/gssd/krb5_util.c | 3 +++
> 7 files changed, 78 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..a41b5ad 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,54 @@ gssd_destroy_client(struct clnt_info *clp)
>
> static void gssd_scan(void);
>
> +/* 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;
> + }
> + pthread_mutex_lock(&pmutex);
> + while(!thread_started)
> + pthread_cond_wait(&pcond, &pmutex);
> + thread_started = false;
> + pthread_mutex_unlock(&pmutex);
> + pthread_detach(th);
This synchronization works! I'm no longer seeing any read errors.
But I would like document what they are doing.
So we don't have the same comments in two different
places, I suggest you put all this calls in a macro, like
wait_for_cthread() or something... whatever you thinks
works..
Then where the macro is define you can document what is
doing and way its needed... Plus this condenses 6
lines into one (hopefully) meaningful line.
> }
>
> static void
> gssd_clnt_krb5_cb(int UNUSED(fd), short UNUSED(which), void *data)
> {
> struct clnt_info *clp = data;
> + 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;
> + }
>
> - handle_krb5_upcall(clp);
> + pthread_mutex_lock(&pmutex);
> + while(!thread_started)
> + pthread_cond_wait(&pcond, &pmutex);
> + thread_started = false;
> + pthread_mutex_unlock(&pmutex);
> + pthread_detach(th);
The same macro here
> }
>
> 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..581a125 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)
> @@ -739,8 +709,14 @@ void
> handle_krb5_upcall(struct clnt_info *clp)
> {
> uid_t uid;
> -
> - if (read(clp->krb5_fd, &uid, sizeof(uid)) < (ssize_t)sizeof(uid)) {
> + int status;
> +
> + status = read(clp->krb5_fd, &uid, sizeof(uid)) < (ssize_t)sizeof(uid);
> + pthread_mutex_lock(&pmutex);
> + thread_started = true;
> + pthread_cond_signal(&pcond);
> + pthread_mutex_unlock(&pmutex);
A second macro here like phone_home() 8-) That's a joke! :-)
> + if (status) {
> printerr(0, "WARNING: failed reading uid from krb5 "
> "upcall pipe: %s\n", strerror(errno));
> return;
> @@ -765,6 +741,10 @@ handle_gssd_upcall(struct clnt_info *clp)
> char *enctypes = NULL;
>
> lbuflen = read(clp->gssd_fd, lbuf, sizeof(lbuf));
> + pthread_mutex_lock(&pmutex);
> + thread_started = true;
> + pthread_cond_signal(&pcond);
> + pthread_mutex_unlock(&pmutex);
The same second macro here as well...
Nice work!!
steved.
> 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;
> }
>
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
