Testing upstream patch by Doug Nazar:
Manually patching gssd.c, gssd_proc.c and gssd.h wihtin nfs-utils_1.3.4 source.
Rebuilding binary package nfs-common-1.3.4-2.5 from nfs-utils_1.3.4 source.
Testing altered binary packages nfs-common_1.3.4-2.5_amd64.deb and
nfs-common-dbgsym_1.3.4-2.5_amd64.deb on one machine.
ToDo:
Correctly backporting Nazar's patch to nfs-utils_1.3.4 source of Debian Stable
and Oldstable.
Roll-out to all NFS file servers.
Jump into nightmare's heaven oUt
Sebastian
________________________
Sebastian Kraus
Team IT am Institut für Chemie
Gebäude C, Straße des 17. Juni 115, Raum C7
Technische Universität Berlin
Fakultät II
Institut für Chemie
Sekretariat C3
Straße des 17. Juni 135
10623 Berlin
Email: sebastian.kr...@tu-berlin.de
________________________________________
From: linux-nfs-ow...@vger.kernel.org <linux-nfs-ow...@vger.kernel.org> on
behalf of Doug Nazar <naz...@nazar.ca>
Sent: Friday, June 26, 2020 23:30
To: J. Bruce Fields
Cc: Kraus, Sebastian; linux-...@vger.kernel.org; Steve Dickson; Olga
Kornievskaia
Subject: [PATCH v2] Re: Strange segmentation violations of rpc.gssd in Debian
Buster
On 2020-06-26 17:02, J. Bruce Fields wrote:
> Unless I'm missing something--an upcall thread could still be using this
> file descriptor.
>
> If we're particularly unlucky, we could do a new open in a moment and
> reuse this file descriptor number, and then then writes in do_downcall()
> could end up going to some other random file.
>
> I think we want these closes done by gssd_free_client() in the !refcnt
> case?
Makes sense. I was thinking more that it was an abort situation and we
shouldn't be sending any data to the kernel but re-use is definitely a
concern.
I've split it so that we are removed from the event loop in destroy()
but the close happens in free().
Doug
From 8ef49081e8a42bfa05bb63265cd4f951e2b23413 Mon Sep 17 00:00:00 2001
From: Doug Nazar <naz...@nazar.ca>
Date: Fri, 26 Jun 2020 16:02:04 -0400
Subject: [PATCH] gssd: Refcount struct clnt_info to protect multithread usage
Struct clnt_info is shared with the various upcall threads so
we need to ensure that it stays around even if the client dir
gets removed.
Reported-by: Sebastian Kraus <sebastian.kr...@tu-berlin.de>
Signed-off-by: Doug Nazar <naz...@nazar.ca>
---
utils/gssd/gssd.c | 67 ++++++++++++++++++++++++++++++++----------
utils/gssd/gssd.h | 5 ++--
utils/gssd/gssd_proc.c | 4 +--
3 files changed, 55 insertions(+), 21 deletions(-)
diff --git a/utils/gssd/gssd.c b/utils/gssd/gssd.c
index 588da0fb..b40c3220 100644
--- a/utils/gssd/gssd.c
+++ b/utils/gssd/gssd.c
@@ -90,9 +90,7 @@ char *ccachedir = NULL;
/* Avoid DNS reverse lookups on server names */
static bool avoid_dns = true;
static bool use_gssproxy = false;
-int thread_started = false;
-pthread_mutex_t pmutex = PTHREAD_MUTEX_INITIALIZER;
-pthread_cond_t pcond = PTHREAD_COND_INITIALIZER;
+pthread_mutex_t clp_lock = PTHREAD_MUTEX_INITIALIZER;
TAILQ_HEAD(topdir_list_head, topdir) topdir_list;
@@ -359,20 +357,28 @@ out:
free(port);
}
+/* Actually frees clp and fields that might be used from other
+ * threads if was last reference.
+ */
static void
-gssd_destroy_client(struct clnt_info *clp)
+gssd_free_client(struct clnt_info *clp)
{
- if (clp->krb5_fd >= 0) {
+ int refcnt;
+
+ pthread_mutex_lock(&clp_lock);
+ refcnt = --clp->refcount;
+ pthread_mutex_unlock(&clp_lock);
+ if (refcnt > 0)
+ return;
+
+ printerr(3, "freeing client %s\n", clp->relpath);
+
+ if (clp->krb5_fd >= 0)
close(clp->krb5_fd);
- event_del(&clp->krb5_ev);
- }
- if (clp->gssd_fd >= 0) {
+ if (clp->gssd_fd >= 0)
close(clp->gssd_fd);
- event_del(&clp->gssd_ev);
- }
- inotify_rm_watch(inotify_fd, clp->wd);
free(clp->relpath);
free(clp->servicename);
free(clp->servername);
@@ -380,6 +386,24 @@ gssd_destroy_client(struct clnt_info *clp)
free(clp);
}
+/* Called when removing from clnt_list to tear down event handling.
+ * Will then free clp if was last reference.
+ */
+static void
+gssd_destroy_client(struct clnt_info *clp)
+{
+ printerr(3, "destroying client %s\n", clp->relpath);
+
+ if (clp->krb5_fd >= 0)
+ event_del(&clp->krb5_ev);
+
+ if (clp->gssd_fd >= 0)
+ event_del(&clp->gssd_ev);
+
+ inotify_rm_watch(inotify_fd, clp->wd);
+ gssd_free_client(clp);
+}
+
static void gssd_scan(void);
static int
@@ -416,11 +440,21 @@ static struct clnt_upcall_info *alloc_upcall_info(struct
clnt_info *clp)
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.
@@ -438,13 +472,13 @@ gssd_clnt_gssd_cb(int UNUSED(fd), short UNUSED(which),
void *data)
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(info);
+ free_upcall_info(info);
return;
}
info->lbuf[info->lbuflen-1] = 0;
if (start_upcall_thread(handle_gssd_upcall, info))
- free(info);
+ free_upcall_info(info);
}
static void
@@ -461,12 +495,12 @@ gssd_clnt_krb5_cb(int UNUSED(fd), short UNUSED(which),
void *data)
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(info);
+ free_upcall_info(info);
return;
}
if (start_upcall_thread(handle_krb5_upcall, info))
- free(info);
+ free_upcall_info(info);
}
static struct clnt_info *
@@ -501,6 +535,7 @@ gssd_get_clnt(struct topdir *tdi, const char *name)
clp->name = clp->relpath + strlen(tdi->name) + 1;
clp->krb5_fd = -1;
clp->gssd_fd = -1;
+ clp->refcount = 1;
TAILQ_INSERT_HEAD(&tdi->clnt_list, clp, list);
return clp;
@@ -651,7 +686,7 @@ gssd_scan_topdir(const char *name)
if (clp->scanned)
continue;
- printerr(3, "destroying client %s\n", clp->relpath);
+ printerr(3, "orphaned client %s\n", clp->relpath);
saveprev = clp->list.tqe_prev;
TAILQ_REMOVE(&tdi->clnt_list, clp, list);
gssd_destroy_client(clp);
diff --git a/utils/gssd/gssd.h b/utils/gssd/gssd.h
index f4f59754..d33e4dff 100644
--- a/utils/gssd/gssd.h
+++ b/utils/gssd/gssd.h
@@ -63,12 +63,10 @@ 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;
+ int refcount;
int wd;
bool scanned;
char *name;
@@ -94,6 +92,7 @@ struct clnt_upcall_info {
void handle_krb5_upcall(struct clnt_upcall_info *clp);
void handle_gssd_upcall(struct clnt_upcall_info *clp);
+void free_upcall_info(struct clnt_upcall_info *info);
#endif /* _RPC_GSSD_H_ */
diff --git a/utils/gssd/gssd_proc.c b/utils/gssd/gssd_proc.c
index 8fe6605b..05c1da64 100644
--- a/utils/gssd/gssd_proc.c
+++ b/utils/gssd/gssd_proc.c
@@ -730,7 +730,7 @@ handle_krb5_upcall(struct clnt_upcall_info *info)
printerr(2, "\n%s: uid %d (%s)\n", __func__, info->uid, clp->relpath);
process_krb5_upcall(clp, info->uid, clp->krb5_fd, NULL, NULL, NULL);
- free(info);
+ free_upcall_info(info);
}
void
@@ -830,6 +830,6 @@ handle_gssd_upcall(struct clnt_upcall_info *info)
out:
free(upcall_str);
out_nomem:
- free(info);
+ free_upcall_info(info);
return;
}
--
2.26.2
