Re: [PATCH] cifs: guard against hardlinking directories

On Wed, 12 May 2010 14:49:45 +0530
Suresh Jayaraman <[email protected]> wrote:

> On 05/12/2010 02:14 AM, Steve French wrote:
> > On Tue, May 11, 2010 at 3:42 PM, Steve French <[email protected]> wrote:
> >> On Tue, May 11, 2010 at 2:21 PM, Jeff Layton <[email protected]> wrote:
> >>> On Tue, 11 May 2010 13:43:27 -0500
> >>> Steve French <[email protected]> wrote:
> >>>
> >>>> On Tue, May 11, 2010 at 1:15 PM, Jeff Layton <[email protected]> wrote:
> >>>>> On Tue, 11 May 2010 12:15:51 -0500
> >>>>> Steve French <[email protected]> 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 <[email protected]>
> >>>>>
> >>>>
> >>>> 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 <[email protected]>
> 
> 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.

-- 
Jeff Layton <[email protected]>