[PATCH] mount.nfs: return error if proto= option specified IPv6 when IPv6 isn't supported

[PATCH] mount.nfs: return error if proto= option specified IPv6 when IPv6 isn't supported

[Jeff Layton]

Right now, there's nothing that expressly forbids someone from
specifying proto=tcp6 for instance, even when nfs-utils it built without
IPv6 support. This may not work well if (for instance) they are using
NFSv3, since statd won't support IPv6. Explicitly return an error if
someone specifies an IPv6 proto= or mountproto= option and IPv6 isn't
supported.

Signed-off-by: Jeff Layton <jlayton@redhat.com>
---
 utils/mount/network.c |   34 +++++++++++++++++++++++++++-------
 1 files changed, 27 insertions(+), 7 deletions(-)

diff --git a/utils/mount/network.c b/utils/mount/network.c
index c400dd8..ad165f4 100644
--- a/utils/mount/network.c
+++ b/utils/mount/network.c
@@ -1334,9 +1334,26 @@ nfs_nfs_port(struct mount_options *options, unsigned long *port)
 
 #ifdef IPV6_SUPPORTED
 sa_family_t	config_default_family = AF_UNSPEC;
-#else
+
+static int
+nfs_verify_family(sa_family_t family)
+{
+	return 1;
+}
+#else /* IPV6_SUPPORTED */
 sa_family_t	config_default_family = AF_INET;
-#endif
+
+static int
+nfs_verify_family(sa_family_t family)
+{
+	if (family != AF_INET) {
+		errno = EAFNOSUPPORT;
+		return 0;
+	}
+
+	return 1;
+}
+#endif /* IPV6_SUPPORTED */
 
 /*
  * Returns TRUE and fills in @family if a valid NFS protocol option
@@ -1357,15 +1374,15 @@ int nfs_nfs_proto_family(struct mount_options *options,
 		return 1;
 	case 2: /* proto */
 		option = po_get(options, "proto");
-		if (option != NULL)
-			return nfs_get_proto(option, family, &protocol);
+		if (option != NULL && !nfs_get_proto(option, family, &protocol))
+			return 0;
 	}
 
 	/*
 	 * NFS transport protocol wasn't specified.  Return the
 	 * default address family.
 	 */
-	return 1;
+	return nfs_verify_family(*family);
 }
 
 /*
@@ -1494,8 +1511,11 @@ int nfs_mount_proto_family(struct mount_options *options,
 	*family = config_default_family;
 
 	option = po_get(options, "mountproto");
-	if (option != NULL)
-		return nfs_get_proto(option, family, &protocol);
+	if (option != NULL) {
+		if (!nfs_get_proto(option, family, &protocol))
+			return 0;
+		return nfs_verify_family(*family);
+	}
 
 	/*
 	 * MNT transport protocol wasn't specified.  If the NFS
-- 
1.6.6

Re: [PATCH] mount.nfs: return error if proto= option specified IPv6 when IPv6 isn't supported

[Chuck Lever]

On 02/08/2010 12:14 PM, Jeff Layton wrote:
> Right now, there's nothing that expressly forbids someone from
> specifying proto=tcp6 for instance, even when nfs-utils it built without
> IPv6 support. This may not work well if (for instance) they are using
> NFSv3, since statd won't support IPv6. Explicitly return an error if
> someone specifies an IPv6 proto= or mountproto= option and IPv6 isn't
> supported.
>
> Signed-off-by: Jeff Layton<jlayton@redhat.com>
> ---
>   utils/mount/network.c |   34 +++++++++++++++++++++++++++-------
>   1 files changed, 27 insertions(+), 7 deletions(-)
>
> diff --git a/utils/mount/network.c b/utils/mount/network.c
> index c400dd8..ad165f4 100644
> --- a/utils/mount/network.c
> +++ b/utils/mount/network.c
> @@ -1334,9 +1334,26 @@ nfs_nfs_port(struct mount_options *options, unsigned long *port)
>
>   #ifdef IPV6_SUPPORTED
>   sa_family_t	config_default_family = AF_UNSPEC;
> -#else
> +
> +static int
> +nfs_verify_family(sa_family_t family)
> +{
> +	return 1;
> +}
> +#else /* IPV6_SUPPORTED */
>   sa_family_t	config_default_family = AF_INET;
> -#endif
> +
> +static int
> +nfs_verify_family(sa_family_t family)
> +{
> +	if (family != AF_INET) {
> +		errno = EAFNOSUPPORT;
> +		return 0;

I assume you do this so that mount.nfs emits a proper error message in 
this case.  What about below where nfs_get_proto() returns false?

> +	}
> +
> +	return 1;
> +}
> +#endif /* IPV6_SUPPORTED */
>
>   /*
>    * Returns TRUE and fills in @family if a valid NFS protocol option
> @@ -1357,15 +1374,15 @@ int nfs_nfs_proto_family(struct mount_options *options,
>   		return 1;
>   	case 2: /* proto */
>   		option = po_get(options, "proto");
> -		if (option != NULL)
> -			return nfs_get_proto(option, family,&protocol);
> +		if (option != NULL&&  !nfs_get_proto(option, family,&protocol))
> +			return 0;
>   	}
>
>   	/*
>   	 * NFS transport protocol wasn't specified.  Return the
>   	 * default address family.
>   	 */

It would help if this comment mentioned why you need to verify *family 
in the default case.

> -	return 1;
> +	return nfs_verify_family(*family);
>   }
>
>   /*
> @@ -1494,8 +1511,11 @@ int nfs_mount_proto_family(struct mount_options *options,
>   	*family = config_default_family;
>
>   	option = po_get(options, "mountproto");
> -	if (option != NULL)
> -		return nfs_get_proto(option, family,&protocol);
> +	if (option != NULL) {
> +		if (!nfs_get_proto(option, family,&protocol))
> +			return 0;
> +		return nfs_verify_family(*family);
> +	}
>
>   	/*
>   	 * MNT transport protocol wasn't specified.  If the NFS

-- 
chuck[dot]lever[at]oracle[dot]com

Re: [PATCH] mount.nfs: return error if proto= option specified IPv6 when IPv6 isn't supported

[Jeff Layton]

On Mon, 08 Feb 2010 12:27:24 -0500
Chuck Lever <chuck.lever@oracle.com> wrote:

> On 02/08/2010 12:14 PM, Jeff Layton wrote:
> > Right now, there's nothing that expressly forbids someone from
> > specifying proto=tcp6 for instance, even when nfs-utils it built without
> > IPv6 support. This may not work well if (for instance) they are using
> > NFSv3, since statd won't support IPv6. Explicitly return an error if
> > someone specifies an IPv6 proto= or mountproto= option and IPv6 isn't
> > supported.
> >
> > Signed-off-by: Jeff Layton<jlayton@redhat.com>
> > ---
> >   utils/mount/network.c |   34 +++++++++++++++++++++++++++-------
> >   1 files changed, 27 insertions(+), 7 deletions(-)
> >
> > diff --git a/utils/mount/network.c b/utils/mount/network.c
> > index c400dd8..ad165f4 100644
> > --- a/utils/mount/network.c
> > +++ b/utils/mount/network.c
> > @@ -1334,9 +1334,26 @@ nfs_nfs_port(struct mount_options *options, unsigned long *port)
> >
> >   #ifdef IPV6_SUPPORTED
> >   sa_family_t	config_default_family = AF_UNSPEC;
> > -#else
> > +
> > +static int
> > +nfs_verify_family(sa_family_t family)
> > +{
> > +	return 1;
> > +}
> > +#else /* IPV6_SUPPORTED */
> >   sa_family_t	config_default_family = AF_INET;
> > -#endif
> > +
> > +static int
> > +nfs_verify_family(sa_family_t family)
> > +{
> > +	if (family != AF_INET) {
> > +		errno = EAFNOSUPPORT;
> > +		return 0;
> 
> I assume you do this so that mount.nfs emits a proper error message in 
> this case.  What about below where nfs_get_proto() returns false?
> 

Yep, otherwise if you specify a "Defaultproto=tcp6" in the config file,
you'll get:

mount.nfs: Unable to set default family : Success

...which is a little confusing.

I'm not sure what to do about nfs_get_proto returning false. default_value()
calls this before the address family stuff:

                        if (!nfs_nfs_protocol(options, &config_default_proto)) {
                                xlog_warn("Unable to set default protocol : %s",
                                        strerror(errno));
                        }

...and I assume that it'll also always display "Success" there too,
since errno doesn't seem to be set by nfs_get_proto. Maybe
nfs_get_proto() should set errno?

Again though, that sort of seems like a separate patch from this.

> > +	}
> > +
> > +	return 1;
> > +}
> > +#endif /* IPV6_SUPPORTED */
> >
> >   /*
> >    * Returns TRUE and fills in @family if a valid NFS protocol option
> > @@ -1357,15 +1374,15 @@ int nfs_nfs_proto_family(struct mount_options *options,
> >   		return 1;
> >   	case 2: /* proto */
> >   		option = po_get(options, "proto");
> > -		if (option != NULL)
> > -			return nfs_get_proto(option, family,&protocol);
> > +		if (option != NULL&&  !nfs_get_proto(option, family,&protocol))
> > +			return 0;
> >   	}
> >
> >   	/*
> >   	 * NFS transport protocol wasn't specified.  Return the
> >   	 * default address family.
> >   	 */
> 
> It would help if this comment mentioned why you need to verify *family 
> in the default case.
> 
> > -	return 1;
> > +	return nfs_verify_family(*family);

I suppose we don't really need to check it for the default case. I just
did that since it made fewer conditionals in the code. I can reorganize
it so that it's not needed if that'll be clearer.

> >   }
> >
> >   /*
> > @@ -1494,8 +1511,11 @@ int nfs_mount_proto_family(struct mount_options *options,
> >   	*family = config_default_family;
> >
> >   	option = po_get(options, "mountproto");
> > -	if (option != NULL)
> > -		return nfs_get_proto(option, family,&protocol);
> > +	if (option != NULL) {
> > +		if (!nfs_get_proto(option, family,&protocol))
> > +			return 0;
> > +		return nfs_verify_family(*family);
> > +	}
> >
> >   	/*
> >   	 * MNT transport protocol wasn't specified.  If the NFS
> 


-- 
Jeff Layton <jlayton@redhat.com>

Re: [PATCH] mount.nfs: return error if proto= option specified IPv6 when IPv6 isn't supported

[Chuck Lever]

On 02/08/2010 12:43 PM, Jeff Layton wrote:
> On Mon, 08 Feb 2010 12:27:24 -0500
> Chuck Lever<chuck.lever@oracle.com>  wrote:
>
>> On 02/08/2010 12:14 PM, Jeff Layton wrote:
>>> Right now, there's nothing that expressly forbids someone from
>>> specifying proto=tcp6 for instance, even when nfs-utils it built without
>>> IPv6 support. This may not work well if (for instance) they are using
>>> NFSv3, since statd won't support IPv6. Explicitly return an error if
>>> someone specifies an IPv6 proto= or mountproto= option and IPv6 isn't
>>> supported.
>>>
>>> Signed-off-by: Jeff Layton<jlayton@redhat.com>
>>> ---
>>>    utils/mount/network.c |   34 +++++++++++++++++++++++++++-------
>>>    1 files changed, 27 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/utils/mount/network.c b/utils/mount/network.c
>>> index c400dd8..ad165f4 100644
>>> --- a/utils/mount/network.c
>>> +++ b/utils/mount/network.c
>>> @@ -1334,9 +1334,26 @@ nfs_nfs_port(struct mount_options *options, unsigned long *port)
>>>
>>>    #ifdef IPV6_SUPPORTED
>>>    sa_family_t	config_default_family = AF_UNSPEC;
>>> -#else
>>> +
>>> +static int
>>> +nfs_verify_family(sa_family_t family)
>>> +{
>>> +	return 1;
>>> +}
>>> +#else /* IPV6_SUPPORTED */
>>>    sa_family_t	config_default_family = AF_INET;
>>> -#endif
>>> +
>>> +static int
>>> +nfs_verify_family(sa_family_t family)
>>> +{
>>> +	if (family != AF_INET) {
>>> +		errno = EAFNOSUPPORT;
>>> +		return 0;
>>
>> I assume you do this so that mount.nfs emits a proper error message in
>> this case.  What about below where nfs_get_proto() returns false?
>>
>
> Yep, otherwise if you specify a "Defaultproto=tcp6" in the config file,
> you'll get:
>
> mount.nfs: Unable to set default family : Success
>
> ...which is a little confusing.
>
> I'm not sure what to do about nfs_get_proto returning false. default_value()
> calls this before the address family stuff:
>
>                          if (!nfs_nfs_protocol(options,&config_default_proto)) {
>                                  xlog_warn("Unable to set default protocol : %s",
>                                          strerror(errno));
>                          }

By the way, "%m" is equivalent to "%s", strerror(errno)

> ...and I assume that it'll also always display "Success" there too,
> since errno doesn't seem to be set by nfs_get_proto. Maybe
> nfs_get_proto() should set errno?

Maybe.

I would think that nfs_nfs_protocol, being a part of the mount command 
proper, should worry about what error message mount.nfs should display. 
  The functions in getport.c are supposed to be more generic, and often 
set the rpccreate_err fields instead of or in addition to errno.  (Never 
mind that nobody but mount.nfs uses them).

So, I'd say you probably should have nfs_nfs_protocol set errno as 
needed, as you did with nfs_verify_family.

> Again though, that sort of seems like a separate patch from this.
 >
>>> +	}
>>> +
>>> +	return 1;
>>> +}
>>> +#endif /* IPV6_SUPPORTED */
>>>
>>>    /*
>>>     * Returns TRUE and fills in @family if a valid NFS protocol option
>>> @@ -1357,15 +1374,15 @@ int nfs_nfs_proto_family(struct mount_options *options,
>>>    		return 1;
>>>    	case 2: /* proto */
>>>    		option = po_get(options, "proto");
>>> -		if (option != NULL)
>>> -			return nfs_get_proto(option, family,&protocol);
>>> +		if (option != NULL&&   !nfs_get_proto(option, family,&protocol))
>>> +			return 0;
>>>    	}
>>>
>>>    	/*
>>>    	 * NFS transport protocol wasn't specified.  Return the
>>>    	 * default address family.
>>>    	 */
>>
>> It would help if this comment mentioned why you need to verify *family
>> in the default case.
>>
>>> -	return 1;
>>> +	return nfs_verify_family(*family);
>
> I suppose we don't really need to check it for the default case. I just
> did that since it made fewer conditionals in the code. I can reorganize
> it so that it's not needed if that'll be clearer.
>
>>>    }
>>>
>>>    /*
>>> @@ -1494,8 +1511,11 @@ int nfs_mount_proto_family(struct mount_options *options,
>>>    	*family = config_default_family;
>>>
>>>    	option = po_get(options, "mountproto");
>>> -	if (option != NULL)
>>> -		return nfs_get_proto(option, family,&protocol);
>>> +	if (option != NULL) {
>>> +		if (!nfs_get_proto(option, family,&protocol))
>>> +			return 0;
>>> +		return nfs_verify_family(*family);
>>> +	}
>>>
>>>    	/*
>>>    	 * MNT transport protocol wasn't specified.  If the NFS
>>
>
>


-- 
chuck[dot]lever[at]oracle[dot]com