From mboxrd@z Thu Jan  1 00:00:00 1970
From: Trond Myklebust <trond.myklebust@fys.uio.no>
Subject: Re: [PATCH] NFS: Invalid mount option values should fail even with
 "sloppy"
Date: Sat, 13 Jun 2009 13:07:10 -0400
Message-ID: <1244912830.6803.4.camel@heimdal.trondhjem.org>
References: <20090612221659.25437.19858.stgit@isabey.1015granger.net>
Mime-Version: 1.0
Content-Type: text/plain
Cc: linux-nfs@vger.kernel.org
To: Chuck Lever <chuck.lever@oracle.com>
Return-path: <linux-nfs-owner@vger.kernel.org>
Received: from mail-out1.uio.no ([129.240.10.57]:43418 "EHLO mail-out1.uio.no"
	rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP
	id S1753556AbZFMRHM (ORCPT <rfc822;linux-nfs@vger.kernel.org>);
	Sat, 13 Jun 2009 13:07:12 -0400
In-Reply-To: <20090612221659.25437.19858.stgit-r85ClMMopbrwdl/1UfZZQIVfYA8g3rJ/@public.gmane.org>
Sender: linux-nfs-owner@vger.kernel.org
List-ID: <linux-nfs.vger.kernel.org>

On Fri, 2009-06-12 at 18:19 -0400, Chuck Lever wrote:
> Ian Kent reports:
> 
> "I've noticed a couple of other regressions with the options vers
> and proto option of mount.nfs(8).
> 
> The commands:
> 
> mount -t nfs -o vers=<invalid version> <server>:/<path> /<mountpoint>
> mount -t nfs -o proto=<invalid proto> <server>:/<path> /<mountpoint>
> 
> both immediately fail.
> 
> But if the "-s" option is also used they both succeed with the
> mount falling back to defaults (by the look of it).
> 
> In the past these failed even when the sloppy option was given, as
> I think they should. I believe the sloppy option is meant to allow
> the mount command to still function for mount options (for example
> in shared autofs maps) that exist on other Unix implementations but
> aren't present in the Linux mount.nfs(8). So, an invalid value
> specified for a known mount option is different to an unknown mount
> option and should fail appropriately."
> 
> See RH bugzilla 486266.
> 
> Address the problem by separating parsing errors into two categories.
> Invalid options will fail only if sloppy isn't set, but invalid
> values will always fail.
> 
> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
> ---
> 
> Trond-
> 
> Request For Comments only.  Ian hasn't tested this yet, but I was hoping
> it could find its way into 2.6.31 at some point, if it looks correct to
> you.
> 
>  fs/nfs/super.c |   57 +++++++++++++++++++++++++++++++-------------------------
>  1 files changed, 32 insertions(+), 25 deletions(-)
> 
> diff --git a/fs/nfs/super.c b/fs/nfs/super.c
> index 66ffd5d..e88c527 100644
> --- a/fs/nfs/super.c
> +++ b/fs/nfs/super.c
> @@ -957,7 +957,7 @@ static int nfs_parse_mount_options(char *raw,
>  				   struct nfs_parsed_mount_data *mnt)
>  {
>  	char *p, *string, *secdata;
> -	int rc, sloppy = 0, errors = 0;
> +	int rc, sloppy = 0, invalid_options = 0, invalid_values = 0;

Why add a counter? Why not just add a variable to hold the return value

          int ret = 0;

...and have the invalid values set ret=1?

>  	if (!raw) {
>  		dfprintk(MOUNT, "NFS: mount options string was NULL.\n");
> @@ -1092,77 +1092,77 @@ static int nfs_parse_mount_options(char *raw,
>  		case Opt_port:
>  			if (match_int(args, &option) ||
>  			    option < 0 || option > USHORT_MAX) {
> -				errors++;
> +				invalid_values++;
>  				nfs_parse_invalid_value("port");
>  			} else
>  				mnt->nfs_server.port = option;
>  			break;
>  		case Opt_rsize:
>  			if (match_int(args, &option) || option < 0) {
> -				errors++;
> +				invalid_values++;
>  				nfs_parse_invalid_value("rsize");
>  			} else
>  				mnt->rsize = option;
>  			break;
>  		case Opt_wsize:
>  			if (match_int(args, &option) || option < 0) {
> -				errors++;
> +				invalid_values++;
>  				nfs_parse_invalid_value("wsize");
>  			} else
>  				mnt->wsize = option;
>  			break;
>  		case Opt_bsize:
>  			if (match_int(args, &option) || option < 0) {
> -				errors++;
> +				invalid_values++;
>  				nfs_parse_invalid_value("bsize");
>  			} else
>  				mnt->bsize = option;
>  			break;
>  		case Opt_timeo:
>  			if (match_int(args, &option) || option <= 0) {
> -				errors++;
> +				invalid_values++;
>  				nfs_parse_invalid_value("timeo");
>  			} else
>  				mnt->timeo = option;
>  			break;
>  		case Opt_retrans:
>  			if (match_int(args, &option) || option <= 0) {
> -				errors++;
> +				invalid_values++;
>  				nfs_parse_invalid_value("retrans");
>  			} else
>  				mnt->retrans = option;
>  			break;
>  		case Opt_acregmin:
>  			if (match_int(args, &option) || option < 0) {
> -				errors++;
> +				invalid_values++;
>  				nfs_parse_invalid_value("acregmin");
>  			} else
>  				mnt->acregmin = option;
>  			break;
>  		case Opt_acregmax:
>  			if (match_int(args, &option) || option < 0) {
> -				errors++;
> +				invalid_values++;
>  				nfs_parse_invalid_value("acregmax");
>  			} else
>  				mnt->acregmax = option;
>  			break;
>  		case Opt_acdirmin:
>  			if (match_int(args, &option) || option < 0) {
> -				errors++;
> +				invalid_values++;
>  				nfs_parse_invalid_value("acdirmin");
>  			} else
>  				mnt->acdirmin = option;
>  			break;
>  		case Opt_acdirmax:
>  			if (match_int(args, &option) || option < 0) {
> -				errors++;
> +				invalid_values++;
>  				nfs_parse_invalid_value("acdirmax");
>  			} else
>  				mnt->acdirmax = option;
>  			break;
>  		case Opt_actimeo:
>  			if (match_int(args, &option) || option < 0) {
> -				errors++;
> +				invalid_values++;
>  				nfs_parse_invalid_value("actimeo");
>  			} else
>  				mnt->acregmin = mnt->acregmax =
> @@ -1170,7 +1170,7 @@ static int nfs_parse_mount_options(char *raw,
>  			break;
>  		case Opt_namelen:
>  			if (match_int(args, &option) || option < 0) {
> -				errors++;
> +				invalid_values++;
>  				nfs_parse_invalid_value("namlen");
>  			} else
>  				mnt->namlen = option;
> @@ -1178,7 +1178,7 @@ static int nfs_parse_mount_options(char *raw,
>  		case Opt_mountport:
>  			if (match_int(args, &option) ||
>  			    option < 0 || option > USHORT_MAX) {
> -				errors++;
> +				invalid_values++;
>  				nfs_parse_invalid_value("mountport");
>  			} else
>  				mnt->mount_server.port = option;
> @@ -1187,14 +1187,14 @@ static int nfs_parse_mount_options(char *raw,
>  			if (match_int(args, &option) ||
>  			    option < NFS_MNT_VERSION ||
>  			    option > NFS_MNT3_VERSION) {
> -				errors++;
> +				invalid_values++;
>  				nfs_parse_invalid_value("mountvers");
>  			} else
>  				mnt->mount_server.version = option;
>  			break;
>  		case Opt_nfsvers:
>  			if (match_int(args, &option)) {
> -				errors++;
> +				invalid_values++;
>  				nfs_parse_invalid_value("nfsvers");
>  				break;
>  			}
> @@ -1206,7 +1206,7 @@ static int nfs_parse_mount_options(char *raw,
>  				mnt->flags |= NFS_MOUNT_VER3;
>  				break;
>  			default:
> -				errors++;
> +				invalid_values++;
>  				nfs_parse_invalid_value("nfsvers");
>  			}
>  			break;
> @@ -1221,7 +1221,7 @@ static int nfs_parse_mount_options(char *raw,
>  			rc = nfs_parse_security_flavors(string, mnt);
>  			kfree(string);
>  			if (!rc) {
> -				errors++;
> +				invalid_values++;
>  				dfprintk(MOUNT, "NFS:   unrecognized "
>  						"security flavor\n");
>  			}
> @@ -1249,7 +1249,7 @@ static int nfs_parse_mount_options(char *raw,
>  				xprt_load_transport(string);
>  				break;
>  			default:
> -				errors++;
> +				invalid_values++;
>  				dfprintk(MOUNT, "NFS:   unrecognized "
>  						"transport protocol\n");
>  			}
> @@ -1272,7 +1272,7 @@ static int nfs_parse_mount_options(char *raw,
>  				break;
>  			case Opt_xprt_rdma: /* not used for side protocols */
>  			default:
> -				errors++;
> +				invalid_values++;
>  				dfprintk(MOUNT, "NFS:   unrecognized "
>  						"transport protocol\n");
>  			}
> @@ -1330,7 +1330,7 @@ static int nfs_parse_mount_options(char *raw,
>  					mnt->flags |= NFS_MOUNT_LOOKUP_CACHE_NONEG|NFS_MOUNT_LOOKUP_CACHE_NONE;
>  					break;
>  				default:
> -					errors++;
> +					invalid_values++;
>  					dfprintk(MOUNT, "NFS:   invalid "
>  							"lookupcache argument\n");
>  			};
> @@ -1350,15 +1350,22 @@ static int nfs_parse_mount_options(char *raw,
>  			break;
>  
>  		default:
> -			errors++;
> +			invalid_options++;
>  			dfprintk(MOUNT, "NFS:   unrecognized mount option "
>  					"'%s'\n", p);
>  		}
>  	}
>  
> -	if (errors > 0) {
> -		dfprintk(MOUNT, "NFS: parsing encountered %d error%s\n",
> -				errors, (errors == 1 ? "" : "s"));
> +	if (invalid_values > 0) {
> +		dfprintk(MOUNT, "NFS: parsing encountered %d invalid value%s\n",
> +				invalid_values,
> +				invalid_values == 1 ? "" : "s");
> +		return 0;
> +	}
> +	if (invalid_options > 0) {
> +		dfprintk(MOUNT, "NFS: parsing encountered %d invalid option%s\n",
> +				invalid_options,
> +				invalid_options == 1 ? "" : "s");
>  		if (!sloppy)
>  			return 0;
>  	}
> 
> --
> 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