[PATCH] cifs: revalidate directories instiantiated via FIND_* in order to handle DFS referrals

[Steve French]

On Mon, Apr 22, 2013 at 6:20 AM, Jeff Layton <jlayton at redhat.com> wrote:
> On Mon, 22 Apr 2013 11:43:11 +0100
> Sachin Prabhu <sprabhu at redhat.com> wrote:
>
>> From: Jeff Layton <jlayton at redhat.com>
>>
>> We've had a long-standing problem with DFS referral points. CIFS servers
>> generally try to make them look like directories in FIND_FIRST/NEXT
>> responses. When you go to try to do a FIND_FIRST on them though, the
>> server will then (correctly) return STATUS_PATH_NOT_COVERED. Mostly this
>> manifests as spurious EREMOTE errors back to userland.
>>
>> This patch attempts to fix this by marking directories that are
>> discovered via FIND_FIRST/NEXT for revaldiation. When the lookup code
>> runs across them again, we'll reissue a QPathInfo against them and that
>> will make it chase the referral properly.
>>
>> There is some performance penalty involved here and no I haven't
>> measured it -- it'll be highly dependent upon the workload and contents
>> of the mounted share. To try and mitigate that though, the code only
>> marks the inode for revalidation when it's possible to run across a DFS
>> referral. i.e.: when the kernel has DFS support built in and the share
>> is "in DFS".
>>
>> This also fixes a security issue where a user can cause an Oops due to
>> uninitialised inode pointers. Reproducer available at
>> https://patchwork.kernel.org/patch/1980301/
>
> While it might cause performance to regress in some cases, I see no
> real alternative to this patch. It's necessary to do some operation
> specifically against the path before allowing someone to chdir into it.
> Doing anything else means that you'll miss your chance to trigger an
> automount if it does turn out to be a DFS referral.
>
> That said, there are other possible races too, so I think we also need
> a patch to fix the client not to call cifs_set_ops unless the inode is
> still in I_NEW state.

That latter point mentioned  by Jeff, to address the oops itself, is fairly
urgent, as the readdir change you describe doesn't fix all of the scenarios.
For your suggested patch (don't cache stat or readdir information for
directories) - I am uncomfortable with the performance impact so want
to find out two key things before we turn off caching as you suggest
for this case:

1) Is there anyway to tell the difference (using a higher Find info level
for example, especially in SMB2/SMB3 but also in cifs if possible)
between a directory and a DFS junction in directory search results?

2) Is there anyway to narrow the negative performance impact
(ie cache some of these but not others, for example if we know
that a directory with certain other common attributes set can never
be a DFS referral so can be safely cached).   I am uncomfortable,
especially given Samba's performance on recursive directory searches
with turning off caching of these unless there is no other way to tell
the difference between directories and DFS junctions, at least for
the POSIX case (Unix Extensions, ie Samba server),
if not also for the Windows server case



-- 
Thanks,

Steve