| Message ID | 20100511152149.55240df2@tlielax.poochiereds.net |
|---|---|
| State | New |
| Headers | show |
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?
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.
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,
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.
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.
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