[PATCH] NFS: Restore missing hunk in NFS mount option parser

[PATCH] NFS: Restore missing hunk in NFS mount option parser

[Chuck Lever]

Commit f45663ce5fb30f76a3414ab3ac69f4dd320e760a was missing a hunk that
prevented the new "sloppy" mount option from having any effect.

Tested against 2.6.27-rc.  2.6.26 is not affected.

Thanks to Neil Brown for reporting this.

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

 fs/nfs/super.c |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)

diff --git a/fs/nfs/super.c b/fs/nfs/super.c
index 5b2aa04..e3ac650 100644
--- a/fs/nfs/super.c
+++ b/fs/nfs/super.c
@@ -1279,6 +1279,12 @@ static int nfs_parse_mount_options(char *raw,
 		}
 	}
 
+	if (errors > 0) {
+		dfprintk(MOUNT, "NFS: parsing encountered %d error%s\n",
+				errors, (errors == 1 ? "" : "s"));
+		if (!sloppy)
+			return 0;
+	}
 	return 1;
 
 out_nomem:

Re: [PATCH] NFS: Restore missing hunk in NFS mount option parser

[J. Bruce Fields]

On Fri, Sep 05, 2008 at 02:16:07PM -0400, Chuck Lever wrote:
> Commit f45663ce5fb30f76a3414ab3ac69f4dd320e760a was missing a hunk that
> prevented the new "sloppy" mount option from having any effect.

I don't think that on its own would justify sending it in for 2.6.27.[1]

> 
> Tested against 2.6.27-rc.  2.6.26 is not affected.

But if I understand right, the effect of leaving out this chunk was to
make the *default* behavior "sloppy"?  Which seems a drastic change from
the previous behavior.  And it's a simple enough patch.

So I'd be inclined to agree and send it in for 2.6.27....  If Trond
doesn't poke his head up by tomorrow, let's go ahead--feel free to send
it to Linus with my

	Acked-by: J. Bruce Fields <bfields@citi.umich.edu>

if I fall off the face of the earth tomorrow.

--b.

[1] Linus has been pretty hard on -rc patches lately:

	http://marc.info/?l=linux-netdev&m=122048427801318&w=2

	"Here's a simple rule of thumb:
	 - if it's not on the regression list
	 - if it's not a reported security hole
	 - if it's not on the reported oopses list
	then why are people sending it to me?"


> 
> Thanks to Neil Brown for reporting this.
> 
> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
> ---
> 
>  fs/nfs/super.c |    6 ++++++
>  1 files changed, 6 insertions(+), 0 deletions(-)
> 
> diff --git a/fs/nfs/super.c b/fs/nfs/super.c
> index 5b2aa04..e3ac650 100644
> --- a/fs/nfs/super.c
> +++ b/fs/nfs/super.c
> @@ -1279,6 +1279,12 @@ static int nfs_parse_mount_options(char *raw,
>  		}
>  	}
>  
> +	if (errors > 0) {
> +		dfprintk(MOUNT, "NFS: parsing encountered %d error%s\n",
> +				errors, (errors == 1 ? "" : "s"));
> +		if (!sloppy)
> +			return 0;
> +	}
>  	return 1;
>  
>  out_nomem:
> 

Re: [PATCH] NFS: Restore missing hunk in NFS mount option parser

[Chuck Lever]

On Fri, Sep 5, 2008 at 5:11 PM, J. Bruce Fields <bfields@fieldses.org> wrote:
> On Fri, Sep 05, 2008 at 02:16:07PM -0400, Chuck Lever wrote:
>> Commit f45663ce5fb30f76a3414ab3ac69f4dd320e760a was missing a hunk that
>> prevented the new "sloppy" mount option from having any effect.
>
> I don't think that on its own would justify sending it in for 2.6.27.[1]

The original patch for 27 was supposed to fix a regression (ie
automounter stopped working in heterogenous environments).  This patch
does fix the full regression.

There is already logic in nfs-utils-1.1.3 that maps the "-s" option
(which has been around for EVAR) to "-o sloppy".  This logic is
enabled for 2.6.27 kernels and later.  So this does need to go in 27.

Would it help if I rewrote the description?

>> Tested against 2.6.27-rc.  2.6.26 is not affected.
>
> But if I understand right, the effect of leaving out this chunk was to
> make the *default* behavior "sloppy"?  Which seems a drastic change from
> the previous behavior.  And it's a simple enough patch.

No, the default behavior is as before.  The behavior without this
patch is that the kernel recognizes "sloppy" but it doesn't do
anything about it.

> So I'd be inclined to agree and send it in for 2.6.27....  If Trond
> doesn't poke his head up by tomorrow, let's go ahead--feel free to send
> it to Linus with my
>
>        Acked-by: J. Bruce Fields <bfields@citi.umich.edu>
>
> if I fall off the face of the earth tomorrow.
>
> --b.
>
> [1] Linus has been pretty hard on -rc patches lately:
>
>        http://marc.info/?l=linux-netdev&m=122048427801318&w=2
>
>        "Here's a simple rule of thumb:
>         - if it's not on the regression list
>         - if it's not a reported security hole
>         - if it's not on the reported oopses list
>        then why are people sending it to me?"
>>
>> Thanks to Neil Brown for reporting this.
>>
>> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
>> ---
>>
>>  fs/nfs/super.c |    6 ++++++
>>  1 files changed, 6 insertions(+), 0 deletions(-)
>>
>> diff --git a/fs/nfs/super.c b/fs/nfs/super.c
>> index 5b2aa04..e3ac650 100644
>> --- a/fs/nfs/super.c
>> +++ b/fs/nfs/super.c
>> @@ -1279,6 +1279,12 @@ static int nfs_parse_mount_options(char *raw,
>>               }
>>       }
>>
>> +     if (errors > 0) {
>> +             dfprintk(MOUNT, "NFS: parsing encountered %d error%s\n",
>> +                             errors, (errors == 1 ? "" : "s"));
>> +             if (!sloppy)
>> +                     return 0;
>> +     }
>>       return 1;
>>
>>  out_nomem:
>>
> --
> 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
>



-- 
"If you simplify your English, you are freed from the worst follies of
orthodoxy."
 -- George Orwell

Re: [PATCH] NFS: Restore missing hunk in NFS mount option parser

[J. Bruce Fields]

On Fri, Sep 05, 2008 at 05:25:31PM -0400, Chuck Lever wrote:
> On Fri, Sep 5, 2008 at 5:11 PM, J. Bruce Fields <bfields@fieldses.org> wrote:
> > On Fri, Sep 05, 2008 at 02:16:07PM -0400, Chuck Lever wrote:
> >> Commit f45663ce5fb30f76a3414ab3ac69f4dd320e760a was missing a hunk that
> >> prevented the new "sloppy" mount option from having any effect.
> >
> > I don't think that on its own would justify sending it in for 2.6.27.[1]
> 
> The original patch for 27 was supposed to fix a regression (ie
> automounter stopped working in heterogenous environments).  This patch
> does fix the full regression.
> 
> There is already logic in nfs-utils-1.1.3 that maps the "-s" option
> (which has been around for EVAR) to "-o sloppy".  This logic is
> enabled for 2.6.27 kernels and later.  So this does need to go in 27.
> 
> Would it help if I rewrote the description?

Could be.  Assume I'm tired and stupid....

> >> Tested against 2.6.27-rc.  2.6.26 is not affected.
> >
> > But if I understand right, the effect of leaving out this chunk was to
> > make the *default* behavior "sloppy"?  Which seems a drastic change from
> > the previous behavior.  And it's a simple enough patch.
> 
> No, the default behavior is as before.  The behavior without this
> patch is that the kernel recognizes "sloppy" but it doesn't do
> anything about it.

That can't be right:

> >> +     if (errors > 0) {
> >> +             dfprintk(MOUNT, "NFS: parsing encountered %d error%s\n",
> >> +                             errors, (errors == 1 ? "" : "s"));
> >> +             if (!sloppy)
> >> +                     return 0;
> >> +     }
> >>       return 1;

Ignoring the printk, the *only* change in behavior here happens when
sloppy is *not* set.  Right?

--b.  (not a big fan of not's)

Re: [PATCH] NFS: Restore missing hunk in NFS mount option parser

[Chuck Lever]

On Sep 5, 2008, at Sep 5, 2008, 5:34 PM, J. Bruce Fields wrote:
> On Fri, Sep 05, 2008 at 05:25:31PM -0400, Chuck Lever wrote:
>> On Fri, Sep 5, 2008 at 5:11 PM, J. Bruce Fields  
>> <bfields@fieldses.org> wrote:
>>> On Fri, Sep 05, 2008 at 02:16:07PM -0400, Chuck Lever wrote:
>>>> Commit f45663ce5fb30f76a3414ab3ac69f4dd320e760a was missing a  
>>>> hunk that
>>>> prevented the new "sloppy" mount option from having any effect.
>>>
>>> I don't think that on its own would justify sending it in for  
>>> 2.6.27.[1]
>>
>> The original patch for 27 was supposed to fix a regression (ie
>> automounter stopped working in heterogenous environments).  This  
>> patch
>> does fix the full regression.
>>
>> There is already logic in nfs-utils-1.1.3 that maps the "-s" option
>> (which has been around for EVAR) to "-o sloppy".  This logic is
>> enabled for 2.6.27 kernels and later.  So this does need to go in 27.
>>
>> Would it help if I rewrote the description?
>
> Could be.  Assume I'm tired and stupid....
>
>>>> Tested against 2.6.27-rc.  2.6.26 is not affected.
>>>
>>> But if I understand right, the effect of leaving out this chunk  
>>> was to
>>> make the *default* behavior "sloppy"?  Which seems a drastic  
>>> change from
>>> the previous behavior.  And it's a simple enough patch.
>>
>> No, the default behavior is as before.  The behavior without this
>> patch is that the kernel recognizes "sloppy" but it doesn't do
>> anything about it.
>
> That can't be right:
>
>>>> +     if (errors > 0) {
>>>> +             dfprintk(MOUNT, "NFS: parsing encountered %d error%s 
>>>> \n",
>>>> +                             errors, (errors == 1 ? "" : "s"));
>>>> +             if (!sloppy)
>>>> +                     return 0;
>>>> +     }
>>>>      return 1;
>
> Ignoring the printk, the *only* change in behavior here happens when
> sloppy is *not* set.  Right?


Hrm.  Without today's patch, the behavior is always sloppy, and  
"sloppy" is recognized as a valid option (not that it matters).  With  
the patch, the default behavior is as before, and the sloppy option is  
recognized.

So yes, this is a regression in both senses.


I'll rewrite the description and repost.

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

[PATCH] NFS: Restore missing hunk in NFS mount option parser

[Chuck Lever]

Automounter maps can contain mount options valid for other NFS
implementations but not for Linux.  The Linux automounter uses the
mount command's "-s" command line option ("s" for "sloppy") so that
mount requests containing such options are not rejected.

Commit f45663ce5fb30f76a3414ab3ac69f4dd320e760a attempted to address a
known regression with text-based NFS mount option parsing.  Unrecognized
mount options would cause mount requests to fail, even if the "-s"
option was used on the mount command line.

Unfortunately, this commit was not complete as submitted.  The commit
adds a new mount option, "sloppy".  But it is missing a hunk, so it now
allows NFS mounts with unrecognized mount options, even if the "sloppy"
option is not present.  This could be a problem if a required critical
mount option such as "sync" is misspelled, for example, and is considered
a regression from 2.6.26.

This patch restores the missing hunk.  Now, the default behavior of
text-based NFS mount options is as before: any unrecognized mount option
will cause the mount to fail.

Please include this in 2.6.27-rc.

Thanks to Neil Brown for spotting this.

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

 fs/nfs/super.c |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)

diff --git a/fs/nfs/super.c b/fs/nfs/super.c
index 5b2aa04..e3ac650 100644
--- a/fs/nfs/super.c
+++ b/fs/nfs/super.c
@@ -1279,6 +1279,12 @@ static int nfs_parse_mount_options(char *raw,
 		}
 	}
 
+	if (errors > 0) {
+		dfprintk(MOUNT, "NFS: parsing encountered %d error%s\n",
+				errors, (errors == 1 ? "" : "s"));
+		if (!sloppy)
+			return 0;
+	}
 	return 1;
 
 out_nomem:

[PATCH] NFS: Restore missing hunk in NFS mount option parser

[Chuck Lever]

Automounter maps can contain mount options valid for other NFS
implementations but not for Linux.  The Linux automounter uses the
mount command's "-s" command line option ("s" for "sloppy") so that
mount requests containing such options are not rejected.

Commit f45663ce5fb30f76a3414ab3ac69f4dd320e760a attempted to address a
known regression with text-based NFS mount option parsing.  Unrecognized
mount options would cause mount requests to fail, even if the "-s"
option was used on the mount command line.

Unfortunately, this commit was not complete as submitted.  It adds a
new mount option, "sloppy".  But it is missing a hunk, so it now allows
NFS mounts with unrecognized mount options, even if the "sloppy" option
is not present.  This could be a problem if a required critical mount
option such as "sync" is misspelled, for example, and is considered a
regression from 2.6.26.

This patch restores the missing hunk.  Now, the default behavior of
text-based NFS mount options is as before: any unrecognized mount option
will cause the mount to fail.

Please include this in 2.6.27-rc.

Thanks to Neil Brown for reporting this.

Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
Acked-by: J. Bruce Fields <bfields@citi.umich.edu>
---

 fs/nfs/super.c |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)

diff --git a/fs/nfs/super.c b/fs/nfs/super.c
index 5b2aa04..e3ac650 100644
--- a/fs/nfs/super.c
+++ b/fs/nfs/super.c
@@ -1279,6 +1279,12 @@ static int nfs_parse_mount_options(char *raw,
 		}
 	}
 
+	if (errors > 0) {
+		dfprintk(MOUNT, "NFS: parsing encountered %d error%s\n",
+				errors, (errors == 1 ? "" : "s"));
+		if (!sloppy)
+			return 0;
+	}
 	return 1;
 
 out_nomem: