The following series converts gssd to use libevent and inotify instead
of a handrolled event loop and dnotify. Lots of cleanups in the process
(e.g. removing a lot of arbitrary limitations and fixed size buffers).
All in all a nice reduction in code size (what can I say, I was bored).
I've even managed to mount NFS shares with the patched server :)
---
David Härdeman (19):
nfs-utils: cleanup daemonization code
nfs-utils: gssd - merge gssd_main_loop.c and gssd.c
nfs-utils: gssd - simplify some option handling
nfs-utils: gssd - remove arbitrary GSSD_MAX_CCACHE_SEARCH limitation
nfs-utils: gssd - simplify topdirs path
nfs-utils: gssd - move over pipfs scanning code
nfs-utils: gssd - simplify client dir scanning code
nfs-utils: gssd - use libevent
nfs-utils: gssd - remove "close me" code
nfs-utils: gssd - make the client lists per-topdir
nfs-utils: gssd - keep the rpc_pipefs dir open
nfs-utils: gssd - use more relative paths
nfs-utils: gssd - simplify topdir scanning
nfs-utils: gssd - simplify client scanning
nfs-utils: gssd - cleanup read_service_info
nfs-utils: gssd - change dnotify to inotify
nfs-utils: gssd - further shorten some pathnames
nfs-utils: gssd - improve inotify
nfs-utils: gssd - simplify handle_gssd_upcall
support/include/nfslib.h | 5
support/nfs/mydaemon.c | 92 +++--
utils/gssd/Makefile.am | 24 +
utils/gssd/gss_util.h | 2
utils/gssd/gssd.c | 785 +++++++++++++++++++++++++++++++++++++++++--
utils/gssd/gssd.h | 46 +--
utils/gssd/gssd_main_loop.c | 263 --------------
utils/gssd/gssd_proc.c | 654 ++----------------------------------
utils/gssd/svcgssd.c | 8
utils/idmapd/idmapd.c | 6
utils/statd/statd.c | 66 +---
11 files changed, 878 insertions(+), 1073 deletions(-)
delete mode 100644 utils/gssd/gssd_main_loop.c
--
David Härdeman
The daemonization init/ready functions have parameters that are never used,
require the caller to keep track of some pipefds that it has no interest in
and which might not be used in some scenarios. Cleanup both functions a bit.
The idea here is also that these two functions might be good points to insert
more systemd init code later (sd_notify()).
Also, statd had a private copy of the daemonization code for unknown
reasons...so make it use the generic version instead.
Signed-off-by: David Härdeman <[email protected]>
---
support/include/nfslib.h | 5 +-
support/nfs/mydaemon.c | 92 ++++++++++++++++++++++---------------------
utils/gssd/gssd.c | 4 --
utils/gssd/gssd.h | 1
utils/gssd/gssd_main_loop.c | 3 -
utils/gssd/svcgssd.c | 8 +---
utils/idmapd/idmapd.c | 6 +--
utils/statd/statd.c | 66 +++++--------------------------
8 files changed, 67 insertions(+), 118 deletions(-)
diff --git a/support/include/nfslib.h b/support/include/nfslib.h
index c5dc6f8..c9a13cb 100644
--- a/support/include/nfslib.h
+++ b/support/include/nfslib.h
@@ -17,6 +17,7 @@
#include <sys/socket.h>
#include <netinet/in.h>
#include <stdio.h>
+#include <stdbool.h>
#include <paths.h>
#include <rpcsvc/nfs_prot.h>
#include <nfs/nfs.h>
@@ -129,8 +130,8 @@ void fendrmtabent(FILE *fp);
void frewindrmtabent(FILE *fp);
/* mydaemon */
-void mydaemon(int nochdir, int noclose, int *pipefds);
-void release_parent(int *pipefds);
+void daemon_init(bool fg);
+void daemon_ready(void);
/*
* wildmat borrowed from INN
diff --git a/support/nfs/mydaemon.c b/support/nfs/mydaemon.c
index e885d60..3391eff 100644
--- a/support/nfs/mydaemon.c
+++ b/support/nfs/mydaemon.c
@@ -46,56 +46,61 @@
#include <errno.h>
#include <unistd.h>
#include <stdio.h>
+#include <stdbool.h>
#include <stdlib.h>
#include <string.h>
#include <xlog.h>
+#include "nfslib.h"
+
+static int pipefds[2] = { -1, -1};
+
/**
- * mydaemon - daemonize, but have parent wait to exit
- * @nochdir: skip chdir()'ing the child to / after forking if true
- * @noclose: skip closing stdin/stdout/stderr if true
- * @pipefds: pointer to 2 element array of pipefds
+ * daemon_init - initial daemon setup
+ * @fg: whether to run in the foreground
*
* This function is like daemon(), but with our own special sauce to delay
* the exit of the parent until the child is set up properly. A pipe is created
* between parent and child. The parent process will wait to exit until the
- * child dies or writes a '1' on the pipe signaling that it started
- * successfully.
+ * child dies or writes an int on the pipe signaling its status.
*/
void
-mydaemon(int nochdir, int noclose, int *pipefds)
+daemon_init(bool fg)
{
int pid, status, tempfd;
+ if (fg)
+ return;
+
if (pipe(pipefds) < 0) {
xlog_err("mydaemon: pipe() failed: errno %d (%s)\n",
errno, strerror(errno));
- exit(1);
+ exit(EXIT_FAILURE);
}
- if ((pid = fork ()) < 0) {
+
+ pid = fork();
+ if (pid < 0) {
xlog_err("mydaemon: fork() failed: errno %d (%s)\n",
errno, strerror(errno));
- exit(1);
+ exit(EXIT_FAILURE);
}
- if (pid != 0) {
- /*
- * Parent. Wait for status from child.
- */
+ if (pid > 0) {
+ /* Parent */
close(pipefds[1]);
- if (read(pipefds[0], &status, 1) != 1)
- exit(1);
- exit (0);
+ if (read(pipefds[0], &status, sizeof(status)) != sizeof(status))
+ exit(EXIT_FAILURE);
+ exit(status);
}
- /* Child. */
+
+ /* Child */
close(pipefds[0]);
setsid ();
- if (nochdir == 0) {
- if (chdir ("/") == -1) {
- xlog_err("mydaemon: chdir() failed: errno %d (%s)\n",
- errno, strerror(errno));
- exit(1);
- }
+
+ if (chdir ("/")) {
+ xlog_err("mydaemon: chdir() failed: errno %d (%s)\n",
+ errno, strerror(errno));
+ exit(EXIT_FAILURE);
}
while (pipefds[1] <= 2) {
@@ -103,41 +108,38 @@ mydaemon(int nochdir, int noclose, int *pipefds)
if (pipefds[1] < 0) {
xlog_err("mydaemon: dup() failed: errno %d (%s)\n",
errno, strerror(errno));
- exit(1);
+ exit(EXIT_FAILURE);
}
}
- if (noclose == 0) {
- tempfd = open("/dev/null", O_RDWR);
- if (tempfd >= 0) {
- dup2(tempfd, 0);
- dup2(tempfd, 1);
- dup2(tempfd, 2);
- close(tempfd);
- } else {
- xlog_err("mydaemon: can't open /dev/null: errno %d "
- "(%s)\n", errno, strerror(errno));
- exit(1);
- }
+ tempfd = open("/dev/null", O_RDWR);
+ if (tempfd < 0) {
+ xlog_err("mydaemon: can't open /dev/null: errno %d "
+ "(%s)\n", errno, strerror(errno));
+ exit(EXIT_FAILURE);
}
- return;
+ dup2(tempfd, 0);
+ dup2(tempfd, 1);
+ dup2(tempfd, 2);
+ dup2(pipefds[1], 3);
+ pipefds[1] = 3;
+ closeall(4);
}
/**
- * release_parent - tell the parent that it can exit now
- * @pipefds: pipefd array that was previously passed to mydaemon()
+ * daemon_ready - tell interested parties that the daemon is ready
*
- * This function tells the parent process of mydaemon() that it's now clear
- * to exit(0).
+ * This function tells e.g. the parent process that the daemon is up
+ * and running.
*/
void
-release_parent(int *pipefds)
+daemon_ready(void)
{
- int status;
+ int status = 0;
if (pipefds[1] > 0) {
- if (write(pipefds[1], &status, 1) != 1) {
+ if (write(pipefds[1], &status, sizeof(status)) != sizeof(status)) {
xlog_err("WARN: writing to parent pipe failed: errno "
"%d (%s)\n", errno, strerror(errno));
}
diff --git a/utils/gssd/gssd.c b/utils/gssd/gssd.c
index 6b8b863..dc84b3e 100644
--- a/utils/gssd/gssd.c
+++ b/utils/gssd/gssd.c
@@ -66,7 +66,6 @@ int root_uses_machine_creds = 1;
unsigned int context_timeout = 0;
unsigned int rpc_timeout = 5;
char *preferred_realm = NULL;
-int pipefds[2] = { -1, -1 };
void
sig_die(int signal)
@@ -206,8 +205,7 @@ main(int argc, char *argv[])
if (gssd_check_mechs() != 0)
errx(1, "Problem with gssapi library");
- if (!fg)
- mydaemon(0, 0, pipefds);
+ daemon_init(fg);
signal(SIGINT, sig_die);
signal(SIGTERM, sig_die);
diff --git a/utils/gssd/gssd.h b/utils/gssd/gssd.h
index 48f4ad8..84479e8 100644
--- a/utils/gssd/gssd.h
+++ b/utils/gssd/gssd.h
@@ -68,7 +68,6 @@ extern int root_uses_machine_creds;
extern unsigned int context_timeout;
extern unsigned int rpc_timeout;
extern char *preferred_realm;
-extern int pipefds[2];
TAILQ_HEAD(clnt_list_head, clnt_info) clnt_list;
diff --git a/utils/gssd/gssd_main_loop.c b/utils/gssd/gssd_main_loop.c
index 6946ab6..9787883 100644
--- a/utils/gssd/gssd_main_loop.c
+++ b/utils/gssd/gssd_main_loop.c
@@ -252,8 +252,7 @@ gssd_run()
exit(1);
}
- /* release the parent after the initial dir scan */
- release_parent(pipefds);
+ daemon_ready();
}
gssd_poll(pollarray, pollsize);
}
diff --git a/utils/gssd/svcgssd.c b/utils/gssd/svcgssd.c
index 0385725..f1b4347 100644
--- a/utils/gssd/svcgssd.c
+++ b/utils/gssd/svcgssd.c
@@ -62,8 +62,6 @@
#include "gss_util.h"
#include "err_util.h"
-static int pipefds[2] = { -1, -1 };
-
void
sig_die(int signal)
{
@@ -157,8 +155,7 @@ main(int argc, char *argv[])
exit(1);
}
- if (!fg)
- mydaemon(0, 0, pipefds);
+ daemon_init(fg);
signal(SIGINT, sig_die);
signal(SIGTERM, sig_die);
@@ -187,8 +184,7 @@ main(int argc, char *argv[])
}
}
- if (!fg)
- release_parent(pipefds);
+ daemon_ready();
nfs4_init_name_mapping(NULL); /* XXX: should only do this once */
gssd_run();
diff --git a/utils/idmapd/idmapd.c b/utils/idmapd/idmapd.c
index c02849b..aff89d1 100644
--- a/utils/idmapd/idmapd.c
+++ b/utils/idmapd/idmapd.c
@@ -164,7 +164,6 @@ static char pipefsdir[PATH_MAX];
static char *nobodyuser, *nobodygroup;
static uid_t nobodyuid;
static gid_t nobodygid;
-static int pipefds[2] = { -1, -1 };
/* Used by conffile.c in libnfs.a */
char *conf_path;
@@ -302,8 +301,7 @@ main(int argc, char **argv)
if (nfs4_init_name_mapping(conf_path))
errx(1, "Unable to create name to user id mappings.");
- if (!fg)
- mydaemon(0, 0, pipefds);
+ daemon_init(fg);
event_init();
@@ -380,7 +378,7 @@ main(int argc, char **argv)
if (nfsdret != 0 && fd == 0)
xlog_err("main: Neither NFS client nor NFSd found");
- release_parent(pipefds);
+ daemon_ready();
if (event_dispatch() < 0)
xlog_err("main: event_dispatch returns errno %d (%s)",
diff --git a/utils/statd/statd.c b/utils/statd/statd.c
index 51a016e..60ce6d1 100644
--- a/utils/statd/statd.c
+++ b/utils/statd/statd.c
@@ -248,13 +248,12 @@ int main (int argc, char **argv)
int nlm_udp = 0, nlm_tcp = 0;
struct rlimit rlim;
- int pipefds[2] = { -1, -1};
- char status;
-
/* Default: daemon mode, no other options */
run_mode = 0;
- xlog_stderr(0);
- xlog_syslog(1);
+
+ /* Log to stderr if there's an error during startup */
+ xlog_stderr(1);
+ xlog_syslog(0);
/* Set the basename */
if ((name_p = strrchr(argv[0],'/')) != NULL) {
@@ -394,52 +393,17 @@ int main (int argc, char **argv)
simulator (--argc, ++argv); /* simulator() does exit() */
#endif
- if (!(run_mode & MODE_NODAEMON)) {
- int tempfd;
-
- if (pipe(pipefds)<0) {
- perror("statd: unable to create pipe");
- exit(1);
- }
- if ((pid = fork ()) < 0) {
- perror ("statd: Could not fork");
- exit (1);
- } else if (pid != 0) {
- /* Parent.
- * Wait for status from child.
- */
- close(pipefds[1]);
- if (read(pipefds[0], &status, 1) != 1)
- exit(1);
- exit (0);
- }
- /* Child. */
- close(pipefds[0]);
- setsid ();
-
- while (pipefds[1] <= 2) {
- pipefds[1] = dup(pipefds[1]);
- if (pipefds[1]<0) {
- perror("statd: dup");
- exit(1);
- }
- }
- tempfd = open("/dev/null", O_RDWR);
- dup2(tempfd, 0);
- dup2(tempfd, 1);
- dup2(tempfd, 2);
- dup2(pipefds[1], 3);
- pipefds[1] = 3;
- closeall(4);
- }
-
- /* Child. */
+ daemon_init(!(run_mode & MODE_NODAEMON));
if (run_mode & MODE_LOG_STDERR) {
xlog_syslog(0);
xlog_stderr(1);
xlog_config(D_ALL, 1);
+ } else {
+ xlog_syslog(1);
+ xlog_stderr(0);
}
+
xlog_open(name_p);
xlog(L_NOTICE, "Version " VERSION " starting");
@@ -512,16 +476,8 @@ int main (int argc, char **argv)
}
atexit(statd_unregister);
- /* If we got this far, we have successfully started, so notify parent */
- if (pipefds[1] > 0) {
- status = 0;
- if (write(pipefds[1], &status, 1) != 1) {
- xlog_warn("writing to parent pipe failed: errno %d (%s)\n",
- errno, strerror(errno));
- }
- close(pipefds[1]);
- pipefds[1] = -1;
- }
+ /* If we got this far, we have successfully started */
+ daemon_ready();
for (;;) {
/*
Having all the main loop code in one file is important in preparation
for later patches which add inotify and libevent.
Signed-off-by: David Härdeman <[email protected]>
---
utils/gssd/Makefile.am | 1
utils/gssd/gssd.c | 224 ++++++++++++++++++++++++++++++++++++-
utils/gssd/gssd.h | 4 -
utils/gssd/gssd_main_loop.c | 262 -------------------------------------------
4 files changed, 220 insertions(+), 271 deletions(-)
delete mode 100644 utils/gssd/gssd_main_loop.c
diff --git a/utils/gssd/Makefile.am b/utils/gssd/Makefile.am
index 62a70af..0f0142b 100644
--- a/utils/gssd/Makefile.am
+++ b/utils/gssd/Makefile.am
@@ -36,7 +36,6 @@ COMMON_SRCS = \
gssd_SOURCES = \
$(COMMON_SRCS) \
gssd.c \
- gssd_main_loop.c \
gssd_proc.c \
krb5_util.c \
\
diff --git a/utils/gssd/gssd.c b/utils/gssd/gssd.c
index dc84b3e..5e580e7 100644
--- a/utils/gssd/gssd.c
+++ b/utils/gssd/gssd.c
@@ -1,7 +1,7 @@
/*
gssd.c
- Copyright (c) 2000 The Regents of the University of Michigan.
+ Copyright (c) 2000, 2004 The Regents of the University of Michigan.
All rights reserved.
Copyright (c) 2000 Dug Song <[email protected]>.
@@ -40,9 +40,15 @@
#include <config.h>
#endif /* HAVE_CONFIG_H */
+#ifndef _GNU_SOURCE
+#define _GNU_SOURCE
+#endif
+
#include <sys/param.h>
#include <sys/socket.h>
+#include <sys/poll.h>
#include <rpc/rpc.h>
+#include <netinet/in.h>
#include <unistd.h>
#include <err.h>
@@ -51,13 +57,17 @@
#include <stdlib.h>
#include <string.h>
#include <signal.h>
+#include <memory.h>
+#include <fcntl.h>
+#include <dirent.h>
+
#include "gssd.h"
#include "err_util.h"
#include "gss_util.h"
#include "krb5_util.h"
#include "nfslib.h"
-char pipefs_dir[PATH_MAX] = GSSD_PIPEFS_DIR;
+static char pipefs_dir[PATH_MAX] = GSSD_PIPEFS_DIR;
char keytabfile[PATH_MAX] = GSSD_DEFAULT_KEYTAB_FILE;
char ccachedir[PATH_MAX] = GSSD_DEFAULT_CRED_DIR ":" GSSD_USER_CRED_DIR;
char *ccachesearch[GSSD_MAX_CCACHE_SEARCH + 1];
@@ -66,8 +76,213 @@ int root_uses_machine_creds = 1;
unsigned int context_timeout = 0;
unsigned int rpc_timeout = 5;
char *preferred_realm = NULL;
+extern struct pollfd *pollarray;
+extern unsigned long pollsize;
+
+#define POLL_MILLISECS 500
+
+static volatile int dir_changed = 1;
+
+static void dir_notify_handler(__attribute__((unused))int sig)
+{
+ dir_changed = 1;
+}
+
+static void
+scan_poll_results(int ret)
+{
+ int i;
+ struct clnt_info *clp;
+
+ for (clp = clnt_list.tqh_first; clp != NULL; clp = clp->list.tqe_next)
+ {
+ i = clp->gssd_poll_index;
+ if (i >= 0 && pollarray[i].revents) {
+ if (pollarray[i].revents & POLLHUP) {
+ clp->gssd_close_me = 1;
+ dir_changed = 1;
+ }
+ if (pollarray[i].revents & POLLIN)
+ handle_gssd_upcall(clp);
+ pollarray[clp->gssd_poll_index].revents = 0;
+ ret--;
+ if (!ret)
+ break;
+ }
+ i = clp->krb5_poll_index;
+ if (i >= 0 && pollarray[i].revents) {
+ if (pollarray[i].revents & POLLHUP) {
+ clp->krb5_close_me = 1;
+ dir_changed = 1;
+ }
+ if (pollarray[i].revents & POLLIN)
+ handle_krb5_upcall(clp);
+ pollarray[clp->krb5_poll_index].revents = 0;
+ ret--;
+ if (!ret)
+ break;
+ }
+ }
+}
+
+static int
+topdirs_add_entry(struct dirent *dent)
+{
+ struct topdirs_info *tdi;
+
+ tdi = calloc(sizeof(struct topdirs_info), 1);
+ if (tdi == NULL) {
+ printerr(0, "ERROR: Couldn't allocate struct topdirs_info\n");
+ return -1;
+ }
+ tdi->dirname = malloc(PATH_MAX);
+ if (tdi->dirname == NULL) {
+ printerr(0, "ERROR: Couldn't allocate directory name\n");
+ free(tdi);
+ return -1;
+ }
+ snprintf(tdi->dirname, PATH_MAX, "%s/%s", pipefs_dir, dent->d_name);
+ tdi->fd = open(tdi->dirname, O_RDONLY);
+ if (tdi->fd == -1) {
+ printerr(0, "ERROR: failed to open %s\n", tdi->dirname);
+ free(tdi);
+ return -1;
+ }
+ fcntl(tdi->fd, F_SETSIG, DNOTIFY_SIGNAL);
+ fcntl(tdi->fd, F_NOTIFY, DN_CREATE|DN_DELETE|DN_MODIFY|DN_MULTISHOT);
+
+ TAILQ_INSERT_HEAD(&topdirs_list, tdi, list);
+ return 0;
+}
+
+static void
+topdirs_free_list(void)
+{
+ struct topdirs_info *tdi;
+
+ TAILQ_FOREACH(tdi, &topdirs_list, list) {
+ free(tdi->dirname);
+ if (tdi->fd != -1)
+ close(tdi->fd);
+ TAILQ_REMOVE(&topdirs_list, tdi, list);
+ free(tdi);
+ }
+}
+
+static int
+topdirs_init_list(void)
+{
+ DIR *pipedir;
+ struct dirent *dent;
+ int ret;
-void
+ TAILQ_INIT(&topdirs_list);
+
+ pipedir = opendir(pipefs_dir);
+ if (pipedir == NULL) {
+ printerr(0, "ERROR: could not open rpc_pipefs directory '%s': "
+ "%s\n", pipefs_dir, strerror(errno));
+ return -1;
+ }
+ for (dent = readdir(pipedir); dent != NULL; dent = readdir(pipedir)) {
+ if (dent->d_type != DT_DIR ||
+ strcmp(dent->d_name, ".") == 0 ||
+ strcmp(dent->d_name, "..") == 0) {
+ continue;
+ }
+ ret = topdirs_add_entry(dent);
+ if (ret)
+ goto out_err;
+ }
+ if (TAILQ_EMPTY(&topdirs_list)) {
+ printerr(0, "ERROR: rpc_pipefs directory '%s' is empty!\n", pipefs_dir);
+ return -1;
+ }
+ closedir(pipedir);
+ return 0;
+out_err:
+ topdirs_free_list();
+ return -1;
+}
+
+#ifdef HAVE_PPOLL
+static void gssd_poll(struct pollfd *fds, unsigned long nfds)
+{
+ sigset_t emptyset;
+ int ret;
+
+ sigemptyset(&emptyset);
+ ret = ppoll(fds, nfds, NULL, &emptyset);
+ if (ret < 0) {
+ if (errno != EINTR)
+ printerr(0, "WARNING: error return from poll\n");
+ } else if (ret == 0) {
+ printerr(0, "WARNING: unexpected timeout\n");
+ } else {
+ scan_poll_results(ret);
+ }
+}
+#else /* !HAVE_PPOLL */
+static void gssd_poll(struct pollfd *fds, unsigned long nfds)
+{
+ int ret;
+
+ /* race condition here: dir_changed could be set before we
+ * enter the poll, and we'd never notice if it weren't for the
+ * timeout. */
+ ret = poll(fds, nfds, POLL_MILLISECS);
+ if (ret < 0) {
+ if (errno != EINTR)
+ printerr(0, "WARNING: error return from poll\n");
+ } else if (ret == 0) {
+ /* timeout */
+ } else { /* ret > 0 */
+ scan_poll_results(ret);
+ }
+}
+#endif /* !HAVE_PPOLL */
+
+static void
+gssd_run(void)
+{
+ struct sigaction dn_act = {
+ .sa_handler = dir_notify_handler
+ };
+ sigset_t set;
+
+ sigemptyset(&dn_act.sa_mask);
+ sigaction(DNOTIFY_SIGNAL, &dn_act, NULL);
+
+ /* just in case the signal is blocked... */
+ sigemptyset(&set);
+ sigaddset(&set, DNOTIFY_SIGNAL);
+ sigprocmask(SIG_UNBLOCK, &set, NULL);
+
+ if (topdirs_init_list() != 0) {
+ /* Error msg is already printed */
+ exit(1);
+ }
+ init_client_list();
+
+ printerr(1, "beginning poll\n");
+ while (1) {
+ while (dir_changed) {
+ dir_changed = 0;
+ if (update_client_list()) {
+ /* Error msg is already printed */
+ exit(1);
+ }
+
+ daemon_ready();
+ }
+ gssd_poll(pollarray, pollsize);
+ }
+ topdirs_free_list();
+
+ return;
+}
+
+static void
sig_die(int signal)
{
/* destroy krb5 machine creds */
@@ -77,7 +292,7 @@ sig_die(int signal)
exit(0);
}
-void
+static void
sig_hup(int signal)
{
/* don't exit on SIGHUP */
@@ -215,3 +430,4 @@ main(int argc, char *argv[])
printerr(0, "gssd_run returned!\n");
abort();
}
+
diff --git a/utils/gssd/gssd.h b/utils/gssd/gssd.h
index 84479e8..e16b187 100644
--- a/utils/gssd/gssd.h
+++ b/utils/gssd/gssd.h
@@ -58,9 +58,6 @@
*/
enum {AUTHTYPE_KRB5, AUTHTYPE_LIPKEY};
-
-
-extern char pipefs_dir[PATH_MAX];
extern char keytabfile[PATH_MAX];
extern char *ccachesearch[];
extern int use_memcache;
@@ -102,7 +99,6 @@ void init_client_list(void);
int update_client_list(void);
void handle_krb5_upcall(struct clnt_info *clp);
void handle_gssd_upcall(struct clnt_info *clp);
-void gssd_run(void);
#endif /* _RPC_GSSD_H_ */
diff --git a/utils/gssd/gssd_main_loop.c b/utils/gssd/gssd_main_loop.c
deleted file mode 100644
index 9787883..0000000
--- a/utils/gssd/gssd_main_loop.c
+++ /dev/null
@@ -1,262 +0,0 @@
-/*
- Copyright (c) 2004 The Regents of the University of Michigan.
- All rights reserved.
-
- Redistribution and use in source and binary forms, with or without
- modification, are permitted provided that the following conditions
- are met:
-
- 1. Redistributions of source code must retain the above copyright
- notice, this list of conditions and the following disclaimer.
- 2. Redistributions in binary form must reproduce the above copyright
- notice, this list of conditions and the following disclaimer in the
- documentation and/or other materials provided with the distribution.
- 3. Neither the name of the University nor the names of its
- contributors may be used to endorse or promote products derived
- from this software without specific prior written permission.
-
- THIS SOFTWARE IS PROVIDED ``AS IS'' AND ANY EXPRESS OR IMPLIED
- WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED WARRANTIES OF
- MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE
- DISCLAIMED. IN NO EVENT SHALL THE REGENTS OR CONTRIBUTORS BE LIABLE
- FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR
- CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF
- SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR
- BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF
- LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING
- NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS
- SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
-*/
-
-#ifdef HAVE_CONFIG_H
-#include <config.h>
-#endif /* HAVE_CONFIG_H */
-
-#ifndef _GNU_SOURCE
-#define _GNU_SOURCE
-#endif
-
-#include <sys/param.h>
-#include <sys/socket.h>
-#include <sys/poll.h>
-#include <netinet/in.h>
-
-#include <stdio.h>
-#include <stdlib.h>
-#include <string.h>
-#include <memory.h>
-#include <errno.h>
-#include <fcntl.h>
-#include <signal.h>
-#include <unistd.h>
-#include <dirent.h>
-
-#include "gssd.h"
-#include "err_util.h"
-#include "nfslib.h"
-
-extern struct pollfd *pollarray;
-extern unsigned long pollsize;
-
-#define POLL_MILLISECS 500
-
-static volatile int dir_changed = 1;
-
-static void dir_notify_handler(__attribute__((unused))int sig)
-{
- dir_changed = 1;
-}
-
-static void
-scan_poll_results(int ret)
-{
- int i;
- struct clnt_info *clp;
-
- for (clp = clnt_list.tqh_first; clp != NULL; clp = clp->list.tqe_next)
- {
- i = clp->gssd_poll_index;
- if (i >= 0 && pollarray[i].revents) {
- if (pollarray[i].revents & POLLHUP) {
- clp->gssd_close_me = 1;
- dir_changed = 1;
- }
- if (pollarray[i].revents & POLLIN)
- handle_gssd_upcall(clp);
- pollarray[clp->gssd_poll_index].revents = 0;
- ret--;
- if (!ret)
- break;
- }
- i = clp->krb5_poll_index;
- if (i >= 0 && pollarray[i].revents) {
- if (pollarray[i].revents & POLLHUP) {
- clp->krb5_close_me = 1;
- dir_changed = 1;
- }
- if (pollarray[i].revents & POLLIN)
- handle_krb5_upcall(clp);
- pollarray[clp->krb5_poll_index].revents = 0;
- ret--;
- if (!ret)
- break;
- }
- }
-}
-
-static int
-topdirs_add_entry(struct dirent *dent)
-{
- struct topdirs_info *tdi;
-
- tdi = calloc(sizeof(struct topdirs_info), 1);
- if (tdi == NULL) {
- printerr(0, "ERROR: Couldn't allocate struct topdirs_info\n");
- return -1;
- }
- tdi->dirname = malloc(PATH_MAX);
- if (tdi->dirname == NULL) {
- printerr(0, "ERROR: Couldn't allocate directory name\n");
- free(tdi);
- return -1;
- }
- snprintf(tdi->dirname, PATH_MAX, "%s/%s", pipefs_dir, dent->d_name);
- tdi->fd = open(tdi->dirname, O_RDONLY);
- if (tdi->fd == -1) {
- printerr(0, "ERROR: failed to open %s\n", tdi->dirname);
- free(tdi);
- return -1;
- }
- fcntl(tdi->fd, F_SETSIG, DNOTIFY_SIGNAL);
- fcntl(tdi->fd, F_NOTIFY, DN_CREATE|DN_DELETE|DN_MODIFY|DN_MULTISHOT);
-
- TAILQ_INSERT_HEAD(&topdirs_list, tdi, list);
- return 0;
-}
-
-static void
-topdirs_free_list(void)
-{
- struct topdirs_info *tdi;
-
- TAILQ_FOREACH(tdi, &topdirs_list, list) {
- free(tdi->dirname);
- if (tdi->fd != -1)
- close(tdi->fd);
- TAILQ_REMOVE(&topdirs_list, tdi, list);
- free(tdi);
- }
-}
-
-static int
-topdirs_init_list(void)
-{
- DIR *pipedir;
- struct dirent *dent;
- int ret;
-
- TAILQ_INIT(&topdirs_list);
-
- pipedir = opendir(pipefs_dir);
- if (pipedir == NULL) {
- printerr(0, "ERROR: could not open rpc_pipefs directory '%s': "
- "%s\n", pipefs_dir, strerror(errno));
- return -1;
- }
- for (dent = readdir(pipedir); dent != NULL; dent = readdir(pipedir)) {
- if (dent->d_type != DT_DIR ||
- strcmp(dent->d_name, ".") == 0 ||
- strcmp(dent->d_name, "..") == 0) {
- continue;
- }
- ret = topdirs_add_entry(dent);
- if (ret)
- goto out_err;
- }
- if (TAILQ_EMPTY(&topdirs_list)) {
- printerr(0, "ERROR: rpc_pipefs directory '%s' is empty!\n", pipefs_dir);
- return -1;
- }
- closedir(pipedir);
- return 0;
-out_err:
- topdirs_free_list();
- return -1;
-}
-
-#ifdef HAVE_PPOLL
-static void gssd_poll(struct pollfd *fds, unsigned long nfds)
-{
- sigset_t emptyset;
- int ret;
-
- sigemptyset(&emptyset);
- ret = ppoll(fds, nfds, NULL, &emptyset);
- if (ret < 0) {
- if (errno != EINTR)
- printerr(0, "WARNING: error return from poll\n");
- } else if (ret == 0) {
- printerr(0, "WARNING: unexpected timeout\n");
- } else {
- scan_poll_results(ret);
- }
-}
-#else /* !HAVE_PPOLL */
-static void gssd_poll(struct pollfd *fds, unsigned long nfds)
-{
- int ret;
-
- /* race condition here: dir_changed could be set before we
- * enter the poll, and we'd never notice if it weren't for the
- * timeout. */
- ret = poll(fds, nfds, POLL_MILLISECS);
- if (ret < 0) {
- if (errno != EINTR)
- printerr(0, "WARNING: error return from poll\n");
- } else if (ret == 0) {
- /* timeout */
- } else { /* ret > 0 */
- scan_poll_results(ret);
- }
-}
-#endif /* !HAVE_PPOLL */
-
-void
-gssd_run()
-{
- struct sigaction dn_act = {
- .sa_handler = dir_notify_handler
- };
- sigset_t set;
-
- sigemptyset(&dn_act.sa_mask);
- sigaction(DNOTIFY_SIGNAL, &dn_act, NULL);
-
- /* just in case the signal is blocked... */
- sigemptyset(&set);
- sigaddset(&set, DNOTIFY_SIGNAL);
- sigprocmask(SIG_UNBLOCK, &set, NULL);
-
- if (topdirs_init_list() != 0) {
- /* Error msg is already printed */
- exit(1);
- }
- init_client_list();
-
- printerr(1, "beginning poll\n");
- while (1) {
- while (dir_changed) {
- dir_changed = 0;
- if (update_client_list()) {
- /* Error msg is already printed */
- exit(1);
- }
-
- daemon_ready();
- }
- gssd_poll(pollarray, pollsize);
- }
- topdirs_free_list();
-
- return;
-}
Using PATH_MAX in modern code is almost always a bad idea. Simplify
the code and remove that arbitrary limitation at the same time.
Signed-off-by: David Härdeman <[email protected]>
---
utils/gssd/gssd.c | 12 ++++--------
utils/gssd/gssd.h | 4 ++--
2 files changed, 6 insertions(+), 10 deletions(-)
diff --git a/utils/gssd/gssd.c b/utils/gssd/gssd.c
index 5e580e7..6147d30 100644
--- a/utils/gssd/gssd.c
+++ b/utils/gssd/gssd.c
@@ -67,8 +67,8 @@
#include "krb5_util.h"
#include "nfslib.h"
-static char pipefs_dir[PATH_MAX] = GSSD_PIPEFS_DIR;
-char keytabfile[PATH_MAX] = GSSD_DEFAULT_KEYTAB_FILE;
+static char *pipefs_dir = GSSD_PIPEFS_DIR;
+char *keytabfile = GSSD_DEFAULT_KEYTAB_FILE;
char ccachedir[PATH_MAX] = GSSD_DEFAULT_CRED_DIR ":" GSSD_USER_CRED_DIR;
char *ccachesearch[GSSD_MAX_CCACHE_SEARCH + 1];
int use_memcache = 0;
@@ -341,14 +341,10 @@ main(int argc, char *argv[])
rpc_verbosity++;
break;
case 'p':
- strncpy(pipefs_dir, optarg, sizeof(pipefs_dir));
- if (pipefs_dir[sizeof(pipefs_dir)-1] != '\0')
- errx(1, "pipefs path name too long");
+ pipefs_dir = optarg;
break;
case 'k':
- strncpy(keytabfile, optarg, sizeof(keytabfile));
- if (keytabfile[sizeof(keytabfile)-1] != '\0')
- errx(1, "keytab path name too long");
+ keytabfile = optarg;
break;
case 'd':
strncpy(ccachedir, optarg, sizeof(ccachedir));
diff --git a/utils/gssd/gssd.h b/utils/gssd/gssd.h
index e16b187..4059544 100644
--- a/utils/gssd/gssd.h
+++ b/utils/gssd/gssd.h
@@ -58,8 +58,8 @@
*/
enum {AUTHTYPE_KRB5, AUTHTYPE_LIPKEY};
-extern char keytabfile[PATH_MAX];
-extern char *ccachesearch[];
+extern char *keytabfile;
+extern char *ccachesearch[];
extern int use_memcache;
extern int root_uses_machine_creds;
extern unsigned int context_timeout;
Get rid of another arbitrary limitation and PATH_MAX array.
Signed-off-by: David Härdeman <[email protected]>
---
utils/gssd/gssd.c | 45 ++++++++++++++++++++++++++++++++++-----------
utils/gssd/gssd.h | 3 +--
2 files changed, 35 insertions(+), 13 deletions(-)
diff --git a/utils/gssd/gssd.c b/utils/gssd/gssd.c
index 6147d30..60e753c 100644
--- a/utils/gssd/gssd.c
+++ b/utils/gssd/gssd.c
@@ -69,8 +69,7 @@
static char *pipefs_dir = GSSD_PIPEFS_DIR;
char *keytabfile = GSSD_DEFAULT_KEYTAB_FILE;
-char ccachedir[PATH_MAX] = GSSD_DEFAULT_CRED_DIR ":" GSSD_USER_CRED_DIR;
-char *ccachesearch[GSSD_MAX_CCACHE_SEARCH + 1];
+char **ccachesearch;
int use_memcache = 0;
int root_uses_machine_creds = 1;
unsigned int context_timeout = 0;
@@ -318,8 +317,8 @@ main(int argc, char *argv[])
int i;
extern char *optarg;
char *progname;
+ char *ccachedir = NULL;
- memset(ccachesearch, 0, sizeof(ccachesearch));
while ((opt = getopt(argc, argv, "DfvrlmnMp:k:d:t:T:R:")) != -1) {
switch (opt) {
case 'f':
@@ -347,9 +346,7 @@ main(int argc, char *argv[])
keytabfile = optarg;
break;
case 'd':
- strncpy(ccachedir, optarg, sizeof(ccachedir));
- if (ccachedir[sizeof(ccachedir)-1] != '\0')
- errx(1, "ccachedir path name too long");
+ ccachedir = optarg;
break;
case 't':
context_timeout = atoi(optarg);
@@ -388,11 +385,37 @@ main(int argc, char *argv[])
exit(1);
}
- i = 0;
- ccachesearch[i++] = strtok(ccachedir, ":");
- do {
- ccachesearch[i++] = strtok(NULL, ":");
- } while (ccachesearch[i-1] != NULL && i < GSSD_MAX_CCACHE_SEARCH);
+ if (ccachedir) {
+ char *ccachedir_copy;
+ char *ptr;
+
+ for (ptr = ccachedir, i = 2; *ptr; ptr++)
+ if (*ptr == ':')
+ i++;
+
+ ccachesearch = malloc(i * sizeof(char *));
+ ccachedir_copy = strdup(ccachedir);
+ if (!ccachedir_copy || !ccachesearch) {
+ printerr(0, "malloc failure\n");
+ exit(EXIT_FAILURE);
+ }
+
+ i = 0;
+ ccachesearch[i++] = strtok(ccachedir, ":");
+ while(ccachesearch[i - 1])
+ ccachesearch[i++] = strtok(NULL, ":");
+
+ } else {
+ ccachesearch = malloc(3 * sizeof(char *));
+ if (!ccachesearch) {
+ printerr(0, "malloc failure\n");
+ exit(EXIT_FAILURE);
+ }
+
+ ccachesearch[0] = GSSD_DEFAULT_CRED_DIR;
+ ccachesearch[1] = GSSD_USER_CRED_DIR;
+ ccachesearch[2] = NULL;
+ }
if (preferred_realm == NULL)
gssd_k5_get_default_realm(&preferred_realm);
diff --git a/utils/gssd/gssd.h b/utils/gssd/gssd.h
index 4059544..3320d5e 100644
--- a/utils/gssd/gssd.h
+++ b/utils/gssd/gssd.h
@@ -51,7 +51,6 @@
#define GSSD_DEFAULT_KEYTAB_FILE "/etc/krb5.keytab"
#define GSSD_SERVICE_NAME "nfs"
#define GSSD_SERVICE_NAME_LEN 3
-#define GSSD_MAX_CCACHE_SEARCH 16
/*
* The gss mechanisms that we can handle
@@ -59,7 +58,7 @@
enum {AUTHTYPE_KRB5, AUTHTYPE_LIPKEY};
extern char *keytabfile;
-extern char *ccachesearch[];
+extern char **ccachesearch;
extern int use_memcache;
extern int root_uses_machine_creds;
extern unsigned int context_timeout;
By chdir():ing to the root of the rpc_pipefs dir and making paths
relative from there (gssd already keeps a number of files open
in rpc_pipefs so chdir doesn't suddenly make it impossible to
umount rpc_pipefs because of this patch).
Signed-off-by: David Härdeman <[email protected]>
---
utils/gssd/gssd.c | 93 +++++++++++++++++++++--------------------------------
utils/gssd/gssd.h | 6 ++-
2 files changed, 40 insertions(+), 59 deletions(-)
diff --git a/utils/gssd/gssd.c b/utils/gssd/gssd.c
index 60e753c..716a387 100644
--- a/utils/gssd/gssd.c
+++ b/utils/gssd/gssd.c
@@ -125,28 +125,26 @@ scan_poll_results(int ret)
}
static int
-topdirs_add_entry(struct dirent *dent)
+topdirs_add_entry(int pfd, const char *name)
{
struct topdirs_info *tdi;
- tdi = calloc(sizeof(struct topdirs_info), 1);
- if (tdi == NULL) {
+ tdi = malloc(sizeof(*tdi) + strlen(pipefs_dir) + strlen(name) + 2);
+ if (!tdi) {
printerr(0, "ERROR: Couldn't allocate struct topdirs_info\n");
return -1;
}
- tdi->dirname = malloc(PATH_MAX);
- if (tdi->dirname == NULL) {
- printerr(0, "ERROR: Couldn't allocate directory name\n");
- free(tdi);
- return -1;
- }
- snprintf(tdi->dirname, PATH_MAX, "%s/%s", pipefs_dir, dent->d_name);
- tdi->fd = open(tdi->dirname, O_RDONLY);
- if (tdi->fd == -1) {
- printerr(0, "ERROR: failed to open %s\n", tdi->dirname);
+
+ sprintf(tdi->dirname, "%s/%s", pipefs_dir, name);
+
+ tdi->fd = openat(pfd, name, O_RDONLY);
+ if (tdi->fd < 0) {
+ printerr(0, "ERROR: failed to open %s: %s\n",
+ tdi->dirname, strerror(errno));
free(tdi);
return -1;
}
+
fcntl(tdi->fd, F_SETSIG, DNOTIFY_SIGNAL);
fcntl(tdi->fd, F_NOTIFY, DN_CREATE|DN_DELETE|DN_MODIFY|DN_MULTISHOT);
@@ -155,53 +153,37 @@ topdirs_add_entry(struct dirent *dent)
}
static void
-topdirs_free_list(void)
-{
- struct topdirs_info *tdi;
-
- TAILQ_FOREACH(tdi, &topdirs_list, list) {
- free(tdi->dirname);
- if (tdi->fd != -1)
- close(tdi->fd);
- TAILQ_REMOVE(&topdirs_list, tdi, list);
- free(tdi);
- }
-}
-
-static int
topdirs_init_list(void)
{
- DIR *pipedir;
- struct dirent *dent;
- int ret;
+ DIR *pipedir;
+ struct dirent *dent;
TAILQ_INIT(&topdirs_list);
- pipedir = opendir(pipefs_dir);
- if (pipedir == NULL) {
- printerr(0, "ERROR: could not open rpc_pipefs directory '%s': "
- "%s\n", pipefs_dir, strerror(errno));
- return -1;
+ pipedir = opendir(".");
+ if (!pipedir) {
+ printerr(0, "ERROR: could not open rpc_pipefs directory: '%s'\n",
+ strerror(errno));
+ exit(EXIT_FAILURE);
}
- for (dent = readdir(pipedir); dent != NULL; dent = readdir(pipedir)) {
- if (dent->d_type != DT_DIR ||
- strcmp(dent->d_name, ".") == 0 ||
- strcmp(dent->d_name, "..") == 0) {
+
+ while ((dent = readdir(pipedir))) {
+ if (dent->d_type != DT_DIR)
continue;
- }
- ret = topdirs_add_entry(dent);
- if (ret)
- goto out_err;
+
+ if (dent->d_name[0] == '.')
+ continue;
+
+ if (topdirs_add_entry(dirfd(pipedir), dent->d_name))
+ exit(EXIT_FAILURE);
}
+
if (TAILQ_EMPTY(&topdirs_list)) {
- printerr(0, "ERROR: rpc_pipefs directory '%s' is empty!\n", pipefs_dir);
- return -1;
+ printerr(0, "ERROR: the rpc_pipefs directory is empty!\n");
+ exit(EXIT_FAILURE);
}
+
closedir(pipedir);
- return 0;
-out_err:
- topdirs_free_list();
- return -1;
}
#ifdef HAVE_PPOLL
@@ -257,10 +239,7 @@ gssd_run(void)
sigaddset(&set, DNOTIFY_SIGNAL);
sigprocmask(SIG_UNBLOCK, &set, NULL);
- if (topdirs_init_list() != 0) {
- /* Error msg is already printed */
- exit(1);
- }
+ topdirs_init_list();
init_client_list();
printerr(1, "beginning poll\n");
@@ -276,9 +255,6 @@ gssd_run(void)
}
gssd_poll(pollarray, pollsize);
}
- topdirs_free_list();
-
- return;
}
static void
@@ -441,6 +417,11 @@ main(int argc, char *argv[])
daemon_init(fg);
+ if (chdir(pipefs_dir)) {
+ printerr(1, "ERROR: chdir(%s) failed: %s\n", pipefs_dir, strerror(errno));
+ exit(EXIT_FAILURE);
+ }
+
signal(SIGINT, sig_die);
signal(SIGTERM, sig_die);
signal(SIGHUP, sig_hup);
diff --git a/utils/gssd/gssd.h b/utils/gssd/gssd.h
index 3320d5e..91be83b 100644
--- a/utils/gssd/gssd.h
+++ b/utils/gssd/gssd.h
@@ -89,9 +89,9 @@ struct clnt_info {
TAILQ_HEAD(topdirs_list_head, topdirs_info) topdirs_list;
struct topdirs_info {
- TAILQ_ENTRY(topdirs_info) list;
- char *dirname;
- int fd;
+ TAILQ_ENTRY(topdirs_info) list;
+ int fd;
+ char dirname[];
};
void init_client_list(void);
Move all rpc_pipefs scanning code from gssd_proc.c to gssd.c in
preparation for later patches.
Signed-off-by: David Härdeman <[email protected]>
---
utils/gssd/gss_util.h | 2
utils/gssd/gssd.c | 568 ++++++++++++++++++++++++++++++++++++++++++++++++
utils/gssd/gssd.h | 16 -
utils/gssd/gssd_proc.c | 543 ----------------------------------------------
4 files changed, 566 insertions(+), 563 deletions(-)
diff --git a/utils/gssd/gss_util.h b/utils/gssd/gss_util.h
index c81fc5a..aa9f778 100644
--- a/utils/gssd/gss_util.h
+++ b/utils/gssd/gss_util.h
@@ -52,6 +52,4 @@ int gssd_check_mechs(void);
gss_krb5_set_allowable_enctypes(min, cred, num, types)
#endif
-extern int avoid_dns;
-
#endif /* _GSS_UTIL_H_ */
diff --git a/utils/gssd/gssd.c b/utils/gssd/gssd.c
index 716a387..21abaed 100644
--- a/utils/gssd/gssd.c
+++ b/utils/gssd/gssd.c
@@ -46,9 +46,12 @@
#include <sys/param.h>
#include <sys/socket.h>
+#include <sys/time.h>
+#include <sys/resource.h>
#include <sys/poll.h>
#include <rpc/rpc.h>
#include <netinet/in.h>
+#include <arpa/inet.h>
#include <unistd.h>
#include <err.h>
@@ -60,6 +63,7 @@
#include <memory.h>
#include <fcntl.h>
#include <dirent.h>
+#include <netdb.h>
#include "gssd.h"
#include "err_util.h"
@@ -75,11 +79,19 @@ int root_uses_machine_creds = 1;
unsigned int context_timeout = 0;
unsigned int rpc_timeout = 5;
char *preferred_realm = NULL;
-extern struct pollfd *pollarray;
-extern unsigned long pollsize;
#define POLL_MILLISECS 500
+TAILQ_HEAD(clnt_list_head, clnt_info) clnt_list;
+
+TAILQ_HEAD(topdirs_list_head, topdirs_info) topdirs_list;
+
+struct topdirs_info {
+ TAILQ_ENTRY(topdirs_info) list;
+ int fd;
+ char dirname[];
+};
+
static volatile int dir_changed = 1;
static void dir_notify_handler(__attribute__((unused))int sig)
@@ -87,6 +99,536 @@ static void dir_notify_handler(__attribute__((unused))int sig)
dir_changed = 1;
}
+
+/*
+ * pollarray:
+ * array of struct pollfd suitable to pass to poll. initialized to
+ * zero - a zero struct is ignored by poll() because the events mask is 0.
+ *
+ * clnt_list:
+ * linked list of struct clnt_info which associates a clntXXX directory
+ * with an index into pollarray[], and other basic data about that client.
+ *
+ * Directory structure: created by the kernel
+ * {rpc_pipefs}/{dir}/clntXX : one per rpc_clnt struct in the kernel
+ * {rpc_pipefs}/{dir}/clntXX/krb5 : read uid for which kernel wants
+ * a context, write the resulting context
+ * {rpc_pipefs}/{dir}/clntXX/info : stores info such as server name
+ * {rpc_pipefs}/{dir}/clntXX/gssd : pipe for all gss mechanisms using
+ * a text-based string of parameters
+ *
+ * Algorithm:
+ * Poll all {rpc_pipefs}/{dir}/clntXX/YYYY files. When data is ready,
+ * read and process; performs rpcsec_gss context initialization protocol to
+ * get a cred for that user. Writes result to corresponding krb5 file
+ * in a form the kernel code will understand.
+ * In addition, we make sure we are notified whenever anything is
+ * created or destroyed in {rpc_pipefs} or in any of the clntXX directories,
+ * and rescan the whole {rpc_pipefs} when this happens.
+ */
+
+static struct pollfd * pollarray;
+
+static unsigned long pollsize; /* the size of pollaray (in pollfd's) */
+
+/* Avoid DNS reverse lookups on server names */
+static int avoid_dns = 1;
+
+/*
+ * convert a presentation address string to a sockaddr_storage struct. Returns
+ * true on success or false on failure.
+ *
+ * Note that we do not populate the sin6_scope_id field here for IPv6 addrs.
+ * gssd nececessarily relies on hostname resolution and DNS AAAA records
+ * do not generally contain scope-id's. This means that GSSAPI auth really
+ * can't work with IPv6 link-local addresses.
+ *
+ * We *could* consider changing this if we did something like adopt the
+ * Microsoft "standard" of using the ipv6-literal.net domainname, but it's
+ * not really feasible at present.
+ */
+static int
+addrstr_to_sockaddr(struct sockaddr *sa, const char *node, const char *port)
+{
+ int rc;
+ struct addrinfo *res;
+ struct addrinfo hints = { .ai_flags = AI_NUMERICHOST | AI_NUMERICSERV };
+
+#ifndef IPV6_SUPPORTED
+ hints.ai_family = AF_INET;
+#endif /* IPV6_SUPPORTED */
+
+ rc = getaddrinfo(node, port, &hints, &res);
+ if (rc) {
+ printerr(0, "ERROR: unable to convert %s|%s to sockaddr: %s\n",
+ node, port, rc == EAI_SYSTEM ? strerror(errno) :
+ gai_strerror(rc));
+ return 0;
+ }
+
+#ifdef IPV6_SUPPORTED
+ /*
+ * getnameinfo ignores the scopeid. If the address turns out to have
+ * a non-zero scopeid, we can't use it -- the resolved host might be
+ * completely different from the one intended.
+ */
+ if (res->ai_addr->sa_family == AF_INET6) {
+ struct sockaddr_in6 *sin6 = (struct sockaddr_in6 *)res->ai_addr;
+ if (sin6->sin6_scope_id) {
+ printerr(0, "ERROR: address %s has non-zero "
+ "sin6_scope_id!\n", node);
+ freeaddrinfo(res);
+ return 0;
+ }
+ }
+#endif /* IPV6_SUPPORTED */
+
+ memcpy(sa, res->ai_addr, res->ai_addrlen);
+ freeaddrinfo(res);
+ return 1;
+}
+
+/*
+ * convert a sockaddr to a hostname
+ */
+static char *
+get_servername(const char *name, const struct sockaddr *sa, const char *addr)
+{
+ socklen_t addrlen;
+ int err;
+ char *hostname;
+ char hbuf[NI_MAXHOST];
+ unsigned char buf[sizeof(struct in6_addr)];
+
+ if (avoid_dns) {
+ /*
+ * Determine if this is a server name, or an IP address.
+ * If it is an IP address, do the DNS lookup otherwise
+ * skip the DNS lookup.
+ */
+ int is_fqdn = 1;
+ if (strchr(name, '.') == NULL)
+ is_fqdn = 0; /* local name */
+ else if (inet_pton(AF_INET, name, buf) == 1)
+ is_fqdn = 0; /* IPv4 address */
+ else if (inet_pton(AF_INET6, name, buf) == 1)
+ is_fqdn = 0; /* IPv6 addrss */
+
+ if (is_fqdn) {
+ return strdup(name);
+ }
+ /* Sorry, cannot avoid dns after all */
+ }
+
+ switch (sa->sa_family) {
+ case AF_INET:
+ addrlen = sizeof(struct sockaddr_in);
+ break;
+#ifdef IPV6_SUPPORTED
+ case AF_INET6:
+ addrlen = sizeof(struct sockaddr_in6);
+ break;
+#endif /* IPV6_SUPPORTED */
+ default:
+ printerr(0, "ERROR: unrecognized addr family %d\n",
+ sa->sa_family);
+ return NULL;
+ }
+
+ err = getnameinfo(sa, addrlen, hbuf, sizeof(hbuf), NULL, 0,
+ NI_NAMEREQD);
+ if (err) {
+ printerr(0, "ERROR: unable to resolve %s to hostname: %s\n",
+ addr, err == EAI_SYSTEM ? strerror(errno) :
+ gai_strerror(err));
+ return NULL;
+ }
+
+ hostname = strdup(hbuf);
+
+ return hostname;
+}
+
+/* XXX buffer problems: */
+static int
+read_service_info(char *info_file_name, char **servicename, char **servername,
+ int *prog, int *vers, char **protocol,
+ struct sockaddr *addr) {
+#define INFOBUFLEN 256
+ char buf[INFOBUFLEN + 1];
+ static char server[128];
+ int nbytes;
+ static char service[128];
+ static char address[128];
+ char program[16];
+ char version[16];
+ char protoname[16];
+ char port[128];
+ char *p;
+ int fd = -1;
+ int numfields;
+
+ *servicename = *servername = *protocol = NULL;
+
+ if ((fd = open(info_file_name, O_RDONLY)) == -1) {
+ printerr(0, "ERROR: can't open %s: %s\n", info_file_name,
+ strerror(errno));
+ goto fail;
+ }
+ if ((nbytes = read(fd, buf, INFOBUFLEN)) == -1)
+ goto fail;
+ close(fd);
+ fd = -1;
+ buf[nbytes] = '\0';
+
+ numfields = sscanf(buf,"RPC server: %127s\n"
+ "service: %127s %15s version %15s\n"
+ "address: %127s\n"
+ "protocol: %15s\n",
+ server,
+ service, program, version,
+ address,
+ protoname);
+
+ if (numfields == 5) {
+ strcpy(protoname, "tcp");
+ } else if (numfields != 6) {
+ goto fail;
+ }
+
+ port[0] = '\0';
+ if ((p = strstr(buf, "port")) != NULL)
+ sscanf(p, "port: %127s\n", port);
+
+ /* get program, and version numbers */
+ *prog = atoi(program + 1); /* skip open paren */
+ *vers = atoi(version);
+
+ if (!addrstr_to_sockaddr(addr, address, port))
+ goto fail;
+
+ *servername = get_servername(server, addr, address);
+ if (*servername == NULL)
+ goto fail;
+
+ nbytes = snprintf(buf, INFOBUFLEN, "%s@%s", service, *servername);
+ if (nbytes > INFOBUFLEN)
+ goto fail;
+
+ if (!(*servicename = calloc(strlen(buf) + 1, 1)))
+ goto fail;
+ memcpy(*servicename, buf, strlen(buf));
+
+ if (!(*protocol = strdup(protoname)))
+ goto fail;
+ return 0;
+fail:
+ printerr(0, "ERROR: failed to read service info\n");
+ if (fd != -1) close(fd);
+ free(*servername);
+ free(*servicename);
+ free(*protocol);
+ *servicename = *servername = *protocol = NULL;
+ return -1;
+}
+
+static void
+destroy_client(struct clnt_info *clp)
+{
+ if (clp->krb5_poll_index != -1)
+ memset(&pollarray[clp->krb5_poll_index], 0,
+ sizeof(struct pollfd));
+ if (clp->gssd_poll_index != -1)
+ memset(&pollarray[clp->gssd_poll_index], 0,
+ sizeof(struct pollfd));
+ if (clp->dir_fd != -1) close(clp->dir_fd);
+ if (clp->krb5_fd != -1) close(clp->krb5_fd);
+ if (clp->gssd_fd != -1) close(clp->gssd_fd);
+ free(clp->dirname);
+ free(clp->pdir);
+ free(clp->servicename);
+ free(clp->servername);
+ free(clp->protocol);
+ free(clp);
+}
+
+static struct clnt_info *
+insert_new_clnt(void)
+{
+ struct clnt_info *clp = NULL;
+
+ if (!(clp = (struct clnt_info *)calloc(1,sizeof(struct clnt_info)))) {
+ printerr(0, "ERROR: can't malloc clnt_info: %s\n",
+ strerror(errno));
+ goto out;
+ }
+ clp->krb5_poll_index = -1;
+ clp->gssd_poll_index = -1;
+ clp->krb5_fd = -1;
+ clp->gssd_fd = -1;
+ clp->dir_fd = -1;
+
+ TAILQ_INSERT_HEAD(&clnt_list, clp, list);
+out:
+ return clp;
+}
+
+static int
+process_clnt_dir_files(struct clnt_info * clp)
+{
+ char name[PATH_MAX];
+ char gname[PATH_MAX];
+ char info_file_name[PATH_MAX];
+
+ if (clp->gssd_close_me) {
+ printerr(2, "Closing 'gssd' pipe for %s\n", clp->dirname);
+ close(clp->gssd_fd);
+ memset(&pollarray[clp->gssd_poll_index], 0,
+ sizeof(struct pollfd));
+ clp->gssd_fd = -1;
+ clp->gssd_poll_index = -1;
+ clp->gssd_close_me = 0;
+ }
+ if (clp->krb5_close_me) {
+ printerr(2, "Closing 'krb5' pipe for %s\n", clp->dirname);
+ close(clp->krb5_fd);
+ memset(&pollarray[clp->krb5_poll_index], 0,
+ sizeof(struct pollfd));
+ clp->krb5_fd = -1;
+ clp->krb5_poll_index = -1;
+ clp->krb5_close_me = 0;
+ }
+
+ if (clp->gssd_fd == -1) {
+ snprintf(gname, sizeof(gname), "%s/gssd", clp->dirname);
+ clp->gssd_fd = open(gname, O_RDWR);
+ }
+ if (clp->gssd_fd == -1) {
+ if (clp->krb5_fd == -1) {
+ snprintf(name, sizeof(name), "%s/krb5", clp->dirname);
+ clp->krb5_fd = open(name, O_RDWR);
+ }
+
+ /* If we opened a gss-specific pipe, let's try opening
+ * the new upcall pipe again. If we succeed, close
+ * gss-specific pipe(s).
+ */
+ if (clp->krb5_fd != -1) {
+ clp->gssd_fd = open(gname, O_RDWR);
+ if (clp->gssd_fd != -1) {
+ if (clp->krb5_fd != -1)
+ close(clp->krb5_fd);
+ clp->krb5_fd = -1;
+ }
+ }
+ }
+
+ if ((clp->krb5_fd == -1) && (clp->gssd_fd == -1))
+ return -1;
+ snprintf(info_file_name, sizeof(info_file_name), "%s/info",
+ clp->dirname);
+ if (clp->prog == 0)
+ read_service_info(info_file_name, &clp->servicename,
+ &clp->servername, &clp->prog, &clp->vers,
+ &clp->protocol, (struct sockaddr *) &clp->addr);
+ return 0;
+}
+
+static int
+get_poll_index(int *ind)
+{
+ unsigned int i;
+
+ *ind = -1;
+ for (i=0; i<pollsize; i++) {
+ if (pollarray[i].events == 0) {
+ *ind = i;
+ break;
+ }
+ }
+ if (*ind == -1) {
+ printerr(0, "ERROR: No pollarray slots open\n");
+ return -1;
+ }
+ return 0;
+}
+
+
+static int
+insert_clnt_poll(struct clnt_info *clp)
+{
+ if ((clp->gssd_fd != -1) && (clp->gssd_poll_index == -1)) {
+ if (get_poll_index(&clp->gssd_poll_index)) {
+ printerr(0, "ERROR: Too many gssd clients\n");
+ return -1;
+ }
+ pollarray[clp->gssd_poll_index].fd = clp->gssd_fd;
+ pollarray[clp->gssd_poll_index].events |= POLLIN;
+ }
+
+ if ((clp->krb5_fd != -1) && (clp->krb5_poll_index == -1)) {
+ if (get_poll_index(&clp->krb5_poll_index)) {
+ printerr(0, "ERROR: Too many krb5 clients\n");
+ return -1;
+ }
+ pollarray[clp->krb5_poll_index].fd = clp->krb5_fd;
+ pollarray[clp->krb5_poll_index].events |= POLLIN;
+ }
+
+ return 0;
+}
+
+static void
+process_clnt_dir(char *dir, char *pdir)
+{
+ struct clnt_info * clp;
+
+ if (!(clp = insert_new_clnt()))
+ goto fail_destroy_client;
+
+ if (!(clp->pdir = strdup(pdir)))
+ goto fail_destroy_client;
+
+ /* An extra for the '/', and an extra for the null */
+ if (!(clp->dirname = calloc(strlen(dir) + strlen(pdir) + 2, 1))) {
+ goto fail_destroy_client;
+ }
+ sprintf(clp->dirname, "%s/%s", pdir, dir);
+ if ((clp->dir_fd = open(clp->dirname, O_RDONLY)) == -1) {
+ if (errno != ENOENT)
+ printerr(0, "ERROR: can't open %s: %s\n",
+ clp->dirname, strerror(errno));
+ goto fail_destroy_client;
+ }
+ fcntl(clp->dir_fd, F_SETSIG, DNOTIFY_SIGNAL);
+ fcntl(clp->dir_fd, F_NOTIFY, DN_CREATE | DN_DELETE | DN_MULTISHOT);
+
+ if (process_clnt_dir_files(clp))
+ goto fail_keep_client;
+
+ if (insert_clnt_poll(clp))
+ goto fail_destroy_client;
+
+ return;
+
+fail_destroy_client:
+ if (clp) {
+ TAILQ_REMOVE(&clnt_list, clp, list);
+ destroy_client(clp);
+ }
+fail_keep_client:
+ /* We couldn't find some subdirectories, but we keep the client
+ * around in case we get a notification on the directory when the
+ * subdirectories are created. */
+ return;
+}
+
+/*
+ * This is run after a DNOTIFY signal, and should clear up any
+ * directories that are no longer around, and re-scan any existing
+ * directories, since the DNOTIFY could have been in there.
+ */
+static void
+update_old_clients(struct dirent **namelist, int size, char *pdir)
+{
+ struct clnt_info *clp;
+ void *saveprev;
+ int i, stillhere;
+ char fname[PATH_MAX];
+
+ for (clp = clnt_list.tqh_first; clp != NULL; clp = clp->list.tqe_next) {
+ /* only compare entries in the global list that are from the
+ * same pipefs parent directory as "pdir"
+ */
+ if (strcmp(clp->pdir, pdir) != 0) continue;
+
+ stillhere = 0;
+ for (i=0; i < size; i++) {
+ snprintf(fname, sizeof(fname), "%s/%s",
+ pdir, namelist[i]->d_name);
+ if (strcmp(clp->dirname, fname) == 0) {
+ stillhere = 1;
+ break;
+ }
+ }
+ if (!stillhere) {
+ printerr(2, "destroying client %s\n", clp->dirname);
+ saveprev = clp->list.tqe_prev;
+ TAILQ_REMOVE(&clnt_list, clp, list);
+ destroy_client(clp);
+ clp = saveprev;
+ }
+ }
+ for (clp = clnt_list.tqh_first; clp != NULL; clp = clp->list.tqe_next) {
+ if (!process_clnt_dir_files(clp))
+ insert_clnt_poll(clp);
+ }
+}
+
+/* Search for a client by directory name, return 1 if found, 0 otherwise */
+static int
+find_client(char *dirname, char *pdir)
+{
+ struct clnt_info *clp;
+ char fname[PATH_MAX];
+
+ for (clp = clnt_list.tqh_first; clp != NULL; clp = clp->list.tqe_next) {
+ snprintf(fname, sizeof(fname), "%s/%s", pdir, dirname);
+ if (strcmp(clp->dirname, fname) == 0)
+ return 1;
+ }
+ return 0;
+}
+
+static int
+process_pipedir(char *pipe_name)
+{
+ struct dirent **namelist;
+ int i, j;
+
+ if (chdir(pipe_name) < 0) {
+ printerr(0, "ERROR: can't chdir to %s: %s\n",
+ pipe_name, strerror(errno));
+ return -1;
+ }
+
+ j = scandir(pipe_name, &namelist, NULL, alphasort);
+ if (j < 0) {
+ printerr(0, "ERROR: can't scandir %s: %s\n",
+ pipe_name, strerror(errno));
+ return -1;
+ }
+
+ update_old_clients(namelist, j, pipe_name);
+ for (i=0; i < j; i++) {
+ if (!strncmp(namelist[i]->d_name, "clnt", 4)
+ && !find_client(namelist[i]->d_name, pipe_name))
+ process_clnt_dir(namelist[i]->d_name, pipe_name);
+ free(namelist[i]);
+ }
+
+ free(namelist);
+
+ return 0;
+}
+
+/* Used to read (and re-read) list of clients, set up poll array. */
+static int
+update_client_list(void)
+{
+ int retval = -1;
+ struct topdirs_info *tdi;
+
+ TAILQ_FOREACH(tdi, &topdirs_list, list) {
+ retval = process_pipedir(tdi->dirname);
+ if (retval)
+ printerr(1, "WARNING: error processing %s\n",
+ tdi->dirname);
+
+ }
+ return retval;
+}
+
static void
scan_poll_results(int ret)
{
@@ -223,6 +765,28 @@ static void gssd_poll(struct pollfd *fds, unsigned long nfds)
}
#endif /* !HAVE_PPOLL */
+
+#define FD_ALLOC_BLOCK 256
+static void
+init_client_list(void)
+{
+ struct rlimit rlim;
+
+ TAILQ_INIT(&clnt_list);
+
+ /* Eventually plan to grow/shrink poll array: */
+ if (!getrlimit(RLIMIT_NOFILE, &rlim) && rlim.rlim_cur != RLIM_INFINITY)
+ pollsize = rlim.rlim_cur;
+ else
+ pollsize = FD_ALLOC_BLOCK;
+
+ pollarray = calloc(pollsize, sizeof(struct pollfd));
+ if (!pollarray) {
+ printerr(1, "ERROR: calloc failed\n");
+ exit(EXIT_FAILURE);
+ }
+}
+
static void
gssd_run(void)
{
diff --git a/utils/gssd/gssd.h b/utils/gssd/gssd.h
index 91be83b..72e68d9 100644
--- a/utils/gssd/gssd.h
+++ b/utils/gssd/gssd.h
@@ -35,13 +35,9 @@
#include <sys/queue.h>
#include <gssapi/gssapi.h>
-#define MAX_FILE_NAMELEN 32
-#define FD_ALLOC_BLOCK 256
#ifndef GSSD_PIPEFS_DIR
#define GSSD_PIPEFS_DIR "/var/lib/nfs/rpc_pipefs"
#endif
-#define INFO "info"
-#define KRB5 "krb5"
#define DNOTIFY_SIGNAL (SIGRTMIN + 3)
#define GSSD_DEFAULT_CRED_DIR "/tmp"
@@ -50,7 +46,6 @@
#define GSSD_DEFAULT_MACHINE_CRED_SUFFIX "machine"
#define GSSD_DEFAULT_KEYTAB_FILE "/etc/krb5.keytab"
#define GSSD_SERVICE_NAME "nfs"
-#define GSSD_SERVICE_NAME_LEN 3
/*
* The gss mechanisms that we can handle
@@ -65,8 +60,6 @@ extern unsigned int context_timeout;
extern unsigned int rpc_timeout;
extern char *preferred_realm;
-TAILQ_HEAD(clnt_list_head, clnt_info) clnt_list;
-
struct clnt_info {
TAILQ_ENTRY(clnt_info) list;
char *dirname;
@@ -86,16 +79,7 @@ struct clnt_info {
struct sockaddr_storage addr;
};
-TAILQ_HEAD(topdirs_list_head, topdirs_info) topdirs_list;
-
-struct topdirs_info {
- TAILQ_ENTRY(topdirs_info) list;
- int fd;
- char dirname[];
-};
-void init_client_list(void);
-int update_client_list(void);
void handle_krb5_upcall(struct clnt_info *clp);
void handle_gssd_upcall(struct clnt_info *clp);
diff --git a/utils/gssd/gssd_proc.c b/utils/gssd/gssd_proc.c
index 1d8e6a7..8957b27 100644
--- a/utils/gssd/gssd_proc.c
+++ b/utils/gssd/gssd_proc.c
@@ -52,7 +52,6 @@
#include <sys/socket.h>
#include <arpa/inet.h>
#include <sys/fsuid.h>
-#include <sys/resource.h>
#include <stdio.h>
#include <stdlib.h>
@@ -80,548 +79,6 @@
#include "gss_names.h"
#include "misc.h"
-/*
- * pollarray:
- * array of struct pollfd suitable to pass to poll. initialized to
- * zero - a zero struct is ignored by poll() because the events mask is 0.
- *
- * clnt_list:
- * linked list of struct clnt_info which associates a clntXXX directory
- * with an index into pollarray[], and other basic data about that client.
- *
- * Directory structure: created by the kernel
- * {rpc_pipefs}/{dir}/clntXX : one per rpc_clnt struct in the kernel
- * {rpc_pipefs}/{dir}/clntXX/krb5 : read uid for which kernel wants
- * a context, write the resulting context
- * {rpc_pipefs}/{dir}/clntXX/info : stores info such as server name
- * {rpc_pipefs}/{dir}/clntXX/gssd : pipe for all gss mechanisms using
- * a text-based string of parameters
- *
- * Algorithm:
- * Poll all {rpc_pipefs}/{dir}/clntXX/YYYY files. When data is ready,
- * read and process; performs rpcsec_gss context initialization protocol to
- * get a cred for that user. Writes result to corresponding krb5 file
- * in a form the kernel code will understand.
- * In addition, we make sure we are notified whenever anything is
- * created or destroyed in {rpc_pipefs} or in any of the clntXX directories,
- * and rescan the whole {rpc_pipefs} when this happens.
- */
-
-struct pollfd * pollarray;
-
-unsigned long pollsize; /* the size of pollaray (in pollfd's) */
-
-/* Avoid DNS reverse lookups on server names */
-int avoid_dns = 1;
-
-/*
- * convert a presentation address string to a sockaddr_storage struct. Returns
- * true on success or false on failure.
- *
- * Note that we do not populate the sin6_scope_id field here for IPv6 addrs.
- * gssd nececessarily relies on hostname resolution and DNS AAAA records
- * do not generally contain scope-id's. This means that GSSAPI auth really
- * can't work with IPv6 link-local addresses.
- *
- * We *could* consider changing this if we did something like adopt the
- * Microsoft "standard" of using the ipv6-literal.net domainname, but it's
- * not really feasible at present.
- */
-static int
-addrstr_to_sockaddr(struct sockaddr *sa, const char *node, const char *port)
-{
- int rc;
- struct addrinfo *res;
- struct addrinfo hints = { .ai_flags = AI_NUMERICHOST | AI_NUMERICSERV };
-
-#ifndef IPV6_SUPPORTED
- hints.ai_family = AF_INET;
-#endif /* IPV6_SUPPORTED */
-
- rc = getaddrinfo(node, port, &hints, &res);
- if (rc) {
- printerr(0, "ERROR: unable to convert %s|%s to sockaddr: %s\n",
- node, port, rc == EAI_SYSTEM ? strerror(errno) :
- gai_strerror(rc));
- return 0;
- }
-
-#ifdef IPV6_SUPPORTED
- /*
- * getnameinfo ignores the scopeid. If the address turns out to have
- * a non-zero scopeid, we can't use it -- the resolved host might be
- * completely different from the one intended.
- */
- if (res->ai_addr->sa_family == AF_INET6) {
- struct sockaddr_in6 *sin6 = (struct sockaddr_in6 *)res->ai_addr;
- if (sin6->sin6_scope_id) {
- printerr(0, "ERROR: address %s has non-zero "
- "sin6_scope_id!\n", node);
- freeaddrinfo(res);
- return 0;
- }
- }
-#endif /* IPV6_SUPPORTED */
-
- memcpy(sa, res->ai_addr, res->ai_addrlen);
- freeaddrinfo(res);
- return 1;
-}
-
-/*
- * convert a sockaddr to a hostname
- */
-static char *
-get_servername(const char *name, const struct sockaddr *sa, const char *addr)
-{
- socklen_t addrlen;
- int err;
- char *hostname;
- char hbuf[NI_MAXHOST];
- unsigned char buf[sizeof(struct in6_addr)];
-
- if (avoid_dns) {
- /*
- * Determine if this is a server name, or an IP address.
- * If it is an IP address, do the DNS lookup otherwise
- * skip the DNS lookup.
- */
- int is_fqdn = 1;
- if (strchr(name, '.') == NULL)
- is_fqdn = 0; /* local name */
- else if (inet_pton(AF_INET, name, buf) == 1)
- is_fqdn = 0; /* IPv4 address */
- else if (inet_pton(AF_INET6, name, buf) == 1)
- is_fqdn = 0; /* IPv6 addrss */
-
- if (is_fqdn) {
- return strdup(name);
- }
- /* Sorry, cannot avoid dns after all */
- }
-
- switch (sa->sa_family) {
- case AF_INET:
- addrlen = sizeof(struct sockaddr_in);
- break;
-#ifdef IPV6_SUPPORTED
- case AF_INET6:
- addrlen = sizeof(struct sockaddr_in6);
- break;
-#endif /* IPV6_SUPPORTED */
- default:
- printerr(0, "ERROR: unrecognized addr family %d\n",
- sa->sa_family);
- return NULL;
- }
-
- err = getnameinfo(sa, addrlen, hbuf, sizeof(hbuf), NULL, 0,
- NI_NAMEREQD);
- if (err) {
- printerr(0, "ERROR: unable to resolve %s to hostname: %s\n",
- addr, err == EAI_SYSTEM ? strerror(errno) :
- gai_strerror(err));
- return NULL;
- }
-
- hostname = strdup(hbuf);
-
- return hostname;
-}
-
-/* XXX buffer problems: */
-static int
-read_service_info(char *info_file_name, char **servicename, char **servername,
- int *prog, int *vers, char **protocol,
- struct sockaddr *addr) {
-#define INFOBUFLEN 256
- char buf[INFOBUFLEN + 1];
- static char server[128];
- int nbytes;
- static char service[128];
- static char address[128];
- char program[16];
- char version[16];
- char protoname[16];
- char port[128];
- char *p;
- int fd = -1;
- int numfields;
-
- *servicename = *servername = *protocol = NULL;
-
- if ((fd = open(info_file_name, O_RDONLY)) == -1) {
- printerr(0, "ERROR: can't open %s: %s\n", info_file_name,
- strerror(errno));
- goto fail;
- }
- if ((nbytes = read(fd, buf, INFOBUFLEN)) == -1)
- goto fail;
- close(fd);
- fd = -1;
- buf[nbytes] = '\0';
-
- numfields = sscanf(buf,"RPC server: %127s\n"
- "service: %127s %15s version %15s\n"
- "address: %127s\n"
- "protocol: %15s\n",
- server,
- service, program, version,
- address,
- protoname);
-
- if (numfields == 5) {
- strcpy(protoname, "tcp");
- } else if (numfields != 6) {
- goto fail;
- }
-
- port[0] = '\0';
- if ((p = strstr(buf, "port")) != NULL)
- sscanf(p, "port: %127s\n", port);
-
- /* get program, and version numbers */
- *prog = atoi(program + 1); /* skip open paren */
- *vers = atoi(version);
-
- if (!addrstr_to_sockaddr(addr, address, port))
- goto fail;
-
- *servername = get_servername(server, addr, address);
- if (*servername == NULL)
- goto fail;
-
- nbytes = snprintf(buf, INFOBUFLEN, "%s@%s", service, *servername);
- if (nbytes > INFOBUFLEN)
- goto fail;
-
- if (!(*servicename = calloc(strlen(buf) + 1, 1)))
- goto fail;
- memcpy(*servicename, buf, strlen(buf));
-
- if (!(*protocol = strdup(protoname)))
- goto fail;
- return 0;
-fail:
- printerr(0, "ERROR: failed to read service info\n");
- if (fd != -1) close(fd);
- free(*servername);
- free(*servicename);
- free(*protocol);
- *servicename = *servername = *protocol = NULL;
- return -1;
-}
-
-static void
-destroy_client(struct clnt_info *clp)
-{
- if (clp->krb5_poll_index != -1)
- memset(&pollarray[clp->krb5_poll_index], 0,
- sizeof(struct pollfd));
- if (clp->gssd_poll_index != -1)
- memset(&pollarray[clp->gssd_poll_index], 0,
- sizeof(struct pollfd));
- if (clp->dir_fd != -1) close(clp->dir_fd);
- if (clp->krb5_fd != -1) close(clp->krb5_fd);
- if (clp->gssd_fd != -1) close(clp->gssd_fd);
- free(clp->dirname);
- free(clp->pdir);
- free(clp->servicename);
- free(clp->servername);
- free(clp->protocol);
- free(clp);
-}
-
-static struct clnt_info *
-insert_new_clnt(void)
-{
- struct clnt_info *clp = NULL;
-
- if (!(clp = (struct clnt_info *)calloc(1,sizeof(struct clnt_info)))) {
- printerr(0, "ERROR: can't malloc clnt_info: %s\n",
- strerror(errno));
- goto out;
- }
- clp->krb5_poll_index = -1;
- clp->gssd_poll_index = -1;
- clp->krb5_fd = -1;
- clp->gssd_fd = -1;
- clp->dir_fd = -1;
-
- TAILQ_INSERT_HEAD(&clnt_list, clp, list);
-out:
- return clp;
-}
-
-static int
-process_clnt_dir_files(struct clnt_info * clp)
-{
- char name[PATH_MAX];
- char gname[PATH_MAX];
- char info_file_name[PATH_MAX];
-
- if (clp->gssd_close_me) {
- printerr(2, "Closing 'gssd' pipe for %s\n", clp->dirname);
- close(clp->gssd_fd);
- memset(&pollarray[clp->gssd_poll_index], 0,
- sizeof(struct pollfd));
- clp->gssd_fd = -1;
- clp->gssd_poll_index = -1;
- clp->gssd_close_me = 0;
- }
- if (clp->krb5_close_me) {
- printerr(2, "Closing 'krb5' pipe for %s\n", clp->dirname);
- close(clp->krb5_fd);
- memset(&pollarray[clp->krb5_poll_index], 0,
- sizeof(struct pollfd));
- clp->krb5_fd = -1;
- clp->krb5_poll_index = -1;
- clp->krb5_close_me = 0;
- }
-
- if (clp->gssd_fd == -1) {
- snprintf(gname, sizeof(gname), "%s/gssd", clp->dirname);
- clp->gssd_fd = open(gname, O_RDWR);
- }
- if (clp->gssd_fd == -1) {
- if (clp->krb5_fd == -1) {
- snprintf(name, sizeof(name), "%s/krb5", clp->dirname);
- clp->krb5_fd = open(name, O_RDWR);
- }
-
- /* If we opened a gss-specific pipe, let's try opening
- * the new upcall pipe again. If we succeed, close
- * gss-specific pipe(s).
- */
- if (clp->krb5_fd != -1) {
- clp->gssd_fd = open(gname, O_RDWR);
- if (clp->gssd_fd != -1) {
- if (clp->krb5_fd != -1)
- close(clp->krb5_fd);
- clp->krb5_fd = -1;
- }
- }
- }
-
- if ((clp->krb5_fd == -1) && (clp->gssd_fd == -1))
- return -1;
- snprintf(info_file_name, sizeof(info_file_name), "%s/info",
- clp->dirname);
- if (clp->prog == 0)
- read_service_info(info_file_name, &clp->servicename,
- &clp->servername, &clp->prog, &clp->vers,
- &clp->protocol, (struct sockaddr *) &clp->addr);
- return 0;
-}
-
-static int
-get_poll_index(int *ind)
-{
- unsigned int i;
-
- *ind = -1;
- for (i=0; i<pollsize; i++) {
- if (pollarray[i].events == 0) {
- *ind = i;
- break;
- }
- }
- if (*ind == -1) {
- printerr(0, "ERROR: No pollarray slots open\n");
- return -1;
- }
- return 0;
-}
-
-
-static int
-insert_clnt_poll(struct clnt_info *clp)
-{
- if ((clp->gssd_fd != -1) && (clp->gssd_poll_index == -1)) {
- if (get_poll_index(&clp->gssd_poll_index)) {
- printerr(0, "ERROR: Too many gssd clients\n");
- return -1;
- }
- pollarray[clp->gssd_poll_index].fd = clp->gssd_fd;
- pollarray[clp->gssd_poll_index].events |= POLLIN;
- }
-
- if ((clp->krb5_fd != -1) && (clp->krb5_poll_index == -1)) {
- if (get_poll_index(&clp->krb5_poll_index)) {
- printerr(0, "ERROR: Too many krb5 clients\n");
- return -1;
- }
- pollarray[clp->krb5_poll_index].fd = clp->krb5_fd;
- pollarray[clp->krb5_poll_index].events |= POLLIN;
- }
-
- return 0;
-}
-
-static void
-process_clnt_dir(char *dir, char *pdir)
-{
- struct clnt_info * clp;
-
- if (!(clp = insert_new_clnt()))
- goto fail_destroy_client;
-
- if (!(clp->pdir = strdup(pdir)))
- goto fail_destroy_client;
-
- /* An extra for the '/', and an extra for the null */
- if (!(clp->dirname = calloc(strlen(dir) + strlen(pdir) + 2, 1))) {
- goto fail_destroy_client;
- }
- sprintf(clp->dirname, "%s/%s", pdir, dir);
- if ((clp->dir_fd = open(clp->dirname, O_RDONLY)) == -1) {
- if (errno != ENOENT)
- printerr(0, "ERROR: can't open %s: %s\n",
- clp->dirname, strerror(errno));
- goto fail_destroy_client;
- }
- fcntl(clp->dir_fd, F_SETSIG, DNOTIFY_SIGNAL);
- fcntl(clp->dir_fd, F_NOTIFY, DN_CREATE | DN_DELETE | DN_MULTISHOT);
-
- if (process_clnt_dir_files(clp))
- goto fail_keep_client;
-
- if (insert_clnt_poll(clp))
- goto fail_destroy_client;
-
- return;
-
-fail_destroy_client:
- if (clp) {
- TAILQ_REMOVE(&clnt_list, clp, list);
- destroy_client(clp);
- }
-fail_keep_client:
- /* We couldn't find some subdirectories, but we keep the client
- * around in case we get a notification on the directory when the
- * subdirectories are created. */
- return;
-}
-
-void
-init_client_list(void)
-{
- struct rlimit rlim;
- TAILQ_INIT(&clnt_list);
- /* Eventually plan to grow/shrink poll array: */
- pollsize = FD_ALLOC_BLOCK;
- if (getrlimit(RLIMIT_NOFILE, &rlim) == 0 &&
- rlim.rlim_cur != RLIM_INFINITY)
- pollsize = rlim.rlim_cur;
- pollarray = calloc(pollsize, sizeof(struct pollfd));
-}
-
-/*
- * This is run after a DNOTIFY signal, and should clear up any
- * directories that are no longer around, and re-scan any existing
- * directories, since the DNOTIFY could have been in there.
- */
-static void
-update_old_clients(struct dirent **namelist, int size, char *pdir)
-{
- struct clnt_info *clp;
- void *saveprev;
- int i, stillhere;
- char fname[PATH_MAX];
-
- for (clp = clnt_list.tqh_first; clp != NULL; clp = clp->list.tqe_next) {
- /* only compare entries in the global list that are from the
- * same pipefs parent directory as "pdir"
- */
- if (strcmp(clp->pdir, pdir) != 0) continue;
-
- stillhere = 0;
- for (i=0; i < size; i++) {
- snprintf(fname, sizeof(fname), "%s/%s",
- pdir, namelist[i]->d_name);
- if (strcmp(clp->dirname, fname) == 0) {
- stillhere = 1;
- break;
- }
- }
- if (!stillhere) {
- printerr(2, "destroying client %s\n", clp->dirname);
- saveprev = clp->list.tqe_prev;
- TAILQ_REMOVE(&clnt_list, clp, list);
- destroy_client(clp);
- clp = saveprev;
- }
- }
- for (clp = clnt_list.tqh_first; clp != NULL; clp = clp->list.tqe_next) {
- if (!process_clnt_dir_files(clp))
- insert_clnt_poll(clp);
- }
-}
-
-/* Search for a client by directory name, return 1 if found, 0 otherwise */
-static int
-find_client(char *dirname, char *pdir)
-{
- struct clnt_info *clp;
- char fname[PATH_MAX];
-
- for (clp = clnt_list.tqh_first; clp != NULL; clp = clp->list.tqe_next) {
- snprintf(fname, sizeof(fname), "%s/%s", pdir, dirname);
- if (strcmp(clp->dirname, fname) == 0)
- return 1;
- }
- return 0;
-}
-
-static int
-process_pipedir(char *pipe_name)
-{
- struct dirent **namelist;
- int i, j;
-
- if (chdir(pipe_name) < 0) {
- printerr(0, "ERROR: can't chdir to %s: %s\n",
- pipe_name, strerror(errno));
- return -1;
- }
-
- j = scandir(pipe_name, &namelist, NULL, alphasort);
- if (j < 0) {
- printerr(0, "ERROR: can't scandir %s: %s\n",
- pipe_name, strerror(errno));
- return -1;
- }
-
- update_old_clients(namelist, j, pipe_name);
- for (i=0; i < j; i++) {
- if (!strncmp(namelist[i]->d_name, "clnt", 4)
- && !find_client(namelist[i]->d_name, pipe_name))
- process_clnt_dir(namelist[i]->d_name, pipe_name);
- free(namelist[i]);
- }
-
- free(namelist);
-
- return 0;
-}
-
-/* Used to read (and re-read) list of clients, set up poll array. */
-int
-update_client_list(void)
-{
- int retval = -1;
- struct topdirs_info *tdi;
-
- TAILQ_FOREACH(tdi, &topdirs_list, list) {
- retval = process_pipedir(tdi->dirname);
- if (retval)
- printerr(1, "WARNING: error processing %s\n",
- tdi->dirname);
-
- }
- return retval;
-}
-
/* Encryption types supported by the kernel rpcsec_gss code */
int num_krb5_enctypes = 0;
krb5_enctype *krb5_enctypes = NULL;
Simplify the client directory scanning and merge it into one
function that can be called both when a client initially
appears and later when it needs to be updated.
Signed-off-by: David Härdeman <[email protected]>
---
utils/gssd/gssd.c | 85 ++++++++++++++++++++++-------------------------------
1 file changed, 36 insertions(+), 49 deletions(-)
diff --git a/utils/gssd/gssd.c b/utils/gssd/gssd.c
index 21abaed..7fa27c8 100644
--- a/utils/gssd/gssd.c
+++ b/utils/gssd/gssd.c
@@ -374,11 +374,29 @@ out:
}
static int
+get_poll_index(int *ind)
+{
+ unsigned int i;
+
+ *ind = -1;
+ for (i=0; i<pollsize; i++) {
+ if (pollarray[i].events == 0) {
+ *ind = i;
+ break;
+ }
+ }
+ if (*ind == -1) {
+ printerr(0, "ERROR: No pollarray slots open\n");
+ return -1;
+ }
+ return 0;
+}
+
+static int
process_clnt_dir_files(struct clnt_info * clp)
{
char name[PATH_MAX];
char gname[PATH_MAX];
- char info_file_name[PATH_MAX];
if (clp->gssd_close_me) {
printerr(2, "Closing 'gssd' pipe for %s\n", clp->dirname);
@@ -424,39 +442,18 @@ process_clnt_dir_files(struct clnt_info * clp)
}
if ((clp->krb5_fd == -1) && (clp->gssd_fd == -1))
- return -1;
- snprintf(info_file_name, sizeof(info_file_name), "%s/info",
- clp->dirname);
- if (clp->prog == 0)
+ /* not fatal, files might appear later */
+ return 0;
+
+ if (clp->prog == 0) {
+ char info_file_name[strlen(clp->dirname) + 6];
+
+ sprintf(info_file_name, "%s/info", clp->dirname);
read_service_info(info_file_name, &clp->servicename,
&clp->servername, &clp->prog, &clp->vers,
&clp->protocol, (struct sockaddr *) &clp->addr);
- return 0;
-}
-
-static int
-get_poll_index(int *ind)
-{
- unsigned int i;
-
- *ind = -1;
- for (i=0; i<pollsize; i++) {
- if (pollarray[i].events == 0) {
- *ind = i;
- break;
- }
}
- if (*ind == -1) {
- printerr(0, "ERROR: No pollarray slots open\n");
- return -1;
- }
- return 0;
-}
-
-static int
-insert_clnt_poll(struct clnt_info *clp)
-{
if ((clp->gssd_fd != -1) && (clp->gssd_poll_index == -1)) {
if (get_poll_index(&clp->gssd_poll_index)) {
printerr(0, "ERROR: Too many gssd clients\n");
@@ -484,43 +481,35 @@ process_clnt_dir(char *dir, char *pdir)
struct clnt_info * clp;
if (!(clp = insert_new_clnt()))
- goto fail_destroy_client;
+ goto out;
if (!(clp->pdir = strdup(pdir)))
- goto fail_destroy_client;
+ goto out;
/* An extra for the '/', and an extra for the null */
- if (!(clp->dirname = calloc(strlen(dir) + strlen(pdir) + 2, 1))) {
- goto fail_destroy_client;
- }
+ if (!(clp->dirname = calloc(strlen(dir) + strlen(pdir) + 2, 1)))
+ goto out;
+
sprintf(clp->dirname, "%s/%s", pdir, dir);
if ((clp->dir_fd = open(clp->dirname, O_RDONLY)) == -1) {
if (errno != ENOENT)
printerr(0, "ERROR: can't open %s: %s\n",
clp->dirname, strerror(errno));
- goto fail_destroy_client;
+ goto out;
}
fcntl(clp->dir_fd, F_SETSIG, DNOTIFY_SIGNAL);
fcntl(clp->dir_fd, F_NOTIFY, DN_CREATE | DN_DELETE | DN_MULTISHOT);
if (process_clnt_dir_files(clp))
- goto fail_keep_client;
-
- if (insert_clnt_poll(clp))
- goto fail_destroy_client;
+ goto out;
return;
-fail_destroy_client:
+out:
if (clp) {
TAILQ_REMOVE(&clnt_list, clp, list);
destroy_client(clp);
}
-fail_keep_client:
- /* We couldn't find some subdirectories, but we keep the client
- * around in case we get a notification on the directory when the
- * subdirectories are created. */
- return;
}
/*
@@ -559,10 +548,8 @@ update_old_clients(struct dirent **namelist, int size, char *pdir)
clp = saveprev;
}
}
- for (clp = clnt_list.tqh_first; clp != NULL; clp = clp->list.tqe_next) {
- if (!process_clnt_dir_files(clp))
- insert_clnt_poll(clp);
- }
+ for (clp = clnt_list.tqh_first; clp != NULL; clp = clp->list.tqe_next)
+ process_clnt_dir_files(clp);
}
/* Search for a client by directory name, return 1 if found, 0 otherwise */
Using libevent (which is already in use in idmap) saves about a hundred
lines of hand-rolled event loop code.
Signed-off-by: David Härdeman <[email protected]>
---
utils/gssd/Makefile.am | 23 +++-
utils/gssd/gssd.c | 309 ++++++++++++++----------------------------------
utils/gssd/gssd.h | 15 +-
3 files changed, 116 insertions(+), 231 deletions(-)
diff --git a/utils/gssd/Makefile.am b/utils/gssd/Makefile.am
index 0f0142b..cb040b3 100644
--- a/utils/gssd/Makefile.am
+++ b/utils/gssd/Makefile.am
@@ -43,12 +43,23 @@ gssd_SOURCES = \
krb5_util.h \
write_bytes.h
-gssd_LDADD = ../../support/nfs/libnfs.a \
- $(RPCSECGSS_LIBS) $(KRBLIBS) $(GSSAPI_LIBS) $(LIBTIRPC)
-gssd_LDFLAGS = $(KRBLDFLAGS)
-
-gssd_CFLAGS = $(AM_CFLAGS) $(CFLAGS) \
- $(RPCSECGSS_CFLAGS) $(KRBCFLAGS) $(GSSAPI_CFLAGS)
+gssd_LDADD = \
+ ../../support/nfs/libnfs.a \
+ $(LIBEVENT) \
+ $(RPCSECGSS_LIBS) \
+ $(KRBLIBS) \
+ $(GSSAPI_LIBS) \
+ $(LIBTIRPC)
+
+gssd_LDFLAGS = \
+ $(KRBLDFLAGS)
+
+gssd_CFLAGS = \
+ $(AM_CFLAGS) \
+ $(CFLAGS) \
+ $(RPCSECGSS_CFLAGS) \
+ $(KRBCFLAGS) \
+ $(GSSAPI_CFLAGS)
svcgssd_SOURCES = \
$(COMMON_SRCS) \
diff --git a/utils/gssd/gssd.c b/utils/gssd/gssd.c
index 7fa27c8..632d1a1 100644
--- a/utils/gssd/gssd.c
+++ b/utils/gssd/gssd.c
@@ -64,6 +64,7 @@
#include <fcntl.h>
#include <dirent.h>
#include <netdb.h>
+#include <event.h>
#include "gssd.h"
#include "err_util.h"
@@ -80,8 +81,6 @@ unsigned int context_timeout = 0;
unsigned int rpc_timeout = 5;
char *preferred_realm = NULL;
-#define POLL_MILLISECS 500
-
TAILQ_HEAD(clnt_list_head, clnt_info) clnt_list;
TAILQ_HEAD(topdirs_list_head, topdirs_info) topdirs_list;
@@ -92,22 +91,9 @@ struct topdirs_info {
char dirname[];
};
-static volatile int dir_changed = 1;
-
-static void dir_notify_handler(__attribute__((unused))int sig)
-{
- dir_changed = 1;
-}
-
-
/*
- * pollarray:
- * array of struct pollfd suitable to pass to poll. initialized to
- * zero - a zero struct is ignored by poll() because the events mask is 0.
- *
* clnt_list:
- * linked list of struct clnt_info which associates a clntXXX directory
- * with an index into pollarray[], and other basic data about that client.
+ * linked list of struct clnt_info with basic data about a clntXXX dir.
*
* Directory structure: created by the kernel
* {rpc_pipefs}/{dir}/clntXX : one per rpc_clnt struct in the kernel
@@ -127,10 +113,6 @@ static void dir_notify_handler(__attribute__((unused))int sig)
* and rescan the whole {rpc_pipefs} when this happens.
*/
-static struct pollfd * pollarray;
-
-static unsigned long pollsize; /* the size of pollaray (in pollfd's) */
-
/* Avoid DNS reverse lookups on server names */
static int avoid_dns = 1;
@@ -335,15 +317,19 @@ fail:
static void
destroy_client(struct clnt_info *clp)
{
- if (clp->krb5_poll_index != -1)
- memset(&pollarray[clp->krb5_poll_index], 0,
- sizeof(struct pollfd));
- if (clp->gssd_poll_index != -1)
- memset(&pollarray[clp->gssd_poll_index], 0,
- sizeof(struct pollfd));
- if (clp->dir_fd != -1) close(clp->dir_fd);
- if (clp->krb5_fd != -1) close(clp->krb5_fd);
- if (clp->gssd_fd != -1) close(clp->gssd_fd);
+ if (clp->krb5_fd >= 0) {
+ close(clp->krb5_fd);
+ event_del(&clp->krb5_ev);
+ }
+
+ if (clp->gssd_fd >= 0) {
+ close(clp->gssd_fd);
+ event_del(&clp->gssd_ev);
+ }
+
+ if (clp->dir_fd >= 0)
+ close(clp->dir_fd);
+
free(clp->dirname);
free(clp->pdir);
free(clp->servicename);
@@ -362,8 +348,7 @@ insert_new_clnt(void)
strerror(errno));
goto out;
}
- clp->krb5_poll_index = -1;
- clp->gssd_poll_index = -1;
+
clp->krb5_fd = -1;
clp->gssd_fd = -1;
clp->dir_fd = -1;
@@ -373,23 +358,34 @@ out:
return clp;
}
-static int
-get_poll_index(int *ind)
+static void gssd_update_clients(void);
+
+static void
+gssd_clnt_gssd_cb(int UNUSED(fd), short which, void *data)
{
- unsigned int i;
+ struct clnt_info *clp = data;
- *ind = -1;
- for (i=0; i<pollsize; i++) {
- if (pollarray[i].events == 0) {
- *ind = i;
- break;
- }
+ if (which != EV_READ) {
+ clp->gssd_close_me = true;
+ gssd_update_clients();
+ return;
}
- if (*ind == -1) {
- printerr(0, "ERROR: No pollarray slots open\n");
- return -1;
+
+ handle_gssd_upcall(clp);
+}
+
+static void
+gssd_clnt_krb5_cb(int UNUSED(fd), short which, void *data)
+{
+ struct clnt_info *clp = data;
+
+ if (which != EV_READ) {
+ clp->krb5_close_me = true;
+ gssd_update_clients();
+ return;
}
- return 0;
+
+ handle_krb5_upcall(clp);
}
static int
@@ -397,30 +393,33 @@ process_clnt_dir_files(struct clnt_info * clp)
{
char name[PATH_MAX];
char gname[PATH_MAX];
+ bool gssd_was_closed;
+ bool krb5_was_closed;
if (clp->gssd_close_me) {
printerr(2, "Closing 'gssd' pipe for %s\n", clp->dirname);
close(clp->gssd_fd);
- memset(&pollarray[clp->gssd_poll_index], 0,
- sizeof(struct pollfd));
+ event_del(&clp->gssd_ev);
clp->gssd_fd = -1;
- clp->gssd_poll_index = -1;
- clp->gssd_close_me = 0;
+ clp->gssd_close_me = false;
}
+
if (clp->krb5_close_me) {
printerr(2, "Closing 'krb5' pipe for %s\n", clp->dirname);
close(clp->krb5_fd);
- memset(&pollarray[clp->krb5_poll_index], 0,
- sizeof(struct pollfd));
+ event_del(&clp->krb5_ev);
clp->krb5_fd = -1;
- clp->krb5_poll_index = -1;
- clp->krb5_close_me = 0;
+ clp->krb5_close_me = false;
}
+ gssd_was_closed = clp->gssd_fd < 0 ? true : false;
+ krb5_was_closed = clp->krb5_fd < 0 ? true : false;
+
if (clp->gssd_fd == -1) {
snprintf(gname, sizeof(gname), "%s/gssd", clp->dirname);
clp->gssd_fd = open(gname, O_RDWR);
}
+
if (clp->gssd_fd == -1) {
if (clp->krb5_fd == -1) {
snprintf(name, sizeof(name), "%s/krb5", clp->dirname);
@@ -441,6 +440,18 @@ process_clnt_dir_files(struct clnt_info * clp)
}
}
+ if (gssd_was_closed && clp->gssd_fd >= 0) {
+ event_set(&clp->gssd_ev, clp->gssd_fd, EV_READ | EV_PERSIST,
+ gssd_clnt_gssd_cb, clp);
+ event_add(&clp->gssd_ev, NULL);
+ }
+
+ if (krb5_was_closed && clp->krb5_fd >= 0) {
+ event_set(&clp->krb5_ev, clp->krb5_fd, EV_READ | EV_PERSIST,
+ gssd_clnt_krb5_cb, clp);
+ event_add(&clp->krb5_ev, NULL);
+ }
+
if ((clp->krb5_fd == -1) && (clp->gssd_fd == -1))
/* not fatal, files might appear later */
return 0;
@@ -454,24 +465,6 @@ process_clnt_dir_files(struct clnt_info * clp)
&clp->protocol, (struct sockaddr *) &clp->addr);
}
- if ((clp->gssd_fd != -1) && (clp->gssd_poll_index == -1)) {
- if (get_poll_index(&clp->gssd_poll_index)) {
- printerr(0, "ERROR: Too many gssd clients\n");
- return -1;
- }
- pollarray[clp->gssd_poll_index].fd = clp->gssd_fd;
- pollarray[clp->gssd_poll_index].events |= POLLIN;
- }
-
- if ((clp->krb5_fd != -1) && (clp->krb5_poll_index == -1)) {
- if (get_poll_index(&clp->krb5_poll_index)) {
- printerr(0, "ERROR: Too many krb5 clients\n");
- return -1;
- }
- pollarray[clp->krb5_poll_index].fd = clp->krb5_fd;
- pollarray[clp->krb5_poll_index].events |= POLLIN;
- }
-
return 0;
}
@@ -600,10 +593,10 @@ process_pipedir(char *pipe_name)
}
/* Used to read (and re-read) list of clients, set up poll array. */
-static int
-update_client_list(void)
+static void
+gssd_update_clients(void)
{
- int retval = -1;
+ int retval;
struct topdirs_info *tdi;
TAILQ_FOREACH(tdi, &topdirs_list, list) {
@@ -613,44 +606,12 @@ update_client_list(void)
tdi->dirname);
}
- return retval;
}
static void
-scan_poll_results(int ret)
+gssd_update_clients_cb(int UNUSED(ifd), short UNUSED(which), void *UNUSED(data))
{
- int i;
- struct clnt_info *clp;
-
- for (clp = clnt_list.tqh_first; clp != NULL; clp = clp->list.tqe_next)
- {
- i = clp->gssd_poll_index;
- if (i >= 0 && pollarray[i].revents) {
- if (pollarray[i].revents & POLLHUP) {
- clp->gssd_close_me = 1;
- dir_changed = 1;
- }
- if (pollarray[i].revents & POLLIN)
- handle_gssd_upcall(clp);
- pollarray[clp->gssd_poll_index].revents = 0;
- ret--;
- if (!ret)
- break;
- }
- i = clp->krb5_poll_index;
- if (i >= 0 && pollarray[i].revents) {
- if (pollarray[i].revents & POLLHUP) {
- clp->krb5_close_me = 1;
- dir_changed = 1;
- }
- if (pollarray[i].revents & POLLIN)
- handle_krb5_upcall(clp);
- pollarray[clp->krb5_poll_index].revents = 0;
- ret--;
- if (!ret)
- break;
- }
- }
+ gssd_update_clients();
}
static int
@@ -715,115 +676,11 @@ topdirs_init_list(void)
closedir(pipedir);
}
-#ifdef HAVE_PPOLL
-static void gssd_poll(struct pollfd *fds, unsigned long nfds)
-{
- sigset_t emptyset;
- int ret;
-
- sigemptyset(&emptyset);
- ret = ppoll(fds, nfds, NULL, &emptyset);
- if (ret < 0) {
- if (errno != EINTR)
- printerr(0, "WARNING: error return from poll\n");
- } else if (ret == 0) {
- printerr(0, "WARNING: unexpected timeout\n");
- } else {
- scan_poll_results(ret);
- }
-}
-#else /* !HAVE_PPOLL */
-static void gssd_poll(struct pollfd *fds, unsigned long nfds)
-{
- int ret;
-
- /* race condition here: dir_changed could be set before we
- * enter the poll, and we'd never notice if it weren't for the
- * timeout. */
- ret = poll(fds, nfds, POLL_MILLISECS);
- if (ret < 0) {
- if (errno != EINTR)
- printerr(0, "WARNING: error return from poll\n");
- } else if (ret == 0) {
- /* timeout */
- } else { /* ret > 0 */
- scan_poll_results(ret);
- }
-}
-#endif /* !HAVE_PPOLL */
-
-
-#define FD_ALLOC_BLOCK 256
-static void
-init_client_list(void)
-{
- struct rlimit rlim;
-
- TAILQ_INIT(&clnt_list);
-
- /* Eventually plan to grow/shrink poll array: */
- if (!getrlimit(RLIMIT_NOFILE, &rlim) && rlim.rlim_cur != RLIM_INFINITY)
- pollsize = rlim.rlim_cur;
- else
- pollsize = FD_ALLOC_BLOCK;
-
- pollarray = calloc(pollsize, sizeof(struct pollfd));
- if (!pollarray) {
- printerr(1, "ERROR: calloc failed\n");
- exit(EXIT_FAILURE);
- }
-}
-
static void
-gssd_run(void)
+gssd_atexit(void)
{
- struct sigaction dn_act = {
- .sa_handler = dir_notify_handler
- };
- sigset_t set;
-
- sigemptyset(&dn_act.sa_mask);
- sigaction(DNOTIFY_SIGNAL, &dn_act, NULL);
-
- /* just in case the signal is blocked... */
- sigemptyset(&set);
- sigaddset(&set, DNOTIFY_SIGNAL);
- sigprocmask(SIG_UNBLOCK, &set, NULL);
-
- topdirs_init_list();
- init_client_list();
-
- printerr(1, "beginning poll\n");
- while (1) {
- while (dir_changed) {
- dir_changed = 0;
- if (update_client_list()) {
- /* Error msg is already printed */
- exit(1);
- }
-
- daemon_ready();
- }
- gssd_poll(pollarray, pollsize);
- }
-}
-
-static void
-sig_die(int signal)
-{
- /* destroy krb5 machine creds */
if (root_uses_machine_creds)
gssd_destroy_krb5_machine_creds();
- printerr(1, "exiting on signal %d\n", signal);
- exit(0);
-}
-
-static void
-sig_hup(int signal)
-{
- /* don't exit on SIGHUP */
- printerr(1, "Received SIGHUP(%d)... Ignoring.\n", signal);
- return;
}
static void
@@ -845,6 +702,8 @@ main(int argc, char *argv[])
extern char *optarg;
char *progname;
char *ccachedir = NULL;
+ struct event sighup_ev;
+ struct event sigdnotify_ev;
while ((opt = getopt(argc, argv, "DfvrlmnMp:k:d:t:T:R:")) != -1) {
switch (opt) {
@@ -968,17 +827,31 @@ main(int argc, char *argv[])
daemon_init(fg);
+ event_init();
+
if (chdir(pipefs_dir)) {
printerr(1, "ERROR: chdir(%s) failed: %s\n", pipefs_dir, strerror(errno));
exit(EXIT_FAILURE);
}
- signal(SIGINT, sig_die);
- signal(SIGTERM, sig_die);
- signal(SIGHUP, sig_hup);
+ if (atexit(gssd_atexit)) {
+ printerr(1, "ERROR: atexit failed: %s\n", strerror(errno));
+ exit(EXIT_FAILURE);
+ }
+
+ signal_set(&sighup_ev, SIGHUP, gssd_update_clients_cb, NULL);
+ signal_add(&sighup_ev, NULL);
+ signal_set(&sigdnotify_ev, DNOTIFY_SIGNAL, gssd_update_clients_cb, NULL);
+ signal_add(&sigdnotify_ev, NULL);
+
+ topdirs_init_list();
+ TAILQ_INIT(&clnt_list);
+ gssd_update_clients();
+ daemon_ready();
+
+ event_dispatch();
- gssd_run();
- printerr(0, "gssd_run returned!\n");
- abort();
+ printerr(1, "ERROR: event_dispatch() returned!\n");
+ return EXIT_FAILURE;
}
diff --git a/utils/gssd/gssd.h b/utils/gssd/gssd.h
index 72e68d9..2417579 100644
--- a/utils/gssd/gssd.h
+++ b/utils/gssd/gssd.h
@@ -34,6 +34,8 @@
#include <sys/types.h>
#include <sys/queue.h>
#include <gssapi/gssapi.h>
+#include <event.h>
+#include <stdbool.h>
#ifndef GSSD_PIPEFS_DIR
#define GSSD_PIPEFS_DIR "/var/lib/nfs/rpc_pipefs"
@@ -71,15 +73,14 @@ struct clnt_info {
int vers;
char *protocol;
int krb5_fd;
- int krb5_poll_index;
- int krb5_close_me;
- int gssd_fd;
- int gssd_poll_index;
- int gssd_close_me;
- struct sockaddr_storage addr;
+ struct event krb5_ev;
+ bool krb5_close_me;
+ int gssd_fd;
+ struct event gssd_ev;
+ bool gssd_close_me;
+ struct sockaddr_storage addr;
};
-
void handle_krb5_upcall(struct clnt_info *clp);
void handle_gssd_upcall(struct clnt_info *clp);
This code is mostly just confusing. Close the fds immediately
instead of doing so later.
Signed-off-by: David Härdeman <[email protected]>
---
utils/gssd/gssd.c | 26 ++++++++------------------
utils/gssd/gssd.h | 2 --
2 files changed, 8 insertions(+), 20 deletions(-)
diff --git a/utils/gssd/gssd.c b/utils/gssd/gssd.c
index 632d1a1..2187562 100644
--- a/utils/gssd/gssd.c
+++ b/utils/gssd/gssd.c
@@ -366,7 +366,10 @@ gssd_clnt_gssd_cb(int UNUSED(fd), short which, void *data)
struct clnt_info *clp = data;
if (which != EV_READ) {
- clp->gssd_close_me = true;
+ printerr(2, "Closing 'gssd' pipe for %s\n", clp->dirname);
+ close(clp->gssd_fd);
+ clp->gssd_fd = -1;
+ event_del(&clp->gssd_ev);
gssd_update_clients();
return;
}
@@ -380,7 +383,10 @@ gssd_clnt_krb5_cb(int UNUSED(fd), short which, void *data)
struct clnt_info *clp = data;
if (which != EV_READ) {
- clp->krb5_close_me = true;
+ printerr(2, "Closing 'krb5' pipe for %s\n", clp->dirname);
+ close(clp->krb5_fd);
+ clp->krb5_fd = -1;
+ event_del(&clp->krb5_ev);
gssd_update_clients();
return;
}
@@ -396,22 +402,6 @@ process_clnt_dir_files(struct clnt_info * clp)
bool gssd_was_closed;
bool krb5_was_closed;
- if (clp->gssd_close_me) {
- printerr(2, "Closing 'gssd' pipe for %s\n", clp->dirname);
- close(clp->gssd_fd);
- event_del(&clp->gssd_ev);
- clp->gssd_fd = -1;
- clp->gssd_close_me = false;
- }
-
- if (clp->krb5_close_me) {
- printerr(2, "Closing 'krb5' pipe for %s\n", clp->dirname);
- close(clp->krb5_fd);
- event_del(&clp->krb5_ev);
- clp->krb5_fd = -1;
- clp->krb5_close_me = false;
- }
-
gssd_was_closed = clp->gssd_fd < 0 ? true : false;
krb5_was_closed = clp->krb5_fd < 0 ? true : false;
diff --git a/utils/gssd/gssd.h b/utils/gssd/gssd.h
index 2417579..cea2b92 100644
--- a/utils/gssd/gssd.h
+++ b/utils/gssd/gssd.h
@@ -74,10 +74,8 @@ struct clnt_info {
char *protocol;
int krb5_fd;
struct event krb5_ev;
- bool krb5_close_me;
int gssd_fd;
struct event gssd_ev;
- bool gssd_close_me;
struct sockaddr_storage addr;
};
This makes it easier to keep track of which client belongs
to which topdir.
Signed-off-by: David Härdeman <[email protected]>
---
utils/gssd/gssd.c | 138 +++++++++++++++++++++++++----------------------------
utils/gssd/gssd.h | 1
2 files changed, 66 insertions(+), 73 deletions(-)
diff --git a/utils/gssd/gssd.c b/utils/gssd/gssd.c
index 2187562..d0eae16 100644
--- a/utils/gssd/gssd.c
+++ b/utils/gssd/gssd.c
@@ -81,30 +81,33 @@ unsigned int context_timeout = 0;
unsigned int rpc_timeout = 5;
char *preferred_realm = NULL;
-TAILQ_HEAD(clnt_list_head, clnt_info) clnt_list;
+TAILQ_HEAD(topdir_list_head, topdir) topdir_list;
-TAILQ_HEAD(topdirs_list_head, topdirs_info) topdirs_list;
-
-struct topdirs_info {
- TAILQ_ENTRY(topdirs_info) list;
- int fd;
- char dirname[];
+struct topdir {
+ TAILQ_ENTRY(topdir) list;
+ TAILQ_HEAD(clnt_list_head, clnt_info) clnt_list;
+ int fd;
+ char dirname[];
};
/*
+ * topdir_list:
+ * linked list of struct topdir with basic data about a topdir.
+ *
* clnt_list:
- * linked list of struct clnt_info with basic data about a clntXXX dir.
+ * linked list of struct clnt_info with basic data about a clntXXX dir,
+ * one per topdir.
*
* Directory structure: created by the kernel
- * {rpc_pipefs}/{dir}/clntXX : one per rpc_clnt struct in the kernel
- * {rpc_pipefs}/{dir}/clntXX/krb5 : read uid for which kernel wants
+ * {rpc_pipefs}/{topdir}/clntXX : one per rpc_clnt struct in the kernel
+ * {rpc_pipefs}/{topdir}/clntXX/krb5 : read uid for which kernel wants
* a context, write the resulting context
- * {rpc_pipefs}/{dir}/clntXX/info : stores info such as server name
- * {rpc_pipefs}/{dir}/clntXX/gssd : pipe for all gss mechanisms using
+ * {rpc_pipefs}/{topdir}/clntXX/info : stores info such as server name
+ * {rpc_pipefs}/{topdir}/clntXX/gssd : pipe for all gss mechanisms using
* a text-based string of parameters
*
* Algorithm:
- * Poll all {rpc_pipefs}/{dir}/clntXX/YYYY files. When data is ready,
+ * Poll all {rpc_pipefs}/{topdir}/clntXX/YYYY files. When data is ready,
* read and process; performs rpcsec_gss context initialization protocol to
* get a cred for that user. Writes result to corresponding krb5 file
* in a form the kernel code will understand.
@@ -331,7 +334,6 @@ destroy_client(struct clnt_info *clp)
close(clp->dir_fd);
free(clp->dirname);
- free(clp->pdir);
free(clp->servicename);
free(clp->servername);
free(clp->protocol);
@@ -339,22 +341,22 @@ destroy_client(struct clnt_info *clp)
}
static struct clnt_info *
-insert_new_clnt(void)
+insert_new_clnt(struct topdir *tdi)
{
- struct clnt_info *clp = NULL;
+ struct clnt_info *clp;
- if (!(clp = (struct clnt_info *)calloc(1,sizeof(struct clnt_info)))) {
+ clp = calloc(1, sizeof(struct clnt_info));
+ if (!clp) {
printerr(0, "ERROR: can't malloc clnt_info: %s\n",
strerror(errno));
- goto out;
+ return NULL;
}
clp->krb5_fd = -1;
clp->gssd_fd = -1;
clp->dir_fd = -1;
- TAILQ_INSERT_HEAD(&clnt_list, clp, list);
-out:
+ TAILQ_INSERT_HEAD(&tdi->clnt_list, clp, list);
return clp;
}
@@ -459,21 +461,18 @@ process_clnt_dir_files(struct clnt_info * clp)
}
static void
-process_clnt_dir(char *dir, char *pdir)
+process_clnt_dir(struct topdir *tdi, char *dir)
{
- struct clnt_info * clp;
-
- if (!(clp = insert_new_clnt()))
- goto out;
+ struct clnt_info *clp;
- if (!(clp->pdir = strdup(pdir)))
+ if (!(clp = insert_new_clnt(tdi)))
goto out;
/* An extra for the '/', and an extra for the null */
- if (!(clp->dirname = calloc(strlen(dir) + strlen(pdir) + 2, 1)))
+ if (!(clp->dirname = calloc(strlen(dir) + strlen(tdi->dirname) + 2, 1)))
goto out;
- sprintf(clp->dirname, "%s/%s", pdir, dir);
+ sprintf(clp->dirname, "%s/%s", tdi->dirname, dir);
if ((clp->dir_fd = open(clp->dirname, O_RDONLY)) == -1) {
if (errno != ENOENT)
printerr(0, "ERROR: can't open %s: %s\n",
@@ -490,7 +489,7 @@ process_clnt_dir(char *dir, char *pdir)
out:
if (clp) {
- TAILQ_REMOVE(&clnt_list, clp, list);
+ TAILQ_REMOVE(&tdi->clnt_list, clp, list);
destroy_client(clp);
}
}
@@ -501,79 +500,74 @@ out:
* directories, since the DNOTIFY could have been in there.
*/
static void
-update_old_clients(struct dirent **namelist, int size, char *pdir)
+update_old_clients(struct topdir *tdi, struct dirent **namelist, int size)
{
struct clnt_info *clp;
void *saveprev;
- int i, stillhere;
+ int i;
+ bool stillhere;
char fname[PATH_MAX];
- for (clp = clnt_list.tqh_first; clp != NULL; clp = clp->list.tqe_next) {
- /* only compare entries in the global list that are from the
- * same pipefs parent directory as "pdir"
- */
- if (strcmp(clp->pdir, pdir) != 0) continue;
+ TAILQ_FOREACH(clp, &tdi->clnt_list, list) {
+ stillhere = false;
- stillhere = 0;
- for (i=0; i < size; i++) {
+ for (i = 0; i < size; i++) {
snprintf(fname, sizeof(fname), "%s/%s",
- pdir, namelist[i]->d_name);
+ tdi->dirname, namelist[i]->d_name);
if (strcmp(clp->dirname, fname) == 0) {
- stillhere = 1;
+ stillhere = true;
break;
}
}
+
if (!stillhere) {
printerr(2, "destroying client %s\n", clp->dirname);
saveprev = clp->list.tqe_prev;
- TAILQ_REMOVE(&clnt_list, clp, list);
+ TAILQ_REMOVE(&tdi->clnt_list, clp, list);
destroy_client(clp);
clp = saveprev;
}
}
- for (clp = clnt_list.tqh_first; clp != NULL; clp = clp->list.tqe_next)
+
+ TAILQ_FOREACH(clp, &tdi->clnt_list, list)
process_clnt_dir_files(clp);
}
/* Search for a client by directory name, return 1 if found, 0 otherwise */
-static int
-find_client(char *dirname, char *pdir)
+static bool
+find_client(struct topdir *tdi, const char *dirname)
{
- struct clnt_info *clp;
+ struct clnt_info *clp;
char fname[PATH_MAX];
- for (clp = clnt_list.tqh_first; clp != NULL; clp = clp->list.tqe_next) {
- snprintf(fname, sizeof(fname), "%s/%s", pdir, dirname);
- if (strcmp(clp->dirname, fname) == 0)
- return 1;
+ TAILQ_FOREACH(clp, &tdi->clnt_list, list) {
+ snprintf(fname, sizeof(fname), "%s/%s", tdi->dirname, dirname);
+ if (!strcmp(clp->dirname, fname))
+ return true;
}
- return 0;
+
+ return false;
}
static int
-process_pipedir(char *pipe_name)
+process_pipedir(struct topdir *tdi)
{
struct dirent **namelist;
int i, j;
- if (chdir(pipe_name) < 0) {
- printerr(0, "ERROR: can't chdir to %s: %s\n",
- pipe_name, strerror(errno));
- return -1;
- }
-
- j = scandir(pipe_name, &namelist, NULL, alphasort);
+ j = scandir(tdi->dirname, &namelist, NULL, alphasort);
if (j < 0) {
printerr(0, "ERROR: can't scandir %s: %s\n",
- pipe_name, strerror(errno));
+ tdi->dirname, strerror(errno));
return -1;
}
- update_old_clients(namelist, j, pipe_name);
- for (i=0; i < j; i++) {
+ update_old_clients(tdi, namelist, j);
+
+ for (i = 0; i < j; i++) {
if (!strncmp(namelist[i]->d_name, "clnt", 4)
- && !find_client(namelist[i]->d_name, pipe_name))
- process_clnt_dir(namelist[i]->d_name, pipe_name);
+ && !find_client(tdi, namelist[i]->d_name))
+ process_clnt_dir(tdi, namelist[i]->d_name);
free(namelist[i]);
}
@@ -587,10 +581,10 @@ static void
gssd_update_clients(void)
{
int retval;
- struct topdirs_info *tdi;
+ struct topdir *tdi;
- TAILQ_FOREACH(tdi, &topdirs_list, list) {
- retval = process_pipedir(tdi->dirname);
+ TAILQ_FOREACH(tdi, &topdir_list, list) {
+ retval = process_pipedir(tdi);
if (retval)
printerr(1, "WARNING: error processing %s\n",
tdi->dirname);
@@ -607,15 +601,16 @@ gssd_update_clients_cb(int UNUSED(ifd), short UNUSED(which), void *UNUSED(data))
static int
topdirs_add_entry(int pfd, const char *name)
{
- struct topdirs_info *tdi;
+ struct topdir *tdi;
tdi = malloc(sizeof(*tdi) + strlen(pipefs_dir) + strlen(name) + 2);
if (!tdi) {
- printerr(0, "ERROR: Couldn't allocate struct topdirs_info\n");
+ printerr(0, "ERROR: Couldn't allocate struct topdir\n");
return -1;
}
sprintf(tdi->dirname, "%s/%s", pipefs_dir, name);
+ TAILQ_INIT(&tdi->clnt_list);
tdi->fd = openat(pfd, name, O_RDONLY);
if (tdi->fd < 0) {
@@ -628,7 +623,7 @@ topdirs_add_entry(int pfd, const char *name)
fcntl(tdi->fd, F_SETSIG, DNOTIFY_SIGNAL);
fcntl(tdi->fd, F_NOTIFY, DN_CREATE|DN_DELETE|DN_MODIFY|DN_MULTISHOT);
- TAILQ_INSERT_HEAD(&topdirs_list, tdi, list);
+ TAILQ_INSERT_HEAD(&topdir_list, tdi, list);
return 0;
}
@@ -638,7 +633,7 @@ topdirs_init_list(void)
DIR *pipedir;
struct dirent *dent;
- TAILQ_INIT(&topdirs_list);
+ TAILQ_INIT(&topdir_list);
pipedir = opendir(".");
if (!pipedir) {
@@ -658,7 +653,7 @@ topdirs_init_list(void)
exit(EXIT_FAILURE);
}
- if (TAILQ_EMPTY(&topdirs_list)) {
+ if (TAILQ_EMPTY(&topdir_list)) {
printerr(0, "ERROR: the rpc_pipefs directory is empty!\n");
exit(EXIT_FAILURE);
}
@@ -835,7 +830,6 @@ main(int argc, char *argv[])
signal_add(&sigdnotify_ev, NULL);
topdirs_init_list();
- TAILQ_INIT(&clnt_list);
gssd_update_clients();
daemon_ready();
diff --git a/utils/gssd/gssd.h b/utils/gssd/gssd.h
index cea2b92..8a882d3 100644
--- a/utils/gssd/gssd.h
+++ b/utils/gssd/gssd.h
@@ -65,7 +65,6 @@ extern char *preferred_realm;
struct clnt_info {
TAILQ_ENTRY(clnt_info) list;
char *dirname;
- char *pdir;
int dir_fd;
char *servicename;
char *servername;
Keep the rpc_pipefs dir open and just do a rewind/rescan when
necessary.
Signed-off-by: David Härdeman <[email protected]>
---
utils/gssd/gssd.c | 86 +++++++++++++++++++++++------------------------------
1 file changed, 37 insertions(+), 49 deletions(-)
diff --git a/utils/gssd/gssd.c b/utils/gssd/gssd.c
index d0eae16..d03df2c 100644
--- a/utils/gssd/gssd.c
+++ b/utils/gssd/gssd.c
@@ -72,7 +72,9 @@
#include "krb5_util.h"
#include "nfslib.h"
-static char *pipefs_dir = GSSD_PIPEFS_DIR;
+static char *pipefs_path = GSSD_PIPEFS_DIR;
+static DIR *pipefs_dir;
+
char *keytabfile = GSSD_DEFAULT_KEYTAB_FILE;
char **ccachesearch;
int use_memcache = 0;
@@ -87,6 +89,7 @@ struct topdir {
TAILQ_ENTRY(topdir) list;
TAILQ_HEAD(clnt_list_head, clnt_info) clnt_list;
int fd;
+ char *name;
char dirname[];
};
@@ -550,7 +553,7 @@ find_client(struct topdir *tdi, const char *dirname)
}
static int
-process_pipedir(struct topdir *tdi)
+gssd_process_topdir(struct topdir *tdi)
{
struct dirent **namelist;
int i, j;
@@ -576,40 +579,23 @@ process_pipedir(struct topdir *tdi)
return 0;
}
-/* Used to read (and re-read) list of clients, set up poll array. */
-static void
-gssd_update_clients(void)
-{
- int retval;
- struct topdir *tdi;
-
- TAILQ_FOREACH(tdi, &topdir_list, list) {
- retval = process_pipedir(tdi);
- if (retval)
- printerr(1, "WARNING: error processing %s\n",
- tdi->dirname);
-
- }
-}
-
-static void
-gssd_update_clients_cb(int UNUSED(ifd), short UNUSED(which), void *UNUSED(data))
-{
- gssd_update_clients();
-}
-
static int
-topdirs_add_entry(int pfd, const char *name)
+gssd_add_topdir(int pfd, const char *name)
{
struct topdir *tdi;
- tdi = malloc(sizeof(*tdi) + strlen(pipefs_dir) + strlen(name) + 2);
+ TAILQ_FOREACH(tdi, &topdir_list, list)
+ if (!strcmp(tdi->name, name))
+ return gssd_process_topdir(tdi);
+
+ tdi = malloc(sizeof(*tdi) + strlen(pipefs_path) + strlen(name) + 2);
if (!tdi) {
printerr(0, "ERROR: Couldn't allocate struct topdir\n");
return -1;
}
- sprintf(tdi->dirname, "%s/%s", pipefs_dir, name);
+ sprintf(tdi->dirname, "%s/%s", pipefs_path, name);
+ tdi->name = tdi->dirname + strlen(pipefs_path) + 1;
TAILQ_INIT(&tdi->clnt_list);
tdi->fd = openat(pfd, name, O_RDONLY);
@@ -624,43 +610,39 @@ topdirs_add_entry(int pfd, const char *name)
fcntl(tdi->fd, F_NOTIFY, DN_CREATE|DN_DELETE|DN_MODIFY|DN_MULTISHOT);
TAILQ_INSERT_HEAD(&topdir_list, tdi, list);
- return 0;
+ return gssd_process_topdir(tdi);
}
static void
-topdirs_init_list(void)
+gssd_update_clients(void)
{
- DIR *pipedir;
- struct dirent *dent;
+ struct dirent *d;
- TAILQ_INIT(&topdir_list);
+ rewinddir(pipefs_dir);
- pipedir = opendir(".");
- if (!pipedir) {
- printerr(0, "ERROR: could not open rpc_pipefs directory: '%s'\n",
- strerror(errno));
- exit(EXIT_FAILURE);
- }
-
- while ((dent = readdir(pipedir))) {
- if (dent->d_type != DT_DIR)
+ while ((d = readdir(pipefs_dir))) {
+ if (d->d_type != DT_DIR)
continue;
- if (dent->d_name[0] == '.')
+ if (d->d_name[0] == '.')
continue;
- if (topdirs_add_entry(dirfd(pipedir), dent->d_name))
- exit(EXIT_FAILURE);
+ gssd_add_topdir(dirfd(pipefs_dir), d->d_name);
}
if (TAILQ_EMPTY(&topdir_list)) {
printerr(0, "ERROR: the rpc_pipefs directory is empty!\n");
exit(EXIT_FAILURE);
}
+}
- closedir(pipedir);
+static void
+gssd_update_clients_cb(int UNUSED(ifd), short UNUSED(which), void *UNUSED(data))
+{
+ gssd_update_clients();
}
+
static void
gssd_atexit(void)
{
@@ -711,7 +693,7 @@ main(int argc, char *argv[])
rpc_verbosity++;
break;
case 'p':
- pipefs_dir = optarg;
+ pipefs_path = optarg;
break;
case 'k':
keytabfile = optarg;
@@ -814,8 +796,14 @@ main(int argc, char *argv[])
event_init();
- if (chdir(pipefs_dir)) {
- printerr(1, "ERROR: chdir(%s) failed: %s\n", pipefs_dir, strerror(errno));
+ pipefs_dir = opendir(pipefs_path);
+ if (!pipefs_dir) {
+ printerr(1, "ERROR: opendir(%s) failed: %s\n", pipefs_path, strerror(errno));
+ exit(EXIT_FAILURE);
+ }
+
+ if (fchdir(dirfd(pipefs_dir))) {
+ printerr(1, "ERROR: fchdir(%s) failed: %s\n", pipefs_path, strerror(errno));
exit(EXIT_FAILURE);
}
@@ -829,7 +817,7 @@ main(int argc, char *argv[])
signal_set(&sigdnotify_ev, DNOTIFY_SIGNAL, gssd_update_clients_cb, NULL);
signal_add(&sigdnotify_ev, NULL);
- topdirs_init_list();
+ TAILQ_INIT(&topdir_list);
gssd_update_clients();
daemon_ready();
Using more relative paths saves memory and lets us get rid of more
PATH_MAX fixed arrays.
Signed-off-by: David Härdeman <[email protected]>
---
utils/gssd/gssd.c | 74 +++++++++++++++++++++++-------------------------
utils/gssd/gssd.h | 3 +-
utils/gssd/gssd_proc.c | 4 +--
3 files changed, 40 insertions(+), 41 deletions(-)
diff --git a/utils/gssd/gssd.c b/utils/gssd/gssd.c
index d03df2c..fc0cbdc 100644
--- a/utils/gssd/gssd.c
+++ b/utils/gssd/gssd.c
@@ -74,6 +74,7 @@
static char *pipefs_path = GSSD_PIPEFS_DIR;
static DIR *pipefs_dir;
+static int pipefs_fd;
char *keytabfile = GSSD_DEFAULT_KEYTAB_FILE;
char **ccachesearch;
@@ -336,7 +337,7 @@ destroy_client(struct clnt_info *clp)
if (clp->dir_fd >= 0)
close(clp->dir_fd);
- free(clp->dirname);
+ free(clp->relpath);
free(clp->servicename);
free(clp->servername);
free(clp->protocol);
@@ -371,7 +372,7 @@ gssd_clnt_gssd_cb(int UNUSED(fd), short which, void *data)
struct clnt_info *clp = data;
if (which != EV_READ) {
- printerr(2, "Closing 'gssd' pipe for %s\n", clp->dirname);
+ printerr(2, "Closing 'gssd' pipe %s\n", clp->relpath);
close(clp->gssd_fd);
clp->gssd_fd = -1;
event_del(&clp->gssd_ev);
@@ -388,7 +389,7 @@ gssd_clnt_krb5_cb(int UNUSED(fd), short which, void *data)
struct clnt_info *clp = data;
if (which != EV_READ) {
- printerr(2, "Closing 'krb5' pipe for %s\n", clp->dirname);
+ printerr(2, "Closing 'krb5' pipe %s\n", clp->relpath);
close(clp->krb5_fd);
clp->krb5_fd = -1;
event_del(&clp->krb5_ev);
@@ -402,34 +403,32 @@ gssd_clnt_krb5_cb(int UNUSED(fd), short which, void *data)
static int
process_clnt_dir_files(struct clnt_info * clp)
{
- char name[PATH_MAX];
- char gname[PATH_MAX];
+ char name[strlen(clp->relpath) + strlen("/krb5") + 1];
+ char gname[strlen(clp->relpath) + strlen("/gssd") + 1];
bool gssd_was_closed;
bool krb5_was_closed;
gssd_was_closed = clp->gssd_fd < 0 ? true : false;
krb5_was_closed = clp->krb5_fd < 0 ? true : false;
- if (clp->gssd_fd == -1) {
- snprintf(gname, sizeof(gname), "%s/gssd", clp->dirname);
- clp->gssd_fd = open(gname, O_RDWR);
- }
+ sprintf(gname, "%s/gssd", clp->relpath);
+ sprintf(name, "%s/krb5", clp->relpath);
+
+ if (clp->gssd_fd == -1)
+ clp->gssd_fd = openat(pipefs_fd, gname, O_RDWR);
if (clp->gssd_fd == -1) {
- if (clp->krb5_fd == -1) {
- snprintf(name, sizeof(name), "%s/krb5", clp->dirname);
- clp->krb5_fd = open(name, O_RDWR);
- }
+ if (clp->krb5_fd == -1)
+ clp->krb5_fd = openat(pipefs_fd, name, O_RDWR);
/* If we opened a gss-specific pipe, let's try opening
* the new upcall pipe again. If we succeed, close
* gss-specific pipe(s).
*/
if (clp->krb5_fd != -1) {
- clp->gssd_fd = open(gname, O_RDWR);
+ clp->gssd_fd = openat(pipefs_fd, gname, O_RDWR);
if (clp->gssd_fd != -1) {
- if (clp->krb5_fd != -1)
- close(clp->krb5_fd);
+ close(clp->krb5_fd);
clp->krb5_fd = -1;
}
}
@@ -452,9 +451,9 @@ process_clnt_dir_files(struct clnt_info * clp)
return 0;
if (clp->prog == 0) {
- char info_file_name[strlen(clp->dirname) + 6];
+ char info_file_name[strlen(clp->relpath) + strlen("/info") + 1];
- sprintf(info_file_name, "%s/info", clp->dirname);
+ sprintf(info_file_name, "%s/info", clp->relpath);
read_service_info(info_file_name, &clp->servicename,
&clp->servername, &clp->prog, &clp->vers,
&clp->protocol, (struct sockaddr *) &clp->addr);
@@ -464,24 +463,28 @@ process_clnt_dir_files(struct clnt_info * clp)
}
static void
-process_clnt_dir(struct topdir *tdi, char *dir)
+process_clnt_dir(struct topdir *tdi, const char *name)
{
struct clnt_info *clp;
- if (!(clp = insert_new_clnt(tdi)))
+ clp = insert_new_clnt(tdi);
+ if (!clp)
goto out;
- /* An extra for the '/', and an extra for the null */
- if (!(clp->dirname = calloc(strlen(dir) + strlen(tdi->dirname) + 2, 1)))
+ clp->relpath = malloc(strlen(tdi->name) + strlen("/") + strlen(name) + 1);
+ if (!clp->relpath)
goto out;
- sprintf(clp->dirname, "%s/%s", tdi->dirname, dir);
- if ((clp->dir_fd = open(clp->dirname, O_RDONLY)) == -1) {
+ sprintf(clp->relpath, "%s/%s", tdi->name, name);
+ clp->name = clp->relpath + strlen(tdi->name) + 1;
+
+ if ((clp->dir_fd = open(clp->relpath, O_RDONLY)) == -1) {
if (errno != ENOENT)
printerr(0, "ERROR: can't open %s: %s\n",
- clp->dirname, strerror(errno));
+ clp->relpath, strerror(errno));
goto out;
}
+
fcntl(clp->dir_fd, F_SETSIG, DNOTIFY_SIGNAL);
fcntl(clp->dir_fd, F_NOTIFY, DN_CREATE | DN_DELETE | DN_MULTISHOT);
@@ -509,22 +512,19 @@ update_old_clients(struct topdir *tdi, struct dirent **namelist, int size)
void *saveprev;
int i;
bool stillhere;
- char fname[PATH_MAX];
TAILQ_FOREACH(clp, &tdi->clnt_list, list) {
stillhere = false;
for (i = 0; i < size; i++) {
- snprintf(fname, sizeof(fname), "%s/%s",
- tdi->dirname, namelist[i]->d_name);
- if (strcmp(clp->dirname, fname) == 0) {
+ if (!strcmp(clp->name, namelist[i]->d_name)) {
stillhere = true;
break;
}
}
if (!stillhere) {
- printerr(2, "destroying client %s\n", clp->dirname);
+ printerr(2, "destroying client %s\n", clp->relpath);
saveprev = clp->list.tqe_prev;
TAILQ_REMOVE(&tdi->clnt_list, clp, list);
destroy_client(clp);
@@ -538,16 +538,13 @@ update_old_clients(struct topdir *tdi, struct dirent **namelist, int size)
/* Search for a client by directory name, return 1 if found, 0 otherwise */
static bool
-find_client(struct topdir *tdi, const char *dirname)
+find_client(struct topdir *tdi, const char *name)
{
struct clnt_info *clp;
- char fname[PATH_MAX];
- TAILQ_FOREACH(clp, &tdi->clnt_list, list) {
- snprintf(fname, sizeof(fname), "%s/%s", tdi->dirname, dirname);
- if (!strcmp(clp->dirname, fname))
+ TAILQ_FOREACH(clp, &tdi->clnt_list, list)
+ if (!strcmp(clp->name, name))
return true;
- }
return false;
}
@@ -627,7 +624,7 @@ gssd_update_clients(void)
if (d->d_name[0] == '.')
continue;
- gssd_add_topdir(dirfd(pipefs_dir), d->d_name);
+ gssd_add_topdir(pipefs_fd, d->d_name);
}
if (TAILQ_EMPTY(&topdir_list)) {
@@ -802,7 +799,8 @@ main(int argc, char *argv[])
exit(EXIT_FAILURE);
}
- if (fchdir(dirfd(pipefs_dir))) {
+ pipefs_fd = dirfd(pipefs_dir);
+ if (fchdir(pipefs_fd)) {
printerr(1, "ERROR: fchdir(%s) failed: %s\n", pipefs_path, strerror(errno));
exit(EXIT_FAILURE);
}
diff --git a/utils/gssd/gssd.h b/utils/gssd/gssd.h
index 8a882d3..e602d18 100644
--- a/utils/gssd/gssd.h
+++ b/utils/gssd/gssd.h
@@ -64,8 +64,9 @@ extern char *preferred_realm;
struct clnt_info {
TAILQ_ENTRY(clnt_info) list;
- char *dirname;
int dir_fd;
+ char *name;
+ char *relpath;
char *servicename;
char *servername;
int prog;
diff --git a/utils/gssd/gssd_proc.c b/utils/gssd/gssd_proc.c
index 8957b27..c16f111 100644
--- a/utils/gssd/gssd_proc.c
+++ b/utils/gssd/gssd_proc.c
@@ -527,7 +527,7 @@ process_krb5_upcall(struct clnt_info *clp, uid_t uid, int fd, char *tgtname,
return;
}
- printerr(1, "handling krb5 upcall (%s)\n", clp->dirname);
+ printerr(1, "handling krb5 upcall (%s)\n", clp->relpath);
token.length = 0;
token.value = NULL;
@@ -716,7 +716,7 @@ handle_gssd_upcall(struct clnt_info *clp)
char *service = NULL;
char *enctypes = NULL;
- printerr(1, "handling gssd upcall (%s)\n", clp->dirname);
+ printerr(1, "handling gssd upcall (%s)\n", clp->relpath);
lbuflen = read(clp->gssd_fd, lbuf, sizeof(lbuf));
if (lbuflen <= 0 || lbuf[lbuflen-1] != '\n') {
Simplify and refactor the code that does the topdir scanning, this
is in preparation for the inotify patches.
Signed-off-by: David Härdeman <[email protected]>
---
utils/gssd/gssd.c | 172 +++++++++++++++++++++++++----------------------------
utils/gssd/gssd.h | 1
2 files changed, 81 insertions(+), 92 deletions(-)
diff --git a/utils/gssd/gssd.c b/utils/gssd/gssd.c
index fc0cbdc..9e48840 100644
--- a/utils/gssd/gssd.c
+++ b/utils/gssd/gssd.c
@@ -364,7 +364,7 @@ insert_new_clnt(struct topdir *tdi)
return clp;
}
-static void gssd_update_clients(void);
+static void gssd_scan(void);
static void
gssd_clnt_gssd_cb(int UNUSED(fd), short which, void *data)
@@ -376,7 +376,7 @@ gssd_clnt_gssd_cb(int UNUSED(fd), short which, void *data)
close(clp->gssd_fd);
clp->gssd_fd = -1;
event_del(&clp->gssd_ev);
- gssd_update_clients();
+ gssd_scan();
return;
}
@@ -393,7 +393,7 @@ gssd_clnt_krb5_cb(int UNUSED(fd), short which, void *data)
close(clp->krb5_fd);
clp->krb5_fd = -1;
event_del(&clp->krb5_ev);
- gssd_update_clients();
+ gssd_scan();
return;
}
@@ -459,6 +459,7 @@ process_clnt_dir_files(struct clnt_info * clp)
&clp->protocol, (struct sockaddr *) &clp->addr);
}
+ clp->scanned = true;
return 0;
}
@@ -500,118 +501,105 @@ out:
}
}
-/*
- * This is run after a DNOTIFY signal, and should clear up any
- * directories that are no longer around, and re-scan any existing
- * directories, since the DNOTIFY could have been in there.
- */
-static void
-update_old_clients(struct topdir *tdi, struct dirent **namelist, int size)
+static struct topdir *
+gssd_get_topdir(const char *name)
{
- struct clnt_info *clp;
- void *saveprev;
- int i;
- bool stillhere;
-
- TAILQ_FOREACH(clp, &tdi->clnt_list, list) {
- stillhere = false;
+ struct topdir *tdi;
- for (i = 0; i < size; i++) {
- if (!strcmp(clp->name, namelist[i]->d_name)) {
- stillhere = true;
- break;
- }
- }
+ TAILQ_FOREACH(tdi, &topdir_list, list)
+ if (!strcmp(tdi->name, name))
+ return tdi;
- if (!stillhere) {
- printerr(2, "destroying client %s\n", clp->relpath);
- saveprev = clp->list.tqe_prev;
- TAILQ_REMOVE(&tdi->clnt_list, clp, list);
- destroy_client(clp);
- clp = saveprev;
- }
+ tdi = malloc(sizeof(*tdi) + strlen(pipefs_path) + strlen(name) + 2);
+ if (!tdi) {
+ printerr(0, "ERROR: Couldn't allocate struct topdir\n");
+ return NULL;
}
- TAILQ_FOREACH(clp, &tdi->clnt_list, list)
- process_clnt_dir_files(clp);
-}
+ sprintf(tdi->dirname, "%s/%s", pipefs_path, name);
+ tdi->name = tdi->dirname + strlen(pipefs_path) + 1;
+ TAILQ_INIT(&tdi->clnt_list);
-/* Search for a client by directory name, return 1 if found, 0 otherwise */
-static bool
-find_client(struct topdir *tdi, const char *name)
-{
- struct clnt_info *clp;
+ tdi->fd = openat(pipefs_fd, name, O_RDONLY);
+ if (tdi->fd < 0) {
+ printerr(0, "ERROR: failed to open %s: %s\n",
+ tdi->dirname, strerror(errno));
+ free(tdi);
+ return NULL;
+ }
- TAILQ_FOREACH(clp, &tdi->clnt_list, list)
- if (!strcmp(clp->name, name))
- return true;
+ fcntl(tdi->fd, F_SETSIG, DNOTIFY_SIGNAL);
+ fcntl(tdi->fd, F_NOTIFY, DN_CREATE|DN_DELETE|DN_MODIFY|DN_MULTISHOT);
- return false;
+ TAILQ_INSERT_HEAD(&topdir_list, tdi, list);
+ return tdi;
}
-static int
-gssd_process_topdir(struct topdir *tdi)
+static void
+gssd_scan_topdir(const char *name)
{
- struct dirent **namelist;
- int i, j;
+ struct topdir *tdi;
+ int dfd;
+ DIR *dir;
+ struct clnt_info *clp;
+ struct dirent *d;
+
+ tdi = gssd_get_topdir(name);
+ if (!tdi)
+ return;
- j = scandir(tdi->dirname, &namelist, NULL, alphasort);
- if (j < 0) {
- printerr(0, "ERROR: can't scandir %s: %s\n",
+ dfd = openat(pipefs_fd, tdi->name, O_RDONLY);
+ if (dfd < 0) {
+ printerr(0, "ERROR: can't openat %s: %s\n",
tdi->dirname, strerror(errno));
- return -1;
+ return;
}
- update_old_clients(tdi, namelist, j);
-
- for (i = 0; i < j; i++) {
- if (!strncmp(namelist[i]->d_name, "clnt", 4)
- && !find_client(tdi, namelist[i]->d_name))
- process_clnt_dir(tdi, namelist[i]->d_name);
- free(namelist[i]);
+ dir = fdopendir(dfd);
+ if (!dir) {
+ printerr(0, "ERROR: can't fdopendir %s: %s\n",
+ tdi->dirname, strerror(errno));
+ return;
}
- free(namelist);
+ TAILQ_FOREACH(clp, &tdi->clnt_list, list)
+ clp->scanned = false;
- return 0;
-}
+ while ((d = readdir(dir))) {
+ if (d->d_type != DT_DIR)
+ continue;
-static int
-gssd_add_topdir(int pfd, const char *name)
-{
- struct topdir *tdi;
+ if (strncmp(d->d_name, "clnt", strlen("clnt")))
+ continue;
- TAILQ_FOREACH(tdi, &topdir_list, list)
- if (!strcmp(tdi->name, name))
- return gssd_process_topdir(tdi);
+ TAILQ_FOREACH(clp, &tdi->clnt_list, list)
+ if (!strcmp(clp->name, d->d_name))
+ break;
- tdi = malloc(sizeof(*tdi) + strlen(pipefs_path) + strlen(name) + 2);
- if (!tdi) {
- printerr(0, "ERROR: Couldn't allocate struct topdir\n");
- return -1;
+ if (clp)
+ process_clnt_dir_files(clp);
+ else
+ process_clnt_dir(tdi, d->d_name);
}
- sprintf(tdi->dirname, "%s/%s", pipefs_path, name);
- tdi->name = tdi->dirname + strlen(pipefs_path) + 1;
- TAILQ_INIT(&tdi->clnt_list);
+ closedir(dir);
- tdi->fd = openat(pfd, name, O_RDONLY);
- if (tdi->fd < 0) {
- printerr(0, "ERROR: failed to open %s: %s\n",
- tdi->dirname, strerror(errno));
- free(tdi);
- return -1;
- }
+ TAILQ_FOREACH(clp, &tdi->clnt_list, list) {
+ void *saveprev;
- fcntl(tdi->fd, F_SETSIG, DNOTIFY_SIGNAL);
- fcntl(tdi->fd, F_NOTIFY, DN_CREATE|DN_DELETE|DN_MODIFY|DN_MULTISHOT);
+ if (clp->scanned)
+ continue;
- TAILQ_INSERT_HEAD(&topdir_list, tdi, list);
- return gssd_process_topdir(tdi);
+ printerr(2, "destroying client %s\n", clp->relpath);
+ saveprev = clp->list.tqe_prev;
+ TAILQ_REMOVE(&tdi->clnt_list, clp, list);
+ destroy_client(clp);
+ clp = saveprev;
+ }
}
static void
-gssd_update_clients(void)
+gssd_scan(void)
{
struct dirent *d;
@@ -624,7 +612,7 @@ gssd_update_clients(void)
if (d->d_name[0] == '.')
continue;
- gssd_add_topdir(pipefs_fd, d->d_name);
+ gssd_scan_topdir(d->d_name);
}
if (TAILQ_EMPTY(&topdir_list)) {
@@ -634,9 +622,9 @@ gssd_update_clients(void)
}
static void
-gssd_update_clients_cb(int UNUSED(ifd), short UNUSED(which), void *UNUSED(data))
+gssd_scan_cb(int UNUSED(ifd), short UNUSED(which), void *UNUSED(data))
{
- gssd_update_clients();
+ gssd_scan();
}
@@ -810,13 +798,13 @@ main(int argc, char *argv[])
exit(EXIT_FAILURE);
}
- signal_set(&sighup_ev, SIGHUP, gssd_update_clients_cb, NULL);
+ signal_set(&sighup_ev, SIGHUP, gssd_scan_cb, NULL);
signal_add(&sighup_ev, NULL);
- signal_set(&sigdnotify_ev, DNOTIFY_SIGNAL, gssd_update_clients_cb, NULL);
+ signal_set(&sigdnotify_ev, DNOTIFY_SIGNAL, gssd_scan_cb, NULL);
signal_add(&sigdnotify_ev, NULL);
TAILQ_INIT(&topdir_list);
- gssd_update_clients();
+ gssd_scan();
daemon_ready();
event_dispatch();
diff --git a/utils/gssd/gssd.h b/utils/gssd/gssd.h
index e602d18..7792035 100644
--- a/utils/gssd/gssd.h
+++ b/utils/gssd/gssd.h
@@ -65,6 +65,7 @@ extern char *preferred_realm;
struct clnt_info {
TAILQ_ENTRY(clnt_info) list;
int dir_fd;
+ bool scanned;
char *name;
char *relpath;
char *servicename;
Simplify the code responsible for the client dir scanning. This
is also in preparation for the inotify patches.
Signed-off-by: David Härdeman <[email protected]>
---
utils/gssd/gssd.c | 171 +++++++++++++++++++++++++----------------------------
1 file changed, 80 insertions(+), 91 deletions(-)
diff --git a/utils/gssd/gssd.c b/utils/gssd/gssd.c
index 9e48840..91e3e05 100644
--- a/utils/gssd/gssd.c
+++ b/utils/gssd/gssd.c
@@ -240,7 +240,7 @@ get_servername(const char *name, const struct sockaddr *sa, const char *addr)
/* XXX buffer problems: */
static int
-read_service_info(char *info_file_name, char **servicename, char **servername,
+read_service_info(int fd, char **servicename, char **servername,
int *prog, int *vers, char **protocol,
struct sockaddr *addr) {
#define INFOBUFLEN 256
@@ -254,20 +254,13 @@ read_service_info(char *info_file_name, char **servicename, char **servername,
char protoname[16];
char port[128];
char *p;
- int fd = -1;
int numfields;
*servicename = *servername = *protocol = NULL;
- if ((fd = open(info_file_name, O_RDONLY)) == -1) {
- printerr(0, "ERROR: can't open %s: %s\n", info_file_name,
- strerror(errno));
- goto fail;
- }
if ((nbytes = read(fd, buf, INFOBUFLEN)) == -1)
goto fail;
- close(fd);
- fd = -1;
+
buf[nbytes] = '\0';
numfields = sscanf(buf,"RPC server: %127s\n"
@@ -313,7 +306,6 @@ read_service_info(char *info_file_name, char **servicename, char **servername,
return 0;
fail:
printerr(0, "ERROR: failed to read service info\n");
- if (fd != -1) close(fd);
free(*servername);
free(*servicename);
free(*protocol);
@@ -322,7 +314,7 @@ fail:
}
static void
-destroy_client(struct clnt_info *clp)
+gssd_destroy_client(struct clnt_info *clp)
{
if (clp->krb5_fd >= 0) {
close(clp->krb5_fd);
@@ -344,26 +336,6 @@ destroy_client(struct clnt_info *clp)
free(clp);
}
-static struct clnt_info *
-insert_new_clnt(struct topdir *tdi)
-{
- struct clnt_info *clp;
-
- clp = calloc(1, sizeof(struct clnt_info));
- if (!clp) {
- printerr(0, "ERROR: can't malloc clnt_info: %s\n",
- strerror(errno));
- return NULL;
- }
-
- clp->krb5_fd = -1;
- clp->gssd_fd = -1;
- clp->dir_fd = -1;
-
- TAILQ_INSERT_HEAD(&tdi->clnt_list, clp, list);
- return clp;
-}
-
static void gssd_scan(void);
static void
@@ -400,33 +372,86 @@ gssd_clnt_krb5_cb(int UNUSED(fd), short which, void *data)
handle_krb5_upcall(clp);
}
+static struct clnt_info *
+gssd_get_clnt(struct topdir *tdi, const char *name)
+{
+ struct clnt_info *clp;
+
+ TAILQ_FOREACH(clp, &tdi->clnt_list, list)
+ if (!strcmp(clp->name, name))
+ return clp;
+
+ clp = calloc(1, sizeof(struct clnt_info));
+ if (!clp) {
+ printerr(0, "ERROR: can't malloc clnt_info: %s\n",
+ strerror(errno));
+ return NULL;
+ }
+
+ if (asprintf(&clp->relpath, "%s/%s", tdi->name, name) < 0) {
+ clp->relpath = NULL;
+ goto out;
+ }
+
+ clp->name = clp->relpath + strlen(tdi->name) + 1;
+ clp->krb5_fd = -1;
+ clp->gssd_fd = -1;
+ clp->dir_fd = -1;
+
+ if ((clp->dir_fd = open(clp->relpath, O_RDONLY)) == -1) {
+ if (errno != ENOENT)
+ printerr(0, "ERROR: can't open %s: %s\n",
+ clp->relpath, strerror(errno));
+ goto out;
+ }
+
+ fcntl(clp->dir_fd, F_SETSIG, DNOTIFY_SIGNAL);
+ fcntl(clp->dir_fd, F_NOTIFY, DN_CREATE | DN_DELETE | DN_MULTISHOT);
+
+ TAILQ_INSERT_HEAD(&tdi->clnt_list, clp, list);
+ return clp;
+
+out:
+ free(clp->relpath);
+ free(clp);
+ return NULL;
+}
+
static int
-process_clnt_dir_files(struct clnt_info * clp)
+gssd_scan_clnt(struct topdir *tdi, const char *name)
{
- char name[strlen(clp->relpath) + strlen("/krb5") + 1];
- char gname[strlen(clp->relpath) + strlen("/gssd") + 1];
+ struct clnt_info *clp;
+ int clntfd;
bool gssd_was_closed;
bool krb5_was_closed;
+ clp = gssd_get_clnt(tdi, name);
+ if (!clp)
+ return -1;
+
gssd_was_closed = clp->gssd_fd < 0 ? true : false;
krb5_was_closed = clp->krb5_fd < 0 ? true : false;
- sprintf(gname, "%s/gssd", clp->relpath);
- sprintf(name, "%s/krb5", clp->relpath);
+ clntfd = openat(pipefs_fd, clp->relpath, O_RDONLY);
+ if (clntfd < 0) {
+ printerr(0, "ERROR: can't openat %s: %s\n",
+ clp->relpath, strerror(errno));
+ return -1;
+ }
if (clp->gssd_fd == -1)
- clp->gssd_fd = openat(pipefs_fd, gname, O_RDWR);
+ clp->gssd_fd = openat(clntfd, "gssd", O_RDWR);
if (clp->gssd_fd == -1) {
if (clp->krb5_fd == -1)
- clp->krb5_fd = openat(pipefs_fd, name, O_RDWR);
+ clp->krb5_fd = openat(clntfd, "krb5", O_RDWR);
/* If we opened a gss-specific pipe, let's try opening
* the new upcall pipe again. If we succeed, close
* gss-specific pipe(s).
*/
if (clp->krb5_fd != -1) {
- clp->gssd_fd = openat(pipefs_fd, gname, O_RDWR);
+ clp->gssd_fd = openat(clntfd, "gssd", O_RDWR);
if (clp->gssd_fd != -1) {
close(clp->krb5_fd);
clp->krb5_fd = -1;
@@ -448,58 +473,29 @@ process_clnt_dir_files(struct clnt_info * clp)
if ((clp->krb5_fd == -1) && (clp->gssd_fd == -1))
/* not fatal, files might appear later */
- return 0;
+ goto out;
if (clp->prog == 0) {
- char info_file_name[strlen(clp->relpath) + strlen("/info") + 1];
+ int infofd;
- sprintf(info_file_name, "%s/info", clp->relpath);
- read_service_info(info_file_name, &clp->servicename,
- &clp->servername, &clp->prog, &clp->vers,
- &clp->protocol, (struct sockaddr *) &clp->addr);
+ infofd = openat(clntfd, "info", O_RDONLY);
+ if (infofd < 0) {
+ printerr(0, "ERROR: can't open %s/info: %s\n",
+ clp->relpath, strerror(errno));
+ } else {
+ read_service_info(infofd, &clp->servicename,
+ &clp->servername, &clp->prog, &clp->vers,
+ &clp->protocol, (struct sockaddr *) &clp->addr);
+ close(infofd);
+ }
}
+out:
+ close(clntfd);
clp->scanned = true;
return 0;
}
-static void
-process_clnt_dir(struct topdir *tdi, const char *name)
-{
- struct clnt_info *clp;
-
- clp = insert_new_clnt(tdi);
- if (!clp)
- goto out;
-
- clp->relpath = malloc(strlen(tdi->name) + strlen("/") + strlen(name) + 1);
- if (!clp->relpath)
- goto out;
-
- sprintf(clp->relpath, "%s/%s", tdi->name, name);
- clp->name = clp->relpath + strlen(tdi->name) + 1;
-
- if ((clp->dir_fd = open(clp->relpath, O_RDONLY)) == -1) {
- if (errno != ENOENT)
- printerr(0, "ERROR: can't open %s: %s\n",
- clp->relpath, strerror(errno));
- goto out;
- }
-
- fcntl(clp->dir_fd, F_SETSIG, DNOTIFY_SIGNAL);
- fcntl(clp->dir_fd, F_NOTIFY, DN_CREATE | DN_DELETE | DN_MULTISHOT);
-
- if (process_clnt_dir_files(clp))
- goto out;
-
- return;
-
-out:
- if (clp) {
- TAILQ_REMOVE(&tdi->clnt_list, clp, list);
- destroy_client(clp);
- }
-}
static struct topdir *
gssd_get_topdir(const char *name)
@@ -572,14 +568,7 @@ gssd_scan_topdir(const char *name)
if (strncmp(d->d_name, "clnt", strlen("clnt")))
continue;
- TAILQ_FOREACH(clp, &tdi->clnt_list, list)
- if (!strcmp(clp->name, d->d_name))
- break;
-
- if (clp)
- process_clnt_dir_files(clp);
- else
- process_clnt_dir(tdi, d->d_name);
+ gssd_scan_clnt(tdi, d->d_name);
}
closedir(dir);
@@ -593,7 +582,7 @@ gssd_scan_topdir(const char *name)
printerr(2, "destroying client %s\n", clp->relpath);
saveprev = clp->list.tqe_prev;
TAILQ_REMOVE(&tdi->clnt_list, clp, list);
- destroy_client(clp);
+ gssd_destroy_client(clp);
clp = saveprev;
}
}
There's a lot of fixed buffers in use here. Clean up the code and
add more documentation on the different formats that have been
used by the kernel.
Signed-off-by: David Härdeman <[email protected]>
---
utils/gssd/gssd.c | 208 ++++++++++++++++++++++++++++-------------------------
1 file changed, 110 insertions(+), 98 deletions(-)
diff --git a/utils/gssd/gssd.c b/utils/gssd/gssd.c
index 91e3e05..78b836d 100644
--- a/utils/gssd/gssd.c
+++ b/utils/gssd/gssd.c
@@ -83,6 +83,9 @@ int root_uses_machine_creds = 1;
unsigned int context_timeout = 0;
unsigned int rpc_timeout = 5;
char *preferred_realm = NULL;
+/* Avoid DNS reverse lookups on server names */
+static bool avoid_dns = true;
+
TAILQ_HEAD(topdir_list_head, topdir) topdir_list;
@@ -120,9 +123,6 @@ struct topdir {
* and rescan the whole {rpc_pipefs} when this happens.
*/
-/* Avoid DNS reverse lookups on server names */
-static int avoid_dns = 1;
-
/*
* convert a presentation address string to a sockaddr_storage struct. Returns
* true on success or false on failure.
@@ -136,8 +136,8 @@ static int avoid_dns = 1;
* Microsoft "standard" of using the ipv6-literal.net domainname, but it's
* not really feasible at present.
*/
-static int
-addrstr_to_sockaddr(struct sockaddr *sa, const char *node, const char *port)
+static bool
+gssd_addrstr_to_sockaddr(struct sockaddr *sa, const char *node, const char *port)
{
int rc;
struct addrinfo *res;
@@ -150,9 +150,9 @@ addrstr_to_sockaddr(struct sockaddr *sa, const char *node, const char *port)
rc = getaddrinfo(node, port, &hints, &res);
if (rc) {
printerr(0, "ERROR: unable to convert %s|%s to sockaddr: %s\n",
- node, port, rc == EAI_SYSTEM ? strerror(errno) :
- gai_strerror(rc));
- return 0;
+ node, port,
+ rc == EAI_SYSTEM ? strerror(errno) : gai_strerror(rc));
+ return false;
}
#ifdef IPV6_SUPPORTED
@@ -167,46 +167,41 @@ addrstr_to_sockaddr(struct sockaddr *sa, const char *node, const char *port)
printerr(0, "ERROR: address %s has non-zero "
"sin6_scope_id!\n", node);
freeaddrinfo(res);
- return 0;
+ return false;
}
}
#endif /* IPV6_SUPPORTED */
memcpy(sa, res->ai_addr, res->ai_addrlen);
freeaddrinfo(res);
- return 1;
+ return true;
}
/*
* convert a sockaddr to a hostname
*/
static char *
-get_servername(const char *name, const struct sockaddr *sa, const char *addr)
+gssd_get_servername(const char *name, const struct sockaddr *sa, const char *addr)
{
socklen_t addrlen;
int err;
- char *hostname;
char hbuf[NI_MAXHOST];
unsigned char buf[sizeof(struct in6_addr)];
- if (avoid_dns) {
+ while (avoid_dns) {
/*
* Determine if this is a server name, or an IP address.
* If it is an IP address, do the DNS lookup otherwise
* skip the DNS lookup.
*/
- int is_fqdn = 1;
if (strchr(name, '.') == NULL)
- is_fqdn = 0; /* local name */
+ break; /* local name */
else if (inet_pton(AF_INET, name, buf) == 1)
- is_fqdn = 0; /* IPv4 address */
+ break; /* IPv4 address */
else if (inet_pton(AF_INET6, name, buf) == 1)
- is_fqdn = 0; /* IPv6 addrss */
+ break; /* IPv6 addrss */
- if (is_fqdn) {
- return strdup(name);
- }
- /* Sorry, cannot avoid dns after all */
+ return strdup(name);
}
switch (sa->sa_family) {
@@ -233,84 +228,113 @@ get_servername(const char *name, const struct sockaddr *sa, const char *addr)
return NULL;
}
- hostname = strdup(hbuf);
-
- return hostname;
+ return strdup(hbuf);
}
-/* XXX buffer problems: */
-static int
-read_service_info(int fd, char **servicename, char **servername,
- int *prog, int *vers, char **protocol,
- struct sockaddr *addr) {
-#define INFOBUFLEN 256
- char buf[INFOBUFLEN + 1];
- static char server[128];
- int nbytes;
- static char service[128];
- static char address[128];
- char program[16];
- char version[16];
- char protoname[16];
- char port[128];
- char *p;
- int numfields;
-
- *servicename = *servername = *protocol = NULL;
-
- if ((nbytes = read(fd, buf, INFOBUFLEN)) == -1)
+static void
+gssd_read_service_info(int dirfd, struct clnt_info *clp)
+{
+ int fd;
+ FILE *info = NULL;
+ int numfields;
+ char *server = NULL;
+ char *service = NULL;
+ int program;
+ int version;
+ char *address = NULL;
+ char *protoname = NULL;
+ char *port = NULL;
+ char *servername = NULL;
+
+ fd = openat(dirfd, "info", O_RDONLY);
+ if (fd < 0) {
+ printerr(0, "ERROR: can't open %s/info: %s\n",
+ clp->relpath, strerror(errno));
goto fail;
+ }
- buf[nbytes] = '\0';
-
- numfields = sscanf(buf,"RPC server: %127s\n"
- "service: %127s %15s version %15s\n"
- "address: %127s\n"
- "protocol: %15s\n",
- server,
- service, program, version,
- address,
- protoname);
-
- if (numfields == 5) {
- strcpy(protoname, "tcp");
- } else if (numfields != 6) {
+ info = fdopen(fd, "r");
+ if (!info) {
+ printerr(0, "ERROR: can't fdopen %s/info: %s\n",
+ clp->relpath, strerror(errno));
+ close(fd);
goto fail;
}
- port[0] = '\0';
- if ((p = strstr(buf, "port")) != NULL)
- sscanf(p, "port: %127s\n", port);
-
- /* get program, and version numbers */
- *prog = atoi(program + 1); /* skip open paren */
- *vers = atoi(version);
-
- if (!addrstr_to_sockaddr(addr, address, port))
+ /*
+ * Some history:
+ *
+ * The first three lines were added with rpc_pipefs in 2003-01-13.
+ * (commit af2f003391786fb632889c02142c941b212ba4ff)
+ *
+ * The 'protocol' line was added in 2003-06-11.
+ * (commit 9bd741ae48785d0c0e75cf906ff66f893d600c2d)
+ *
+ * The 'port' line was added in 2007-09-26.
+ * (commit bf19aacecbeebccb2c3d150a8bd9416b7dba81fe)
+ */
+ numfields = fscanf(info,
+ "RPC server: %ms\n"
+ "service: %ms (%d) version %d\n"
+ "address: %ms\n"
+ "protocol: %ms\n"
+ "port: %ms\n",
+ &server,
+ &service, &program, &version,
+ &address,
+ &protoname,
+ &port);
+
+
+ switch (numfields) {
+ case 5:
+ protoname = strdup("tcp");
+ if (!protoname)
+ goto fail;
+ /* fall through */
+ case 6:
+ /* fall through */
+ case 7:
+ break;
+ default:
goto fail;
+ }
- *servername = get_servername(server, addr, address);
- if (*servername == NULL)
+ if (!gssd_addrstr_to_sockaddr((struct sockaddr *)&clp->addr,
+ address, port ? port : ""))
goto fail;
- nbytes = snprintf(buf, INFOBUFLEN, "%s@%s", service, *servername);
- if (nbytes > INFOBUFLEN)
+ servername = gssd_get_servername(server, (struct sockaddr *)&clp->addr, address);
+ if (!servername)
goto fail;
- if (!(*servicename = calloc(strlen(buf) + 1, 1)))
+ if (asprintf(&clp->servicename, "%s@%s", service, servername) < 0)
goto fail;
- memcpy(*servicename, buf, strlen(buf));
- if (!(*protocol = strdup(protoname)))
- goto fail;
- return 0;
+ clp->servername = servername;
+ clp->prog = program;
+ clp->vers = version;
+ clp->protocol = protoname;
+
+ goto out;
+
fail:
- printerr(0, "ERROR: failed to read service info\n");
- free(*servername);
- free(*servicename);
- free(*protocol);
- *servicename = *servername = *protocol = NULL;
- return -1;
+ printerr(0, "ERROR: failed to parse %s/info\n", clp->relpath);
+ free(servername);
+ free(protoname);
+ clp->servicename = NULL;
+ clp->servername = NULL;
+ clp->prog = 0;
+ clp->vers = 0;
+ clp->protocol = NULL;
+out:
+ if (info)
+ fclose(info);
+
+ free(server);
+ free(service);
+ free(address);
+ free(port);
}
static void
@@ -475,20 +499,8 @@ gssd_scan_clnt(struct topdir *tdi, const char *name)
/* not fatal, files might appear later */
goto out;
- if (clp->prog == 0) {
- int infofd;
-
- infofd = openat(clntfd, "info", O_RDONLY);
- if (infofd < 0) {
- printerr(0, "ERROR: can't open %s/info: %s\n",
- clp->relpath, strerror(errno));
- } else {
- read_service_info(infofd, &clp->servicename,
- &clp->servername, &clp->prog, &clp->vers,
- &clp->protocol, (struct sockaddr *) &clp->addr);
- close(infofd);
- }
- }
+ if (clp->prog == 0)
+ gssd_read_service_info(clntfd, clp);
out:
close(clntfd);
@@ -692,7 +704,7 @@ main(int argc, char *argv[])
#endif
break;
case 'D':
- avoid_dns = 0;
+ avoid_dns = false;
break;
default:
usage(argv[0]);
This is just the first step, replacing dnotify with an inotify
implementation that is not much better (still does a complete
rescan of the whole rpc_pipefs tree on each change).
Signed-off-by: David Härdeman <[email protected]>
---
utils/gssd/gssd.c | 101 ++++++++++++++++++++++++++++++++++++++---------------
utils/gssd/gssd.h | 2 +
2 files changed, 73 insertions(+), 30 deletions(-)
diff --git a/utils/gssd/gssd.c b/utils/gssd/gssd.c
index 78b836d..4409ad6 100644
--- a/utils/gssd/gssd.c
+++ b/utils/gssd/gssd.c
@@ -48,7 +48,7 @@
#include <sys/socket.h>
#include <sys/time.h>
#include <sys/resource.h>
-#include <sys/poll.h>
+#include <sys/inotify.h>
#include <rpc/rpc.h>
#include <netinet/in.h>
#include <arpa/inet.h>
@@ -75,6 +75,8 @@
static char *pipefs_path = GSSD_PIPEFS_DIR;
static DIR *pipefs_dir;
static int pipefs_fd;
+static int inotify_fd;
+struct event inotify_ev;
char *keytabfile = GSSD_DEFAULT_KEYTAB_FILE;
char **ccachesearch;
@@ -92,7 +94,7 @@ TAILQ_HEAD(topdir_list_head, topdir) topdir_list;
struct topdir {
TAILQ_ENTRY(topdir) list;
TAILQ_HEAD(clnt_list_head, clnt_info) clnt_list;
- int fd;
+ int wd;
char *name;
char dirname[];
};
@@ -350,9 +352,7 @@ gssd_destroy_client(struct clnt_info *clp)
event_del(&clp->gssd_ev);
}
- if (clp->dir_fd >= 0)
- close(clp->dir_fd);
-
+ inotify_rm_watch(inotify_fd, clp->wd);
free(clp->relpath);
free(clp->servicename);
free(clp->servername);
@@ -417,20 +417,16 @@ gssd_get_clnt(struct topdir *tdi, const char *name)
goto out;
}
- clp->name = clp->relpath + strlen(tdi->name) + 1;
- clp->krb5_fd = -1;
- clp->gssd_fd = -1;
- clp->dir_fd = -1;
-
- if ((clp->dir_fd = open(clp->relpath, O_RDONLY)) == -1) {
- if (errno != ENOENT)
- printerr(0, "ERROR: can't open %s: %s\n",
- clp->relpath, strerror(errno));
+ clp->wd = inotify_add_watch(inotify_fd, clp->relpath, IN_CREATE | IN_DELETE);
+ if (clp->wd < 0) {
+ printerr(0, "ERROR: inotify_add_watch failed for %s: %s\n",
+ clp->relpath, strerror(errno));
goto out;
}
- fcntl(clp->dir_fd, F_SETSIG, DNOTIFY_SIGNAL);
- fcntl(clp->dir_fd, F_NOTIFY, DN_CREATE | DN_DELETE | DN_MULTISHOT);
+ clp->name = clp->relpath + strlen(tdi->name) + 1;
+ clp->krb5_fd = -1;
+ clp->gssd_fd = -1;
TAILQ_INSERT_HEAD(&tdi->clnt_list, clp, list);
return clp;
@@ -524,20 +520,17 @@ gssd_get_topdir(const char *name)
return NULL;
}
- sprintf(tdi->dirname, "%s/%s", pipefs_path, name);
- tdi->name = tdi->dirname + strlen(pipefs_path) + 1;
- TAILQ_INIT(&tdi->clnt_list);
-
- tdi->fd = openat(pipefs_fd, name, O_RDONLY);
- if (tdi->fd < 0) {
- printerr(0, "ERROR: failed to open %s: %s\n",
+ tdi->wd = inotify_add_watch(inotify_fd, name, IN_CREATE | IN_DELETE);
+ if (tdi->wd < 0) {
+ printerr(0, "ERROR: inotify_add_watch failed for %s: %s\n",
tdi->dirname, strerror(errno));
free(tdi);
return NULL;
}
- fcntl(tdi->fd, F_SETSIG, DNOTIFY_SIGNAL);
- fcntl(tdi->fd, F_NOTIFY, DN_CREATE|DN_DELETE|DN_MODIFY|DN_MULTISHOT);
+ sprintf(tdi->dirname, "%s/%s", pipefs_path, name);
+ tdi->name = tdi->dirname + strlen(pipefs_path) + 1;
+ TAILQ_INIT(&tdi->clnt_list);
TAILQ_INSERT_HEAD(&topdir_list, tdi, list);
return tdi;
@@ -623,11 +616,56 @@ gssd_scan(void)
}
static void
-gssd_scan_cb(int UNUSED(ifd), short UNUSED(which), void *UNUSED(data))
+gssd_scan_cb(int UNUSED(fd), short UNUSED(which), void *UNUSED(data))
{
gssd_scan();
}
+static void
+gssd_inotify_cb(int ifd, short UNUSED(which), void *UNUSED(data))
+{
+ bool rescan = true; /* unconditional rescan, for now */
+ struct topdir *tdi;
+ struct clnt_info *clp;
+
+ while (true) {
+ char buf[4096] __attribute__ ((aligned(__alignof__(struct inotify_event))));
+ const struct inotify_event *ev;
+ ssize_t len;
+ char *ptr;
+
+ len = read(ifd, buf, sizeof(buf));
+ if (len == -1 && errno == EINTR)
+ continue;
+
+ if (len <= 0)
+ break;
+
+ for (ptr = buf; ptr < buf + len;
+ ptr += sizeof(struct inotify_event) + ev->len) {
+ ev = (const struct inotify_event *)ptr;
+
+ if (ev->mask & IN_Q_OVERFLOW) {
+ printerr(0, "ERROR: inotify queue overflow\n");
+ rescan = true;
+ }
+
+ /* NOTE: Set rescan if wd not found */
+ TAILQ_FOREACH(tdi, &topdir_list, list) {
+ if (tdi->wd == ev->wd)
+ break;
+
+ TAILQ_FOREACH(clp, &tdi->clnt_list, list) {
+ if (clp->wd == ev->wd)
+ break;
+ }
+ }
+ }
+ }
+
+ if (rescan)
+ gssd_scan();
+}
static void
gssd_atexit(void)
@@ -656,7 +694,6 @@ main(int argc, char *argv[])
char *progname;
char *ccachedir = NULL;
struct event sighup_ev;
- struct event sigdnotify_ev;
while ((opt = getopt(argc, argv, "DfvrlmnMp:k:d:t:T:R:")) != -1) {
switch (opt) {
@@ -799,10 +836,16 @@ main(int argc, char *argv[])
exit(EXIT_FAILURE);
}
+ inotify_fd = inotify_init1(IN_NONBLOCK);
+ if (inotify_fd == -1) {
+ printerr(1, "ERROR: inotify_init1 failed: %s\n", strerror(errno));
+ exit(EXIT_FAILURE);
+ }
+
signal_set(&sighup_ev, SIGHUP, gssd_scan_cb, NULL);
signal_add(&sighup_ev, NULL);
- signal_set(&sigdnotify_ev, DNOTIFY_SIGNAL, gssd_scan_cb, NULL);
- signal_add(&sigdnotify_ev, NULL);
+ event_set(&inotify_ev, inotify_fd, EV_READ | EV_PERSIST, gssd_inotify_cb, NULL);
+ event_add(&inotify_ev, NULL);
TAILQ_INIT(&topdir_list);
gssd_scan();
diff --git a/utils/gssd/gssd.h b/utils/gssd/gssd.h
index 7792035..c6937c5 100644
--- a/utils/gssd/gssd.h
+++ b/utils/gssd/gssd.h
@@ -64,7 +64,7 @@ extern char *preferred_realm;
struct clnt_info {
TAILQ_ENTRY(clnt_info) list;
- int dir_fd;
+ int wd;
bool scanned;
char *name;
char *relpath;
Save some more memory by using relative pathnames.
Signed-off-by: David Härdeman <[email protected]>
---
utils/gssd/gssd.c | 16 +++++++---------
1 file changed, 7 insertions(+), 9 deletions(-)
diff --git a/utils/gssd/gssd.c b/utils/gssd/gssd.c
index 4409ad6..b7a4950 100644
--- a/utils/gssd/gssd.c
+++ b/utils/gssd/gssd.c
@@ -95,8 +95,7 @@ struct topdir {
TAILQ_ENTRY(topdir) list;
TAILQ_HEAD(clnt_list_head, clnt_info) clnt_list;
int wd;
- char *name;
- char dirname[];
+ char name[];
};
/*
@@ -514,7 +513,7 @@ gssd_get_topdir(const char *name)
if (!strcmp(tdi->name, name))
return tdi;
- tdi = malloc(sizeof(*tdi) + strlen(pipefs_path) + strlen(name) + 2);
+ tdi = malloc(sizeof(*tdi) + strlen(name) + 1);
if (!tdi) {
printerr(0, "ERROR: Couldn't allocate struct topdir\n");
return NULL;
@@ -522,14 +521,13 @@ gssd_get_topdir(const char *name)
tdi->wd = inotify_add_watch(inotify_fd, name, IN_CREATE | IN_DELETE);
if (tdi->wd < 0) {
- printerr(0, "ERROR: inotify_add_watch failed for %s: %s\n",
- tdi->dirname, strerror(errno));
+ printerr(0, "ERROR: inotify_add_watch failed for top dir %s: %s\n",
+ tdi->name, strerror(errno));
free(tdi);
return NULL;
}
- sprintf(tdi->dirname, "%s/%s", pipefs_path, name);
- tdi->name = tdi->dirname + strlen(pipefs_path) + 1;
+ strcpy(tdi->name, name);
TAILQ_INIT(&tdi->clnt_list);
TAILQ_INSERT_HEAD(&topdir_list, tdi, list);
@@ -552,14 +550,14 @@ gssd_scan_topdir(const char *name)
dfd = openat(pipefs_fd, tdi->name, O_RDONLY);
if (dfd < 0) {
printerr(0, "ERROR: can't openat %s: %s\n",
- tdi->dirname, strerror(errno));
+ tdi->name, strerror(errno));
return;
}
dir = fdopendir(dfd);
if (!dir) {
printerr(0, "ERROR: can't fdopendir %s: %s\n",
- tdi->dirname, strerror(errno));
+ tdi->name, strerror(errno));
return;
}
Make full use of inotify by not rescanning the whole tree on each change,
instead keep track of the inotify events and make sure that the minimum
work (scan/create/delete) clients is done in most cases. Still detect
anomalies and perform a full rescan in those cases.
Signed-off-by: David Härdeman <[email protected]>
---
utils/gssd/gssd.c | 167 +++++++++++++++++++++++++++++++++---------------
utils/gssd/gssd_proc.c | 1
2 files changed, 116 insertions(+), 52 deletions(-)
diff --git a/utils/gssd/gssd.c b/utils/gssd/gssd.c
index b7a4950..2a768ea 100644
--- a/utils/gssd/gssd.c
+++ b/utils/gssd/gssd.c
@@ -362,36 +362,18 @@ gssd_destroy_client(struct clnt_info *clp)
static void gssd_scan(void);
static void
-gssd_clnt_gssd_cb(int UNUSED(fd), short which, void *data)
+gssd_clnt_gssd_cb(int UNUSED(fd), short UNUSED(which), void *data)
{
struct clnt_info *clp = data;
- if (which != EV_READ) {
- printerr(2, "Closing 'gssd' pipe %s\n", clp->relpath);
- close(clp->gssd_fd);
- clp->gssd_fd = -1;
- event_del(&clp->gssd_ev);
- gssd_scan();
- return;
- }
-
handle_gssd_upcall(clp);
}
static void
-gssd_clnt_krb5_cb(int UNUSED(fd), short which, void *data)
+gssd_clnt_krb5_cb(int UNUSED(fd), short UNUSED(which), void *data)
{
struct clnt_info *clp = data;
- if (which != EV_READ) {
- printerr(2, "Closing 'krb5' pipe %s\n", clp->relpath);
- close(clp->krb5_fd);
- clp->krb5_fd = -1;
- event_del(&clp->krb5_ev);
- gssd_scan();
- return;
- }
-
handle_krb5_upcall(clp);
}
@@ -437,17 +419,12 @@ out:
}
static int
-gssd_scan_clnt(struct topdir *tdi, const char *name)
+gssd_scan_clnt(struct clnt_info *clp)
{
- struct clnt_info *clp;
int clntfd;
bool gssd_was_closed;
bool krb5_was_closed;
- clp = gssd_get_clnt(tdi, name);
- if (!clp)
- return -1;
-
gssd_was_closed = clp->gssd_fd < 0 ? true : false;
krb5_was_closed = clp->krb5_fd < 0 ? true : false;
@@ -459,24 +436,10 @@ gssd_scan_clnt(struct topdir *tdi, const char *name)
}
if (clp->gssd_fd == -1)
- clp->gssd_fd = openat(clntfd, "gssd", O_RDWR);
+ clp->gssd_fd = openat(clntfd, "gssd", O_RDWR | O_NONBLOCK);
- if (clp->gssd_fd == -1) {
- if (clp->krb5_fd == -1)
- clp->krb5_fd = openat(clntfd, "krb5", O_RDWR);
-
- /* If we opened a gss-specific pipe, let's try opening
- * the new upcall pipe again. If we succeed, close
- * gss-specific pipe(s).
- */
- if (clp->krb5_fd != -1) {
- clp->gssd_fd = openat(clntfd, "gssd", O_RDWR);
- if (clp->gssd_fd != -1) {
- close(clp->krb5_fd);
- clp->krb5_fd = -1;
- }
- }
- }
+ if (clp->gssd_fd == -1 && clp->krb5_fd == -1)
+ clp->krb5_fd = openat(clntfd, "krb5", O_RDWR | O_NONBLOCK);
if (gssd_was_closed && clp->gssd_fd >= 0) {
event_set(&clp->gssd_ev, clp->gssd_fd, EV_READ | EV_PERSIST,
@@ -490,7 +453,7 @@ gssd_scan_clnt(struct topdir *tdi, const char *name)
event_add(&clp->krb5_ev, NULL);
}
- if ((clp->krb5_fd == -1) && (clp->gssd_fd == -1))
+ if (clp->krb5_fd == -1 && clp->gssd_fd == -1)
/* not fatal, files might appear later */
goto out;
@@ -503,6 +466,17 @@ out:
return 0;
}
+static int
+gssd_create_clnt(struct topdir *tdi, const char *name)
+{
+ struct clnt_info *clp;
+
+ clp = gssd_get_clnt(tdi, name);
+ if (!clp)
+ return -1;
+
+ return gssd_scan_clnt(clp);
+}
static struct topdir *
gssd_get_topdir(const char *name)
@@ -519,7 +493,7 @@ gssd_get_topdir(const char *name)
return NULL;
}
- tdi->wd = inotify_add_watch(inotify_fd, name, IN_CREATE | IN_DELETE);
+ tdi->wd = inotify_add_watch(inotify_fd, name, IN_CREATE);
if (tdi->wd < 0) {
printerr(0, "ERROR: inotify_add_watch failed for top dir %s: %s\n",
tdi->name, strerror(errno));
@@ -571,7 +545,7 @@ gssd_scan_topdir(const char *name)
if (strncmp(d->d_name, "clnt", strlen("clnt")))
continue;
- gssd_scan_clnt(tdi, d->d_name);
+ gssd_create_clnt(tdi, d->d_name);
}
closedir(dir);
@@ -595,6 +569,7 @@ gssd_scan(void)
{
struct dirent *d;
+ printerr(3, "doing a full rescan\n");
rewinddir(pipefs_dir);
while ((d = readdir(pipefs_dir))) {
@@ -619,10 +594,84 @@ gssd_scan_cb(int UNUSED(fd), short UNUSED(which), void *UNUSED(data))
gssd_scan();
}
+static bool
+gssd_inotify_topdir(struct topdir *tdi, const struct inotify_event *ev)
+{
+ printerr(5, "inotify event for topdir (%s) - "
+ "ev->wd (%d) ev->name (%s) ev->mask (0x%08x)\n",
+ tdi->name, ev->wd, ev->len > 0 ? ev->name : "<?>", ev->mask);
+
+ if (ev->mask & IN_IGNORED) {
+ printerr(0, "ERROR: topdir disappeared!\n");
+ return false;
+ }
+
+ if (ev->len == 0)
+ return false;
+
+ if (ev->mask & IN_CREATE) {
+ if (!(ev->mask & IN_ISDIR))
+ return true;
+
+ if (strncmp(ev->name, "clnt", strlen("clnt")))
+ return true;
+
+ if (gssd_create_clnt(tdi, ev->name))
+ return false;
+
+ return true;
+ }
+
+ return false;
+}
+
+static bool
+gssd_inotify_clnt(struct topdir *tdi, struct clnt_info *clp, const struct inotify_event *ev)
+{
+ printerr(5, "inotify event for clntdir (%s) - "
+ "ev->wd (%d) ev->name (%s) ev->mask (0x%08x)\n",
+ clp->relpath, ev->wd, ev->len > 0 ? ev->name : "<?>", ev->mask);
+
+ if (ev->mask & IN_IGNORED) {
+ TAILQ_REMOVE(&tdi->clnt_list, clp, list);
+ gssd_destroy_client(clp);
+ return true;
+ }
+
+ if (ev->len == 0)
+ return false;
+
+ if (ev->mask & IN_CREATE) {
+ if (!strcmp(ev->name, "gssd") ||
+ !strcmp(ev->name, "krb5") ||
+ !strcmp(ev->name, "info"))
+ if (gssd_scan_clnt(clp))
+ return false;
+
+ return true;
+
+ } else if (ev->mask & IN_DELETE) {
+ if (!strcmp(ev->name, "gssd") && clp->gssd_fd >= 0) {
+ close(clp->gssd_fd);
+ event_del(&clp->gssd_ev);
+ clp->gssd_fd = -1;
+
+ } else if (!strcmp(ev->name, "krb5") && clp->krb5_fd >= 0) {
+ close(clp->krb5_fd);
+ event_del(&clp->krb5_ev);
+ clp->krb5_fd = -1;
+ }
+
+ return true;
+ }
+
+ return false;
+}
+
static void
gssd_inotify_cb(int ifd, short UNUSED(which), void *UNUSED(data))
{
- bool rescan = true; /* unconditional rescan, for now */
+ bool rescan = false;
struct topdir *tdi;
struct clnt_info *clp;
@@ -646,18 +695,32 @@ gssd_inotify_cb(int ifd, short UNUSED(which), void *UNUSED(data))
if (ev->mask & IN_Q_OVERFLOW) {
printerr(0, "ERROR: inotify queue overflow\n");
rescan = true;
+ break;
}
- /* NOTE: Set rescan if wd not found */
TAILQ_FOREACH(tdi, &topdir_list, list) {
- if (tdi->wd == ev->wd)
- break;
+ if (tdi->wd == ev->wd) {
+ if (!gssd_inotify_topdir(tdi, ev))
+ rescan = true;
+ goto found;
+ }
TAILQ_FOREACH(clp, &tdi->clnt_list, list) {
- if (clp->wd == ev->wd)
- break;
+ if (clp->wd == ev->wd) {
+ if (!gssd_inotify_clnt(tdi, clp, ev))
+ rescan = true;
+ goto found;
+ }
}
}
+
+found:
+ if (!tdi) {
+ printerr(1, "inotify event for unknown wd!!! - "
+ "ev->wd (%d) ev->name (%s) ev->mask (0x%08x)\n",
+ ev->wd, ev->len > 0 ? ev->name : "<?>", ev->mask);
+ rescan = true;
+ }
}
}
diff --git a/utils/gssd/gssd_proc.c b/utils/gssd/gssd_proc.c
index c16f111..a5a46ef 100644
--- a/utils/gssd/gssd_proc.c
+++ b/utils/gssd/gssd_proc.c
@@ -9,6 +9,7 @@
Copyright (c) 2002 Marius Aamodt Eriksen <[email protected]>.
Copyright (c) 2002 Bruce Fields <[email protected]>
Copyright (c) 2004 Kevin Coffman <[email protected]>
+ Copyright (c) 2014 David Härdeman <[email protected]>
All rights reserved, all wrongs reversed.
Redistribution and use in source and binary forms, with or without
Stumbled across this function, just had to simplify it. No mallocs
necessary, one quick loop to find the parameters. Much simpler.
Signed-off-by: David Härdeman <[email protected]>
---
utils/gssd/gssd_proc.c | 106 +++++++++++++++++-------------------------------
1 file changed, 38 insertions(+), 68 deletions(-)
diff --git a/utils/gssd/gssd_proc.c b/utils/gssd/gssd_proc.c
index a5a46ef..d733339 100644
--- a/utils/gssd/gssd_proc.c
+++ b/utils/gssd/gssd_proc.c
@@ -713,6 +713,7 @@ handle_gssd_upcall(struct clnt_info *clp)
int lbuflen = 0;
char *p;
char *mech = NULL;
+ char *uidstr = NULL;
char *target = NULL;
char *service = NULL;
char *enctypes = NULL;
@@ -729,72 +730,53 @@ handle_gssd_upcall(struct clnt_info *clp)
printerr(2, "%s: '%s'\n", __func__, lbuf);
- /* find the mechanism name */
- if ((p = strstr(lbuf, "mech=")) != NULL) {
- mech = malloc(lbuflen);
- if (!mech)
- goto out;
- if (sscanf(p, "mech=%s", mech) != 1) {
- printerr(0, "WARNING: handle_gssd_upcall: "
- "failed to parse gss mechanism name "
- "in upcall string '%s'\n", lbuf);
- goto out;
- }
- } else {
+ 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=")))
+ uidstr = p + strlen("uid=");
+ else if (!strncmp(p, "enctypes=", strlen("enctypes=")))
+ enctypes = p + strlen("enctypes=");
+ else if (!strncmp(p, "target=", strlen("target=")))
+ target = p + strlen("target=");
+ else if (!strncmp(p, "service=", strlen("service=")))
+ service = p + strlen("service=");
+ }
+
+ if (!mech || strlen(mech) < 1) {
printerr(0, "WARNING: handle_gssd_upcall: "
"failed to find gss mechanism name "
"in upcall string '%s'\n", lbuf);
- goto out;
+ return;
}
- /* read uid */
- if ((p = strstr(lbuf, "uid=")) != NULL) {
- if (sscanf(p, "uid=%d", &uid) != 1) {
- printerr(0, "WARNING: handle_gssd_upcall: "
- "failed to parse uid "
- "in upcall string '%s'\n", lbuf);
- goto out;
- }
- } else {
+ if (uidstr) {
+ uid = (uid_t)strtol(uidstr, &p, 10);
+ if (p == uidstr || *p != '\0')
+ uidstr = NULL;
+ }
+
+ if (!uidstr) {
printerr(0, "WARNING: handle_gssd_upcall: "
"failed to find uid "
"in upcall string '%s'\n", lbuf);
- goto out;
+ return;
}
- /* read supported encryption types if supplied */
- if ((p = strstr(lbuf, "enctypes=")) != NULL) {
- enctypes = malloc(lbuflen);
- if (!enctypes)
- goto out;
- if (sscanf(p, "enctypes=%s", enctypes) != 1) {
- printerr(0, "WARNING: handle_gssd_upcall: "
- "failed to parse encryption types "
- "in upcall string '%s'\n", lbuf);
- goto out;
- }
- if (parse_enctypes(enctypes) != 0) {
- printerr(0, "WARNING: handle_gssd_upcall: "
- "parsing encryption types failed: errno %d\n", errno);
- }
+ if (enctypes && parse_enctypes(enctypes) != 0) {
+ printerr(0, "WARNING: handle_gssd_upcall: "
+ "parsing encryption types failed: errno %d\n", errno);
+ return;
}
- /* read target name */
- if ((p = strstr(lbuf, "target=")) != NULL) {
- target = malloc(lbuflen);
- if (!target)
- goto out;
- if (sscanf(p, "target=%s", target) != 1) {
- printerr(0, "WARNING: handle_gssd_upcall: "
- "failed to parse target name "
- "in upcall string '%s'\n", lbuf);
- goto out;
- }
+ if (target && strlen(target) < 1) {
+ printerr(0, "WARNING: handle_gssd_upcall: "
+ "failed to parse target name "
+ "in upcall string '%s'\n", lbuf);
+ return;
}
/*
- * read the service name
- *
* The presence of attribute "service=" indicates that machine
* credentials should be used for this request. If the value
* is "*", then any machine credentials available can be used.
@@ -802,16 +784,11 @@ handle_gssd_upcall(struct clnt_info *clp)
* the specified service name (always "nfs" for now) should be
* used.
*/
- if ((p = strstr(lbuf, "service=")) != NULL) {
- service = malloc(lbuflen);
- if (!service)
- goto out;
- if (sscanf(p, "service=%s", service) != 1) {
- printerr(0, "WARNING: handle_gssd_upcall: "
- "failed to parse service type "
- "in upcall string '%s'\n", lbuf);
- goto out;
- }
+ if (service && strlen(service) < 1) {
+ printerr(0, "WARNING: handle_gssd_upcall: "
+ "failed to parse service type "
+ "in upcall string '%s'\n", lbuf);
+ return;
}
if (strcmp(mech, "krb5") == 0 && clp->servername)
@@ -822,12 +799,5 @@ handle_gssd_upcall(struct clnt_info *clp)
"received unknown gss mech '%s'\n", mech);
do_error_downcall(clp->gssd_fd, uid, -EACCES);
}
-
-out:
- free(mech);
- free(enctypes);
- free(target);
- free(service);
- return;
}
On Tue, 09 Dec 2014 06:40:40 +0100
David Härdeman <[email protected]> wrote:
> The following series converts gssd to use libevent and inotify instead
> of a handrolled event loop and dnotify. Lots of cleanups in the process
> (e.g. removing a lot of arbitrary limitations and fixed size buffers).
>
> All in all a nice reduction in code size (what can I say, I was bored).
>
> I've even managed to mount NFS shares with the patched server :)
>
> ---
>
> David Härdeman (19):
> nfs-utils: cleanup daemonization code
> nfs-utils: gssd - merge gssd_main_loop.c and gssd.c
> nfs-utils: gssd - simplify some option handling
> nfs-utils: gssd - remove arbitrary GSSD_MAX_CCACHE_SEARCH limitation
> nfs-utils: gssd - simplify topdirs path
> nfs-utils: gssd - move over pipfs scanning code
> nfs-utils: gssd - simplify client dir scanning code
> nfs-utils: gssd - use libevent
> nfs-utils: gssd - remove "close me" code
> nfs-utils: gssd - make the client lists per-topdir
> nfs-utils: gssd - keep the rpc_pipefs dir open
> nfs-utils: gssd - use more relative paths
> nfs-utils: gssd - simplify topdir scanning
> nfs-utils: gssd - simplify client scanning
> nfs-utils: gssd - cleanup read_service_info
> nfs-utils: gssd - change dnotify to inotify
> nfs-utils: gssd - further shorten some pathnames
> nfs-utils: gssd - improve inotify
> nfs-utils: gssd - simplify handle_gssd_upcall
>
>
> support/include/nfslib.h | 5
> support/nfs/mydaemon.c | 92 +++--
> utils/gssd/Makefile.am | 24 +
> utils/gssd/gss_util.h | 2
> utils/gssd/gssd.c | 785 +++++++++++++++++++++++++++++++++++++++++--
> utils/gssd/gssd.h | 46 +--
> utils/gssd/gssd_main_loop.c | 263 --------------
> utils/gssd/gssd_proc.c | 654 ++----------------------------------
> utils/gssd/svcgssd.c | 8
> utils/idmapd/idmapd.c | 6
> utils/statd/statd.c | 66 +---
> 11 files changed, 878 insertions(+), 1073 deletions(-)
> delete mode 100644 utils/gssd/gssd_main_loop.c
>
> --
> David Härdeman
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
Looks like a nice cleanup at first glance, but I haven't gone over it
in detail.
One thing that _would_ be nice while you're in here though would be to
help parallelize more of process_krb5_upcall. Currently it forks before
changing its identity and then the parent waits on that to exit which
keeps everything serialized.
I did it that way so that we didn't need change how we close the fds
afterward. Now that you're already doing some surgery there, that might
be easier to do and may help performance.
--
Jeff Layton <[email protected]>
On 2014-12-09 14:09, Jeff Layton wrote:
> On Tue, 09 Dec 2014 06:40:40 +0100
> David Härdeman <[email protected]> wrote:
>
>> The following series converts gssd to use libevent and inotify instead
>> of a handrolled event loop and dnotify. Lots of cleanups in the
>> process
>> (e.g. removing a lot of arbitrary limitations and fixed size buffers).
...
>
> Looks like a nice cleanup at first glance, but I haven't gone over it
> in detail.
>
> One thing that _would_ be nice while you're in here though would be to
> help parallelize more of process_krb5_upcall. Currently it forks before
> changing its identity and then the parent waits on that to exit which
> keeps everything serialized.
>
> I did it that way so that we didn't need change how we close the fds
> afterward. Now that you're already doing some surgery there, that might
> be easier to do and may help performance.
Thanks, the current patchset doesn't really touch upon that code. After
the patchset is applied, utils/gssd/gssd.c is basically everything that
happens up to the point where data is available to read from the gss
pipe but not after....
But it should certainly make it easier to add parallelism in the
future...
BTW: when you say "how we close the fds"...what do you mean exactly?
//David
On Tue, 09 Dec 2014 14:52:56 +0100
David Härdeman <[email protected]> wrote:
> On 2014-12-09 14:09, Jeff Layton wrote:
> > On Tue, 09 Dec 2014 06:40:40 +0100
> > David Härdeman <[email protected]> wrote:
> >
> >> The following series converts gssd to use libevent and inotify instead
> >> of a handrolled event loop and dnotify. Lots of cleanups in the
> >> process
> >> (e.g. removing a lot of arbitrary limitations and fixed size buffers).
> ...
> >
> > Looks like a nice cleanup at first glance, but I haven't gone over it
> > in detail.
> >
> > One thing that _would_ be nice while you're in here though would be to
> > help parallelize more of process_krb5_upcall. Currently it forks before
> > changing its identity and then the parent waits on that to exit which
> > keeps everything serialized.
> >
> > I did it that way so that we didn't need change how we close the fds
> > afterward. Now that you're already doing some surgery there, that might
> > be easier to do and may help performance.
>
> Thanks, the current patchset doesn't really touch upon that code. After
> the patchset is applied, utils/gssd/gssd.c is basically everything that
> happens up to the point where data is available to read from the gss
> pipe but not after....
>
> But it should certainly make it easier to add parallelism in the
> future...
>
> BTW: when you say "how we close the fds"...what do you mean exactly?
>
>
> //David
Ahh, now I remember. It's not the closing of the fds that's the
problem, but you do need to have some way to reap the exit status from
the processes that are being forked off (so you don't end up with
zombies).
Probably not too hard to handle, but you would need to keep a count or
list of forked children and wait() on them at some point.
--
Jeff Layton <[email protected]>
On Tue, 9 Dec 2014 09:58:13 -0500
Jeff Layton <[email protected]> wrote:
> On Tue, 09 Dec 2014 14:52:56 +0100
> David Härdeman <[email protected]> wrote:
>
> > On 2014-12-09 14:09, Jeff Layton wrote:
> > > On Tue, 09 Dec 2014 06:40:40 +0100
> > > David Härdeman <[email protected]> wrote:
> > >
> > >> The following series converts gssd to use libevent and inotify
> > >> instead of a handrolled event loop and dnotify. Lots of cleanups
> > >> in the process
> > >> (e.g. removing a lot of arbitrary limitations and fixed size
> > >> buffers).
> > ...
> > >
> > > Looks like a nice cleanup at first glance, but I haven't gone
> > > over it in detail.
> > >
> > > One thing that _would_ be nice while you're in here though would
> > > be to help parallelize more of process_krb5_upcall. Currently it
> > > forks before changing its identity and then the parent waits on
> > > that to exit which keeps everything serialized.
> > >
> > > I did it that way so that we didn't need change how we close the
> > > fds afterward. Now that you're already doing some surgery there,
> > > that might be easier to do and may help performance.
> >
> > Thanks, the current patchset doesn't really touch upon that code.
> > After the patchset is applied, utils/gssd/gssd.c is basically
> > everything that happens up to the point where data is available to
> > read from the gss pipe but not after....
> >
> > But it should certainly make it easier to add parallelism in the
> > future...
> >
> > BTW: when you say "how we close the fds"...what do you mean exactly?
> >
> >
> > //David
>
> Ahh, now I remember. It's not the closing of the fds that's the
> problem, but you do need to have some way to reap the exit status from
> the processes that are being forked off (so you don't end up with
> zombies).
>
> Probably not too hard to handle, but you would need to keep a count or
> list of forked children and wait() on them at some point.
Maybe you have a list of children in the main process and a SIGCHLD
handler that clears it up when children dies ?
I think libevent may even have helpers for that (libtevent does).
Simo.
--
Simo Sorce * Red Hat, Inc * New York
On Tue, Dec 09, 2014 at 09:58:13AM -0500, Jeff Layton wrote:
>> On 2014-12-09 14:09, Jeff Layton wrote:
>>> One thing that _would_ be nice while you're in here though would be to
>>> help parallelize more of process_krb5_upcall. Currently it forks before
>>> changing its identity and then the parent waits on that to exit which
>>> keeps everything serialized.
>
>Ahh, now I remember. It's not the closing of the fds that's the
>problem, but you do need to have some way to reap the exit status from
>the processes that are being forked off (so you don't end up with
>zombies).
That's probably something that's been looked into before with libevent
(says he, without doing any research).
Another question that comes to mind...if we're anyway forking a child
per gssd request...how far has the idea of changing rpc.gssd over to a
/sbin/request-key helper been considered? I've seen some traces of
historical discussions via google but nothing concrete so far...
--
David H?rdeman
On Tue, 9 Dec 2014 20:55:30 +0100
David Härdeman <[email protected]> wrote:
> On Tue, Dec 09, 2014 at 09:58:13AM -0500, Jeff Layton wrote:
> >> On 2014-12-09 14:09, Jeff Layton wrote:
> >>> One thing that _would_ be nice while you're in here though would be to
> >>> help parallelize more of process_krb5_upcall. Currently it forks before
> >>> changing its identity and then the parent waits on that to exit which
> >>> keeps everything serialized.
> >
> >Ahh, now I remember. It's not the closing of the fds that's the
> >problem, but you do need to have some way to reap the exit status from
> >the processes that are being forked off (so you don't end up with
> >zombies).
>
> That's probably something that's been looked into before with libevent
> (says he, without doing any research).
>
I imagine so.
> Another question that comes to mind...if we're anyway forking a child
> per gssd request...how far has the idea of changing rpc.gssd over to a
> /sbin/request-key helper been considered? I've seen some traces of
> historical discussions via google but nothing concrete so far...
>
(cc'ing David in case I'm wrong on this point)
Yes. The problem with keyrings is that while the in-kernel parts are
namespace-aware, the upcalls are not. /sbin/request-key is spawned by a
kernel thread that lives in the init namespaces. Any solution that
involves a usermodehelper upcall will need to figure out how to handle
containerized clients.
Another idea might be to scrap rpc.gssd altogether and communicate with
gssproxy directly (but that too involves running a daemon, of course).
--
Jeff Layton <[email protected]>
On 2014-12-10 12:52, Jeff Layton wrote:
> On Tue, 9 Dec 2014 20:55:30 +0100
> David Härdeman <[email protected]> wrote:
>> Another question that comes to mind...if we're anyway forking a child
>> per gssd request...how far has the idea of changing rpc.gssd over to a
>> /sbin/request-key helper been considered? I've seen some traces of
>> historical discussions via google but nothing concrete so far...
>>
>
> (cc'ing David in case I'm wrong on this point)
>
> Yes. The problem with keyrings is that while the in-kernel parts are
> namespace-aware, the upcalls are not. /sbin/request-key is spawned by a
> kernel thread that lives in the init namespaces. Any solution that
> involves a usermodehelper upcall will need to figure out how to handle
> containerized clients.
>
> Another idea might be to scrap rpc.gssd altogether and communicate with
> gssproxy directly (but that too involves running a daemon, of course).
I'm not sure I follow completely...first of all, rpc.gssd is also not
namespace-aware, is it? I mean, sure, it could be run in a given
namespace, but there can still only be one rpc.gssd running?
Also...the nfsidmap binary (the request-key helper) isn't
namespace-aware...is it?
//David
On Wed, 10 Dec 2014 15:08:40 +0100
David Härdeman <[email protected]> wrote:
> On 2014-12-10 12:52, Jeff Layton wrote:
> > On Tue, 9 Dec 2014 20:55:30 +0100
> > David Härdeman <[email protected]> wrote:
> >> Another question that comes to mind...if we're anyway forking a child
> >> per gssd request...how far has the idea of changing rpc.gssd over to a
> >> /sbin/request-key helper been considered? I've seen some traces of
> >> historical discussions via google but nothing concrete so far...
> >>
> >
> > (cc'ing David in case I'm wrong on this point)
> >
> > Yes. The problem with keyrings is that while the in-kernel parts are
> > namespace-aware, the upcalls are not. /sbin/request-key is spawned by a
> > kernel thread that lives in the init namespaces. Any solution that
> > involves a usermodehelper upcall will need to figure out how to handle
> > containerized clients.
> >
> > Another idea might be to scrap rpc.gssd altogether and communicate with
> > gssproxy directly (but that too involves running a daemon, of course).
>
> I'm not sure I follow completely...first of all, rpc.gssd is also not
> namespace-aware, is it? I mean, sure, it could be run in a given
> namespace, but there can still only be one rpc.gssd running?
>
gssd isn't namespace aware, but it doesn't have to be since it gets
started in userland. In principle you could run a gssd per container[1].
As long as each container has its own net namespace, each gssd would
have its own set of rpc_pipefs pipes.
request-key is different. The kernel spawns a thread that execs the
program, but there's no support in that infrastructure for doing so
within a particular container.
> Also...the nfsidmap binary (the request-key helper) isn't
> namespace-aware...is it?
>
No it's not. I'd consider that a bug as well.
[1]: "container" is an overloaded term. In this discussion, I'm using
it as a generic name for a set of namespaces.
--
Jeff Layton <[email protected]>
On 2014-12-10 15:17, Jeff Layton wrote:
> On Wed, 10 Dec 2014 15:08:40 +0100
> David Härdeman <[email protected]> wrote:
>> I'm not sure I follow completely...first of all, rpc.gssd is also not
>> namespace-aware, is it? I mean, sure, it could be run in a given
>> namespace, but there can still only be one rpc.gssd running?
>>
>
> gssd isn't namespace aware, but it doesn't have to be since it gets
> started in userland. In principle you could run a gssd per
> container[1].
> As long as each container has its own net namespace, each gssd would
> have its own set of rpc_pipefs pipes.
>
> request-key is different. The kernel spawns a thread that execs the
> program, but there's no support in that infrastructure for doing so
> within a particular container.
This thread might be interesting:
https://lkml.org/lkml/2014/11/24/885
>> Also...the nfsidmap binary (the request-key helper) isn't
>> namespace-aware...is it?
>>
>
> No it's not. I'd consider that a bug as well.
So basically, a request-key based gssd would be possible if that "bug"
in the request-key infrastructure is fixed, right?
On Wed, 10 Dec 2014 15:31:34 +0100
David Härdeman <[email protected]> wrote:
> On 2014-12-10 15:17, Jeff Layton wrote:
> > On Wed, 10 Dec 2014 15:08:40 +0100
> > David Härdeman <[email protected]> wrote:
> >> I'm not sure I follow completely...first of all, rpc.gssd is also not
> >> namespace-aware, is it? I mean, sure, it could be run in a given
> >> namespace, but there can still only be one rpc.gssd running?
> >>
> >
> > gssd isn't namespace aware, but it doesn't have to be since it gets
> > started in userland. In principle you could run a gssd per
> > container[1].
> > As long as each container has its own net namespace, each gssd would
> > have its own set of rpc_pipefs pipes.
> >
> > request-key is different. The kernel spawns a thread that execs the
> > program, but there's no support in that infrastructure for doing so
> > within a particular container.
>
> This thread might be interesting:
> https://lkml.org/lkml/2014/11/24/885
>
Nice. I wasn't aware that Ian was working on this. I'll take a look.
> >> Also...the nfsidmap binary (the request-key helper) isn't
> >> namespace-aware...is it?
> >>
> >
> > No it's not. I'd consider that a bug as well.
>
> So basically, a request-key based gssd would be possible if that "bug"
> in the request-key infrastructure is fixed, right?
>
Yes, I don't see why not.
--
Jeff Layton <[email protected]>
Jeff Layton <[email protected]> wrote:
> > This thread might be interesting:
> > https://lkml.org/lkml/2014/11/24/885
> >
>
> Nice. I wasn't aware that Ian was working on this. I'll take a look.
I'm not sure what the current state of this is. There was some discussion
over how best to determine which container we need to run in - and it's
complicated by the fact that the mounter may run in a different container to
the program that triggered the mount due to mountpoint propagation.
David
On Wed, 10 Dec 2014 16:03:02 +0000
David Howells <[email protected]> wrote:
> Jeff Layton <[email protected]> wrote:
>
> > > This thread might be interesting:
> > > https://lkml.org/lkml/2014/11/24/885
> > >
> >
> > Nice. I wasn't aware that Ian was working on this. I'll take a look.
>
> I'm not sure what the current state of this is. There was some discussion
> over how best to determine which container we need to run in - and it's
> complicated by the fact that the mounter may run in a different container to
> the program that triggered the mount due to mountpoint propagation.
>
Yes. It's quite a thorny problem.
Part of the issue is that the different namespaces (net, mount, etc...)
are completely orthogonal to one another as far as the kernel is
concerned, but they really can't be when we start talking about
userland stuff.
For example, all of the nfs and nfsd namespace code was tied to the net
namespace. But, once you start involving things like gssd, the mount
namespace matters too as it has to deal with files (libraries and
config files, in particular).
Q: What happens if you have two "containers" that have the same net
namespace but different mount namespaces along with a different krb5
configuration in each? Maybe even with a gssd running in each?
A: A horrible mess, AFAICT...
Without something that really enforces a 1:1 relationship between all
of the different sorts of namespaces, the whole container/namespace
concept quickly descends into a horrid mess. It makes my head hurt.
--
Jeff Layton <[email protected]>
On Wed, Dec 10, 2014 at 02:03:11PM -0500, Jeff Layton wrote:
>On Wed, 10 Dec 2014 16:03:02 +0000
>David Howells <[email protected]> wrote:
>> Jeff Layton <[email protected]> wrote:
>> > > This thread might be interesting:
>> > > https://lkml.org/lkml/2014/11/24/885
>> > >
>> >
>> > Nice. I wasn't aware that Ian was working on this. I'll take a look.
>>
>> I'm not sure what the current state of this is. There was some discussion
>> over how best to determine which container we need to run in - and it's
>> complicated by the fact that the mounter may run in a different container to
>> the program that triggered the mount due to mountpoint propagation.
>>
>
>Yes. It's quite a thorny problem.
>
>Part of the issue is that the different namespaces (net, mount, etc...)
>are completely orthogonal to one another as far as the kernel is
>concerned, but they really can't be when we start talking about
>userland stuff.
>
>For example, all of the nfs and nfsd namespace code was tied to the net
>namespace. But, once you start involving things like gssd, the mount
>namespace matters too as it has to deal with files (libraries and
>config files, in particular).
>
>Q: What happens if you have two "containers" that have the same net
>namespace but different mount namespaces along with a different krb5
>configuration in each? Maybe even with a gssd running in each?
>
>A: A horrible mess, AFAICT...
>
>Without something that really enforces a 1:1 relationship between all
>of the different sorts of namespaces, the whole container/namespace
>concept quickly descends into a horrid mess. It makes my head hurt.
And crossing namespaces could theoretically be a feature as well
(meaning the 1:1 relationship isn't necessarily wanted)? Imagine
generating krb5 tickets in one container that are used in another
container...(though I might be completely mistaken here)?
Anyway....as far as I can tell...rpc.idmapd, nfsidmap and rpc.gssd all
lack namespace awareness...right? And in particular nfsidmap since it
runs in the root namespace (and the other tools run in whichever
namespace they're launched in, which may or may not be the right one)...
But...maybe that particular problem is not a good reason to hold up e.g.
experimentation with a request-key based gssd util (one that would work
for the "normal" case with no containers and namespaces....)?
--
David H?rdeman
On Wed, 2014-12-10 at 14:03 -0500, Jeff Layton wrote:
> On Wed, 10 Dec 2014 16:03:02 +0000
> David Howells <[email protected]> wrote:
>
> > Jeff Layton <[email protected]> wrote:
> >
> > > > This thread might be interesting:
> > > > https://lkml.org/lkml/2014/11/24/885
> > > >
> > >
> > > Nice. I wasn't aware that Ian was working on this. I'll take a look.
> >
> > I'm not sure what the current state of this is. There was some discussion
> > over how best to determine which container we need to run in - and it's
> > complicated by the fact that the mounter may run in a different container to
> > the program that triggered the mount due to mountpoint propagation.
> >
>
> Yes. It's quite a thorny problem.
And I'm still not sure how to work that out ....
>
> Part of the issue is that the different namespaces (net, mount, etc...)
> are completely orthogonal to one another as far as the kernel is
> concerned, but they really can't be when we start talking about
> userland stuff.
Yeah, and the way that open()/setns() works is strange.
The open() gets you access to the proc_ns_operations (umm .. from
memory) used by setns() to install one of the created namespaces into
the process nsproxy but the namespace create always creates all six (or
seven) namespaces and releases them all on subsequent open()/setns()
operations to install a another namespace.
Sure, I don't properly understand this yet but it seems a bit odd.
>
> For example, all of the nfs and nfsd namespace code was tied to the net
> namespace. But, once you start involving things like gssd, the mount
> namespace matters too as it has to deal with files (libraries and
> config files, in particular).
>
> Q: What happens if you have two "containers" that have the same net
> namespace but different mount namespaces along with a different krb5
> configuration in each? Maybe even with a gssd running in each?
I'm assuming that, to execute a helper within a container, the
namespaces of the container itself should always be used.
If we follow the open()/setns() procedure in kernel, similar to
nsenter(1) that should be possible.
Yes, it does seem more complex than it needs to be, due to what I
described above, but we need a starting point.
At the moment I'm struggling to work out where to get an appropriate
"struct cred" and how to override this in the new process.
>
> A: A horrible mess, AFAICT...
>
> Without something that really enforces a 1:1 relationship between all
> of the different sorts of namespaces, the whole container/namespace
> concept quickly descends into a horrid mess. It makes my head hurt.
>
Mine too, ;)
Ian
On Wed, 10 Dec 2014, David Howells wrote:
> Jeff Layton <[email protected]> wrote:
>
> > > This thread might be interesting:
> > > https://lkml.org/lkml/2014/11/24/885
> > >
> >
> > Nice. I wasn't aware that Ian was working on this. I'll take a look.
>
> I'm not sure what the current state of this is. There was some discussion
> over how best to determine which container we need to run in - and it's
> complicated by the fact that the mounter may run in a different container to
> the program that triggered the mount due to mountpoint propagation.
>
> David
The specific problem of how to run /sbin/request-key in the caller's
"container" for idmap and gssd (and other friends) became more generally a
problem of how to solve the namespace (or more generally again, "context")
problem for some users of kmod's call_usermodehelper. The nice thing about
call_usermodehelper is that you don't have to do a lot of work to set up a
process to get something done in userspace -- however it is sounding more
like we do need to work hard to set up context for some users.
The userspace work needs to be done within a context that currently exists
or once existed, so the questions are where do we get that context and how
do we keep it around until we need it?
I think there's agreement that the setup of that context should be basically
what's done in fork() for consistency and future work. So we get LSM and
cgroups, etc.. in addition to namespaces.
There are two suggested approaches:
1) Anytime we think we're going to later need to upcall with a context we
fork and keep a thread around to do that work. For NFS, that would look
like forking a thread for every mount at mount time. The user of this API
would be responsible for creating/maintaining the thread and passing it
along for work.
2) Specify that a usermodehelper should attempt to use a context rather than
the default root context. The context used would be taken from the "init"
process of the current pid_namespace. Either that init_process itself could
be asked to fork/execve or when the pid_namespace is created a separate
helper thread is reserved.
I lean toward the second approach because I think it most closely matches
the context transistions that we have today, and can be more generally
applied. I'm pecking away at getting a rough implementation, which I plan
on asking Ian to review initially.
Ben
On Wed, 2014-12-10 at 18:21 -0500, Benjamin Coddington wrote:
> On Wed, 10 Dec 2014, David Howells wrote:
>
> > Jeff Layton <[email protected]> wrote:
> >
> > > > This thread might be interesting:
> > > > https://lkml.org/lkml/2014/11/24/885
> > > >
> > >
> > > Nice. I wasn't aware that Ian was working on this. I'll take a look.
> >
> > I'm not sure what the current state of this is. There was some discussion
> > over how best to determine which container we need to run in - and it's
> > complicated by the fact that the mounter may run in a different container to
> > the program that triggered the mount due to mountpoint propagation.
> >
> > David
>
> The specific problem of how to run /sbin/request-key in the caller's
> "container" for idmap and gssd (and other friends) became more generally a
> problem of how to solve the namespace (or more generally again, "context")
> problem for some users of kmod's call_usermodehelper. The nice thing about
> call_usermodehelper is that you don't have to do a lot of work to set up a
> process to get something done in userspace -- however it is sounding more
> like we do need to work hard to set up context for some users.
>
> The userspace work needs to be done within a context that currently exists
> or once existed, so the questions are where do we get that context and how
> do we keep it around until we need it?
>
> I think there's agreement that the setup of that context should be basically
> what's done in fork() for consistency and future work. So we get LSM and
> cgroups, etc.. in addition to namespaces.
And that's when the usermode helper init function is called, just before
the exec, so I think that's the place it needs to be done.
>
> There are two suggested approaches:
>
> 1) Anytime we think we're going to later need to upcall with a context we
> fork and keep a thread around to do that work. For NFS, that would look
> like forking a thread for every mount at mount time. The user of this API
> would be responsible for creating/maintaining the thread and passing it
> along for work.
Yeah, I don't think that's workable for large numbers of mounts and I
don't think it's really necessary.
>
> 2) Specify that a usermodehelper should attempt to use a context rather than
> the default root context. The context used would be taken from the "init"
> process of the current pid_namespace. Either that init_process itself could
> be asked to fork/execve or when the pid_namespace is created a separate
> helper thread is reserved.
I think this is doable using open()/setns() in a similar way to
nsenter(1). We can worry about simplifying it once we have a viable
approach to work from.
The reality is that now user mode helpers are executed within the root
context of init so I can't see why we can't use the context of init of
the container for this.
Modifying that along the way with a "struct cred" is probably a good
idea although it isn't done now for user mode callbacks. The "struct
cred" of the root init process surely isn't what needs to be used when
executing in a container so something needs to be done. If we duplicate
the same behaviour we have now for execution outside of a container then
we'd use the "struct cred" of the container init process so maybe we do
know where to get the cred, not sure about that though.
>
> I lean toward the second approach because I think it most closely matches
> the context transistions that we have today, and can be more generally
> applied. I'm pecking away at getting a rough implementation, which I plan
> on asking Ian to review initially.
I also have some patches so it's probably a good idea to share, ;)
Ian
On Thu, 11 Dec 2014, Ian Kent wrote:
> On Wed, 2014-12-10 at 18:21 -0500, Benjamin Coddington wrote:
> > On Wed, 10 Dec 2014, David Howells wrote:
> >
> > > Jeff Layton <[email protected]> wrote:
> > >
> > > > > This thread might be interesting:
> > > > > https://lkml.org/lkml/2014/11/24/885
> > > > >
> > > >
> > > > Nice. I wasn't aware that Ian was working on this. I'll take a look.
> > >
> > > I'm not sure what the current state of this is. There was some discussion
> > > over how best to determine which container we need to run in - and it's
> > > complicated by the fact that the mounter may run in a different container to
> > > the program that triggered the mount due to mountpoint propagation.
> > >
> > > David
> >
> > The specific problem of how to run /sbin/request-key in the caller's
> > "container" for idmap and gssd (and other friends) became more generally a
> > problem of how to solve the namespace (or more generally again, "context")
> > problem for some users of kmod's call_usermodehelper. The nice thing about
> > call_usermodehelper is that you don't have to do a lot of work to set up a
> > process to get something done in userspace -- however it is sounding more
> > like we do need to work hard to set up context for some users.
> >
> > The userspace work needs to be done within a context that currently exists
> > or once existed, so the questions are where do we get that context and how
> > do we keep it around until we need it?
> >
> > I think there's agreement that the setup of that context should be basically
> > what's done in fork() for consistency and future work. So we get LSM and
> > cgroups, etc.. in addition to namespaces.
>
> And that's when the usermode helper init function is called, just before
> the exec, so I think that's the place it needs to be done.
>
> >
> > There are two suggested approaches:
> >
> > 1) Anytime we think we're going to later need to upcall with a context we
> > fork and keep a thread around to do that work. For NFS, that would look
> > like forking a thread for every mount at mount time. The user of this API
> > would be responsible for creating/maintaining the thread and passing it
> > along for work.
>
> Yeah, I don't think that's workable for large numbers of mounts and I
> don't think it's really necessary.
>
> >
> > 2) Specify that a usermodehelper should attempt to use a context rather than
> > the default root context. The context used would be taken from the "init"
> > process of the current pid_namespace. Either that init_process itself could
> > be asked to fork/execve or when the pid_namespace is created a separate
> > helper thread is reserved.
>
> I think this is doable using open()/setns() in a similar way to
> nsenter(1). We can worry about simplifying it once we have a viable
> approach to work from.
>
> The reality is that now user mode helpers are executed within the root
> context of init so I can't see why we can't use the context of init of
> the container for this.
>
> Modifying that along the way with a "struct cred" is probably a good
> idea although it isn't done now for user mode callbacks. The "struct
> cred" of the root init process surely isn't what needs to be used when
> executing in a container so something needs to be done. If we duplicate
> the same behaviour we have now for execution outside of a container then
> we'd use the "struct cred" of the container init process so maybe we do
> know where to get the cred, not sure about that though.
I'm not following you entirely here. Do you mean that the helper should
probably have the container init's cred stripped off or sanitized?
I think maybe you're a bit farther along than I am working through this..
> >
> > I lean toward the second approach because I think it most closely matches
> > the context transistions that we have today, and can be more generally
> > applied. I'm pecking away at getting a rough implementation, which I plan
> > on asking Ian to review initially.
>
> I also have some patches so it's probably a good idea to share, ;)
>
> Ian
Great to hear you're working on this! If I end up getting something
spinning I'll send it along.
Ben
On Wed, 2014-12-10 at 20:54 -0500, Benjamin Coddington wrote:
>
> On Thu, 11 Dec 2014, Ian Kent wrote:
>
> > On Wed, 2014-12-10 at 18:21 -0500, Benjamin Coddington wrote:
> > > On Wed, 10 Dec 2014, David Howells wrote:
> > >
> > > > Jeff Layton <[email protected]> wrote:
> > > >
> > > > > > This thread might be interesting:
> > > > > > https://lkml.org/lkml/2014/11/24/885
> > > > > >
> > > > >
> > > > > Nice. I wasn't aware that Ian was working on this. I'll take a look.
> > > >
> > > > I'm not sure what the current state of this is. There was some discussion
> > > > over how best to determine which container we need to run in - and it's
> > > > complicated by the fact that the mounter may run in a different container to
> > > > the program that triggered the mount due to mountpoint propagation.
> > > >
> > > > David
> > >
> > > The specific problem of how to run /sbin/request-key in the caller's
> > > "container" for idmap and gssd (and other friends) became more generally a
> > > problem of how to solve the namespace (or more generally again, "context")
> > > problem for some users of kmod's call_usermodehelper. The nice thing about
> > > call_usermodehelper is that you don't have to do a lot of work to set up a
> > > process to get something done in userspace -- however it is sounding more
> > > like we do need to work hard to set up context for some users.
> > >
> > > The userspace work needs to be done within a context that currently exists
> > > or once existed, so the questions are where do we get that context and how
> > > do we keep it around until we need it?
> > >
> > > I think there's agreement that the setup of that context should be basically
> > > what's done in fork() for consistency and future work. So we get LSM and
> > > cgroups, etc.. in addition to namespaces.
> >
> > And that's when the usermode helper init function is called, just before
> > the exec, so I think that's the place it needs to be done.
> >
> > >
> > > There are two suggested approaches:
> > >
> > > 1) Anytime we think we're going to later need to upcall with a context we
> > > fork and keep a thread around to do that work. For NFS, that would look
> > > like forking a thread for every mount at mount time. The user of this API
> > > would be responsible for creating/maintaining the thread and passing it
> > > along for work.
> >
> > Yeah, I don't think that's workable for large numbers of mounts and I
> > don't think it's really necessary.
> >
> > >
> > > 2) Specify that a usermodehelper should attempt to use a context rather than
> > > the default root context. The context used would be taken from the "init"
> > > process of the current pid_namespace. Either that init_process itself could
> > > be asked to fork/execve or when the pid_namespace is created a separate
> > > helper thread is reserved.
> >
> > I think this is doable using open()/setns() in a similar way to
> > nsenter(1). We can worry about simplifying it once we have a viable
> > approach to work from.
> >
> > The reality is that now user mode helpers are executed within the root
> > context of init so I can't see why we can't use the context of init of
> > the container for this.
> >
> > Modifying that along the way with a "struct cred" is probably a good
> > idea although it isn't done now for user mode callbacks. The "struct
> > cred" of the root init process surely isn't what needs to be used when
> > executing in a container so something needs to be done. If we duplicate
> > the same behaviour we have now for execution outside of a container then
> > we'd use the "struct cred" of the container init process so maybe we do
> > know where to get the cred, not sure about that though.
>
> I'm not following you entirely here. Do you mean that the helper should
> probably have the container init's cred stripped off or sanitized?
LOL, that's good question.
What I think I'm saying is that, when the usermode helper is run we
don't want to use root init's credentials but some other credentials
relevant to the container, possibly the credentials of the mounter or
nfsd process credentials or the container init credentials.
In any case they will need to be set to something different and
appropriate. I'm not sure how to do that just yet.
Ian
On Thu, 11 Dec 2014 11:21:21 +0800
Ian Kent <[email protected]> wrote:
> On Wed, 2014-12-10 at 20:54 -0500, Benjamin Coddington wrote:
> >
> > On Thu, 11 Dec 2014, Ian Kent wrote:
> >
> > > On Wed, 2014-12-10 at 18:21 -0500, Benjamin Coddington wrote:
> > > > On Wed, 10 Dec 2014, David Howells wrote:
> > > >
> > > > > Jeff Layton <[email protected]> wrote:
> > > > >
> > > > > > > This thread might be interesting:
> > > > > > > https://lkml.org/lkml/2014/11/24/885
> > > > > > >
> > > > > >
> > > > > > Nice. I wasn't aware that Ian was working on this. I'll take a look.
> > > > >
> > > > > I'm not sure what the current state of this is. There was some discussion
> > > > > over how best to determine which container we need to run in - and it's
> > > > > complicated by the fact that the mounter may run in a different container to
> > > > > the program that triggered the mount due to mountpoint propagation.
> > > > >
> > > > > David
> > > >
> > > > The specific problem of how to run /sbin/request-key in the caller's
> > > > "container" for idmap and gssd (and other friends) became more generally a
> > > > problem of how to solve the namespace (or more generally again, "context")
> > > > problem for some users of kmod's call_usermodehelper. The nice thing about
> > > > call_usermodehelper is that you don't have to do a lot of work to set up a
> > > > process to get something done in userspace -- however it is sounding more
> > > > like we do need to work hard to set up context for some users.
> > > >
> > > > The userspace work needs to be done within a context that currently exists
> > > > or once existed, so the questions are where do we get that context and how
> > > > do we keep it around until we need it?
> > > >
> > > > I think there's agreement that the setup of that context should be basically
> > > > what's done in fork() for consistency and future work. So we get LSM and
> > > > cgroups, etc.. in addition to namespaces.
> > >
> > > And that's when the usermode helper init function is called, just before
> > > the exec, so I think that's the place it needs to be done.
> > >
> > > >
> > > > There are two suggested approaches:
> > > >
> > > > 1) Anytime we think we're going to later need to upcall with a context we
> > > > fork and keep a thread around to do that work. For NFS, that would look
> > > > like forking a thread for every mount at mount time. The user of this API
> > > > would be responsible for creating/maintaining the thread and passing it
> > > > along for work.
> > >
> > > Yeah, I don't think that's workable for large numbers of mounts and I
> > > don't think it's really necessary.
> > >
> > > >
> > > > 2) Specify that a usermodehelper should attempt to use a context rather than
> > > > the default root context. The context used would be taken from the "init"
> > > > process of the current pid_namespace. Either that init_process itself could
> > > > be asked to fork/execve or when the pid_namespace is created a separate
> > > > helper thread is reserved.
> > >
> > > I think this is doable using open()/setns() in a similar way to
> > > nsenter(1). We can worry about simplifying it once we have a viable
> > > approach to work from.
> > >
> > > The reality is that now user mode helpers are executed within the root
> > > context of init so I can't see why we can't use the context of init of
> > > the container for this.
> > >
> > > Modifying that along the way with a "struct cred" is probably a good
> > > idea although it isn't done now for user mode callbacks. The "struct
> > > cred" of the root init process surely isn't what needs to be used when
> > > executing in a container so something needs to be done. If we duplicate
> > > the same behaviour we have now for execution outside of a container then
> > > we'd use the "struct cred" of the container init process so maybe we do
> > > know where to get the cred, not sure about that though.
> >
> > I'm not following you entirely here. Do you mean that the helper should
> > probably have the container init's cred stripped off or sanitized?
>
> LOL, that's good question.
>
> What I think I'm saying is that, when the usermode helper is run we
> don't want to use root init's credentials but some other credentials
> relevant to the container, possibly the credentials of the mounter or
> nfsd process credentials or the container init credentials.
>
> In any case they will need to be set to something different and
> appropriate. I'm not sure how to do that just yet.
>
Yes, I think we might need to step back and consider that we have a
number of different use cases here, most of which are currently not
well served.
For instance: module loading clearly needs to be done in the "context"
of the canonical root init process. That's what call_usermodehelper was
originally used for so we need to keep that ability intact.
OTOH, keyring upcalls probably ought to be done in the context of the
task that triggered them. Certainly we ought to be spawning them with
the credentials associated with the keyring.
Today, those tasks not only run in the namespaces, etc of the root init
process, but also with with root's creds. That's unnecessary and seems
wrong. I think it's something that ought to be changed (though doing so
will likely be painful as we'll need to change the upcall programs to
handle that).
There are also other questions:
How should we go about spawning the binary given that we might want to
have it run in a different mount namespace? There are at least two
options:
1) change the mount namespace first and then exec the binary (in effect
run the binary with the given path from inside the container). This is
possibly a security hole if an attacker can trick the kernel into
running a different binary than intended by manipulating namespaces.
...or...
2) find and exec the binary and then change the namespaces afterward.
This has some potential problems if the program does something like
try to dlopen libraries after setns(). You could end up with a mismatch
if the container holds a different set of binaries from the one in the
root container.
--
Jeff Layton <[email protected]>
On Thu, 2014-12-11 at 06:45 -0500, Jeff Layton wrote:
> On Thu, 11 Dec 2014 11:21:21 +0800
> Ian Kent <[email protected]> wrote:
>
> > On Wed, 2014-12-10 at 20:54 -0500, Benjamin Coddington wrote:
> > >
> > > On Thu, 11 Dec 2014, Ian Kent wrote:
> > >
> > > > On Wed, 2014-12-10 at 18:21 -0500, Benjamin Coddington wrote:
> > > > > On Wed, 10 Dec 2014, David Howells wrote:
> > > > >
> > > > > > Jeff Layton <[email protected]> wrote:
> > > > > >
> > > > > > > > This thread might be interesting:
> > > > > > > > https://lkml.org/lkml/2014/11/24/885
> > > > > > > >
> > > > > > >
> > > > > > > Nice. I wasn't aware that Ian was working on this. I'll take a look.
> > > > > >
> > > > > > I'm not sure what the current state of this is. There was some discussion
> > > > > > over how best to determine which container we need to run in - and it's
> > > > > > complicated by the fact that the mounter may run in a different container to
> > > > > > the program that triggered the mount due to mountpoint propagation.
> > > > > >
> > > > > > David
> > > > >
> > > > > The specific problem of how to run /sbin/request-key in the caller's
> > > > > "container" for idmap and gssd (and other friends) became more generally a
> > > > > problem of how to solve the namespace (or more generally again, "context")
> > > > > problem for some users of kmod's call_usermodehelper. The nice thing about
> > > > > call_usermodehelper is that you don't have to do a lot of work to set up a
> > > > > process to get something done in userspace -- however it is sounding more
> > > > > like we do need to work hard to set up context for some users.
> > > > >
> > > > > The userspace work needs to be done within a context that currently exists
> > > > > or once existed, so the questions are where do we get that context and how
> > > > > do we keep it around until we need it?
> > > > >
> > > > > I think there's agreement that the setup of that context should be basically
> > > > > what's done in fork() for consistency and future work. So we get LSM and
> > > > > cgroups, etc.. in addition to namespaces.
> > > >
> > > > And that's when the usermode helper init function is called, just before
> > > > the exec, so I think that's the place it needs to be done.
> > > >
> > > > >
> > > > > There are two suggested approaches:
> > > > >
> > > > > 1) Anytime we think we're going to later need to upcall with a context we
> > > > > fork and keep a thread around to do that work. For NFS, that would look
> > > > > like forking a thread for every mount at mount time. The user of this API
> > > > > would be responsible for creating/maintaining the thread and passing it
> > > > > along for work.
> > > >
> > > > Yeah, I don't think that's workable for large numbers of mounts and I
> > > > don't think it's really necessary.
> > > >
> > > > >
> > > > > 2) Specify that a usermodehelper should attempt to use a context rather than
> > > > > the default root context. The context used would be taken from the "init"
> > > > > process of the current pid_namespace. Either that init_process itself could
> > > > > be asked to fork/execve or when the pid_namespace is created a separate
> > > > > helper thread is reserved.
> > > >
> > > > I think this is doable using open()/setns() in a similar way to
> > > > nsenter(1). We can worry about simplifying it once we have a viable
> > > > approach to work from.
> > > >
> > > > The reality is that now user mode helpers are executed within the root
> > > > context of init so I can't see why we can't use the context of init of
> > > > the container for this.
> > > >
> > > > Modifying that along the way with a "struct cred" is probably a good
> > > > idea although it isn't done now for user mode callbacks. The "struct
> > > > cred" of the root init process surely isn't what needs to be used when
> > > > executing in a container so something needs to be done. If we duplicate
> > > > the same behaviour we have now for execution outside of a container then
> > > > we'd use the "struct cred" of the container init process so maybe we do
> > > > know where to get the cred, not sure about that though.
> > >
> > > I'm not following you entirely here. Do you mean that the helper should
> > > probably have the container init's cred stripped off or sanitized?
> >
> > LOL, that's good question.
> >
> > What I think I'm saying is that, when the usermode helper is run we
> > don't want to use root init's credentials but some other credentials
> > relevant to the container, possibly the credentials of the mounter or
> > nfsd process credentials or the container init credentials.
> >
> > In any case they will need to be set to something different and
> > appropriate. I'm not sure how to do that just yet.
> >
>
> Yes, I think we might need to step back and consider that we have a
> number of different use cases here, most of which are currently not
> well served.
Indeed yes, and what we got was the result I expected from the initial
post of the patches for this, so, I am, ;)
>
> For instance: module loading clearly needs to be done in the "context"
> of the canonical root init process. That's what call_usermodehelper was
> originally used for so we need to keep that ability intact.
Not sure that's an issue since the original call_usermodehelper() will
be left in tact and people will need to make a conscious decision to
call what, so far, is call_usermodehelper_ns() to exec within a
container. At least that's the plan.
>
> OTOH, keyring upcalls probably ought to be done in the context of the
> task that triggered them. Certainly we ought to be spawning them with
> the credentials associated with the keyring.
Yes, but I'm not really there yet so I can't make sensible comments
about it.
>
> Today, those tasks not only run in the namespaces, etc of the root init
> process, but also with with root's creds. That's unnecessary and seems
> wrong. I think it's something that ought to be changed (though doing so
> will likely be painful as we'll need to change the upcall programs to
> handle that).
One thing I believe is that user space programs shouldn't know or need
to to know they are running within a container, I believe this should
have been part of the namespace implementation from the start.
The creds issue is what I'm trying to understand now since I've not had
to concern myself with these before I'm a bit at sea. It may prove not
doable but then maybe not.
>
> There are also other questions:
>
> How should we go about spawning the binary given that we might want to
> have it run in a different mount namespace? There are at least two
> options:
If anything the response to the initial post of these patches showed
that we can't just consider the mount namespace we need to consider the
whole process environment.
>
> 1) change the mount namespace first and then exec the binary (in effect
> run the binary with the given path from inside the container). This is
> possibly a security hole if an attacker can trick the kernel into
> running a different binary than intended by manipulating namespaces.
I believe this has to be the way it's done, after sub-process creation
and before the exec, in the user mode helper runner.
>
> ...or...
>
> 2) find and exec the binary and then change the namespaces afterward.
> This has some potential problems if the program does something like
> try to dlopen libraries after setns(). You could end up with a mismatch
> if the container holds a different set of binaries from the one in the
> root container.
We really shouldn't need to change the user space binaries, I'd like to
try to avoid that if at all possible.
When I've referred to setns() here I'm thinking of an in kernel
equivalent not the user space setns() syscall and that wasn't clear,
sorry.
Ian
On Thu, 11 Dec 2014 20:55:42 +0800
Ian Kent <[email protected]> wrote:
> On Thu, 2014-12-11 at 06:45 -0500, Jeff Layton wrote:
> > On Thu, 11 Dec 2014 11:21:21 +0800
> > Ian Kent <[email protected]> wrote:
> >
> > > On Wed, 2014-12-10 at 20:54 -0500, Benjamin Coddington wrote:
> > > >
> > > > On Thu, 11 Dec 2014, Ian Kent wrote:
> > > >
> > > > > On Wed, 2014-12-10 at 18:21 -0500, Benjamin Coddington wrote:
> > > > > > On Wed, 10 Dec 2014, David Howells wrote:
> > > > > >
> > > > > > > Jeff Layton <[email protected]> wrote:
> > > > > > >
> > > > > > > > > This thread might be interesting:
> > > > > > > > > https://lkml.org/lkml/2014/11/24/885
> > > > > > > > >
> > > > > > > >
> > > > > > > > Nice. I wasn't aware that Ian was working on this. I'll take a look.
> > > > > > >
> > > > > > > I'm not sure what the current state of this is. There was some discussion
> > > > > > > over how best to determine which container we need to run in - and it's
> > > > > > > complicated by the fact that the mounter may run in a different container to
> > > > > > > the program that triggered the mount due to mountpoint propagation.
> > > > > > >
> > > > > > > David
> > > > > >
> > > > > > The specific problem of how to run /sbin/request-key in the caller's
> > > > > > "container" for idmap and gssd (and other friends) became more generally a
> > > > > > problem of how to solve the namespace (or more generally again, "context")
> > > > > > problem for some users of kmod's call_usermodehelper. The nice thing about
> > > > > > call_usermodehelper is that you don't have to do a lot of work to set up a
> > > > > > process to get something done in userspace -- however it is sounding more
> > > > > > like we do need to work hard to set up context for some users.
> > > > > >
> > > > > > The userspace work needs to be done within a context that currently exists
> > > > > > or once existed, so the questions are where do we get that context and how
> > > > > > do we keep it around until we need it?
> > > > > >
> > > > > > I think there's agreement that the setup of that context should be basically
> > > > > > what's done in fork() for consistency and future work. So we get LSM and
> > > > > > cgroups, etc.. in addition to namespaces.
> > > > >
> > > > > And that's when the usermode helper init function is called, just before
> > > > > the exec, so I think that's the place it needs to be done.
> > > > >
> > > > > >
> > > > > > There are two suggested approaches:
> > > > > >
> > > > > > 1) Anytime we think we're going to later need to upcall with a context we
> > > > > > fork and keep a thread around to do that work. For NFS, that would look
> > > > > > like forking a thread for every mount at mount time. The user of this API
> > > > > > would be responsible for creating/maintaining the thread and passing it
> > > > > > along for work.
> > > > >
> > > > > Yeah, I don't think that's workable for large numbers of mounts and I
> > > > > don't think it's really necessary.
> > > > >
> > > > > >
> > > > > > 2) Specify that a usermodehelper should attempt to use a context rather than
> > > > > > the default root context. The context used would be taken from the "init"
> > > > > > process of the current pid_namespace. Either that init_process itself could
> > > > > > be asked to fork/execve or when the pid_namespace is created a separate
> > > > > > helper thread is reserved.
> > > > >
> > > > > I think this is doable using open()/setns() in a similar way to
> > > > > nsenter(1). We can worry about simplifying it once we have a viable
> > > > > approach to work from.
> > > > >
> > > > > The reality is that now user mode helpers are executed within the root
> > > > > context of init so I can't see why we can't use the context of init of
> > > > > the container for this.
> > > > >
> > > > > Modifying that along the way with a "struct cred" is probably a good
> > > > > idea although it isn't done now for user mode callbacks. The "struct
> > > > > cred" of the root init process surely isn't what needs to be used when
> > > > > executing in a container so something needs to be done. If we duplicate
> > > > > the same behaviour we have now for execution outside of a container then
> > > > > we'd use the "struct cred" of the container init process so maybe we do
> > > > > know where to get the cred, not sure about that though.
> > > >
> > > > I'm not following you entirely here. Do you mean that the helper should
> > > > probably have the container init's cred stripped off or sanitized?
> > >
> > > LOL, that's good question.
> > >
> > > What I think I'm saying is that, when the usermode helper is run we
> > > don't want to use root init's credentials but some other credentials
> > > relevant to the container, possibly the credentials of the mounter or
> > > nfsd process credentials or the container init credentials.
> > >
> > > In any case they will need to be set to something different and
> > > appropriate. I'm not sure how to do that just yet.
> > >
> >
> > Yes, I think we might need to step back and consider that we have a
> > number of different use cases here, most of which are currently not
> > well served.
>
> Indeed yes, and what we got was the result I expected from the initial
> post of the patches for this, so, I am, ;)
>
> >
> > For instance: module loading clearly needs to be done in the "context"
> > of the canonical root init process. That's what call_usermodehelper was
> > originally used for so we need to keep that ability intact.
>
> Not sure that's an issue since the original call_usermodehelper() will
> be left in tact and people will need to make a conscious decision to
> call what, so far, is call_usermodehelper_ns() to exec within a
> container. At least that's the plan.
>
> >
> > OTOH, keyring upcalls probably ought to be done in the context of the
> > task that triggered them. Certainly we ought to be spawning them with
> > the credentials associated with the keyring.
>
> Yes, but I'm not really there yet so I can't make sensible comments
> about it.
>
> >
> > Today, those tasks not only run in the namespaces, etc of the root init
> > process, but also with with root's creds. That's unnecessary and seems
> > wrong. I think it's something that ought to be changed (though doing so
> > will likely be painful as we'll need to change the upcall programs to
> > handle that).
>
> One thing I believe is that user space programs shouldn't know or need
> to to know they are running within a container, I believe this should
> have been part of the namespace implementation from the start.
>
> The creds issue is what I'm trying to understand now since I've not had
> to concern myself with these before I'm a bit at sea. It may prove not
> doable but then maybe not.
>
> >
> > There are also other questions:
> >
> > How should we go about spawning the binary given that we might want to
> > have it run in a different mount namespace? There are at least two
> > options:
>
> If anything the response to the initial post of these patches showed
> that we can't just consider the mount namespace we need to consider the
> whole process environment.
>
> >
> > 1) change the mount namespace first and then exec the binary (in effect
> > run the binary with the given path from inside the container). This is
> > possibly a security hole if an attacker can trick the kernel into
> > running a different binary than intended by manipulating namespaces.
>
> I believe this has to be the way it's done, after sub-process creation
> and before the exec, in the user mode helper runner.
>
> >
> > ...or...
> >
> > 2) find and exec the binary and then change the namespaces afterward.
> > This has some potential problems if the program does something like
> > try to dlopen libraries after setns(). You could end up with a mismatch
> > if the container holds a different set of binaries from the one in the
> > root container.
>
> We really shouldn't need to change the user space binaries, I'd like to
> try to avoid that if at all possible.
>
> When I've referred to setns() here I'm thinking of an in kernel
> equivalent not the user space setns() syscall and that wasn't clear,
> sorry.
>
Yeah, I grokked that and I think setting up the process context before
exec is the right thing to do, but you still have a similar problem
there too...
Userland libraries for dynamically linked binaries are loaded by the
dynamic loader in userland. If you load the base binary from the
root container and then switch to a "child" container, how do you
guarantee that the binary has the libraries it needs when it wants to
run in the container?
Maybe that's a userspace problem ;)
--
Jeff Layton <[email protected]>
On Thu, 2014-12-11 at 08:46 -0500, Jeff Layton wrote:
> On Thu, 11 Dec 2014 20:55:42 +0800
> Ian Kent <[email protected]> wrote:
>
> > On Thu, 2014-12-11 at 06:45 -0500, Jeff Layton wrote:
> > > On Thu, 11 Dec 2014 11:21:21 +0800
> > > Ian Kent <[email protected]> wrote:
> > >
> > > > On Wed, 2014-12-10 at 20:54 -0500, Benjamin Coddington wrote:
> > > > >
> > > > > On Thu, 11 Dec 2014, Ian Kent wrote:
> > > > >
> > > > > > On Wed, 2014-12-10 at 18:21 -0500, Benjamin Coddington wrote:
> > > > > > > On Wed, 10 Dec 2014, David Howells wrote:
> > > > > > >
> > > > > > > > Jeff Layton <[email protected]> wrote:
> > > > > > > >
> > > > > > > > > > This thread might be interesting:
> > > > > > > > > > https://lkml.org/lkml/2014/11/24/885
> > > > > > > > > >
> > > > > > > > >
> > > > > > > > > Nice. I wasn't aware that Ian was working on this. I'll take a look.
> > > > > > > >
> > > > > > > > I'm not sure what the current state of this is. There was some discussion
> > > > > > > > over how best to determine which container we need to run in - and it's
> > > > > > > > complicated by the fact that the mounter may run in a different container to
> > > > > > > > the program that triggered the mount due to mountpoint propagation.
> > > > > > > >
> > > > > > > > David
> > > > > > >
> > > > > > > The specific problem of how to run /sbin/request-key in the caller's
> > > > > > > "container" for idmap and gssd (and other friends) became more generally a
> > > > > > > problem of how to solve the namespace (or more generally again, "context")
> > > > > > > problem for some users of kmod's call_usermodehelper. The nice thing about
> > > > > > > call_usermodehelper is that you don't have to do a lot of work to set up a
> > > > > > > process to get something done in userspace -- however it is sounding more
> > > > > > > like we do need to work hard to set up context for some users.
> > > > > > >
> > > > > > > The userspace work needs to be done within a context that currently exists
> > > > > > > or once existed, so the questions are where do we get that context and how
> > > > > > > do we keep it around until we need it?
> > > > > > >
> > > > > > > I think there's agreement that the setup of that context should be basically
> > > > > > > what's done in fork() for consistency and future work. So we get LSM and
> > > > > > > cgroups, etc.. in addition to namespaces.
> > > > > >
> > > > > > And that's when the usermode helper init function is called, just before
> > > > > > the exec, so I think that's the place it needs to be done.
> > > > > >
> > > > > > >
> > > > > > > There are two suggested approaches:
> > > > > > >
> > > > > > > 1) Anytime we think we're going to later need to upcall with a context we
> > > > > > > fork and keep a thread around to do that work. For NFS, that would look
> > > > > > > like forking a thread for every mount at mount time. The user of this API
> > > > > > > would be responsible for creating/maintaining the thread and passing it
> > > > > > > along for work.
> > > > > >
> > > > > > Yeah, I don't think that's workable for large numbers of mounts and I
> > > > > > don't think it's really necessary.
> > > > > >
> > > > > > >
> > > > > > > 2) Specify that a usermodehelper should attempt to use a context rather than
> > > > > > > the default root context. The context used would be taken from the "init"
> > > > > > > process of the current pid_namespace. Either that init_process itself could
> > > > > > > be asked to fork/execve or when the pid_namespace is created a separate
> > > > > > > helper thread is reserved.
> > > > > >
> > > > > > I think this is doable using open()/setns() in a similar way to
> > > > > > nsenter(1). We can worry about simplifying it once we have a viable
> > > > > > approach to work from.
> > > > > >
> > > > > > The reality is that now user mode helpers are executed within the root
> > > > > > context of init so I can't see why we can't use the context of init of
> > > > > > the container for this.
> > > > > >
> > > > > > Modifying that along the way with a "struct cred" is probably a good
> > > > > > idea although it isn't done now for user mode callbacks. The "struct
> > > > > > cred" of the root init process surely isn't what needs to be used when
> > > > > > executing in a container so something needs to be done. If we duplicate
> > > > > > the same behaviour we have now for execution outside of a container then
> > > > > > we'd use the "struct cred" of the container init process so maybe we do
> > > > > > know where to get the cred, not sure about that though.
> > > > >
> > > > > I'm not following you entirely here. Do you mean that the helper should
> > > > > probably have the container init's cred stripped off or sanitized?
> > > >
> > > > LOL, that's good question.
> > > >
> > > > What I think I'm saying is that, when the usermode helper is run we
> > > > don't want to use root init's credentials but some other credentials
> > > > relevant to the container, possibly the credentials of the mounter or
> > > > nfsd process credentials or the container init credentials.
> > > >
> > > > In any case they will need to be set to something different and
> > > > appropriate. I'm not sure how to do that just yet.
> > > >
> > >
> > > Yes, I think we might need to step back and consider that we have a
> > > number of different use cases here, most of which are currently not
> > > well served.
> >
> > Indeed yes, and what we got was the result I expected from the initial
> > post of the patches for this, so, I am, ;)
> >
> > >
> > > For instance: module loading clearly needs to be done in the "context"
> > > of the canonical root init process. That's what call_usermodehelper was
> > > originally used for so we need to keep that ability intact.
> >
> > Not sure that's an issue since the original call_usermodehelper() will
> > be left in tact and people will need to make a conscious decision to
> > call what, so far, is call_usermodehelper_ns() to exec within a
> > container. At least that's the plan.
> >
> > >
> > > OTOH, keyring upcalls probably ought to be done in the context of the
> > > task that triggered them. Certainly we ought to be spawning them with
> > > the credentials associated with the keyring.
> >
> > Yes, but I'm not really there yet so I can't make sensible comments
> > about it.
> >
> > >
> > > Today, those tasks not only run in the namespaces, etc of the root init
> > > process, but also with with root's creds. That's unnecessary and seems
> > > wrong. I think it's something that ought to be changed (though doing so
> > > will likely be painful as we'll need to change the upcall programs to
> > > handle that).
> >
> > One thing I believe is that user space programs shouldn't know or need
> > to to know they are running within a container, I believe this should
> > have been part of the namespace implementation from the start.
> >
> > The creds issue is what I'm trying to understand now since I've not had
> > to concern myself with these before I'm a bit at sea. It may prove not
> > doable but then maybe not.
> >
> > >
> > > There are also other questions:
> > >
> > > How should we go about spawning the binary given that we might want to
> > > have it run in a different mount namespace? There are at least two
> > > options:
> >
> > If anything the response to the initial post of these patches showed
> > that we can't just consider the mount namespace we need to consider the
> > whole process environment.
> >
> > >
> > > 1) change the mount namespace first and then exec the binary (in effect
> > > run the binary with the given path from inside the container). This is
> > > possibly a security hole if an attacker can trick the kernel into
> > > running a different binary than intended by manipulating namespaces.
> >
> > I believe this has to be the way it's done, after sub-process creation
> > and before the exec, in the user mode helper runner.
> >
> > >
> > > ...or...
> > >
> > > 2) find and exec the binary and then change the namespaces afterward.
> > > This has some potential problems if the program does something like
> > > try to dlopen libraries after setns(). You could end up with a mismatch
> > > if the container holds a different set of binaries from the one in the
> > > root container.
> >
> > We really shouldn't need to change the user space binaries, I'd like to
> > try to avoid that if at all possible.
> >
> > When I've referred to setns() here I'm thinking of an in kernel
> > equivalent not the user space setns() syscall and that wasn't clear,
> > sorry.
> >
>
> Yeah, I grokked that and I think setting up the process context before
> exec is the right thing to do, but you still have a similar problem
> there too...
>
> Userland libraries for dynamically linked binaries are loaded by the
> dynamic loader in userland. If you load the base binary from the
> root container and then switch to a "child" container, how do you
> guarantee that the binary has the libraries it needs when it wants to
> run in the container?
I'll need to keep this in mind for later.
Atm I'm assuming this cross container case doesn't occur in that the
user mode helper is called, pid of init of caller namespace is captured,
user mode helper runner sets up environment based on that pid after
process creation but before exec of the binary. Together with
appropriate process creds, chosen by the caller, I'm assuming that's
enough. Obtaining correct process creds might be more difficult than I
expect, don't know yet.
>
> Maybe that's a userspace problem ;)
On Thu, Dec 11, 2014 at 06:45:37AM -0500, Jeff Layton wrote:
> For instance: module loading clearly needs to be done in the "context"
> of the canonical root init process. That's what call_usermodehelper was
> originally used for so we need to keep that ability intact.
>
> OTOH, keyring upcalls probably ought to be done in the context of the
> task that triggered them. Certainly we ought to be spawning them with
> the credentials associated with the keyring.
Isn't the idmapping done as a keyring upcall? You don't want ordinary
users of the filesystem to be able to pollute the id<->name cache.
And in the gssd case, userland doesn't just find the right cred, it also
does the rpcsec_gss context setup with the server. A random user of the
filesystem might not be able to do that part.
--b.
>
> Today, those tasks not only run in the namespaces, etc of the root init
> process, but also with with root's creds. That's unnecessary and seems
> wrong. I think it's something that ought to be changed (though doing so
> will likely be painful as we'll need to change the upcall programs to
> handle that).
>
> There are also other questions:
>
> How should we go about spawning the binary given that we might want to
> have it run in a different mount namespace? There are at least two
> options:
>
> 1) change the mount namespace first and then exec the binary (in effect
> run the binary with the given path from inside the container). This is
> possibly a security hole if an attacker can trick the kernel into
> running a different binary than intended by manipulating namespaces.
>
> ...or...
>
> 2) find and exec the binary and then change the namespaces afterward.
> This has some potential problems if the program does something like
> try to dlopen libraries after setns(). You could end up with a mismatch
> if the container holds a different set of binaries from the one in the
> root container.
>
> --
> Jeff Layton <[email protected]>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, 11 Dec 2014 14:32:40 -0500
"J. Bruce Fields" <[email protected]> wrote:
> On Thu, Dec 11, 2014 at 06:45:37AM -0500, Jeff Layton wrote:
> > For instance: module loading clearly needs to be done in the "context"
> > of the canonical root init process. That's what call_usermodehelper was
> > originally used for so we need to keep that ability intact.
> >
> > OTOH, keyring upcalls probably ought to be done in the context of the
> > task that triggered them. Certainly we ought to be spawning them with
> > the credentials associated with the keyring.
>
> Isn't the idmapping done as a keyring upcall? You don't want ordinary
> users of the filesystem to be able to pollute the id<->name cache.
>
Yes, it is done as a keyring upcall. You wouldn't need to allow random
users to pollute the cache. When you do a keys upcall the process gets
an authorization key that allows it to instantiate the key.
AFAIU, that's what guards against random processes polluting the
keyring, not any particular capabilities or anything.
> And in the gssd case, userland doesn't just find the right cred, it also
> does the rpcsec_gss context setup with the server. A random user of the
> filesystem might not be able to do that part.
>
I don't see why not. We already do that today as an unprivileged user
in gssd. process_krb5_upcall forks and changes identity before getting
a ticket and setting up the context and then handing that back to the
kernel. I don't think that requires any special privileges.
--
Jeff Layton <[email protected]>
On Thu, Dec 11, 2014 at 02:50:29PM -0500, Jeff Layton wrote:
> On Thu, 11 Dec 2014 14:32:40 -0500
> "J. Bruce Fields" <[email protected]> wrote:
>
> > On Thu, Dec 11, 2014 at 06:45:37AM -0500, Jeff Layton wrote:
> > > For instance: module loading clearly needs to be done in the "context"
> > > of the canonical root init process. That's what call_usermodehelper was
> > > originally used for so we need to keep that ability intact.
> > >
> > > OTOH, keyring upcalls probably ought to be done in the context of the
> > > task that triggered them. Certainly we ought to be spawning them with
> > > the credentials associated with the keyring.
> >
> > Isn't the idmapping done as a keyring upcall? You don't want ordinary
> > users of the filesystem to be able to pollute the id<->name cache.
> >
>
> Yes, it is done as a keyring upcall. You wouldn't need to allow random
> users to pollute the cache. When you do a keys upcall the process gets
> an authorization key that allows it to instantiate the key.
>
> AFAIU, that's what guards against random processes polluting the
> keyring, not any particular capabilities or anything.
That doesn't stop it from instantiating the key with the wrong
information.
> > And in the gssd case, userland doesn't just find the right cred, it also
> > does the rpcsec_gss context setup with the server. A random user of the
> > filesystem might not be able to do that part.
> >
>
> I don't see why not. We already do that today as an unprivileged user
> in gssd. process_krb5_upcall forks and changes identity before getting
> a ticket and setting up the context and then handing that back to the
> kernel. I don't think that requires any special privileges.
Why couldn't you give a task access to an nfs filesystem but wall it off
in a network namespace where it doesn't even have access to the
interface that the mount was done over?
(And similarly what's to guarantee that a user of the filesystem is
capable of doing the ldap calls you might need for idmapping?)
--b.
On Thu, 11 Dec 2014 14:55:27 -0500
"J. Bruce Fields" <[email protected]> wrote:
> On Thu, Dec 11, 2014 at 02:50:29PM -0500, Jeff Layton wrote:
> > On Thu, 11 Dec 2014 14:32:40 -0500
> > "J. Bruce Fields" <[email protected]> wrote:
> >
> > > On Thu, Dec 11, 2014 at 06:45:37AM -0500, Jeff Layton wrote:
> > > > For instance: module loading clearly needs to be done in the "context"
> > > > of the canonical root init process. That's what call_usermodehelper was
> > > > originally used for so we need to keep that ability intact.
> > > >
> > > > OTOH, keyring upcalls probably ought to be done in the context of the
> > > > task that triggered them. Certainly we ought to be spawning them with
> > > > the credentials associated with the keyring.
> > >
> > > Isn't the idmapping done as a keyring upcall? You don't want ordinary
> > > users of the filesystem to be able to pollute the id<->name cache.
> > >
> >
> > Yes, it is done as a keyring upcall. You wouldn't need to allow random
> > users to pollute the cache. When you do a keys upcall the process gets
> > an authorization key that allows it to instantiate the key.
> >
> > AFAIU, that's what guards against random processes polluting the
> > keyring, not any particular capabilities or anything.
>
> That doesn't stop it from instantiating the key with the wrong
> information.
>
I guess I'm unclear on the attack vector you're talking about. The
difference is the credentials that we give the process when its run by
call_usermodehelper. Why would it be inherently more secure for that to
be given root credentials rather than something non-privileged?
The only way you can add keys to a keyring you don't own is to have an
authorization key, and that would only be given to the process that got
spawned by the kernel.
> > > And in the gssd case, userland doesn't just find the right cred, it also
> > > does the rpcsec_gss context setup with the server. A random user of the
> > > filesystem might not be able to do that part.
> > >
> >
> > I don't see why not. We already do that today as an unprivileged user
> > in gssd. process_krb5_upcall forks and changes identity before getting
> > a ticket and setting up the context and then handing that back to the
> > kernel. I don't think that requires any special privileges.
>
> Why couldn't you give a task access to an nfs filesystem but wall it off
> in a network namespace where it doesn't even have access to the
> interface that the mount was done over?
>
That's a pathological case. ;) I suppose you could do that, at which
point you'd be screwed. That's the unfortunate side of having all of
these disconnected types of namespaces. You can use these Lego bricks
to build something either awesome or completely non-functional. I don't
think we can really solve all possible use-cases since some of them are
non-sensical anyway. I think all we can do is target the use-cases that
we think make sense and take it from there.
While we're on the subject, having the userland process establish the
GSS context over an entirely separate connection is a hack anyway. We
really ought to be doing that using the same connection. Simo had some
arguments against that scheme a while back, but I don't recall the
details -- seems like maybe it breaks channel bindings? While we're
talking about rejiggering all of this, that would be a good thing to
change as well.
> (And similarly what's to guarantee that a user of the filesystem is
> capable of doing the ldap calls you might need for idmapping?)
>
Yeah, that certainly could be a problem. I'm not sure we'd really want
to do the idmapping upcalls as the user that triggered them in the case
of the idmapper. Perhaps there ought to be a specific set of
credentials associated with the keyring that the idmapper uses instead?
Those don't necessarily need to be root creds of course.
--
Jeff Layton <[email protected]>
On Thu, Dec 11, 2014 at 03:11:35PM -0500, Jeff Layton wrote:
> On Thu, 11 Dec 2014 14:55:27 -0500
> "J. Bruce Fields" <[email protected]> wrote:
>
> > On Thu, Dec 11, 2014 at 02:50:29PM -0500, Jeff Layton wrote:
> > > On Thu, 11 Dec 2014 14:32:40 -0500
> > > "J. Bruce Fields" <[email protected]> wrote:
> > >
> > > > On Thu, Dec 11, 2014 at 06:45:37AM -0500, Jeff Layton wrote:
> > > > > For instance: module loading clearly needs to be done in the "context"
> > > > > of the canonical root init process. That's what call_usermodehelper was
> > > > > originally used for so we need to keep that ability intact.
> > > > >
> > > > > OTOH, keyring upcalls probably ought to be done in the context of the
> > > > > task that triggered them. Certainly we ought to be spawning them with
> > > > > the credentials associated with the keyring.
> > > >
> > > > Isn't the idmapping done as a keyring upcall? You don't want ordinary
> > > > users of the filesystem to be able to pollute the id<->name cache.
> > > >
> > >
> > > Yes, it is done as a keyring upcall. You wouldn't need to allow random
> > > users to pollute the cache. When you do a keys upcall the process gets
> > > an authorization key that allows it to instantiate the key.
> > >
> > > AFAIU, that's what guards against random processes polluting the
> > > keyring, not any particular capabilities or anything.
> >
> > That doesn't stop it from instantiating the key with the wrong
> > information.
> >
>
> I guess I'm unclear on the attack vector you're talking about. The
> difference is the credentials that we give the process when its run by
> call_usermodehelper. Why would it be inherently more secure for that to
> be given root credentials rather than something non-privileged?
>
> The only way you can add keys to a keyring you don't own is to have an
> authorization key, and that would only be given to the process that got
> spawned by the kernel.
OK. I was interpreting "keyring upcalls probably out to be done in the
context of the task that triggered them" to mean they should be done
with all the various namespaces of that task, which would among other
things give unprivileged users control over the mapping.
> > > > And in the gssd case, userland doesn't just find the right cred, it also
> > > > does the rpcsec_gss context setup with the server. A random user of the
> > > > filesystem might not be able to do that part.
> > > >
> > >
> > > I don't see why not. We already do that today as an unprivileged user
> > > in gssd. process_krb5_upcall forks and changes identity before getting
> > > a ticket and setting up the context and then handing that back to the
> > > kernel. I don't think that requires any special privileges.
> >
> > Why couldn't you give a task access to an nfs filesystem but wall it off
> > in a network namespace where it doesn't even have access to the
> > interface that the mount was done over?
> >
>
> That's a pathological case. ;) I suppose you could do that, at which
> point you'd be screwed. That's the unfortunate side of having all of
> these disconnected types of namespaces. You can use these Lego bricks
> to build something either awesome or completely non-functional. I don't
> think we can really solve all possible use-cases since some of them are
> non-sensical anyway. I think all we can do is target the use-cases that
> we think make sense and take it from there.
I think it's pretty normal to sandbox tasks to deny them network access,
and I wouldn't expect that to mean giving up access to mounted nfs
filesystems.
> While we're on the subject, having the userland process establish the
> GSS context over an entirely separate connection is a hack anyway. We
> really ought to be doing that using the same connection. Simo had some
> arguments against that scheme a while back, but I don't recall the
> details -- seems like maybe it breaks channel bindings? While we're
> talking about rejiggering all of this, that would be a good thing to
> change as well.
It was Simo that wanted to move the context establishment into kernel
sunrpc code and Trond that objected to that. And yes one of the only
practical differences we could come up with is that having it in the
kernel and sharing the same connection would allow channel bindings.
But nobody seems to care about channel bindings for now.
It's a long-running argument--the first gssd prototypes basically did
gssapi over rpc (a little like gssproxy) and it was Trond that wanted
the whole negotiation done in userspace.
--b.
> > (And similarly what's to guarantee that a user of the filesystem is
> > capable of doing the ldap calls you might need for idmapping?)
> >
>
> Yeah, that certainly could be a problem. I'm not sure we'd really want
> to do the idmapping upcalls as the user that triggered them in the case
> of the idmapper. Perhaps there ought to be a specific set of
> credentials associated with the keyring that the idmapper uses instead?
> Those don't necessarily need to be root creds of course.
On Thu, 2014-12-11 at 15:38 -0500, J. Bruce Fields wrote:
> On Thu, Dec 11, 2014 at 03:11:35PM -0500, Jeff Layton wrote:
> > On Thu, 11 Dec 2014 14:55:27 -0500
> > "J. Bruce Fields" <[email protected]> wrote:
> >
> > > On Thu, Dec 11, 2014 at 02:50:29PM -0500, Jeff Layton wrote:
> > > > On Thu, 11 Dec 2014 14:32:40 -0500
> > > > "J. Bruce Fields" <[email protected]> wrote:
> > > >
> > > > > On Thu, Dec 11, 2014 at 06:45:37AM -0500, Jeff Layton wrote:
> > > > > > For instance: module loading clearly needs to be done in the "context"
> > > > > > of the canonical root init process. That's what call_usermodehelper was
> > > > > > originally used for so we need to keep that ability intact.
> > > > > >
> > > > > > OTOH, keyring upcalls probably ought to be done in the context of the
> > > > > > task that triggered them. Certainly we ought to be spawning them with
> > > > > > the credentials associated with the keyring.
> > > > >
> > > > > Isn't the idmapping done as a keyring upcall? You don't want ordinary
> > > > > users of the filesystem to be able to pollute the id<->name cache.
> > > > >
> > > >
> > > > Yes, it is done as a keyring upcall. You wouldn't need to allow random
> > > > users to pollute the cache. When you do a keys upcall the process gets
> > > > an authorization key that allows it to instantiate the key.
> > > >
> > > > AFAIU, that's what guards against random processes polluting the
> > > > keyring, not any particular capabilities or anything.
> > >
> > > That doesn't stop it from instantiating the key with the wrong
> > > information.
> > >
> >
> > I guess I'm unclear on the attack vector you're talking about. The
> > difference is the credentials that we give the process when its run by
> > call_usermodehelper. Why would it be inherently more secure for that to
> > be given root credentials rather than something non-privileged?
> >
> > The only way you can add keys to a keyring you don't own is to have an
> > authorization key, and that would only be given to the process that got
> > spawned by the kernel.
>
> OK. I was interpreting "keyring upcalls probably out to be done in the
> context of the task that triggered them" to mean they should be done
> with all the various namespaces of that task, which would among other
> things give unprivileged users control over the mapping.
TBH I think it's too early to decide these issues.
So far I've been thinking the "struct cred", as in process credentials,
should be chosen by the caller. And I'm assuming that's sufficient
although it may not be.
The important thing is to first work out how to execute a binary within
container context in a way that satisfies the security requirements that
previous attempts didn't. Then step back and work out what we have to do
for the specific use cases while maintaining that requirement.
>
> > > > > And in the gssd case, userland doesn't just find the right cred, it also
> > > > > does the rpcsec_gss context setup with the server. A random user of the
> > > > > filesystem might not be able to do that part.
> > > > >
> > > >
> > > > I don't see why not. We already do that today as an unprivileged user
> > > > in gssd. process_krb5_upcall forks and changes identity before getting
> > > > a ticket and setting up the context and then handing that back to the
> > > > kernel. I don't think that requires any special privileges.
> > >
> > > Why couldn't you give a task access to an nfs filesystem but wall it off
> > > in a network namespace where it doesn't even have access to the
> > > interface that the mount was done over?
> > >
> >
> > That's a pathological case. ;) I suppose you could do that, at which
> > point you'd be screwed. That's the unfortunate side of having all of
> > these disconnected types of namespaces. You can use these Lego bricks
> > to build something either awesome or completely non-functional. I don't
> > think we can really solve all possible use-cases since some of them are
> > non-sensical anyway. I think all we can do is target the use-cases that
> > we think make sense and take it from there.
>
> I think it's pretty normal to sandbox tasks to deny them network access,
> and I wouldn't expect that to mean giving up access to mounted nfs
> filesystems.
>
> > While we're on the subject, having the userland process establish the
> > GSS context over an entirely separate connection is a hack anyway. We
> > really ought to be doing that using the same connection. Simo had some
> > arguments against that scheme a while back, but I don't recall the
> > details -- seems like maybe it breaks channel bindings? While we're
> > talking about rejiggering all of this, that would be a good thing to
> > change as well.
>
> It was Simo that wanted to move the context establishment into kernel
> sunrpc code and Trond that objected to that. And yes one of the only
> practical differences we could come up with is that having it in the
> kernel and sharing the same connection would allow channel bindings.
> But nobody seems to care about channel bindings for now.
>
> It's a long-running argument--the first gssd prototypes basically did
> gssapi over rpc (a little like gssproxy) and it was Trond that wanted
> the whole negotiation done in userspace.
>
> --b.
>
> > > (And similarly what's to guarantee that a user of the filesystem is
> > > capable of doing the ldap calls you might need for idmapping?)
> > >
> >
> > Yeah, that certainly could be a problem. I'm not sure we'd really want
> > to do the idmapping upcalls as the user that triggered them in the case
> > of the idmapper. Perhaps there ought to be a specific set of
> > credentials associated with the keyring that the idmapper uses instead?
> > Those don't necessarily need to be root creds of course.
Hello,
On 12/09/2014 12:40 AM, David Härdeman wrote:
> The following series converts gssd to use libevent and inotify instead
> of a handrolled event loop and dnotify. Lots of cleanups in the process
> (e.g. removing a lot of arbitrary limitations and fixed size buffers).
>
> All in all a nice reduction in code size (what can I say, I was bored).
I just have to asked... Does this patch set solve a problem? Fix a Bug?
I know you said you were bored :-) but what was your motivation?
The reason I ask is this patch set just scream out to me were "fixing
something that is not broken". Plus rewrites like this eliminate years
of testing and stability, so we can't take it lightly. gssd is now
an important part of all nfs client mounts...
That said, I did read through the set and there is definitely some
good/needed cleanup as well some superfluous changes which is fine..
Its obvious you do have a clue and you spent some time on them..
So by no means I am against these patches. I guess I need a reason
to apply them... ;-) What do they fix? Are these patches leading use
down to a better place? Is there a noticeable performance gain?
Finally, why the "change dnotify to inotify" a good thing?
>
> I've even managed to mount NFS shares with the patched server :)
Was that mount at least a secure mount? ;-) Seriously was that all
the testing that done?
Again, thank you for your time!
steved.
>
> ---
>
> David Härdeman (19):
> nfs-utils: cleanup daemonization code
> nfs-utils: gssd - merge gssd_main_loop.c and gssd.c
> nfs-utils: gssd - simplify some option handling
> nfs-utils: gssd - remove arbitrary GSSD_MAX_CCACHE_SEARCH limitation
> nfs-utils: gssd - simplify topdirs path
> nfs-utils: gssd - move over pipfs scanning code
> nfs-utils: gssd - simplify client dir scanning code
> nfs-utils: gssd - use libevent
> nfs-utils: gssd - remove "close me" code
> nfs-utils: gssd - make the client lists per-topdir
> nfs-utils: gssd - keep the rpc_pipefs dir open
> nfs-utils: gssd - use more relative paths
> nfs-utils: gssd - simplify topdir scanning
> nfs-utils: gssd - simplify client scanning
> nfs-utils: gssd - cleanup read_service_info
> nfs-utils: gssd - change dnotify to inotify
> nfs-utils: gssd - further shorten some pathnames
> nfs-utils: gssd - improve inotify
> nfs-utils: gssd - simplify handle_gssd_upcall
>
>
> support/include/nfslib.h | 5
> support/nfs/mydaemon.c | 92 +++--
> utils/gssd/Makefile.am | 24 +
> utils/gssd/gss_util.h | 2
> utils/gssd/gssd.c | 785 +++++++++++++++++++++++++++++++++++++++++--
> utils/gssd/gssd.h | 46 +--
> utils/gssd/gssd_main_loop.c | 263 --------------
> utils/gssd/gssd_proc.c | 654 ++----------------------------------
> utils/gssd/svcgssd.c | 8
> utils/idmapd/idmapd.c | 6
> utils/statd/statd.c | 66 +---
> 11 files changed, 878 insertions(+), 1073 deletions(-)
> delete mode 100644 utils/gssd/gssd_main_loop.c
>
> --
> David Härdeman
>
On Tue, Dec 09, 2014 at 11:39:59AM -0500, Steve Dickson wrote:
>Hello,
>
>On 12/09/2014 12:40 AM, David H?rdeman wrote:
>> The following series converts gssd to use libevent and inotify instead
>> of a handrolled event loop and dnotify. Lots of cleanups in the process
>> (e.g. removing a lot of arbitrary limitations and fixed size buffers).
>>
>> All in all a nice reduction in code size (what can I say, I was bored).
>
>I just have to asked... Does this patch set solve a problem? Fix a Bug?
>I know you said you were bored :-) but what was your motivation?
The starting point was/is that I already have a working nfs4/krb5 setup
and I want to add a couple of OpenELEC clients to my network. OpenELEC
doesn't support NFSv4 today and it doesn't support krb5 (both idmap and
gssd are unavailable). So I started mukcing about trying to provide an
OpenELEC nfs-utils package...as part of that I reviewed the gssd
code...and I just got caught up in the moment :)
>The reason I ask is this patch set just scream out to me were "fixing
>something that is not broken".
It's not broken as far as I can tell (only things that appeared to be so
were: the TAILQ_* macros which have no safe version of TAILQ_FOREACH
which allows list manipulation, signals that might cause lots of -EINTR
from various syscalls and a general overreliance on fixed length buffers
(boo).
The TAILQ thing isn't solved by my patch but that's on my radar for the
future.
>Plus rewrites like this eliminate years
>of testing and stability, so we can't take it lightly. gssd is now
>an important part of all nfs client mounts...
Agreed. Though I believe regressions would be noticed rather
quickly...and the ensuing screams would be rather loud? I might be
mistaken though...
>That said, I did read through the set and there is definitely some
>good/needed cleanup as well some superfluous changes which is fine..
Yes, kinda hard to avoid the superfluous stuff when you're mucking about
with everything else...at least for me...
>Its obvious you do have a clue and you spent some time on them..
Starting to sound like a job posting :)
>So by no means I am against these patches. I guess I need a reason
>to apply them... ;-) What do they fix? Are these patches leading use
>down to a better place? Is there a noticeable performance gain?
I don't have the big iron to test the scenarios where there might be a
performance gain. I guess the important things to note are:
a) The old code does a complete rescan on every single change; and
b) The old code keeps one fd open for each directory
And...on a more objective level...the new code is more readable and
understandable...the old code was....less so (IMHO).
>Finally, why the "change dnotify to inotify" a good thing?
Supra.
>> I've even managed to mount NFS shares with the patched server :)
>Was that mount at least a secure mount? ;-)
Yep..
>Seriously was that all the testing that done?
Yep. It runs now in my network...but I have one server and maybe 2-3
clients on average...
>Again, thank you for your time!
NP...I understand your concerns :)
//David
Hello,
On 12/09/2014 03:22 PM, David H?rdeman wrote:
> On Tue, Dec 09, 2014 at 11:39:59AM -0500, Steve Dickson wrote:
>> Hello,
>>
>> On 12/09/2014 12:40 AM, David H?rdeman wrote:
>>> The following series converts gssd to use libevent and inotify instead
>>> of a handrolled event loop and dnotify. Lots of cleanups in the process
>>> (e.g. removing a lot of arbitrary limitations and fixed size buffers).
>>>
>>> All in all a nice reduction in code size (what can I say, I was bored).
>>
>> I just have to asked... Does this patch set solve a problem? Fix a Bug?
>> I know you said you were bored :-) but what was your motivation?
>
> The starting point was/is that I already have a working nfs4/krb5 setup
> and I want to add a couple of OpenELEC clients to my network. OpenELEC
> doesn't support NFSv4 today and it doesn't support krb5 (both idmap and
> gssd are unavailable). So I started mukcing about trying to provide an
> OpenELEC nfs-utils package...as part of that I reviewed the gssd
> code...and I just got caught up in the moment :)
Fair enough...
>
>> The reason I ask is this patch set just scream out to me were "fixing
>> something that is not broken".
>
> It's not broken as far as I can tell (only things that appeared to be so
> were: the TAILQ_* macros which have no safe version of TAILQ_FOREACH
> which allows list manipulation, signals that might cause lots of -EINTR
> from various syscalls and a general overreliance on fixed length buffers
> (boo).
>
> The TAILQ thing isn't solved by my patch but that's on my radar for the
> future.
I have not taken that close of a look.. but I will...
>
>> Plus rewrites like this eliminate years
>> of testing and stability, so we can't take it lightly. gssd is now
>> an important part of all nfs client mounts...
>
> Agreed. Though I believe regressions would be noticed rather
> quickly...and the ensuing screams would be rather loud? I might be
> mistaken though...
Yeah... They will be screaming at me! not you... 8-)
>
>> That said, I did read through the set and there is definitely some
>> good/needed cleanup as well some superfluous changes which is fine..
>
> Yes, kinda hard to avoid the superfluous stuff when you're mucking about
> with everything else...at least for me...
again fair enough...
>
>> Its obvious you do have a clue and you spent some time on them..
>
> Starting to sound like a job posting :)
It isn't... Just a complement...
>
>> So by no means I am against these patches. I guess I need a reason
>> to apply them... ;-) What do they fix? Are these patches leading use
>> down to a better place? Is there a noticeable performance gain?
>
> I don't have the big iron to test the scenarios where there might be a
> performance gain. I guess the important things to note are:
>
> a) The old code does a complete rescan on every single change; and
> b) The old code keeps one fd open for each directory
I did see that...
>
> And...on a more objective level...the new code is more readable and
> understandable...the old code was....less so (IMHO).
I did see a lot of code removal... but time will tell...
>
>> Finally, why the "change dnotify to inotify" a good thing?
>
> Supra.
??
>
>>> I've even managed to mount NFS shares with the patched server :)
>> Was that mount at least a secure mount? ;-)
>
> Yep..
>
>> Seriously was that all the testing that done?
>
> Yep. It runs now in my network...but I have one server and maybe 2-3
> clients on average...
OK..
steved.
On 2014-12-09 22:13, Steve Dickson wrote:
> On 12/09/2014 03:22 PM, David Härdeman wrote:
>> Agreed. Though I believe regressions would be noticed rather
>> quickly...and the ensuing screams would be rather loud? I might be
>> mistaken though...
>
> Yeah... They will be screaming at me! not you... 8-)
Heh, yeah, but my point was rather that any problem will (hopefully) be
spotted and reported rather quickly.
>>> Finally, why the "change dnotify to inotify" a good thing?
I was mostly referring to the EINTR thing...since dnotify is signal
based, there are a lot of functions that should be checked for EINTR and
restarted. Maybe they should anyway, but the need is less urgent with
inotify (the other advantages were the reduced number of fds and not
having to rescan the while rpc_pipefs hierarchy on every change).
On Tue, Dec 09, 2014 at 06:40:40AM +0100, David Härdeman wrote:
> The following series converts gssd to use libevent and inotify instead
> of a handrolled event loop and dnotify. Lots of cleanups in the process
> (e.g. removing a lot of arbitrary limitations and fixed size buffers).
>
> All in all a nice reduction in code size (what can I say, I was bored).
This code has been crying out for help for a while, thanks for taking a
look!
On dnotify/inotify: if I remember correctly, this was written before
inotify existed. So you're probably breaking compatibility with older
kernels, which we normally try not to do.
According to inotify(7):
Inotify was merged into the 2.6.13 Linux kernel. The required
library interfaces were added to glibc in version 2.4.
(IN_DONT_FOLLOW, IN_MASK_ADD, and IN_ONLYDIR were added in
version 2.5.)
So that's almost 10 years. In RHEL terms I think RHEL4 would be the
latest without inotify support.
OK, maybe that's long enough.
If there's an easy way to provide a fallback to dnotify, that would be
nice, otherwise would you mind just adding a note about this somewhere?
(Maybe the gssd man page should say something like "this version of gssd
depends on inotify, introduced in kernel version 2.6.13...".)
--b.
>
> I've even managed to mount NFS shares with the patched server :)
>
> ---
>
> David Härdeman (19):
> nfs-utils: cleanup daemonization code
> nfs-utils: gssd - merge gssd_main_loop.c and gssd.c
> nfs-utils: gssd - simplify some option handling
> nfs-utils: gssd - remove arbitrary GSSD_MAX_CCACHE_SEARCH limitation
> nfs-utils: gssd - simplify topdirs path
> nfs-utils: gssd - move over pipfs scanning code
> nfs-utils: gssd - simplify client dir scanning code
> nfs-utils: gssd - use libevent
> nfs-utils: gssd - remove "close me" code
> nfs-utils: gssd - make the client lists per-topdir
> nfs-utils: gssd - keep the rpc_pipefs dir open
> nfs-utils: gssd - use more relative paths
> nfs-utils: gssd - simplify topdir scanning
> nfs-utils: gssd - simplify client scanning
> nfs-utils: gssd - cleanup read_service_info
> nfs-utils: gssd - change dnotify to inotify
> nfs-utils: gssd - further shorten some pathnames
> nfs-utils: gssd - improve inotify
> nfs-utils: gssd - simplify handle_gssd_upcall
>
>
> support/include/nfslib.h | 5
> support/nfs/mydaemon.c | 92 +++--
> utils/gssd/Makefile.am | 24 +
> utils/gssd/gss_util.h | 2
> utils/gssd/gssd.c | 785 +++++++++++++++++++++++++++++++++++++++++--
> utils/gssd/gssd.h | 46 +--
> utils/gssd/gssd_main_loop.c | 263 --------------
> utils/gssd/gssd_proc.c | 654 ++----------------------------------
> utils/gssd/svcgssd.c | 8
> utils/idmapd/idmapd.c | 6
> utils/statd/statd.c | 66 +---
> 11 files changed, 878 insertions(+), 1073 deletions(-)
> delete mode 100644 utils/gssd/gssd_main_loop.c
>
> --
> David Härdeman
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Dec 10, 2014 at 03:35:11PM -0500, J. Bruce Fields wrote:
>On Tue, Dec 09, 2014 at 06:40:40AM +0100, David H?rdeman wrote:
>> The following series converts gssd to use libevent and inotify instead
>> of a handrolled event loop and dnotify. Lots of cleanups in the process
>> (e.g. removing a lot of arbitrary limitations and fixed size buffers).
>>
>> All in all a nice reduction in code size (what can I say, I was bored).
>
>This code has been crying out for help for a while, thanks for taking a
>look!
:)
>On dnotify/inotify: if I remember correctly, this was written before
>inotify existed. So you're probably breaking compatibility with older
>kernels, which we normally try not to do.
Fair enough...but...there has to be some kind of reasonable limit to
that. Like...unmaintained kernel versions with known CVEs...if an
organization is *that* conservative...I kinda doubt they'd update
nfs-utils either?
>According to inotify(7):
>
> Inotify was merged into the 2.6.13 Linux kernel. The required
> library interfaces were added to glibc in version 2.4.
> (IN_DONT_FOLLOW, IN_MASK_ADD, and IN_ONLYDIR were added in
> version 2.5.)
>
>So that's almost 10 years. In RHEL terms I think RHEL4 would be the
>latest without inotify support.
>
>OK, maybe that's long enough.
I should add that krb5 support was added on 13 January 2003,
(commit e0594725b51b5253237ed11b8bf3cf9ab87d9d48)
which corresponds to kernel version 2.5.57 or .58.
2.6.13 is from 29 August, 2005....so we'd kill support for that time
span...
>If there's an easy way to provide a fallback to dnotify, that would be
>nice
I think you and I have different ideas of the definition of the word
"nice" :)
>otherwise would you mind just adding a note about this somewhere?
>(Maybe the gssd man page should say something like "this version of gssd
>depends on inotify, introduced in kernel version 2.6.13...".)
I could add that in a separate patch...both to the changelog and the man
page.
--
David H?rdeman
On Wed, Dec 10, 2014 at 09:49:17PM +0100, David Härdeman wrote:
> On Wed, Dec 10, 2014 at 03:35:11PM -0500, J. Bruce Fields wrote:
> >On Tue, Dec 09, 2014 at 06:40:40AM +0100, David Härdeman wrote:
> >> The following series converts gssd to use libevent and inotify instead
> >> of a handrolled event loop and dnotify. Lots of cleanups in the process
> >> (e.g. removing a lot of arbitrary limitations and fixed size buffers).
> >>
> >> All in all a nice reduction in code size (what can I say, I was bored).
> >
> >This code has been crying out for help for a while, thanks for taking a
> >look!
>
> :)
>
> >On dnotify/inotify: if I remember correctly, this was written before
> >inotify existed. So you're probably breaking compatibility with older
> >kernels, which we normally try not to do.
>
> Fair enough...but...there has to be some kind of reasonable limit to
> that. Like...unmaintained kernel versions with known CVEs...if an
> organization is *that* conservative...I kinda doubt they'd update
> nfs-utils either?
Personally I worry more about troubleshooting: RHEL4 user runs into nfs
problem that looks like something we fixed in recent nfs-utils, we might
like to be able to tell them just to try latest nfs-utils....
But at this point I think that's unlikely.
> >According to inotify(7):
> >
> > Inotify was merged into the 2.6.13 Linux kernel. The required
> > library interfaces were added to glibc in version 2.4.
> > (IN_DONT_FOLLOW, IN_MASK_ADD, and IN_ONLYDIR were added in
> > version 2.5.)
> >
> >So that's almost 10 years. In RHEL terms I think RHEL4 would be the
> >latest without inotify support.
> >
> >OK, maybe that's long enough.
>
> I should add that krb5 support was added on 13 January 2003,
> (commit e0594725b51b5253237ed11b8bf3cf9ab87d9d48)
> which corresponds to kernel version 2.5.57 or .58.
>
> 2.6.13 is from 29 August, 2005....so we'd kill support for that time
> span...
>
> >If there's an easy way to provide a fallback to dnotify, that would be
> >nice
>
> I think you and I have different ideas of the definition of the word
> "nice" :)
>
> >otherwise would you mind just adding a note about this somewhere?
> >(Maybe the gssd man page should say something like "this version of gssd
> >depends on inotify, introduced in kernel version 2.6.13...".)
>
> I could add that in a separate patch...both to the changelog and the man
> page.
That'd be fine, thanks.--b.
On 12/09/2014 12:40 AM, David Härdeman wrote:
> The following series converts gssd to use libevent and inotify instead
> of a handrolled event loop and dnotify. Lots of cleanups in the process
> (e.g. removing a lot of arbitrary limitations and fixed size buffers).
>
> All in all a nice reduction in code size (what can I say, I was bored).
>
> I've even managed to mount NFS shares with the patched server :)
>
> ---
>
> David Härdeman (19):
> nfs-utils: cleanup daemonization code
> nfs-utils: gssd - merge gssd_main_loop.c and gssd.c
> nfs-utils: gssd - simplify some option handling
> nfs-utils: gssd - remove arbitrary GSSD_MAX_CCACHE_SEARCH limitation
> nfs-utils: gssd - simplify topdirs path
> nfs-utils: gssd - move over pipfs scanning code
> nfs-utils: gssd - simplify client dir scanning code
> nfs-utils: gssd - use libevent
> nfs-utils: gssd - remove "close me" code
> nfs-utils: gssd - make the client lists per-topdir
> nfs-utils: gssd - keep the rpc_pipefs dir open
> nfs-utils: gssd - use more relative paths
> nfs-utils: gssd - simplify topdir scanning
> nfs-utils: gssd - simplify client scanning
> nfs-utils: gssd - cleanup read_service_info
> nfs-utils: gssd - change dnotify to inotify
> nfs-utils: gssd - further shorten some pathnames
> nfs-utils: gssd - improve inotify
> nfs-utils: gssd - simplify handle_gssd_upcall
>
>
> support/include/nfslib.h | 5
> support/nfs/mydaemon.c | 92 +++--
> utils/gssd/Makefile.am | 24 +
> utils/gssd/gss_util.h | 2
> utils/gssd/gssd.c | 785 +++++++++++++++++++++++++++++++++++++++++--
> utils/gssd/gssd.h | 46 +--
> utils/gssd/gssd_main_loop.c | 263 --------------
> utils/gssd/gssd_proc.c | 654 ++----------------------------------
> utils/gssd/svcgssd.c | 8
> utils/idmapd/idmapd.c | 6
> utils/statd/statd.c | 66 +---
> 11 files changed, 878 insertions(+), 1073 deletions(-)
> delete mode 100644 utils/gssd/gssd_main_loop.c
These seem to hold up during the weekend testing... so they are committed!
steved.