[PATCH] NFS: Only warn on unrecognized mount options

[PATCH] NFS: Only warn on unrecognized mount options

[Chuck Lever]

To provide compatibility with automounters who use a common set of mount
options for all file systems, change the NFS in-kernel mount option parser
to ignore mount options it doesn't recognize.

Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
---
Yet another NFS mount patch!  Build tested only.  Comments?

 fs/nfs/super.c |    7 ++-----
 1 files changed, 2 insertions(+), 5 deletions(-)

diff --git a/fs/nfs/super.c b/fs/nfs/super.c
index f921902..a7201f0 100644
--- a/fs/nfs/super.c
+++ b/fs/nfs/super.c
@@ -1044,7 +1044,8 @@ static int nfs_parse_mount_options(char *raw,
 			break;
 
 		default:
-			goto out_unknown;
+			printk(KERN_INFO "NFS: unrecognized mount option '%s'"
+					" ignored\n", p);
 		}
 	}
 
@@ -1070,10 +1071,6 @@ out_unrec_xprt:
 out_unrec_sec:
 	printk(KERN_INFO "NFS: unrecognized security flavor\n");
 	return 0;
-
-out_unknown:
-	printk(KERN_INFO "NFS: unknown mount option: %s\n", p);
-	return 0;
 }
 
 /*

Re: [PATCH] NFS: Only warn on unrecognized mount options

[Peter Staubach]

Chuck Lever wrote:
> To provide compatibility with automounters who use a common set of mount
> options for all file systems, change the NFS in-kernel mount option parser
> to ignore mount options it doesn't recognize.
>
> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
> ---
> Yet another NFS mount patch!  Build tested only.  Comments?
>
>  fs/nfs/super.c |    7 ++-----
>  1 files changed, 2 insertions(+), 5 deletions(-)
>
> diff --git a/fs/nfs/super.c b/fs/nfs/super.c
> index f921902..a7201f0 100644
> --- a/fs/nfs/super.c
> +++ b/fs/nfs/super.c
> @@ -1044,7 +1044,8 @@ static int nfs_parse_mount_options(char *raw,
>  			break;
>  
>  		default:
> -			goto out_unknown;
> +			printk(KERN_INFO "NFS: unrecognized mount option '%s'"
> +					" ignored\n", p);
>  		}
>  	}
>  
> @@ -1070,10 +1071,6 @@ out_unrec_xprt:
>  out_unrec_sec:
>  	printk(KERN_INFO "NFS: unrecognized security flavor\n");
>  	return 0;
> -
> -out_unknown:
> -	printk(KERN_INFO "NFS: unknown mount option: %s\n", p);
> -	return 0;
>  }
>  
>  /*

This will potentially cause a very large number of messages to be
printed in a valid deployment.  Do we really need the message?

       ps

Re: [PATCH] NFS: Only warn on unrecognized mount options

[Chuck Lever]

[-- Attachment #1: Type: text/plain, Size: 1493 bytes --]

Peter Staubach wrote:
> Chuck Lever wrote:
>> To provide compatibility with automounters who use a common set of mount
>> options for all file systems, change the NFS in-kernel mount option 
>> parser
>> to ignore mount options it doesn't recognize.
>>
>> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
>> ---
>> Yet another NFS mount patch!  Build tested only.  Comments?
>>
>>  fs/nfs/super.c |    7 ++-----
>>  1 files changed, 2 insertions(+), 5 deletions(-)
>>
>> diff --git a/fs/nfs/super.c b/fs/nfs/super.c
>> index f921902..a7201f0 100644
>> --- a/fs/nfs/super.c
>> +++ b/fs/nfs/super.c
>> @@ -1044,7 +1044,8 @@ static int nfs_parse_mount_options(char *raw,
>>              break;
>>  
>>          default:
>> -            goto out_unknown;
>> +            printk(KERN_INFO "NFS: unrecognized mount option '%s'"
>> +                    " ignored\n", p);
>>          }
>>      }
>>  
>> @@ -1070,10 +1071,6 @@ out_unrec_xprt:
>>  out_unrec_sec:
>>      printk(KERN_INFO "NFS: unrecognized security flavor\n");
>>      return 0;
>> -
>> -out_unknown:
>> -    printk(KERN_INFO "NFS: unknown mount option: %s\n", p);
>> -    return 0;
>>  }
>>  
>>  /*
> 
> This will potentially cause a very large number of messages to be
> printed in a valid deployment.  Do we really need the message?

I was wondering about that.

I left it in because it's useful to know when a valid mount option is 
misspelled.  In that case it might cause an important option (such as 
"noac") to be ignored.

[-- Attachment #2: chuck_lever.vcf --]
[-- Type: text/x-vcard, Size: 259 bytes --]

begin:vcard
fn:Chuck Lever
n:Lever;Chuck
org:Oracle Corporation;Corporate Architecture: Linux Projects Group
adr:;;1015 Granger Avenue;Ann Arbor;MI;48104;USA
title:Principal Member of Staff
tel;work:+1 248 614 5091
x-mozilla-html:FALSE
version:2.1
end:vcard

Re: [PATCH] NFS: Only warn on unrecognized mount options

[Peter Staubach]

Chuck Lever wrote:
> Peter Staubach wrote:
>> Chuck Lever wrote:
>>> To provide compatibility with automounters who use a common set of 
>>> mount
>>> options for all file systems, change the NFS in-kernel mount option 
>>> parser
>>> to ignore mount options it doesn't recognize.
>>>
>>> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
>>> ---
>>> Yet another NFS mount patch!  Build tested only.  Comments?
>>>
>>>  fs/nfs/super.c |    7 ++-----
>>>  1 files changed, 2 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/fs/nfs/super.c b/fs/nfs/super.c
>>> index f921902..a7201f0 100644
>>> --- a/fs/nfs/super.c
>>> +++ b/fs/nfs/super.c
>>> @@ -1044,7 +1044,8 @@ static int nfs_parse_mount_options(char *raw,
>>>              break;
>>>  
>>>          default:
>>> -            goto out_unknown;
>>> +            printk(KERN_INFO "NFS: unrecognized mount option '%s'"
>>> +                    " ignored\n", p);
>>>          }
>>>      }
>>>  
>>> @@ -1070,10 +1071,6 @@ out_unrec_xprt:
>>>  out_unrec_sec:
>>>      printk(KERN_INFO "NFS: unrecognized security flavor\n");
>>>      return 0;
>>> -
>>> -out_unknown:
>>> -    printk(KERN_INFO "NFS: unknown mount option: %s\n", p);
>>> -    return 0;
>>>  }
>>>  
>>>  /*
>>
>> This will potentially cause a very large number of messages to be
>> printed in a valid deployment.  Do we really need the message?
>
> I was wondering about that.
>
> I left it in because it's useful to know when a valid mount option is 
> misspelled.  In that case it might cause an important option (such as 
> "noac") to be ignored.

There do seem to be valid uses for the message.  However, it
could also end up being a bad thing.

Perhaps we could just gather the unknown options and lump
them together in something that would be visible via /proc/mounts
or some such.  Something like "unknown=..." in the options list.

       ps

Re: [PATCH] NFS: Only warn on unrecognized mount options

[Trond Myklebust]

On Fri, 2008-04-11 at 16:03 -0400, Chuck Lever wrote:
> To provide compatibility with automounters who use a common set of mount
> options for all file systems, change the NFS in-kernel mount option parser
> to ignore mount options it doesn't recognize.
> 
> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
> ---
> Yet another NFS mount patch!  Build tested only.  Comments?
> 
>  fs/nfs/super.c |    7 ++-----
>  1 files changed, 2 insertions(+), 5 deletions(-)
> 
> diff --git a/fs/nfs/super.c b/fs/nfs/super.c
> index f921902..a7201f0 100644
> --- a/fs/nfs/super.c
> +++ b/fs/nfs/super.c
> @@ -1044,7 +1044,8 @@ static int nfs_parse_mount_options(char *raw,
>  			break;
>  
>  		default:
> -			goto out_unknown;
> +			printk(KERN_INFO "NFS: unrecognized mount option '%s'"
> +					" ignored\n", p);
>  		}
>  	}
>  
> @@ -1070,10 +1071,6 @@ out_unrec_xprt:
>  out_unrec_sec:
>  	printk(KERN_INFO "NFS: unrecognized security flavor\n");
>  	return 0;
> -
> -out_unknown:
> -	printk(KERN_INFO "NFS: unknown mount option: %s\n", p);
> -	return 0;
>  }
>  
>  /*

This isn't really a very good solution either. Spamming the syslog on
every option that is being ignored isn't going to help the folks with
the global automounter maps. Either the rules should be that 'all
options are allowed' or they should be that 'only recognised NFS options
are allowed'.

-- 
Trond Myklebust
Linux NFS client maintainer

NetApp
Trond.Myklebust@netapp.com
www.netapp.com

Re: [PATCH] NFS: Only warn on unrecognized mount options

[Chuck Lever]

On Apr 11, 2008, at 4:24 PM, Trond Myklebust wrote:
> On Fri, 2008-04-11 at 16:03 -0400, Chuck Lever wrote:
>> To provide compatibility with automounters who use a common set of  
>> mount
>> options for all file systems, change the NFS in-kernel mount option  
>> parser
>> to ignore mount options it doesn't recognize.
>>
>> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
>> ---
>> Yet another NFS mount patch!  Build tested only.  Comments?
>>
>> fs/nfs/super.c |    7 ++-----
>> 1 files changed, 2 insertions(+), 5 deletions(-)
>>
>> diff --git a/fs/nfs/super.c b/fs/nfs/super.c
>> index f921902..a7201f0 100644
>> --- a/fs/nfs/super.c
>> +++ b/fs/nfs/super.c
>> @@ -1044,7 +1044,8 @@ static int nfs_parse_mount_options(char *raw,
>> 			break;
>>
>> 		default:
>> -			goto out_unknown;
>> +			printk(KERN_INFO "NFS: unrecognized mount option '%s'"
>> +					" ignored\n", p);
>> 		}
>> 	}
>>
>> @@ -1070,10 +1071,6 @@ out_unrec_xprt:
>> out_unrec_sec:
>> 	printk(KERN_INFO "NFS: unrecognized security flavor\n");
>> 	return 0;
>> -
>> -out_unknown:
>> -	printk(KERN_INFO "NFS: unknown mount option: %s\n", p);
>> -	return 0;
>> }
>>
>> /*
>
> This isn't really a very good solution either. Spamming the syslog on
> every option that is being ignored isn't going to help the folks with
> the global automounter maps. Either the rules should be that 'all
> options are allowed' or they should be that 'only recognised NFS  
> options
> are allowed'.


Despite what I posted last week, I like the code the way it is now:   
We should reject any unrecognized mount options with an error  
message.  Anything else invites subtle behavior problems, security  
holes, or even the possibility of data corruption.

Oracle databases, for example, do rely on "sync" mounts actually being  
synchronous.  If you specify Kerberos security but misspell it, I  
think you would want to know that you're not getting the security  
level you expect.

Can someone (maybe Peter) help me understand how exactly this makes  
using an automounter problematic?

Thanks!

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

Re: [PATCH] NFS: Only warn on unrecognized mount options

[Peter Staubach]

Chuck Lever wrote:
> On Apr 11, 2008, at 4:24 PM, Trond Myklebust wrote:
>> On Fri, 2008-04-11 at 16:03 -0400, Chuck Lever wrote:
>>> To provide compatibility with automounters who use a common set of 
>>> mount
>>> options for all file systems, change the NFS in-kernel mount option 
>>> parser
>>> to ignore mount options it doesn't recognize.
>>>
>>> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
>>> ---
>>> Yet another NFS mount patch!  Build tested only.  Comments?
>>>
>>> fs/nfs/super.c |    7 ++-----
>>> 1 files changed, 2 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/fs/nfs/super.c b/fs/nfs/super.c
>>> index f921902..a7201f0 100644
>>> --- a/fs/nfs/super.c
>>> +++ b/fs/nfs/super.c
>>> @@ -1044,7 +1044,8 @@ static int nfs_parse_mount_options(char *raw,
>>>             break;
>>>
>>>         default:
>>> -            goto out_unknown;
>>> +            printk(KERN_INFO "NFS: unrecognized mount option '%s'"
>>> +                    " ignored\n", p);
>>>         }
>>>     }
>>>
>>> @@ -1070,10 +1071,6 @@ out_unrec_xprt:
>>> out_unrec_sec:
>>>     printk(KERN_INFO "NFS: unrecognized security flavor\n");
>>>     return 0;
>>> -
>>> -out_unknown:
>>> -    printk(KERN_INFO "NFS: unknown mount option: %s\n", p);
>>> -    return 0;
>>> }
>>>
>>> /*
>>
>> This isn't really a very good solution either. Spamming the syslog on
>> every option that is being ignored isn't going to help the folks with
>> the global automounter maps. Either the rules should be that 'all
>> options are allowed' or they should be that 'only recognised NFS options
>> are allowed'.
>
>
> Despite what I posted last week, I like the code the way it is now:  
> We should reject any unrecognized mount options with an error 
> message.  Anything else invites subtle behavior problems, security 
> holes, or even the possibility of data corruption.
>
> Oracle databases, for example, do rely on "sync" mounts actually being 
> synchronous.  If you specify Kerberos security but misspell it, I 
> think you would want to know that you're not getting the security 
> level you expect.
>
> Can someone (maybe Peter) help me understand how exactly this makes 
> using an automounter problematic? 

Automounter tools like autofs tend to get their mount options from
global maps, stored in name or directory services like NIS or LDAP.
Many users will be running mixed environment networks, including
systems like Solaris, HP/UX, AIX, Linux, etc.  This means that the
automounter maps may include options which only make sense for
specific systems and aren't applicable to other systems.

One of the features of an automounting feature, other than the
centralized administration, which may or may not be a liability
in this situation, is dynamic mounting and umounting.  This keeps
unused file systems from causing a problem because they get umounted
and then less likely for an application to stumble into and hence,
keeping a dead or very slow server from causing needless delays
and problems.  This also means that the same file system may get
mounted and umounted many times during day.

If the kernel is to print a message every time that it sees an
option that it doesn't understand, than it is possible that many,
many messages could be printed, one for _each_ unknown option
_every time_ that the file system is mounted.

As Trond said, this could lead to spamming the syslog, which will
make it useless.  This might be useful if the unknown options could
be logged once, but logging each individual unknown option, each
time that the file system mounted, makes this much less than
desirable and could potentially lead to a denial of service attack.

The risks outweigh the benefits when viewed from the big picture.

       ps

[PATCH] NFS: Only warn on unrecognized mount options

[Chuck Lever]

To provide compatibility with automounters who use a common set of mount
options for all file systems, change the NFS in-kernel mount option parser
to ignore mount options it doesn't recognize.

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

 fs/nfs/super.c |    7 ++-----
 1 files changed, 2 insertions(+), 5 deletions(-)

diff --git a/fs/nfs/super.c b/fs/nfs/super.c
index f921902..cbf2b37 100644
--- a/fs/nfs/super.c
+++ b/fs/nfs/super.c
@@ -1044,7 +1044,8 @@ static int nfs_parse_mount_options(char *raw,
 			break;
 
 		default:
-			goto out_unknown;
+			dfprintk(MOUNT, "NFS: unrecognized mount option '%s'"
+					" ignored\n", p);
 		}
 	}
 
@@ -1070,10 +1071,6 @@ out_unrec_xprt:
 out_unrec_sec:
 	printk(KERN_INFO "NFS: unrecognized security flavor\n");
 	return 0;
-
-out_unknown:
-	printk(KERN_INFO "NFS: unknown mount option: %s\n", p);
-	return 0;
 }
 
 /*

Re: [PATCH] NFS: Only warn on unrecognized mount options

[Jeff Layton]

On Fri, 11 Apr 2008 16:28:43 -0400
Chuck Lever <chuck.lever@oracle.com> wrote:

> To provide compatibility with automounters who use a common set of mount
> options for all file systems, change the NFS in-kernel mount option parser
> to ignore mount options it doesn't recognize.
> 
> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
> ---
> Take 2.
> 
>  fs/nfs/super.c |    7 ++-----
>  1 files changed, 2 insertions(+), 5 deletions(-)
> 
> diff --git a/fs/nfs/super.c b/fs/nfs/super.c
> index f921902..cbf2b37 100644
> --- a/fs/nfs/super.c
> +++ b/fs/nfs/super.c
> @@ -1044,7 +1044,8 @@ static int nfs_parse_mount_options(char *raw,
>  			break;
>  
>  		default:
> -			goto out_unknown;
> +			dfprintk(MOUNT, "NFS: unrecognized mount option '%s'"
> +					" ignored\n", p);
>  		}
>  	}
>  
> @@ -1070,10 +1071,6 @@ out_unrec_xprt:
>  out_unrec_sec:
>  	printk(KERN_INFO "NFS: unrecognized security flavor\n");
>  	return 0;
> -
> -out_unknown:
> -	printk(KERN_INFO "NFS: unknown mount option: %s\n", p);
> -	return 0;
>  }
>  
>  /*
> 


Sorry for late reply on this...

I wonder if we should make this conditional on the "-s" (sloppy)
option? I think autofs will use that option when it mounts. There are a
couple of ways we could approach this:

1) have -s set a mount flag (which means getting a new MS_SLOPPY flag
added to the VFS).

2) have -s make mount.nfs prepend an implicit "sloppy," to the option
string. That would get parsed first and you could make the behavior of
unrecognized mount options be dependent upon that.

#2 sounds fairly easy to do, actually...

-- 
Jeff Layton <jlayton@redhat.com>