diff mbox

[-,nfs-utils] gssd: always reply to rpc-pipe requests from kernel.

Message ID 20131114120711.4043a60f@notabene.brown
State New, archived
Headers show

Commit Message

NeilBrown Nov. 14, 2013, 1:07 a.m. UTC
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>

Comments

Jeff Layton Nov. 14, 2013, 1:34 p.m. UTC | #1
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
Steve Dickson Nov. 20, 2013, 9:21 p.m. UTC | #2
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 mbox

Patch

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);