[PATCH] nfs-utils: Backgrounding mount broken with NFS versions <4

[PATCH] nfs-utils: Backgrounding mount broken with NFS versions <4

[Wolfram Gloger]

When the NFS version isn't specified in the mount options, mount.nfs
attempts V4 first and appends 'vers=4' to the extra_options string in
the mount options.  If the server isn't immediately reachable, this
attempt fails.  However, if the background option is specified and the
server comes up later on, the extra_options are used again for all
further attempts and thus they fail if the server only supports
vers<4.

Fix this by only amending extra_options on a successful vers=4 mount.

This is now Debian bug #690181 and has apparently been around for
ages.

Signed-off-by: Wolfram Gloger <bugzilla1@malloc.de>

--- utils/mount/stropts.c.orig	2012-08-23 19:41:56.000000000 +0200
+++ utils/mount/stropts.c	2012-10-11 13:46:25.000000000 +0200
@@ -680,6 +680,7 @@
 {
 	struct mount_options *options = po_dup(mi->options);
 	int result = 0;
+	char *extra_opts = NULL;
 
 	if (!options) {
 		errno = ENOMEM;
@@ -715,20 +716,26 @@
 		goto out_fail;
 	}
 
-	/*
-	 * Update option string to be recorded in /etc/mtab.
-	 */
-	if (po_join(options, mi->extra_opts) == PO_FAILED) {
+	if (po_join(options, &extra_opts) == PO_FAILED) {
 		errno = ENOMEM;
 		goto out_fail;
 	}
 
 	if (verbose)
 		printf(_("%s: trying text-based options '%s'\n"),
-			progname, *mi->extra_opts);
+			progname, extra_opts);
 
 	result = nfs_sys_mount(mi, options);
 
+	/*
+	 * If success, update option string to be recorded in /etc/mtab.
+	 */
+	if (result) {
+	    free(*mi->extra_opts);
+	    *mi->extra_opts = extra_opts;
+	} else
+	    free(extra_opts);
+
 out_fail:
 	po_destroy(options);
 	return result;

Re: [PATCH] nfs-utils: Backgrounding mount broken with NFS versions <4

[Chuck Lever]

On Oct 12, 2012, at 8:00 AM, Wolfram Gloger <wmglo@dent.med.uni-muenchen.de> wrote:

> When the NFS version isn't specified in the mount options, mount.nfs
> attempts V4 first and appends 'vers=4' to the extra_options string in
> the mount options.  If the server isn't immediately reachable, this
> attempt fails.  However, if the background option is specified and the
> server comes up later on, the extra_options are used again for all
> further attempts and thus they fail if the server only supports
> vers<4.
> 
> Fix this by only amending extra_options on a successful vers=4 mount.
> 
> This is now Debian bug #690181 and has apparently been around for
> ages.
> 
> Signed-off-by: Wolfram Gloger <bugzilla1@malloc.de>
> 
> --- utils/mount/stropts.c.orig	2012-08-23 19:41:56.000000000 +0200
> +++ utils/mount/stropts.c	2012-10-11 13:46:25.000000000 +0200
> @@ -680,6 +680,7 @@
> {
> 	struct mount_options *options = po_dup(mi->options);
> 	int result = 0;
> +	char *extra_opts = NULL;
> 
> 	if (!options) {
> 		errno = ENOMEM;
> @@ -715,20 +716,26 @@
> 		goto out_fail;
> 	}
> 
> -	/*
> -	 * Update option string to be recorded in /etc/mtab.
> -	 */
> -	if (po_join(options, mi->extra_opts) == PO_FAILED) {
> +	if (po_join(options, &extra_opts) == PO_FAILED) {

This doesn't look right to me, but I haven't had time to test it.  Doesn't this hunk cause the mount system call to ignore what's in mi->extra_opts?

> 		errno = ENOMEM;
> 		goto out_fail;
> 	}
> 
> 	if (verbose)
> 		printf(_("%s: trying text-based options '%s'\n"),
> -			progname, *mi->extra_opts);
> +			progname, extra_opts);

> 	result = nfs_sys_mount(mi, options);
> 
> +	/*
> +	 * If success, update option string to be recorded in /etc/mtab.
> +	 */
> +	if (result) {
> +	    free(*mi->extra_opts);
> +	    *mi->extra_opts = extra_opts;
> +	} else
> +	    free(extra_opts);
> +
> out_fail:
> 	po_destroy(options);
> 	return result;
> --
> 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

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

Re: [PATCH] nfs-utils: Backgrounding mount broken with NFS versions <4

[Wolfram Gloger]

Hi,

> > -	/*
> > -	 * Update option string to be recorded in /etc/mtab.
> > -	 */
> > -	if (po_join(options, mi->extra_opts) == PO_FAILED) {
> > +	if (po_join(options, &extra_opts) == PO_FAILED) {
> 
> This doesn't look right to me, but I haven't had time to test it.  Doesn't this hunk cause the mount system call to ignore what's in mi->extra_opts?
...
> > 	result = nfs_sys_mount(mi, options);

No, nfs_sys_mount() does not use mi->extra_opts at all, only the
binary options.

It would perhaps be clearer to handle the update of mi->extra_opts in
nfs_sys_mount(), but only after a successful mount(2) call.
A more invasive patch.

Regards,
Wolfram.

Re: [PATCH] nfs-utils: Backgrounding mount broken with NFS versions <4

[Chuck Lever]

On Oct 13, 2012, at 1:18 PM, Wolfram Gloger <wmglo@dent.med.uni-muenchen.de> wrote:

> Hi,
> 
>>> -	/*
>>> -	 * Update option string to be recorded in /etc/mtab.
>>> -	 */
>>> -	if (po_join(options, mi->extra_opts) == PO_FAILED) {
>>> +	if (po_join(options, &extra_opts) == PO_FAILED) {
>> 
>> This doesn't look right to me, but I haven't had time to test it.  Doesn't this hunk cause the mount system call to ignore what's in mi->extra_opts?
> ...
>>> 	result = nfs_sys_mount(mi, options);
> 
> No, nfs_sys_mount() does not use mi->extra_opts at all, only the
> binary options.

This is the text-based code, which I wrote.  nfs_sys_mount() passes an options string (NUL-terminated C string) to the kernel, not a binary object.  That string contains all the FS-specific mount options specified by the user.

But your patch makes that string empty, by my reading.  I think this is incorrect.

> It would perhaps be clearer to handle the update of mi->extra_opts in
> nfs_sys_mount(), but only after a successful mount(2) call.
> A more invasive patch.
> 
> Regards,
> Wolfram.

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

Re: [PATCH] nfs-utils: Backgrounding mount broken with NFS versions <4

[Wolfram Gloger]

Hi,

> >>> 	result = nfs_sys_mount(mi, options);
> > 
> > No, nfs_sys_mount() does not use mi->extra_opts at all, only the
> > binary options.
> 
> This is the text-based code, which I wrote.  nfs_sys_mount() passes an options string (NUL-terminated C string) to the kernel, not a binary object.  That string contains all the FS-specific mount options specified by the user.
> 
> But your patch makes that string empty, by my reading.  I think this is incorrect.

Ok, I'm happy to go through this line-by-line.

static int nfs_sys_mount(struct nfsmount_info *mi, struct mount_options *opts)
{
        char *options = NULL;
        int result;

        if (mi->fake)
                return 1;

        if (po_join(opts, &options) == PO_FAILED) { // HERE
                errno = EIO;
                return 0;
        }

        result = mount(mi->spec, mi->node, mi->type,
                        mi->flags & ~(MS_USER|MS_USERS), options);
...
}

nfs_sys_mount() constructs the text options _itself_ purely from the
opts (2nd) argument HERE -- po_join has opts as input and options as
output.

My patch only changes the first argument (mi).  So, no functional change
within nfs_sys_mount() at all.

The functional change is that with my patch, mi->extra_opts is kept
unchanged unless the system call is successful.  mi->extra_opts is
actually reused later throughout the mount program, because
nfsmount_string() has extra_opts as an input _and_ output argument,
and propagates mi->extra_opts into the extra_opts variable in main.c.

I have actually tested this patch extensively and it fixes the
problem.

Regards,
Wolfram.

Re: [PATCH] nfs-utils: Backgrounding mount broken with NFS versions <4

[Chuck Lever]

On Oct 13, 2012, at 5:46 PM, Wolfram Gloger <wmglo@dent.med.uni-muenchen.de> wrote:

> Hi,
> 
>>>>> 	result = nfs_sys_mount(mi, options);
>>> 
>>> No, nfs_sys_mount() does not use mi->extra_opts at all, only the
>>> binary options.
>> 
>> This is the text-based code, which I wrote.  nfs_sys_mount() passes an options string (NUL-terminated C string) to the kernel, not a binary object.  That string contains all the FS-specific mount options specified by the user.
>> 
>> But your patch makes that string empty, by my reading.  I think this is incorrect.
> 
> Ok, I'm happy to go through this line-by-line.
> 
> static int nfs_sys_mount(struct nfsmount_info *mi, struct mount_options *opts)
> {
>        char *options = NULL;
>        int result;
> 
>        if (mi->fake)
>                return 1;
> 
>        if (po_join(opts, &options) == PO_FAILED) { // HERE
>                errno = EIO;
>                return 0;
>        }
> 
>        result = mount(mi->spec, mi->node, mi->type,
>                        mi->flags & ~(MS_USER|MS_USERS), options);
> ...
> }
> 
> nfs_sys_mount() constructs the text options _itself_ purely from the
> opts (2nd) argument HERE -- po_join has opts as input and options as
> output.
> 
> My patch only changes the first argument (mi).  So, no functional change
> within nfs_sys_mount() at all.

Agreed.

> The functional change is that with my patch, mi->extra_opts is kept
> unchanged unless the system call is successful.  mi->extra_opts is
> actually reused later throughout the mount program, because
> nfsmount_string() has extra_opts as an input _and_ output argument,
> and propagates mi->extra_opts into the extra_opts variable in main.c.
> 
> I have actually tested this patch extensively and it fixes the
> problem.

Unfortunately there are a rather unimaginable number of use cases for mount.nfs, which is why it is so complicated.  "Extensively" could mean you've tested it for a long time but with just a few use cases.

In any event:

Reviewed-by: Chuck Lever <chuck.lever@oracle.com>

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

Re: [PATCH] nfs-utils: Backgrounding mount broken with NFS versions <4

[Wolfram Gloger]

Hi,

> Unfortunately there are a rather unimaginable number of use cases for mount.nfs, which is why it is so complicated.  "Extensively" could mean you've tested it for a long time but with just a few use cases.

Admitted, I certainly didn't run an extensive test suite, mainly just
the case in question by simulating server down with a temporary
iptables DROP rule.

> In any event:
> 
> Reviewed-by: Chuck Lever <chuck.lever@oracle.com>

Thanks,
Wolfram.

Re: [PATCH] nfs-utils: Backgrounding mount broken with NFS versions <4

[Steve Dickson]

On 12/10/12 08:00, Wolfram Gloger wrote:
> When the NFS version isn't specified in the mount options, mount.nfs
> attempts V4 first and appends 'vers=4' to the extra_options string in
> the mount options.  If the server isn't immediately reachable, this
> attempt fails.  However, if the background option is specified and the
> server comes up later on, the extra_options are used again for all
> further attempts and thus they fail if the server only supports
> vers<4.
> 
> Fix this by only amending extra_options on a successful vers=4 mount.
> 
> This is now Debian bug #690181 and has apparently been around for
> ages.
> 
> Signed-off-by: Wolfram Gloger <bugzilla1@malloc.de>
Committed....

steved.
> 
> --- utils/mount/stropts.c.orig	2012-08-23 19:41:56.000000000 +0200
> +++ utils/mount/stropts.c	2012-10-11 13:46:25.000000000 +0200
> @@ -680,6 +680,7 @@
>  {
>  	struct mount_options *options = po_dup(mi->options);
>  	int result = 0;
> +	char *extra_opts = NULL;
>  
>  	if (!options) {
>  		errno = ENOMEM;
> @@ -715,20 +716,26 @@
>  		goto out_fail;
>  	}
>  
> -	/*
> -	 * Update option string to be recorded in /etc/mtab.
> -	 */
> -	if (po_join(options, mi->extra_opts) == PO_FAILED) {
> +	if (po_join(options, &extra_opts) == PO_FAILED) {
>  		errno = ENOMEM;
>  		goto out_fail;
>  	}
>  
>  	if (verbose)
>  		printf(_("%s: trying text-based options '%s'\n"),
> -			progname, *mi->extra_opts);
> +			progname, extra_opts);
>  
>  	result = nfs_sys_mount(mi, options);
>  
> +	/*
> +	 * If success, update option string to be recorded in /etc/mtab.
> +	 */
> +	if (result) {
> +	    free(*mi->extra_opts);
> +	    *mi->extra_opts = extra_opts;
> +	} else
> +	    free(extra_opts);
> +
>  out_fail:
>  	po_destroy(options);
>  	return result;
> --
> 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
>