Patchwork cifs: disable the use of server inode numbers by default

login
register
mail settings
Submitter Jeff Layton
Date May 5, 2010, 4:16 p.m.
Message ID <1273076215-8827-1-git-send-email-jlayton@redhat.com>
Download mbox | patch
Permalink /patch/51707/
State New
Headers show

Comments

Jeff Layton - May 5, 2010, 4:16 p.m.
We have a bit of a problem with server inode numbers...

The FILE_UNIX_BASIC_INFO struct has a UniqueID field that is supposed to
be unique across the scope of the share. It turns out that all current
and past versions of Samba simply stuffed the inode number in this
field. If you have a share that spans multiple filesystems on the
server, then it's *likely* that the server will send the same value for
more than one inode, leading to collisions.

Given that, it's really pretty dangerous to trust this info from the
server at this time. This patch simply makes the default "noserverino".
To reenable it, admins will have to mount with "serverino". We'll
continue to pursue fixes for this problem in the meantime, so hopefully
this is just a temporary measure.

Signed-off-by: Jeff Layton <jlayton@redhat.com>
---
 fs/cifs/connect.c |    9 +++++++--
 1 files changed, 7 insertions(+), 2 deletions(-)
Jeff Layton - May 7, 2010, 1:13 p.m.
On Wed,  5 May 2010 18:16:55 +0200
Jeff Layton <jlayton@redhat.com> wrote:

> We have a bit of a problem with server inode numbers...
> 
> The FILE_UNIX_BASIC_INFO struct has a UniqueID field that is supposed to
> be unique across the scope of the share. It turns out that all current
> and past versions of Samba simply stuffed the inode number in this
> field. If you have a share that spans multiple filesystems on the
> server, then it's *likely* that the server will send the same value for
> more than one inode, leading to collisions.
> 
> Given that, it's really pretty dangerous to trust this info from the
> server at this time. This patch simply makes the default "noserverino".
> To reenable it, admins will have to mount with "serverino". We'll
> continue to pursue fixes for this problem in the meantime, so hopefully
> this is just a temporary measure.
> 
> Signed-off-by: Jeff Layton <jlayton@redhat.com>
> ---
>  fs/cifs/connect.c |    9 +++++++--
>  1 files changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
> index 9123c23..a60c977 100644
> --- a/fs/cifs/connect.c
> +++ b/fs/cifs/connect.c
> @@ -839,8 +839,13 @@ cifs_parse_mount_options(char *options, const char *devname,
>  	/* vol->retry default is 0 (i.e. "soft" limited retry not hard retry) */
>  	/* default is always to request posix paths. */
>  	vol->posix_paths = 1;
> -	/* default to using server inode numbers where available */
> -	vol->server_ino = 1;
> +
> +	/*
> +	 * The client cannot default to using server inode numbers until it
> +	 * can reliably guard against misbehaving servers causing inode
> +	 * number collisions.
> +	 */
> +	vol->server_ino = 0;
>  
>  	if (!options)
>  		return 1;

Self-NAK on this patch for now. I think I see a better way to handle
this problem. I'll be sending a new patch in a bit.

Patch

diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
index 9123c23..a60c977 100644
--- a/fs/cifs/connect.c
+++ b/fs/cifs/connect.c
@@ -839,8 +839,13 @@  cifs_parse_mount_options(char *options, const char *devname,
 	/* vol->retry default is 0 (i.e. "soft" limited retry not hard retry) */
 	/* default is always to request posix paths. */
 	vol->posix_paths = 1;
-	/* default to using server inode numbers where available */
-	vol->server_ino = 1;
+
+	/*
+	 * The client cannot default to using server inode numbers until it
+	 * can reliably guard against misbehaving servers causing inode
+	 * number collisions.
+	 */
+	vol->server_ino = 0;
 
 	if (!options)
 		return 1;