Patchwork cifs: guard against hardlinking directories

login
register
mail settings
Submitter Jeff Layton
Date May 7, 2010, 2:03 p.m.
Message ID <1273241032-6404-1-git-send-email-jlayton@redhat.com>
Download mbox | patch
Permalink /patch/51914/
State New
Headers show

Comments

Jeff Layton - May 7, 2010, 2:03 p.m.
When we made serverino the default, we trusted that the field sent by the
server in the "uniqueid" field was actually unique. It turns out that it
isn't reliably so.

Samba, in particular, will just put the st_ino in the uniqueid field when
unix extensions are enabled. When a share spans multiple filesystems, it's
quite possible that there will be collisions. This is a server bug, but
when the inodes in question are a directory (as is often the case) and
there is a collision with the root inode of the mount, the result is a
kernel panic on umount.

Fix this by checking explicitly for directory inodes with the same
uniqueid. If that is the case, then we can assume that using server inode
numbers will be a problem and that they should be disabled.

Signed-off-by: Jeff Layton <jlayton@redhat.com>
---
 fs/cifs/cifsglob.h |    1 +
 fs/cifs/inode.c    |   21 +++++++++++++++++++--
 2 files changed, 20 insertions(+), 2 deletions(-)
Steve French - May 7, 2010, 2:34 p.m.
I have reviewed this and this looks good - just need a tested by or reviewed by.

On Fri, May 7, 2010 at 9:03 AM, Jeff Layton <jlayton@redhat.com> wrote:
> When we made serverino the default, we trusted that the field sent by the
> server in the "uniqueid" field was actually unique. It turns out that it
> isn't reliably so.
>
> Samba, in particular, will just put the st_ino in the uniqueid field when
> unix extensions are enabled. When a share spans multiple filesystems, it's
> quite possible that there will be collisions. This is a server bug, but
> when the inodes in question are a directory (as is often the case) and
> there is a collision with the root inode of the mount, the result is a
> kernel panic on umount.
>
> Fix this by checking explicitly for directory inodes with the same
> uniqueid. If that is the case, then we can assume that using server inode
> numbers will be a problem and that they should be disabled.
>
> Signed-off-by: Jeff Layton <jlayton@redhat.com>
> ---
>  fs/cifs/cifsglob.h |    1 +
>  fs/cifs/inode.c    |   21 +++++++++++++++++++--
>  2 files changed, 20 insertions(+), 2 deletions(-)
>
> diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
> index ecf0ffb..0c2fd17 100644
> --- a/fs/cifs/cifsglob.h
> +++ b/fs/cifs/cifsglob.h
> @@ -502,6 +502,7 @@ struct dfs_info3_param {
>  #define CIFS_FATTR_DFS_REFERRAL                0x1
>  #define CIFS_FATTR_DELETE_PENDING      0x2
>  #define CIFS_FATTR_NEED_REVAL          0x4
> +#define CIFS_FATTR_INO_COLLISION       0x8
>
>  struct cifs_fattr {
>        u32             cf_flags;
> diff --git a/fs/cifs/inode.c b/fs/cifs/inode.c
> index ee35c97..fb20f11 100644
> --- a/fs/cifs/inode.c
> +++ b/fs/cifs/inode.c
> @@ -714,6 +714,16 @@ cifs_find_inode(struct inode *inode, void *opaque)
>        if (CIFS_I(inode)->uniqueid != fattr->cf_uniqueid)
>                return 0;
>
> +       /*
> +        * uh oh -- it's a directory. We can't use it since hardlinked dirs are
> +        * verboten. Disable serverino and return it as if it were found, the
> +        * caller can discard it, generate a uniqueid and retry the find
> +        */
> +       if (S_ISDIR(inode->i_mode)) {
> +               fattr->cf_flags |= CIFS_FATTR_INO_COLLISION;
> +               cifs_autodisable_serverino(CIFS_SB(inode->i_sb));
> +       }
> +
>        return 1;
>  }
>
> @@ -733,15 +743,22 @@ cifs_iget(struct super_block *sb, struct cifs_fattr *fattr)
>        unsigned long hash;
>        struct inode *inode;
>
> +retry_iget5_locked:
>        cFYI(1, ("looking for uniqueid=%llu", fattr->cf_uniqueid));
>
>        /* hash down to 32-bits on 32-bit arch */
>        hash = cifs_uniqueid_to_ino_t(fattr->cf_uniqueid);
>
>        inode = iget5_locked(sb, hash, cifs_find_inode, cifs_init_inode, fattr);
> -
> -       /* we have fattrs in hand, update the inode */
>        if (inode) {
> +               /* was there a problematic inode number collision? */
> +               if (fattr->cf_flags & CIFS_FATTR_INO_COLLISION) {
> +                       iput(inode);
> +                       fattr->cf_uniqueid = iunique(sb, ROOT_I);
> +                       fattr->cf_flags &= ~CIFS_FATTR_INO_COLLISION;
> +                       goto retry_iget5_locked;
> +               }
> +
>                cifs_fattr_to_inode(inode, fattr);
>                if (sb->s_flags & MS_NOATIME)
>                        inode->i_flags |= S_NOATIME | S_NOCMTIME;
> --
> 1.7.0.1
>
>
Suresh Jayaraman - May 7, 2010, 3:08 p.m.
On 05/07/2010 07:33 PM, Jeff Layton wrote:
> When we made serverino the default, we trusted that the field sent by the
> server in the "uniqueid" field was actually unique. It turns out that it
> isn't reliably so.
> 
> Samba, in particular, will just put the st_ino in the uniqueid field when
> unix extensions are enabled. When a share spans multiple filesystems, it's
> quite possible that there will be collisions. This is a server bug, but
> when the inodes in question are a directory (as is often the case) and
> there is a collision with the root inode of the mount, the result is a
> kernel panic on umount.
> 
> Fix this by checking explicitly for directory inodes with the same
> uniqueid. If that is the case, then we can assume that using server inode
> numbers will be a problem and that they should be disabled.
> 
> Signed-off-by: Jeff Layton <jlayton@redhat.com>
> ---
>  fs/cifs/cifsglob.h |    1 +
>  fs/cifs/inode.c    |   21 +++++++++++++++++++--
>  2 files changed, 20 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
> index ecf0ffb..0c2fd17 100644
> --- a/fs/cifs/cifsglob.h
> +++ b/fs/cifs/cifsglob.h
> @@ -502,6 +502,7 @@ struct dfs_info3_param {
>  #define CIFS_FATTR_DFS_REFERRAL		0x1
>  #define CIFS_FATTR_DELETE_PENDING	0x2
>  #define CIFS_FATTR_NEED_REVAL		0x4
> +#define CIFS_FATTR_INO_COLLISION	0x8
>  

Nice and thoughful solution! I was not for disabling serverino as a default.

I have a nice way of reliably reproducing the problem.

- create two small partitions and format (ext3)
- mount first partition as /vol1 and mount the second as /vol1/vol2
  inside the first
- create files on both partitions (say 10 files)
- export them
- mount from the client, access them
- the inode numbers will easily collide
- umount them and the client will crash..

I'm trying out the fix and will be able to verify (hopefully before I
catch my train in another couple of hours). Will revert back after testing.


Thanks,
Suresh Jayaraman - May 7, 2010, 4:19 p.m.
On 05/07/2010 08:38 PM, Suresh Jayaraman wrote:
> On 05/07/2010 07:33 PM, Jeff Layton wrote:
>> When we made serverino the default, we trusted that the field sent by the
>> server in the "uniqueid" field was actually unique. It turns out that it
>> isn't reliably so.
>>
>> Samba, in particular, will just put the st_ino in the uniqueid field when
>> unix extensions are enabled. When a share spans multiple filesystems, it's
>> quite possible that there will be collisions. This is a server bug, but
>> when the inodes in question are a directory (as is often the case) and
>> there is a collision with the root inode of the mount, the result is a
>> kernel panic on umount.
>>
>> Fix this by checking explicitly for directory inodes with the same
>> uniqueid. If that is the case, then we can assume that using server inode
>> numbers will be a problem and that they should be disabled.
>>
>> Signed-off-by: Jeff Layton <jlayton@redhat.com>
>> ---
>>  fs/cifs/cifsglob.h |    1 +
>>  fs/cifs/inode.c    |   21 +++++++++++++++++++--
>>  2 files changed, 20 insertions(+), 2 deletions(-)
>>
>> diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
>> index ecf0ffb..0c2fd17 100644
>> --- a/fs/cifs/cifsglob.h
>> +++ b/fs/cifs/cifsglob.h
>> @@ -502,6 +502,7 @@ struct dfs_info3_param {
>>  #define CIFS_FATTR_DFS_REFERRAL		0x1
>>  #define CIFS_FATTR_DELETE_PENDING	0x2
>>  #define CIFS_FATTR_NEED_REVAL		0x4
>> +#define CIFS_FATTR_INO_COLLISION	0x8
>>  
> 
> Nice and thoughful solution! I was not for disabling serverino as a default.
> 
> I have a nice way of reliably reproducing the problem.
> 
> - create two small partitions and format (ext3)
> - mount first partition as /vol1 and mount the second as /vol1/vol2
>   inside the first
> - create files on both partitions (say 10 files)
> - export them
> - mount from the client, access them
> - the inode numbers will easily collide
> - umount them and the client will crash..
> 
> I'm trying out the fix and will be able to verify (hopefully before I
> catch my train in another couple of hours). Will revert back after testing.
> 

Voila! The patch fixes the panic (and serverino autodisabled if a
duplicate inode number is found -- from dmesg) when testing using the
above mentioned test case.

Reviewed-and-Tested-by: Suresh Jayaraman <sjayaraman@suse.de>


Thanks,
Steve French - May 11, 2010, 5:15 p.m.
Merged into cifs-2.6.git - if the branch for-linus does not look
weird/broken, plan to request upstream tonight.
Jeff Layton - May 11, 2010, 6:15 p.m.
On Tue, 11 May 2010 12:15:51 -0500
Steve French <smfrench@gmail.com> wrote:

> Merged into cifs-2.6.git - if the branch for-linus does not look
> weird/broken, plan to request upstream tonight.
> 
> 
> 

I just made some comments on this to the bug:

https://bugzilla.samba.org/show_bug.cgi?id=7407#c13

...I wonder whether this patch may be too aggressive about disabling
serverino. It seems like we ought to be OK with finding an inode that
is "floating", just not one that has a dentry already attached.
Thoughts?
Steve French - May 11, 2010, 6:43 p.m.
On Tue, May 11, 2010 at 1:15 PM, Jeff Layton <jlayton@redhat.com> wrote:
> On Tue, 11 May 2010 12:15:51 -0500
> Steve French <smfrench@gmail.com> wrote:
>
>> Merged into cifs-2.6.git - if the branch for-linus does not look
>> weird/broken, plan to request upstream tonight.
>>
>>
>>
>
> I just made some comments on this to the bug:
>
> https://bugzilla.samba.org/show_bug.cgi?id=7407#c13
>
> ...I wonder whether this patch may be too aggressive about disabling
> serverino. It seems like we ought to be OK with finding an inode that
> is "floating", just not one that has a dentry already attached.
> Thoughts?
>
> --
> Jeff Layton <jlayton@redhat.com>
>

I agree - but have to deal with the oops first ASAP - narrow it later.

Patch

diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
index ecf0ffb..0c2fd17 100644
--- a/fs/cifs/cifsglob.h
+++ b/fs/cifs/cifsglob.h
@@ -502,6 +502,7 @@  struct dfs_info3_param {
 #define CIFS_FATTR_DFS_REFERRAL		0x1
 #define CIFS_FATTR_DELETE_PENDING	0x2
 #define CIFS_FATTR_NEED_REVAL		0x4
+#define CIFS_FATTR_INO_COLLISION	0x8
 
 struct cifs_fattr {
 	u32		cf_flags;
diff --git a/fs/cifs/inode.c b/fs/cifs/inode.c
index ee35c97..fb20f11 100644
--- a/fs/cifs/inode.c
+++ b/fs/cifs/inode.c
@@ -714,6 +714,16 @@  cifs_find_inode(struct inode *inode, void *opaque)
 	if (CIFS_I(inode)->uniqueid != fattr->cf_uniqueid)
 		return 0;
 
+	/*
+	 * uh oh -- it's a directory. We can't use it since hardlinked dirs are
+	 * verboten. Disable serverino and return it as if it were found, the
+	 * caller can discard it, generate a uniqueid and retry the find
+	 */
+	if (S_ISDIR(inode->i_mode)) {
+		fattr->cf_flags |= CIFS_FATTR_INO_COLLISION;
+		cifs_autodisable_serverino(CIFS_SB(inode->i_sb));
+	}
+
 	return 1;
 }
 
@@ -733,15 +743,22 @@  cifs_iget(struct super_block *sb, struct cifs_fattr *fattr)
 	unsigned long hash;
 	struct inode *inode;
 
+retry_iget5_locked:
 	cFYI(1, ("looking for uniqueid=%llu", fattr->cf_uniqueid));
 
 	/* hash down to 32-bits on 32-bit arch */
 	hash = cifs_uniqueid_to_ino_t(fattr->cf_uniqueid);
 
 	inode = iget5_locked(sb, hash, cifs_find_inode, cifs_init_inode, fattr);
-
-	/* we have fattrs in hand, update the inode */
 	if (inode) {
+		/* was there a problematic inode number collision? */
+		if (fattr->cf_flags & CIFS_FATTR_INO_COLLISION) {
+			iput(inode);
+			fattr->cf_uniqueid = iunique(sb, ROOT_I);
+			fattr->cf_flags &= ~CIFS_FATTR_INO_COLLISION;
+			goto retry_iget5_locked;
+		}
+
 		cifs_fattr_to_inode(inode, fattr);
 		if (sb->s_flags & MS_NOATIME)
 			inode->i_flags |= S_NOATIME | S_NOCMTIME;