On Tue, 25 May 2021, Steve Dickson wrote:
> Hey!
>
> Just a couple of nits...
>
> On 5/25/21 2:00 PM, Scott Mayhew wrote:
> > If we fail to create a thread to handle an upcall, we still need to do a
> > downcall to tell the kernel about the failure, otherwise the process
> > that is trying to establish gss credentials will hang.
> >
> > This patch shifts the thread creation down a level in the call chain so
> > now the main thread does a little more work up front (reading & parsing
> > the data from the pipefs file) so it has the info it needs to be able
> > to do the error downcall.
> >
> > Signed-off-by: Scott Mayhew <smayhew@xxxxxxxxxx>
> > ---
> > utils/gssd/gssd.c | 83 +-----------------
> > utils/gssd/gssd.h | 11 ++-
> > utils/gssd/gssd_proc.c | 190 +++++++++++++++++++++++++++++++++--------
> > 3 files changed, 166 insertions(+), 118 deletions(-)
> >
> > diff --git a/utils/gssd/gssd.c b/utils/gssd/gssd.c
> > index 1541d371..eb440470 100644
> > --- a/utils/gssd/gssd.c
> > +++ b/utils/gssd/gssd.c
> > @@ -364,7 +364,7 @@ out:
> > /* Actually frees clp and fields that might be used from other
> > * threads if was last reference.
> > */
> > -static void
> > +void
> > gssd_free_client(struct clnt_info *clp)
> > {
> > int refcnt;
> > @@ -416,55 +416,6 @@ gssd_destroy_client(struct clnt_info *clp)
> >
> > static void gssd_scan(void);
> >
> > -static int
> > -start_upcall_thread(void (*func)(struct clnt_upcall_info *), void *info)
> > -{
> > - pthread_attr_t attr;
> > - pthread_t th;
> > - int ret;
> > -
> > - ret = pthread_attr_init(&attr);
> > - if (ret != 0) {
> > - printerr(0, "ERROR: failed to init pthread attr: ret %d: %s\n",
> > - ret, strerror(errno));
> > - return ret;
> > - }
> > - ret = pthread_attr_setdetachstate(&attr, PTHREAD_CREATE_DETACHED);
> > - if (ret != 0) {
> > - printerr(0, "ERROR: failed to create pthread attr: ret %d: "
> > - "%s\n", ret, strerror(errno));
> > - return ret;
> > - }
> > -
> > - ret = pthread_create(&th, &attr, (void *)func, (void *)info);
> > - if (ret != 0)
> > - printerr(0, "ERROR: pthread_create failed: ret %d: %s\n",
> > - ret, strerror(errno));
> > - return ret;
> > -}
> > -
> > -static struct clnt_upcall_info *alloc_upcall_info(struct clnt_info *clp)
> > -{
> > - struct clnt_upcall_info *info;
> > -
> > - info = malloc(sizeof(struct clnt_upcall_info));
> > - if (info == NULL)
> > - return NULL;
> > -
> > - pthread_mutex_lock(&clp_lock);
> > - clp->refcount++;
> > - pthread_mutex_unlock(&clp_lock);
> > - info->clp = clp;
> > -
> > - return info;
> > -}
> > -
> > -void free_upcall_info(struct clnt_upcall_info *info)
> > -{
> > - gssd_free_client(info->clp);
> > - free(info);
> > -}
> > -
> > /* For each upcall read the upcall info into the buffer, then create a
> > * thread in a detached state so that resources are released back into
> > * the system without the need for a join.
> > @@ -473,44 +424,16 @@ static void
> > gssd_clnt_gssd_cb(int UNUSED(fd), short UNUSED(which), void *data)
> > {
> > struct clnt_info *clp = data;
> > - struct clnt_upcall_info *info;
> > -
> > - info = alloc_upcall_info(clp);
> > - if (info == NULL)
> > - return;
> >
> > - info->lbuflen = read(clp->gssd_fd, info->lbuf, sizeof(info->lbuf));
> > - if (info->lbuflen <= 0 || info->lbuf[info->lbuflen-1] != '\n') {
> > - printerr(0, "WARNING: %s: failed reading request\n", __func__);
> > - free_upcall_info(info);
> > - return;
> > - }
> > - info->lbuf[info->lbuflen-1] = 0;
> > -
> > - if (start_upcall_thread(handle_gssd_upcall, info))
> > - free_upcall_info(info);
> > + handle_gssd_upcall(clp);
> > }
> >
> > static void
> > gssd_clnt_krb5_cb(int UNUSED(fd), short UNUSED(which), void *data)
> > {
> > struct clnt_info *clp = data;
> > - struct clnt_upcall_info *info;
> > -
> > - info = alloc_upcall_info(clp);
> > - if (info == NULL)
> > - return;
> > -
> > - if (read(clp->krb5_fd, &info->uid,
> > - sizeof(info->uid)) < (ssize_t)sizeof(info->uid)) {
> > - printerr(0, "WARNING: %s: failed reading uid from krb5 "
> > - "upcall pipe: %s\n", __func__, strerror(errno));
> > - free_upcall_info(info);
> > - return;
> > - }
> >
> > - if (start_upcall_thread(handle_krb5_upcall, info))
> > - free_upcall_info(info);
> > + handle_krb5_upcall(clp);
> > }
> >
> > static struct clnt_info *
> > diff --git a/utils/gssd/gssd.h b/utils/gssd/gssd.h
> > index 1e8c58d4..6d53647e 100644
> > --- a/utils/gssd/gssd.h
> > +++ b/utils/gssd/gssd.h
> > @@ -84,14 +84,17 @@ struct clnt_info {
> >
> > struct clnt_upcall_info {
> > struct clnt_info *clp;
> > - char lbuf[RPC_CHAN_BUF_SIZE];
> > - int lbuflen;
> > uid_t uid;
> > + int fd;
> > + char *srchost;
> > + char *target;
> > + char *service;
> > };
> >
> > -void handle_krb5_upcall(struct clnt_upcall_info *clp);
> > -void handle_gssd_upcall(struct clnt_upcall_info *clp);
> > +void handle_krb5_upcall(struct clnt_info *clp);
> > +void handle_gssd_upcall(struct clnt_info *clp);
> > void free_upcall_info(struct clnt_upcall_info *info);
> > +void gssd_free_client(struct clnt_info *clp);
> >
> >
> > #endif /* _RPC_GSSD_H_ */
> > diff --git a/utils/gssd/gssd_proc.c b/utils/gssd/gssd_proc.c
> > index e830f497..ba508b30 100644
> > --- a/utils/gssd/gssd_proc.c
> > +++ b/utils/gssd/gssd_proc.c
> > @@ -80,6 +80,8 @@
> > #include "nfslib.h"
> > #include "gss_names.h"
> >
> > +extern pthread_mutex_t clp_lock;
> > +
> > /* Encryption types supported by the kernel rpcsec_gss code */
> > int num_krb5_enctypes = 0;
> > krb5_enctype *krb5_enctypes = NULL;
> > @@ -723,22 +725,135 @@ out_return_error:
> > goto out;
> > }
> >
> > -void
> > -handle_krb5_upcall(struct clnt_upcall_info *info)
> > +static struct clnt_upcall_info *
> > +alloc_upcall_info(struct clnt_info *clp, uid_t uid, int fd, char *srchost,
> > + char *target, char *service)
> > {
> > - struct clnt_info *clp = info->clp;
> > + struct clnt_upcall_info *info;
> > +
> > + info = malloc(sizeof(struct clnt_upcall_info));
> > + if (info == NULL)
> > + return NULL;
> > +
> > + memset(info, 0, sizeof(*info));
> > + pthread_mutex_lock(&clp_lock);
> > + clp->refcount++;
> > + pthread_mutex_unlock(&clp_lock);
> > + info->clp = clp;
> > + info->uid = uid;
> > + info->fd = fd;
> > + if (srchost) {
> > + info->srchost = strdup(srchost);
> > + if (info->srchost == NULL)
> > + goto out_info;
> > + }
> > + if (target) {
> > + info->target = strdup(target);
> > + if (info->target == NULL)
> > + goto out_srchost;
> > + }
> > + if (service) {
> > + info->service = strdup(service);
> > + if (info->service == NULL)
> > + goto out_target;
> > + }
> > +
> > +out:
> > + return info;
> > +
> > +out_target:
> > + if (info->target)
> > + free(info->target);
> > +out_srchost:
> > + if (info->srchost)
> > + free(info->srchost);
> > +out_info:
> > + free(info);
> > + info = NULL;
> > + goto out;
> > +}
> >
> > - printerr(2, "\n%s: uid %d (%s)\n", __func__, info->uid, clp->relpath);
> > +void free_upcall_info(struct clnt_upcall_info *info)
> > +{
> > + gssd_free_client(info->clp);
> > + if (info->service)
> > + free(info->service);
> > + if (info->target)
> > + free(info->target);
> > + if (info->srchost)
> > + free(info->srchost);
> > + free(info);
> > +}
> >
> > - process_krb5_upcall(clp, info->uid, clp->krb5_fd, NULL, NULL, NULL);
> > +static void
> > +gssd_work_thread_fn(struct clnt_upcall_info *info)
> > +{
> > + process_krb5_upcall(info->clp, info->uid, info->fd, info->srchost, info->target, info->service);
> > free_upcall_info(info);
> > }
> >
> > +static int
> > +start_upcall_thread(void (*func)(struct clnt_upcall_info *), void *info)
> > +{
> > + pthread_attr_t attr;
> > + pthread_t th;
> > + int ret;
> > +
> > + ret = pthread_attr_init(&attr);
> > + if (ret != 0) {
> > + printerr(0, "ERROR: failed to init pthread attr: ret %d: %s\n",
> > + ret, strerror(errno));
> > + return ret;
> > + }
> > + ret = pthread_attr_setdetachstate(&attr, PTHREAD_CREATE_DETACHED);
> > + if (ret != 0) {
> > + printerr(0, "ERROR: failed to create pthread attr: ret %d: "
> > + "%s\n", ret, strerror(errno));
> > + return ret;
> > + }
> > +
> > + ret = pthread_create(&th, &attr, (void *)func, (void *)info);
> > + if (ret != 0)
> > + printerr(0, "ERROR: pthread_create failed: ret %d: %s\n",
> > + ret, strerror(errno));
> > + else
> > + printerr(0, "created thread id 0x%lx\n", th);
> printerr(0) are used for errors not debugging... But I'm wondering
> if this debug statement is even needed... Since we tids are not
> included in other debugging statements, how would they info be useful?
In the 2nd patch I actually changed it to printerr(1), but yeah we can get
rid of it. I had a ton of these sprinkled all over the place and for
some reason decided to keep this one.
>
>
> > + return ret;
> > +}
> > +
> > +void
> > +handle_krb5_upcall(struct clnt_info *clp)
> > +{
> > + uid_t uid;
> > + struct clnt_upcall_info *info;
> > + int err;
> > +
> > + if (read(clp->krb5_fd, &uid, sizeof(uid)) < (ssize_t)sizeof(uid)) {
> > + printerr(0, "WARNING: failed reading uid from krb5 "
> > + "upcall pipe: %s\n", strerror(errno));
> > + return;
> > + }
> > + printerr(2, "\n%s: uid %d (%s)\n", __func__, uid, clp->relpath);
> > +
> > + info = alloc_upcall_info(clp, uid, clp->krb5_fd, NULL, NULL, NULL);
> > + if (info == NULL) {
> > + printerr(0, "%s: failed to allocate clnt_upcall_info\n", __func__);
> > + do_error_downcall(clp->krb5_fd, uid, -EACCES);
> > + return;
> > + }
> > + err = start_upcall_thread(gssd_work_thread_fn, info);
> > + if (err != 0) {
> > + do_error_downcall(clp->krb5_fd, uid, -EACCES);
> > + free_upcall_info(info);
> > + }
> > +}
> Why is -EACCES being used in both failures? Shouldn't errno be used
> or at least ENOMEM for the alloc_upcall_info() failure?
I believe the code that handles the downcall in the kernel only looks
for -EACCES and -EKEYEXPIRED. Anything else gets translated to -EACCES,
so that's why I went ahead and used -EACCES (see gss_fill_context()).
Note that means the kernel will also translate -ETIMEDOUT (from the 2nd
patch) to -EACCES as well... but I don't think it's necessary to change
that behavior really.
-Scott
>
> Again... these are just nits.. so a repost is not necessary
> Just wanted to get your thoughts on them.
>
> steved.
>
> > +
> > void
> > -handle_gssd_upcall(struct clnt_upcall_info *info)
> > +handle_gssd_upcall(struct clnt_info *clp)
> > {
> > - struct clnt_info *clp = info->clp;
> > uid_t uid;
> > + char lbuf[RPC_CHAN_BUF_SIZE];
> > + int lbuflen = 0;
> > char *p;
> > char *mech = NULL;
> > char *uidstr = NULL;
> > @@ -746,20 +861,22 @@ handle_gssd_upcall(struct clnt_upcall_info *info)
> > char *service = NULL;
> > char *srchost = NULL;
> > char *enctypes = NULL;
> > - char *upcall_str;
> > - char *pbuf = info->lbuf;
> > pthread_t tid = pthread_self();
> > + struct clnt_upcall_info *info;
> > + int err;
> >
> > - printerr(2, "\n%s(0x%x): '%s' (%s)\n", __func__, tid,
> > - info->lbuf, clp->relpath);
> > -
> > - upcall_str = strdup(info->lbuf);
> > - if (upcall_str == NULL) {
> > - printerr(0, "ERROR: malloc failure\n");
> > - goto out_nomem;
> > + lbuflen = read(clp->gssd_fd, lbuf, sizeof(lbuf));
> > + if (lbuflen <= 0 || lbuf[lbuflen-1] != '\n') {
> > + printerr(0, "WARNING: handle_gssd_upcall: "
> > + "failed reading request\n");
> > + return;
> > }
> > + lbuf[lbuflen-1] = 0;
> > +
> > + printerr(2, "\n%s(0x%x): '%s' (%s)\n", __func__, tid,
> > + lbuf, clp->relpath);
> >
> > - while ((p = strsep(&pbuf, " "))) {
> > + for (p = strtok(lbuf, " "); p; p = strtok(NULL, " ")) {
> > if (!strncmp(p, "mech=", strlen("mech=")))
> > mech = p + strlen("mech=");
> > else if (!strncmp(p, "uid=", strlen("uid=")))
> > @@ -777,8 +894,8 @@ handle_gssd_upcall(struct clnt_upcall_info *info)
> > if (!mech || strlen(mech) < 1) {
> > printerr(0, "WARNING: handle_gssd_upcall: "
> > "failed to find gss mechanism name "
> > - "in upcall string '%s'\n", upcall_str);
> > - goto out;
> > + "in upcall string '%s'\n", lbuf);
> > + return;
> > }
> >
> > if (uidstr) {
> > @@ -790,21 +907,21 @@ handle_gssd_upcall(struct clnt_upcall_info *info)
> > if (!uidstr) {
> > printerr(0, "WARNING: handle_gssd_upcall: "
> > "failed to find uid "
> > - "in upcall string '%s'\n", upcall_str);
> > - goto out;
> > + "in upcall string '%s'\n", lbuf);
> > + return;
> > }
> >
> > if (enctypes && parse_enctypes(enctypes) != 0) {
> > printerr(0, "WARNING: handle_gssd_upcall: "
> > "parsing encryption types failed: errno %d\n", errno);
> > - goto out;
> > + return;
> > }
> >
> > if (target && strlen(target) < 1) {
> > printerr(0, "WARNING: handle_gssd_upcall: "
> > "failed to parse target name "
> > - "in upcall string '%s'\n", upcall_str);
> > - goto out;
> > + "in upcall string '%s'\n", lbuf);
> > + return;
> > }
> >
> > /*
> > @@ -818,21 +935,26 @@ handle_gssd_upcall(struct clnt_upcall_info *info)
> > if (service && strlen(service) < 1) {
> > printerr(0, "WARNING: handle_gssd_upcall: "
> > "failed to parse service type "
> > - "in upcall string '%s'\n", upcall_str);
> > - goto out;
> > + "in upcall string '%s'\n", lbuf);
> > + return;
> > }
> >
> > - if (strcmp(mech, "krb5") == 0 && clp->servername)
> > - process_krb5_upcall(clp, uid, clp->gssd_fd, srchost, target, service);
> > - else {
> > + if (strcmp(mech, "krb5") == 0 && clp->servername) {
> > + info = alloc_upcall_info(clp, uid, clp->gssd_fd, srchost, target, service);
> > + if (info == NULL) {
> > + printerr(0, "%s: failed to allocate clnt_upcall_info\n", __func__);
> > + do_error_downcall(clp->gssd_fd, uid, -EACCES);
> > + return;
> > + }
> > + err = start_upcall_thread(gssd_work_thread_fn, info);
> > + if (err != 0) {
> > + do_error_downcall(clp->gssd_fd, uid, -EACCES);
> > + free_upcall_info(info);
> > + }
> > + } else {
> > if (clp->servername)
> > printerr(0, "WARNING: handle_gssd_upcall: "
> > "received unknown gss mech '%s'\n", mech);
> > do_error_downcall(clp->gssd_fd, uid, -EACCES);
> > }
> > -out:
> > - free(upcall_str);
> > -out_nomem:
> > - free_upcall_info(info);
> > - return;
> > }
> >
>
