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

[Steve French]

On Mon, Apr 22, 2013 at 1:38 PM, Jeff Layton <jlayton at redhat.com> wrote:
> On Mon, 22 Apr 2013 13:28:49 -0500
> Steve French <smfrench at gmail.com> wrote:
>
>> 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
>>
>
>
> I looked a while back and didn't see one, but I'm happy to be proven
> wrong on this point if you do find a way.
>
> Given that this is a correctness issue, would it be reasonable to go
> ahead and merge this for 3.10 since we don't have another fix queued up
> for this? If it turns out that you find a better way you can always
> back this patch out once that's implemented.

The small suggested fix to address the oops with unitialized ops would be fine
but I think the bigger patch suggested by Satchin is probably 3.11 (ie the oops
is very important to fix ASAP, but the other problem (we can temporarily
cd into a directory that should be an automount) has been around a long
time and so is harder to justify as hot enough to go in this late in 3.10

-- 
Thanks,

Steve