| Message ID | 574d004f-4b42-d4aa-60f6-607ffae6dd1b@RedHat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
> On May 5, 2016, at 10:17 AM, Steve Dickson <SteveD@redhat.com> wrote: > > Hello, > > On 05/04/2016 10:15 AM, Kornievskaia, Olga wrote: >> I’m having a hard time picturing what you have in mind. I don’t see how you can allocate, >> populate and free in one routine. The logic is we allocate memory, then read the upcall >> and only then we create the thread. If either upcall or creation of the thread >> fails we need to free the allocated memory. > Something like this: > > gssd_clnt_gssd_cb(int UNUSED(fd), short UNUSED(which), void *data) > { > struct clnt_info *clp = data; > - pthread_t th; > - int ret; > + struct clnt_upcall_info *info; > > - ret = pthread_create(&th, NULL, (void *)handle_gssd_upcall, > - (void *)clp); > - if (ret != 0) { > - printerr(0, "ERROR: pthread_create failed: ret %d: %s\n", > - ret, strerror(errno)); > + 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(info); > return; > } > - wait_for_child_and_detach(th); > + info->lbuf[info->lbuflen-1] = 0; > + > + start_upcall_thread(handle_gssd_upcall, info); > + > + free(info); > + return; Gotcha! Sounds good! You can make the modifications. Or just in case since you are traveling, I’ll just send another patch. Please also add any other lines like Modified-by or Signed-off-by that are need. > } > > static void > gssd_clnt_krb5_cb(int UNUSED(fd), short UNUSED(which), void *data) > { > struct clnt_info *clp = data; > - pthread_t th; > - int ret; > + struct clnt_upcall_info *info; > > - ret = pthread_create(&th, NULL, (void *)handle_krb5_upcall, > - (void *)clp); > - if (ret != 0) { > - printerr(0, "ERROR: pthread_create failed: ret %d: %s\n", > - ret, strerror(errno)); > + 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(info); > return; > } > - wait_for_child_and_detach(th); > + > + start_upcall_thread(handle_krb5_upcall, info); > + > + free(info); > + return; > } > > Then utils/gssd/gssd_proc.c looks something like this: > > diff --git a/utils/gssd/gssd_proc.c b/utils/gssd/gssd_proc.c > index afaaf9e..f1bd571 100644 > --- a/utils/gssd/gssd_proc.c > +++ b/utils/gssd/gssd_proc.c > @@ -79,7 +79,6 @@ > #include "nfsrpc.h" > #include "nfslib.h" > #include "gss_names.h" > -#include "misc.h" > > /* Encryption types supported by the kernel rpcsec_gss code */ > int num_krb5_enctypes = 0; > @@ -708,45 +707,21 @@ out_return_error: > goto out; > } > > -/* signal to the parent thread that we have read from the file descriptor. > - * it should allow the parent to proceed to poll on the descriptor for > - * the next upcall from the kernel. > - */ > -static inline void > -signal_parent_event_consumed(void) > -{ > - pthread_mutex_lock(&pmutex); > - thread_started = true; > - pthread_cond_signal(&pcond); > - pthread_mutex_unlock(&pmutex); > -} > - > void > -handle_krb5_upcall(struct clnt_info *clp) > +handle_krb5_upcall(struct clnt_upcall_info *info) > { > - uid_t uid; > - int status; > - > - status = read(clp->krb5_fd, &uid, sizeof(uid)) < (ssize_t)sizeof(uid); > - signal_parent_event_consumed(); > - > - if (status) { > - printerr(0, "WARNING: failed reading uid from krb5 " > - "upcall pipe: %s\n", strerror(errno)); > - return; > - } > + struct clnt_info *clp = info->clp; > > - printerr(2, "\n%s: uid %d (%s)\n", __func__, uid, clp->relpath); > + printerr(2, "\n%s: uid %d (%s)\n", __func__, info->uid, clp->relpath); > > - process_krb5_upcall(clp, uid, clp->krb5_fd, NULL, NULL); > + process_krb5_upcall(clp, info->uid, clp->krb5_fd, NULL, NULL); > } > > void > -handle_gssd_upcall(struct clnt_info *clp) > +handle_gssd_upcall(struct clnt_upcall_info *info) > { > + 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; > @@ -754,19 +729,9 @@ handle_gssd_upcall(struct clnt_info *clp) > char *service = NULL; > char *enctypes = NULL; > > - lbuflen = read(clp->gssd_fd, lbuf, sizeof(lbuf)); > - signal_parent_event_consumed(); > - > - 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: '%s' (%s)\n", __func__, lbuf, clp->relpath); > + printerr(2, "\n%s: '%s' (%s)\n", __func__, info->lbuf, clp->relpath); > > - for (p = strtok(lbuf, " "); p; p = strtok(NULL, " ")) { > + for (p = strtok(info->lbuf, " "); p; p = strtok(NULL, " ")) { > if (!strncmp(p, "mech=", strlen("mech="))) > mech = p + strlen("mech="); > else if (!strncmp(p, "uid=", strlen("uid="))) > @@ -782,7 +747,7 @@ handle_gssd_upcall(struct clnt_info *clp) > if (!mech || strlen(mech) < 1) { > printerr(0, "WARNING: handle_gssd_upcall: " > "failed to find gss mechanism name " > - "in upcall string '%s'\n", lbuf); > + "in upcall string '%s'\n", info->lbuf); > return; > } > > @@ -795,7 +760,7 @@ handle_gssd_upcall(struct clnt_info *clp) > if (!uidstr) { > printerr(0, "WARNING: handle_gssd_upcall: " > "failed to find uid " > - "in upcall string '%s'\n", lbuf); > + "in upcall string '%s'\n", info->lbuf); > return; > } > > @@ -808,7 +773,7 @@ handle_gssd_upcall(struct clnt_info *clp) > if (target && strlen(target) < 1) { > printerr(0, "WARNING: handle_gssd_upcall: " > "failed to parse target name " > - "in upcall string '%s'\n", lbuf); > + "in upcall string '%s'\n", info->lbuf); > return; > } > > @@ -823,7 +788,7 @@ handle_gssd_upcall(struct clnt_info *clp) > if (service && strlen(service) < 1) { > printerr(0, "WARNING: handle_gssd_upcall: " > "failed to parse service type " > - "in upcall string '%s'\n", lbuf); > + "in upcall string '%s'\n", info->lbuf); > return; > } > > @@ -835,5 +800,7 @@ handle_gssd_upcall(struct clnt_info *clp) > "received unknown gss mech '%s'\n", mech); > do_error_downcall(clp->gssd_fd, uid, -EACCES); > } > + > + return; > } > > steved. -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 05/05/2016 11:40 AM, Kornievskaia, Olga wrote: > >> On May 5, 2016, at 11:35 AM, Kornievskaia, Olga <Olga.Kornievskaia@netapp.com <mailto:Olga.Kornievskaia@netapp.com>> wrote: >> >>> >>> On May 5, 2016, at 10:17 AM, Steve Dickson <SteveD@redhat.com <mailto:SteveD@redhat.com>> wrote: >>> >>> Hello, >>> >>> On 05/04/2016 10:15 AM, Kornievskaia, Olga wrote: >>>> I’m having a hard time picturing what you have in mind. I don’t see how you can allocate, >>>> populate and free in one routine. The logic is we allocate memory, then read the upcall >>>> and only then we create the thread. If either upcall or creation of the thread >>>> fails we need to free the allocated memory. >>> Something like this: >>> >>> gssd_clnt_gssd_cb(int UNUSED(fd), short UNUSED(which), void *data) >>> { >>> struct clnt_info *clp = data; >>> - pthread_t th; >>> - int ret; >>> + struct clnt_upcall_info *info; >>> >>> - ret = pthread_create(&th, NULL, (void *)handle_gssd_upcall, >>> - (void *)clp); >>> - if (ret != 0) { >>> - printerr(0, "ERROR: pthread_create failed: ret %d: %s\n", >>> - ret, strerror(errno)); >>> + 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(info); >>> return; >>> } >>> - wait_for_child_and_detach(th); >>> + info->lbuf[info->lbuflen-1] = 0; >>> + >>> + start_upcall_thread(handle_gssd_upcall, info); >>> + >>> + free(info); >>> + return; >> >> Gotcha! Sounds good! You can make the modifications. Or just in case since you are traveling, > I’ll just send another patch. Please also add any other lines like Modified-by or Signed-off-by that are need. > > I’m sorry I spoke too soon. “info” is allocated and given to the thread > to process so the main thread can’t free it. I'm curious as to why? The main thread allocated it, is handing allocated memory to a thread something special? What am I missing? steved. -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> On May 5, 2016, at 12:40 PM, Steve Dickson <SteveD@redhat.com> wrote: > > > > On 05/05/2016 11:40 AM, Kornievskaia, Olga wrote: >> >>> On May 5, 2016, at 11:35 AM, Kornievskaia, Olga <Olga.Kornievskaia@netapp.com <mailto:Olga.Kornievskaia@netapp.com>> wrote: >>> >>>> >>>> On May 5, 2016, at 10:17 AM, Steve Dickson <SteveD@redhat.com <mailto:SteveD@redhat.com>> wrote: >>>> >>>> Hello, >>>> >>>> On 05/04/2016 10:15 AM, Kornievskaia, Olga wrote: >>>>> I’m having a hard time picturing what you have in mind. I don’t see how you can allocate, >>>>> populate and free in one routine. The logic is we allocate memory, then read the upcall >>>>> and only then we create the thread. If either upcall or creation of the thread >>>>> fails we need to free the allocated memory. >>>> Something like this: >>>> >>>> gssd_clnt_gssd_cb(int UNUSED(fd), short UNUSED(which), void *data) >>>> { >>>> struct clnt_info *clp = data; >>>> - pthread_t th; >>>> - int ret; >>>> + struct clnt_upcall_info *info; >>>> >>>> - ret = pthread_create(&th, NULL, (void *)handle_gssd_upcall, >>>> - (void *)clp); >>>> - if (ret != 0) { >>>> - printerr(0, "ERROR: pthread_create failed: ret %d: %s\n", >>>> - ret, strerror(errno)); >>>> + 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(info); >>>> return; >>>> } >>>> - wait_for_child_and_detach(th); >>>> + info->lbuf[info->lbuflen-1] = 0; >>>> + >>>> + start_upcall_thread(handle_gssd_upcall, info); >>>> + >>>> + free(info); >>>> + return; >>> >>> Gotcha! Sounds good! You can make the modifications. Or just in case since you are traveling, >> I’ll just send another patch. Please also add any other lines like Modified-by or Signed-off-by that are need. >> >> I’m sorry I spoke too soon. “info” is allocated and given to the thread >> to process so the main thread can’t free it. > I'm curious as to why? The main thread allocated it, is handing allocated memory > to a thread something special? What am I missing? The main thread allocates memory, gives it to the the child thread and goes to the next libevent. We detach the thread so if we free the memory in the main thread, the child thread would segfault. -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 05/05/2016 12:43 PM, Kornievskaia, Olga wrote: > >> On May 5, 2016, at 12:40 PM, Steve Dickson <SteveD@redhat.com> wrote: >> >> >> >> On 05/05/2016 11:40 AM, Kornievskaia, Olga wrote: >>> >>>> On May 5, 2016, at 11:35 AM, Kornievskaia, Olga <Olga.Kornievskaia@netapp.com <mailto:Olga.Kornievskaia@netapp.com>> wrote: >>>> >>>>> >>>>> On May 5, 2016, at 10:17 AM, Steve Dickson <SteveD@redhat.com <mailto:SteveD@redhat.com>> wrote: >>>>> >>>>> Hello, >>>>> >>>>> On 05/04/2016 10:15 AM, Kornievskaia, Olga wrote: >>>>>> I’m having a hard time picturing what you have in mind. I don’t see how you can allocate, >>>>>> populate and free in one routine. The logic is we allocate memory, then read the upcall >>>>>> and only then we create the thread. If either upcall or creation of the thread >>>>>> fails we need to free the allocated memory. >>>>> Something like this: >>>>> >>>>> gssd_clnt_gssd_cb(int UNUSED(fd), short UNUSED(which), void *data) >>>>> { >>>>> struct clnt_info *clp = data; >>>>> - pthread_t th; >>>>> - int ret; >>>>> + struct clnt_upcall_info *info; >>>>> >>>>> - ret = pthread_create(&th, NULL, (void *)handle_gssd_upcall, >>>>> - (void *)clp); >>>>> - if (ret != 0) { >>>>> - printerr(0, "ERROR: pthread_create failed: ret %d: %s\n", >>>>> - ret, strerror(errno)); >>>>> + 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(info); >>>>> return; >>>>> } >>>>> - wait_for_child_and_detach(th); >>>>> + info->lbuf[info->lbuflen-1] = 0; >>>>> + >>>>> + start_upcall_thread(handle_gssd_upcall, info); >>>>> + >>>>> + free(info); >>>>> + return; >>>> >>>> Gotcha! Sounds good! You can make the modifications. Or just in case since you are traveling, >>> I’ll just send another patch. Please also add any other lines like Modified-by or Signed-off-by that are need. >>> >>> I’m sorry I spoke too soon. “info” is allocated and given to the thread >>> to process so the main thread can’t free it. >> I'm curious as to why? The main thread allocated it, is handing allocated memory >> to a thread something special? What am I missing? > > The main thread allocates memory, gives it to the the child thread and goes > to the next libevent. We detach the thread so if we free the memory in the main > thread, the child thread would segfault. Ah... that is the part I missed... You are right... That's what I get for not testing... Let's go with the original patch and move on... steved. -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/utils/gssd/gssd_proc.c b/utils/gssd/gssd_proc.c index afaaf9e..f1bd571 100644 --- a/utils/gssd/gssd_proc.c +++ b/utils/gssd/gssd_proc.c @@ -79,7 +79,6 @@ #include "nfsrpc.h" #include "nfslib.h" #include "gss_names.h" -#include "misc.h" /* Encryption types supported by the kernel rpcsec_gss code */ int num_krb5_enctypes = 0; @@ -708,45 +707,21 @@ out_return_error: goto out; } -/* signal to the parent thread that we have read from the file descriptor. - * it should allow the parent to proceed to poll on the descriptor for - * the next upcall from the kernel. - */ -static inline void -signal_parent_event_consumed(void) -{ - pthread_mutex_lock(&pmutex); - thread_started = true; - pthread_cond_signal(&pcond); - pthread_mutex_unlock(&pmutex); -} - void -handle_krb5_upcall(struct clnt_info *clp) +handle_krb5_upcall(struct clnt_upcall_info *info) { - uid_t uid; - int status; - - status = read(clp->krb5_fd, &uid, sizeof(uid)) < (ssize_t)sizeof(uid); - signal_parent_event_consumed(); - - if (status) { - printerr(0, "WARNING: failed reading uid from krb5 " - "upcall pipe: %s\n", strerror(errno)); - return; - } + struct clnt_info *clp = info->clp; - printerr(2, "\n%s: uid %d (%s)\n", __func__, uid, clp->relpath); + printerr(2, "\n%s: uid %d (%s)\n", __func__, info->uid, clp->relpath); - process_krb5_upcall(clp, uid, clp->krb5_fd, NULL, NULL); + process_krb5_upcall(clp, info->uid, clp->krb5_fd, NULL, NULL); } void -handle_gssd_upcall(struct clnt_info *clp) +handle_gssd_upcall(struct clnt_upcall_info *info) { + 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; @@ -754,19 +729,9 @@ handle_gssd_upcall(struct clnt_info *clp) char *service = NULL; char *enctypes = NULL; - lbuflen = read(clp->gssd_fd, lbuf, sizeof(lbuf)); - signal_parent_event_consumed(); - - 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: '%s' (%s)\n", __func__, lbuf, clp->relpath); + printerr(2, "\n%s: '%s' (%s)\n", __func__, info->lbuf, clp->relpath); - for (p = strtok(lbuf, " "); p; p = strtok(NULL, " ")) { + for (p = strtok(info->lbuf, " "); p; p = strtok(NULL, " ")) { if (!strncmp(p, "mech=", strlen("mech="))) mech = p + strlen("mech="); else if (!strncmp(p, "uid=", strlen("uid="))) @@ -782,7 +747,7 @@ handle_gssd_upcall(struct clnt_info *clp) if (!mech || strlen(mech) < 1) { printerr(0, "WARNING: handle_gssd_upcall: " "failed to find gss mechanism name " - "in upcall string '%s'\n", lbuf); + "in upcall string '%s'\n", info->lbuf); return; } @@ -795,7 +760,7 @@ handle_gssd_upcall(struct clnt_info *clp) if (!uidstr) { printerr(0, "WARNING: handle_gssd_upcall: " "failed to find uid " - "in upcall string '%s'\n", lbuf); + "in upcall string '%s'\n", info->lbuf); return; } @@ -808,7 +773,7 @@ handle_gssd_upcall(struct clnt_info *clp) if (target && strlen(target) < 1) { printerr(0, "WARNING: handle_gssd_upcall: " "failed to parse target name " - "in upcall string '%s'\n", lbuf); + "in upcall string '%s'\n", info->lbuf); return; } @@ -823,7 +788,7 @@ handle_gssd_upcall(struct clnt_info *clp) if (service && strlen(service) < 1) { printerr(0, "WARNING: handle_gssd_upcall: " "failed to parse service type " - "in upcall string '%s'\n", lbuf); + "in upcall string '%s'\n", info->lbuf); return; } @@ -835,5 +800,7 @@ handle_gssd_upcall(struct clnt_info *clp) "received unknown gss mech '%s'\n", mech); do_error_downcall(clp->gssd_fd, uid, -EACCES); } + + return; }