Received-SPF: pass (google.com: domain of linux-nfs-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
From:   Scott Mayhew <smayhew@redhat.com>
To:     linux-nfs@vger.kernel.org
Subject: [nfs-utils RFC PATCH 1/2] gssd: deal with failed thread creation
Date:   Tue, 25 May 2021 14:00:32 -0400
Message-Id: <20210525180033.200404-2-smayhew@redhat.com>
In-Reply-To: <20210525180033.200404-1-smayhew@redhat.com>
References: <20210525180033.200404-1-smayhew@redhat.com>
MIME-Version: 1.0
Content-Transfer-Encoding: 8bit
Precedence: bulk

If we fail to create a thread to handle an upcall, we still need to do a
downcall to tell the kernel about the failure, otherwise the process
that is trying to establish gss credentials will hang.

This patch shifts the thread creation down a level in the call chain so
now the main thread does a little more work up front (reading & parsing
the data from the pipefs file) so it has the info it needs to be able
to do the error downcall.

Signed-off-by: Scott Mayhew <smayhew@redhat.com>
---
 utils/gssd/gssd.c      |  83 +-----------------
 utils/gssd/gssd.h      |  11 ++-
 utils/gssd/gssd_proc.c | 190 +++++++++++++++++++++++++++++++++--------
 3 files changed, 166 insertions(+), 118 deletions(-)

diff --git a/utils/gssd/gssd.c b/utils/gssd/gssd.c
index 1541d371..eb440470 100644
--- a/utils/gssd/gssd.c
+++ b/utils/gssd/gssd.c
@@ -364,7 +364,7 @@ out:
 /* Actually frees clp and fields that might be used from other
  * threads if was last reference.
  */
-static void
+void
 gssd_free_client(struct clnt_info *clp)
 {
 	int refcnt;
@@ -416,55 +416,6 @@ gssd_destroy_client(struct clnt_info *clp)
 
 static void gssd_scan(void);
 
-static int
-start_upcall_thread(void (*func)(struct clnt_upcall_info *), void *info)
-{
-	pthread_attr_t attr;
-	pthread_t th;
-	int ret;
-
-	ret = pthread_attr_init(&attr);
-	if (ret != 0) {
-		printerr(0, "ERROR: failed to init pthread attr: ret %d: %s\n",
-			 ret, strerror(errno));
-		return ret;
-	}
-	ret = pthread_attr_setdetachstate(&attr, PTHREAD_CREATE_DETACHED);
-	if (ret != 0) {
-		printerr(0, "ERROR: failed to create pthread attr: ret %d: "
-			 "%s\n", ret, strerror(errno));
-		return ret;
-	}
-
-	ret = pthread_create(&th, &attr, (void *)func, (void *)info);
-	if (ret != 0)
-		printerr(0, "ERROR: pthread_create failed: ret %d: %s\n",
-			 ret, strerror(errno));
-	return ret;
-}
-
-static struct clnt_upcall_info *alloc_upcall_info(struct clnt_info *clp)
-{
-	struct clnt_upcall_info *info;
-
-	info = malloc(sizeof(struct clnt_upcall_info));
-	if (info == NULL)
-		return NULL;
-
-	pthread_mutex_lock(&clp_lock);
-	clp->refcount++;
-	pthread_mutex_unlock(&clp_lock);
-	info->clp = clp;
-
-	return info;
-}
-
-void free_upcall_info(struct clnt_upcall_info *info)
-{
-	gssd_free_client(info->clp);
-	free(info);
-}
-
 /* For each upcall read the upcall info into the buffer, then create a
  * thread in a detached state so that resources are released back into
  * the system without the need for a join.
@@ -473,44 +424,16 @@ static void
 gssd_clnt_gssd_cb(int UNUSED(fd), short UNUSED(which), void *data)
 {
 	struct clnt_info *clp = data;
-	struct clnt_upcall_info *info;
-
-	info = alloc_upcall_info(clp);
-	if (info == NULL)
-		return;
 
-	info->lbuflen = read(clp->gssd_fd, info->lbuf, sizeof(info->lbuf));
-	if (info->lbuflen <= 0 || info->lbuf[info->lbuflen-1] != '\n') {
-		printerr(0, "WARNING: %s: failed reading request\n", __func__);
-		free_upcall_info(info);
-		return;
-	}
-	info->lbuf[info->lbuflen-1] = 0;
-
-	if (start_upcall_thread(handle_gssd_upcall, info))
-		free_upcall_info(info);
+	handle_gssd_upcall(clp);
 }
 
 static void
 gssd_clnt_krb5_cb(int UNUSED(fd), short UNUSED(which), void *data)
 {
 	struct clnt_info *clp = data;
-	struct clnt_upcall_info *info;
-
-	info = alloc_upcall_info(clp);
-	if (info == NULL)
-		return;
-
-	if (read(clp->krb5_fd, &info->uid,
-			sizeof(info->uid)) < (ssize_t)sizeof(info->uid)) {
-		printerr(0, "WARNING: %s: failed reading uid from krb5 "
-			 "upcall pipe: %s\n", __func__, strerror(errno));
-		free_upcall_info(info);
-		return;
-	}
 
-	if (start_upcall_thread(handle_krb5_upcall, info))
-		free_upcall_info(info);
+	handle_krb5_upcall(clp);
 }
 
 static struct clnt_info *
diff --git a/utils/gssd/gssd.h b/utils/gssd/gssd.h
index 1e8c58d4..6d53647e 100644
--- a/utils/gssd/gssd.h
+++ b/utils/gssd/gssd.h
@@ -84,14 +84,17 @@ struct clnt_info {
 
 struct clnt_upcall_info {
 	struct clnt_info 	*clp;
-	char			lbuf[RPC_CHAN_BUF_SIZE];
-	int			lbuflen;
 	uid_t			uid;
+	int			fd;
+	char			*srchost;
+	char			*target;
+	char			*service;
 };
 
-void handle_krb5_upcall(struct clnt_upcall_info *clp);
-void handle_gssd_upcall(struct clnt_upcall_info *clp);
+void handle_krb5_upcall(struct clnt_info *clp);
+void handle_gssd_upcall(struct clnt_info *clp);
 void free_upcall_info(struct clnt_upcall_info *info);
+void gssd_free_client(struct clnt_info *clp);
 
 
 #endif /* _RPC_GSSD_H_ */
diff --git a/utils/gssd/gssd_proc.c b/utils/gssd/gssd_proc.c
index e830f497..ba508b30 100644
--- a/utils/gssd/gssd_proc.c
+++ b/utils/gssd/gssd_proc.c
@@ -80,6 +80,8 @@
 #include "nfslib.h"
 #include "gss_names.h"
 
+extern pthread_mutex_t clp_lock;
+
 /* Encryption types supported by the kernel rpcsec_gss code */
 int num_krb5_enctypes = 0;
 krb5_enctype *krb5_enctypes = NULL;
@@ -723,22 +725,135 @@ out_return_error:
 	goto out;
 }
 
-void
-handle_krb5_upcall(struct clnt_upcall_info *info)
+static struct clnt_upcall_info *
+alloc_upcall_info(struct clnt_info *clp, uid_t uid, int fd, char *srchost,
+		  char *target, char *service)
 {
-	struct clnt_info *clp = info->clp;
+	struct clnt_upcall_info *info;
+
+	info = malloc(sizeof(struct clnt_upcall_info));
+	if (info == NULL)
+		return NULL;
+
+	memset(info, 0, sizeof(*info));
+	pthread_mutex_lock(&clp_lock);
+	clp->refcount++;
+	pthread_mutex_unlock(&clp_lock);
+	info->clp = clp;
+	info->uid = uid;
+	info->fd = fd;
+	if (srchost) {
+		info->srchost = strdup(srchost);
+		if (info->srchost == NULL)
+			goto out_info;
+	}
+	if (target) {
+		info->target = strdup(target);
+		if (info->target == NULL)
+			goto out_srchost;
+	}
+	if (service) {
+		info->service = strdup(service);
+		if (info->service == NULL)
+			goto out_target;
+	}
+
+out:
+	return info;
+
+out_target:
+	if (info->target)
+		free(info->target);
+out_srchost:
+	if (info->srchost)
+		free(info->srchost);
+out_info:
+	free(info);
+	info = NULL;
+	goto out;
+}
 
-	printerr(2, "\n%s: uid %d (%s)\n", __func__, info->uid, clp->relpath);
+void free_upcall_info(struct clnt_upcall_info *info)
+{
+	gssd_free_client(info->clp);
+	if (info->service)
+		free(info->service);
+	if (info->target)
+		free(info->target);
+	if (info->srchost)
+		free(info->srchost);
+	free(info);
+}
 
-	process_krb5_upcall(clp, info->uid, clp->krb5_fd, NULL, NULL, NULL);
+static void
+gssd_work_thread_fn(struct clnt_upcall_info *info)
+{
+	process_krb5_upcall(info->clp, info->uid, info->fd, info->srchost, info->target, info->service);
 	free_upcall_info(info);
 }
 
+static int
+start_upcall_thread(void (*func)(struct clnt_upcall_info *), void *info)
+{
+	pthread_attr_t attr;
+	pthread_t th;
+	int ret;
+
+	ret = pthread_attr_init(&attr);
+	if (ret != 0) {
+		printerr(0, "ERROR: failed to init pthread attr: ret %d: %s\n",
+			 ret, strerror(errno));
+		return ret;
+	}
+	ret = pthread_attr_setdetachstate(&attr, PTHREAD_CREATE_DETACHED);
+	if (ret != 0) {
+		printerr(0, "ERROR: failed to create pthread attr: ret %d: "
+			 "%s\n", ret, strerror(errno));
+		return ret;
+	}
+
+	ret = pthread_create(&th, &attr, (void *)func, (void *)info);
+	if (ret != 0)
+		printerr(0, "ERROR: pthread_create failed: ret %d: %s\n",
+			 ret, strerror(errno));
+	else
+		printerr(0, "created thread id 0x%lx\n", th);
+	return ret;
+}
+
+void
+handle_krb5_upcall(struct clnt_info *clp)
+{
+	uid_t			uid;
+	struct clnt_upcall_info	*info;
+	int			err;
+
+	if (read(clp->krb5_fd, &uid, sizeof(uid)) < (ssize_t)sizeof(uid)) {
+		printerr(0, "WARNING: failed reading uid from krb5 "
+			    "upcall pipe: %s\n", strerror(errno));
+		return;
+	}
+	printerr(2, "\n%s: uid %d (%s)\n", __func__, uid, clp->relpath);
+
+	info = alloc_upcall_info(clp, uid, clp->krb5_fd, NULL, NULL, NULL);
+	if (info == NULL) {
+		printerr(0, "%s: failed to allocate clnt_upcall_info\n", __func__);
+		do_error_downcall(clp->krb5_fd, uid, -EACCES);
+		return;
+	}
+	err = start_upcall_thread(gssd_work_thread_fn, info);
+	if (err != 0) {
+		do_error_downcall(clp->krb5_fd, uid, -EACCES);
+		free_upcall_info(info);
+	}
+}
+
 void
-handle_gssd_upcall(struct clnt_upcall_info *info)
+handle_gssd_upcall(struct clnt_info *clp)
 {
-	struct clnt_info	*clp = info->clp;
 	uid_t			uid;
+	char			lbuf[RPC_CHAN_BUF_SIZE];
+	int			lbuflen = 0;
 	char			*p;
 	char			*mech = NULL;
 	char			*uidstr = NULL;
@@ -746,20 +861,22 @@ handle_gssd_upcall(struct clnt_upcall_info *info)
 	char			*service = NULL;
 	char			*srchost = NULL;
 	char			*enctypes = NULL;
-	char			*upcall_str;
-	char			*pbuf = info->lbuf;
 	pthread_t tid = pthread_self();
+	struct clnt_upcall_info	*info;
+	int			err;
 
-	printerr(2, "\n%s(0x%x): '%s' (%s)\n", __func__, tid, 
-		info->lbuf, clp->relpath);
-
-	upcall_str = strdup(info->lbuf);
-	if (upcall_str == NULL) {
-		printerr(0, "ERROR: malloc failure\n");
-		goto out_nomem;
+	lbuflen = read(clp->gssd_fd, lbuf, sizeof(lbuf));
+	if (lbuflen <= 0 || lbuf[lbuflen-1] != '\n') {
+		printerr(0, "WARNING: handle_gssd_upcall: "
+			    "failed reading request\n");
+		return;
 	}
+	lbuf[lbuflen-1] = 0;
+
+	printerr(2, "\n%s(0x%x): '%s' (%s)\n", __func__, tid,
+		 lbuf, clp->relpath);
 
-	while ((p = strsep(&pbuf, " "))) {
+	for (p = strtok(lbuf, " "); p; p = strtok(NULL, " ")) {
 		if (!strncmp(p, "mech=", strlen("mech=")))
 			mech = p + strlen("mech=");
 		else if (!strncmp(p, "uid=", strlen("uid=")))
@@ -777,8 +894,8 @@ handle_gssd_upcall(struct clnt_upcall_info *info)
 	if (!mech || strlen(mech) < 1) {
 		printerr(0, "WARNING: handle_gssd_upcall: "
 			    "failed to find gss mechanism name "
-			    "in upcall string '%s'\n", upcall_str);
-		goto out;
+			    "in upcall string '%s'\n", lbuf);
+		return;
 	}
 
 	if (uidstr) {
@@ -790,21 +907,21 @@ handle_gssd_upcall(struct clnt_upcall_info *info)
 	if (!uidstr) {
 		printerr(0, "WARNING: handle_gssd_upcall: "
 			    "failed to find uid "
-			    "in upcall string '%s'\n", upcall_str);
-		goto out;
+			    "in upcall string '%s'\n", lbuf);
+		return;
 	}
 
 	if (enctypes && parse_enctypes(enctypes) != 0) {
 		printerr(0, "WARNING: handle_gssd_upcall: "
 			 "parsing encryption types failed: errno %d\n", errno);
-		goto out;
+		return;
 	}
 
 	if (target && strlen(target) < 1) {
 		printerr(0, "WARNING: handle_gssd_upcall: "
 			 "failed to parse target name "
-			 "in upcall string '%s'\n", upcall_str);
-		goto out;
+			 "in upcall string '%s'\n", lbuf);
+		return;
 	}
 
 	/*
@@ -818,21 +935,26 @@ handle_gssd_upcall(struct clnt_upcall_info *info)
 	if (service && strlen(service) < 1) {
 		printerr(0, "WARNING: handle_gssd_upcall: "
 			 "failed to parse service type "
-			 "in upcall string '%s'\n", upcall_str);
-		goto out;
+			 "in upcall string '%s'\n", lbuf);
+		return;
 	}
 
-	if (strcmp(mech, "krb5") == 0 && clp->servername)
-		process_krb5_upcall(clp, uid, clp->gssd_fd, srchost, target, service);
-	else {
+	if (strcmp(mech, "krb5") == 0 && clp->servername) {
+		info = alloc_upcall_info(clp, uid, clp->gssd_fd, srchost, target, service);
+		if (info == NULL) {
+			printerr(0, "%s: failed to allocate clnt_upcall_info\n", __func__);
+			do_error_downcall(clp->gssd_fd, uid, -EACCES);
+			return;
+		}
+		err = start_upcall_thread(gssd_work_thread_fn, info);
+		if (err != 0) {
+			do_error_downcall(clp->gssd_fd, uid, -EACCES);
+			free_upcall_info(info);
+		}
+	} else {
 		if (clp->servername)
 			printerr(0, "WARNING: handle_gssd_upcall: "
 				 "received unknown gss mech '%s'\n", mech);
 		do_error_downcall(clp->gssd_fd, uid, -EACCES);
 	}
-out:
-	free(upcall_str);
-out_nomem:
-	free_upcall_info(info);
-	return;
 }
-- 
2.30.2