Patchwork cifs: guard against hardlinking directories

login
register
mail settings
Submitter Jeff Layton
Date May 11, 2010, 7:21 p.m.
Message ID <20100511152149.55240df2@tlielax.poochiereds.net>
Download mbox | patch
Permalink /patch/52311/
State New
Headers show

Comments

Jeff Layton - May 11, 2010, 7:21 p.m.
On Tue, 11 May 2010 13:43:27 -0500
Steve French <smfrench@gmail.com> wrote:

> 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.
> 

I tend to agree -- disabling server inode numbers unnecessarily is
better than oopsing at umount. I did just test the attached modified
patch however and it fixes the reproducer I have for this. Thoughts on
going with this instead?
Steve French - May 11, 2010, 8:42 p.m.
On Tue, May 11, 2010 at 2:21 PM, Jeff Layton <jlayton@redhat.com> wrote:
> On Tue, 11 May 2010 13:43:27 -0500
> Steve French <smfrench@gmail.com> wrote:
>
>> 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.
>>
>
> I tend to agree -- disabling server inode numbers unnecessarily is
> better than oopsing at umount. I did just test the attached modified
> patch however and it fixes the reproducer I have for this. Thoughts on
> going with this instead?

Isn't this patch the same as the one you sent this morning? What changed?
Steve French - May 11, 2010, 8:44 p.m.
On Tue, May 11, 2010 at 3:42 PM, Steve French <smfrench@gmail.com> wrote:
> On Tue, May 11, 2010 at 2:21 PM, Jeff Layton <jlayton@redhat.com> wrote:
>> On Tue, 11 May 2010 13:43:27 -0500
>> Steve French <smfrench@gmail.com> wrote:
>>
>>> 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.
>>>
>>
>> I tend to agree -- disabling server inode numbers unnecessarily is
>> better than oopsing at umount. I did just test the attached modified
>> patch however and it fixes the reproducer I have for this. Thoughts on
>> going with this instead?
>
> Isn't this patch the same as the one you sent this morning? What changed?

Nevermind - I see the difference.  Makes sense.  You added the

&& !list_empty(&inode->i_dentry)

on the duplicate inode num check.
Suresh Jayaraman - May 12, 2010, 9:19 a.m.
On 05/12/2010 02:14 AM, Steve French wrote:
> On Tue, May 11, 2010 at 3:42 PM, Steve French <smfrench@gmail.com> wrote:
>> On Tue, May 11, 2010 at 2:21 PM, Jeff Layton <jlayton@redhat.com> wrote:
>>> On Tue, 11 May 2010 13:43:27 -0500
>>> Steve French <smfrench@gmail.com> wrote:
>>>
>>>> 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.
>>>>
>>>
>>> I tend to agree -- disabling server inode numbers unnecessarily is
>>> better than oopsing at umount. I did just test the attached modified
>>> patch however and it fixes the reproducer I have for this. Thoughts on
>>> going with this instead?
>>
>> Isn't this patch the same as the one you sent this morning? What changed?
> 
> Nevermind - I see the difference.  Makes sense.  You added the
> 
> && !list_empty(&inode->i_dentry)
> 
> on the duplicate inode num check.
> 

The change looks good. I found the most recent version works fine during
my testing.

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

On a side note, thinking about ways to find out whether the share spans
multiple filesystems - one way is to rely on the fact that "hardlinks
cannot cross filesystem boundaries". Try and create a hardlink if it
fails with -EXDEV, then we could be sure that it cross filesystem
boundaries. But it is ugly and needs cleanup if it succeeds and would
incur additional SET_PATH_INFO call, not worth the effort..


Thanks,
Jeff Layton - May 12, 2010, 10:56 a.m.
On Wed, 12 May 2010 14:49:45 +0530
Suresh Jayaraman <sjayaraman@suse.de> wrote:

> On 05/12/2010 02:14 AM, Steve French wrote:
> > On Tue, May 11, 2010 at 3:42 PM, Steve French <smfrench@gmail.com> wrote:
> >> On Tue, May 11, 2010 at 2:21 PM, Jeff Layton <jlayton@redhat.com> wrote:
> >>> On Tue, 11 May 2010 13:43:27 -0500
> >>> Steve French <smfrench@gmail.com> wrote:
> >>>
> >>>> 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.
> >>>>
> >>>
> >>> I tend to agree -- disabling server inode numbers unnecessarily is
> >>> better than oopsing at umount. I did just test the attached modified
> >>> patch however and it fixes the reproducer I have for this. Thoughts on
> >>> going with this instead?
> >>
> >> Isn't this patch the same as the one you sent this morning? What changed?
> > 
> > Nevermind - I see the difference.  Makes sense.  You added the
> > 
> > && !list_empty(&inode->i_dentry)
> > 
> > on the duplicate inode num check.
> > 
> 
> The change looks good. I found the most recent version works fine during
> my testing.
> 
>    Reviewed-and-Tested-by: Suresh Jayaraman <sjayaraman@suse.de>
> 
> On a side note, thinking about ways to find out whether the share spans
> multiple filesystems - one way is to rely on the fact that "hardlinks
> cannot cross filesystem boundaries". Try and create a hardlink if it
> fails with -EXDEV, then we could be sure that it cross filesystem
> boundaries. But it is ugly and needs cleanup if it succeeds and would
> incur additional SET_PATH_INFO call, not worth the effort..
> 
> 
> Thanks,
> 

That's probably not feasible -- you'd need to check to see whether you
could create a hardlink across every directory you come across
(anything could be a mountpoint). It could also fail for other reasons
like not having permissions which would make the test inconclusive.
Steve French - May 12, 2010, 1:48 p.m.
On Wed, May 12, 2010 at 4:19 AM, Suresh Jayaraman <sjayaraman@suse.de> wrote:
> The change looks good. I found the most recent version works fine during
> my testing.
>
>   Reviewed-and-Tested-by: Suresh Jayaraman <sjayaraman@suse.de>
>
> On a side note, thinking about ways to find out whether the share spans
> multiple filesystems - one way is to rely on the fact that "hardlinks
> cannot cross filesystem boundaries". Try and create a hardlink if it
> fails with -EXDEV, then we could be sure that it cross filesystem
> boundaries. But it is ugly and needs cleanup if it succeeds and would
> incur additional SET_PATH_INFO call, not worth the effort..

May be easier (and more intuitive) to ask the server to do a DFS
referral to the subdir (since it really is a different device and this
will make the client treat it as such) if it crosses volume boundaries
and can't guarantee inode numbers.

Patch

From 08bfd493bed6e217cfb10c9f4337ebbf834d4c6e Mon Sep 17 00:00:00 2001
From: Jeff Layton <jlayton@redhat.com>
Date: Tue, 11 May 2010 14:59:55 -0400
Subject: [PATCH] cifs: guard against hardlinking directories

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 35ec117..29b9ea2 100644
--- a/fs/cifs/inode.c
+++ b/fs/cifs/inode.c
@@ -715,6 +715,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) && !list_empty(&inode->i_dentry)) {
+		fattr->cf_flags |= CIFS_FATTR_INO_COLLISION;
+		cifs_autodisable_serverino(CIFS_SB(inode->i_sb));
+	}
+
 	return 1;
 }
 
@@ -734,15 +744,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.6.6.1