Re: [PATCH] cifs: guard against hardlinking directories

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,

-- 
Suresh Jayaraman