| Message ID | 20131114120711.4043a60f@notabene.brown (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, 14 Nov 2013 12:07:11 +1100 NeilBrown <neilb@suse.de> wrote: > > Sometimes gssd will open a new rpc-pipe but never read requests from it > or reply to them. This causes the kernel to wait forever for a reply. > > In particular, if a filesystem is mounted by IP, and the IP has no > hostname recorded in /etc/hosts or DNS, then gssd will not listen to > requests and the mount will hang indefinitely. > > The comment in process_clnt_dir() for the "fail_keep_client:" branch > suggests that it is for the case where we couldn't open some > subdirectories. However it is currently also taken if reverse DNS > lookup fails (as well as some other lookup failures). Those failures > should not be treated the same as failure-to-open directories. > > So this patch causes a failure from read_service_info() to *not* be > reported by process_clnt_dir_files. This ensures that insert_clnt_poll() > will be called and requests will be handled. > > In handle_gssd_upcall, the current error path (taken when the mech is > not "krb5") does not reply to the upcall. This is wrong. A reply is > always appropriate. The only replies which aren't treated as > transient errors are EACCES and EKEYEXPIRED, so we return the former. > > If read_service_info() fails then ->servicename will be NULL which will > cause process_krb5_upcall() (quite reasonably) to become confused. So > in that case we don't even try to process the up-call but just reply > with EACCES. > > As clp->servicename==NULL is no longer treated as fatal, it is not > appropraite to use it to test if read_service_info() has been already > called on a client. Instread test clp->prog. > > Finally, the error path of read_service_info() will close 'fd' if it > isn't -1, so when we close it, we should set fd to -1. > > Signed-off-by: NeilBrown <neilb@suse.de> > > diff --git a/utils/gssd/gssd_proc.c b/utils/gssd/gssd_proc.c > index b48d1637cd36..58c2a282524f 100644 > --- a/utils/gssd/gssd_proc.c > +++ b/utils/gssd/gssd_proc.c > @@ -256,6 +256,7 @@ read_service_info(char *info_file_name, char **servicename, char **servername, > if ((nbytes = read(fd, buf, INFOBUFLEN)) == -1) > goto fail; > close(fd); > + fd = -1; > buf[nbytes] = '\0'; > > numfields = sscanf(buf,"RPC server: %127s\n" > @@ -403,11 +404,10 @@ process_clnt_dir_files(struct clnt_info * clp) > return -1; > snprintf(info_file_name, sizeof(info_file_name), "%s/info", > clp->dirname); > - if ((clp->servicename == NULL) && > - read_service_info(info_file_name, &clp->servicename, > - &clp->servername, &clp->prog, &clp->vers, > - &clp->protocol, (struct sockaddr *) &clp->addr)) > - return -1; > + 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; > } > > @@ -1320,11 +1320,14 @@ handle_gssd_upcall(struct clnt_info *clp) > } > } > > - if (strcmp(mech, "krb5") == 0) > + if (strcmp(mech, "krb5") == 0 && clp->servername) > process_krb5_upcall(clp, uid, clp->gssd_fd, target, service); > - else > - printerr(0, "WARNING: handle_gssd_upcall: " > - "received unknown gss mech '%s'\n", mech); > + 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(lbuf); Looks good. Acked-by: Jeff Layton <jlayton@redhat.com> -- 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 13/11/13 20:07, NeilBrown wrote: > > Sometimes gssd will open a new rpc-pipe but never read requests from it > or reply to them. This causes the kernel to wait forever for a reply. > > In particular, if a filesystem is mounted by IP, and the IP has no > hostname recorded in /etc/hosts or DNS, then gssd will not listen to > requests and the mount will hang indefinitely. > > The comment in process_clnt_dir() for the "fail_keep_client:" branch > suggests that it is for the case where we couldn't open some > subdirectories. However it is currently also taken if reverse DNS > lookup fails (as well as some other lookup failures). Those failures > should not be treated the same as failure-to-open directories. > > So this patch causes a failure from read_service_info() to *not* be > reported by process_clnt_dir_files. This ensures that insert_clnt_poll() > will be called and requests will be handled. > > In handle_gssd_upcall, the current error path (taken when the mech is > not "krb5") does not reply to the upcall. This is wrong. A reply is > always appropriate. The only replies which aren't treated as > transient errors are EACCES and EKEYEXPIRED, so we return the former. > > If read_service_info() fails then ->servicename will be NULL which will > cause process_krb5_upcall() (quite reasonably) to become confused. So > in that case we don't even try to process the up-call but just reply > with EACCES. > > As clp->servicename==NULL is no longer treated as fatal, it is not > appropraite to use it to test if read_service_info() has been already > called on a client. Instread test clp->prog. > > Finally, the error path of read_service_info() will close 'fd' if it > isn't -1, so when we close it, we should set fd to -1. > > Signed-off-by: NeilBrown <neilb@suse.de> Committed (tag: nfs-utils-1-2-10-rc1) steved. > > diff --git a/utils/gssd/gssd_proc.c b/utils/gssd/gssd_proc.c > index b48d1637cd36..58c2a282524f 100644 > --- a/utils/gssd/gssd_proc.c > +++ b/utils/gssd/gssd_proc.c > @@ -256,6 +256,7 @@ read_service_info(char *info_file_name, char **servicename, char **servername, > if ((nbytes = read(fd, buf, INFOBUFLEN)) == -1) > goto fail; > close(fd); > + fd = -1; > buf[nbytes] = '\0'; > > numfields = sscanf(buf,"RPC server: %127s\n" > @@ -403,11 +404,10 @@ process_clnt_dir_files(struct clnt_info * clp) > return -1; > snprintf(info_file_name, sizeof(info_file_name), "%s/info", > clp->dirname); > - if ((clp->servicename == NULL) && > - read_service_info(info_file_name, &clp->servicename, > - &clp->servername, &clp->prog, &clp->vers, > - &clp->protocol, (struct sockaddr *) &clp->addr)) > - return -1; > + 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; > } > > @@ -1320,11 +1320,14 @@ handle_gssd_upcall(struct clnt_info *clp) > } > } > > - if (strcmp(mech, "krb5") == 0) > + if (strcmp(mech, "krb5") == 0 && clp->servername) > process_krb5_upcall(clp, uid, clp->gssd_fd, target, service); > - else > - printerr(0, "WARNING: handle_gssd_upcall: " > - "received unknown gss mech '%s'\n", mech); > + 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(lbuf); > -- 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 b48d1637cd36..58c2a282524f 100644 --- a/utils/gssd/gssd_proc.c +++ b/utils/gssd/gssd_proc.c @@ -256,6 +256,7 @@ read_service_info(char *info_file_name, char **servicename, char **servername, if ((nbytes = read(fd, buf, INFOBUFLEN)) == -1) goto fail; close(fd); + fd = -1; buf[nbytes] = '\0'; numfields = sscanf(buf,"RPC server: %127s\n" @@ -403,11 +404,10 @@ process_clnt_dir_files(struct clnt_info * clp) return -1; snprintf(info_file_name, sizeof(info_file_name), "%s/info", clp->dirname); - if ((clp->servicename == NULL) && - read_service_info(info_file_name, &clp->servicename, - &clp->servername, &clp->prog, &clp->vers, - &clp->protocol, (struct sockaddr *) &clp->addr)) - return -1; + 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; } @@ -1320,11 +1320,14 @@ handle_gssd_upcall(struct clnt_info *clp) } } - if (strcmp(mech, "krb5") == 0) + if (strcmp(mech, "krb5") == 0 && clp->servername) process_krb5_upcall(clp, uid, clp->gssd_fd, target, service); - else - printerr(0, "WARNING: handle_gssd_upcall: " - "received unknown gss mech '%s'\n", mech); + 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(lbuf);
Sometimes gssd will open a new rpc-pipe but never read requests from it or reply to them. This causes the kernel to wait forever for a reply. In particular, if a filesystem is mounted by IP, and the IP has no hostname recorded in /etc/hosts or DNS, then gssd will not listen to requests and the mount will hang indefinitely. The comment in process_clnt_dir() for the "fail_keep_client:" branch suggests that it is for the case where we couldn't open some subdirectories. However it is currently also taken if reverse DNS lookup fails (as well as some other lookup failures). Those failures should not be treated the same as failure-to-open directories. So this patch causes a failure from read_service_info() to *not* be reported by process_clnt_dir_files. This ensures that insert_clnt_poll() will be called and requests will be handled. In handle_gssd_upcall, the current error path (taken when the mech is not "krb5") does not reply to the upcall. This is wrong. A reply is always appropriate. The only replies which aren't treated as transient errors are EACCES and EKEYEXPIRED, so we return the former. If read_service_info() fails then ->servicename will be NULL which will cause process_krb5_upcall() (quite reasonably) to become confused. So in that case we don't even try to process the up-call but just reply with EACCES. As clp->servicename==NULL is no longer treated as fatal, it is not appropraite to use it to test if read_service_info() has been already called on a client. Instread test clp->prog. Finally, the error path of read_service_info() will close 'fd' if it isn't -1, so when we close it, we should set fd to -1. Signed-off-by: NeilBrown <neilb@suse.de>