| Message ID | 1366627391-14753-1-git-send-email-sprabhu@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, 22 Apr 2013 11:43:11 +0100 Sachin Prabhu <sprabhu@redhat.com> wrote: > From: Jeff Layton <jlayton@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/ > > Signed-off-by: Jeff Layton <jlayton@redhat.com> > Signed-off-by: Sachin Prabhu <sprabhu@redhat.com> > --- > fs/cifs/readdir.c | 24 ++++++++++++++++++++++++ > 1 file changed, 24 insertions(+) > > diff --git a/fs/cifs/readdir.c b/fs/cifs/readdir.c > index df40cc5..49d44f3 100644 > --- a/fs/cifs/readdir.c > +++ b/fs/cifs/readdir.c > @@ -126,6 +126,22 @@ out: > dput(dentry); > } > > +/* > + * Is it possible that this directory might turn out to be a DFS referral > + * once we go to try and use it? > + */ > +static bool > +cifs_dfs_is_possible(struct cifs_sb_info *cifs_sb) > +{ > +#ifdef CONFIG_CIFS_DFS_UPCALL > + struct cifs_tcon *tcon = cifs_sb_master_tcon(cifs_sb); > + > + if (tcon->Flags & SMB_SHARE_IS_IN_DFS) > + return true; > +#endif > + return false; > +} > + > static void > cifs_fill_common_info(struct cifs_fattr *fattr, struct cifs_sb_info *cifs_sb) > { > @@ -135,6 +151,14 @@ cifs_fill_common_info(struct cifs_fattr *fattr, struct cifs_sb_info *cifs_sb) > if (fattr->cf_cifsattrs & ATTR_DIRECTORY) { > fattr->cf_mode = S_IFDIR | cifs_sb->mnt_dir_mode; > fattr->cf_dtype = DT_DIR; > + /* > + * CIFS servers generally make DFS referrals look like > + * directories in FIND_* responses. Since there's no way to > + * tell the difference, we must revalidate directory inodes > + * before trying to use them. > + */ > + if (cifs_dfs_is_possible(cifs_sb)) > + fattr->cf_flags |= CIFS_FATTR_NEED_REVAL; > } else { > fattr->cf_mode = S_IFREG | cifs_sb->mnt_file_mode; > fattr->cf_dtype = DT_REG; 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.
On Mon, Apr 22, 2013 at 6:20 AM, Jeff Layton <jlayton@redhat.com> wrote: > On Mon, 22 Apr 2013 11:43:11 +0100 > Sachin Prabhu <sprabhu@redhat.com> wrote: > >> From: Jeff Layton <jlayton@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
On Mon, 22 Apr 2013 13:28:49 -0500 Steve French <smfrench@gmail.com> wrote: > On Mon, Apr 22, 2013 at 6:20 AM, Jeff Layton <jlayton@redhat.com> wrote: > > On Mon, 22 Apr 2013 11:43:11 +0100 > > Sachin Prabhu <sprabhu@redhat.com> wrote: > > > >> From: Jeff Layton <jlayton@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.
On Mon, Apr 22, 2013 at 1:38 PM, Jeff Layton <jlayton@redhat.com> wrote: > On Mon, 22 Apr 2013 13:28:49 -0500 > Steve French <smfrench@gmail.com> wrote: > >> On Mon, Apr 22, 2013 at 6:20 AM, Jeff Layton <jlayton@redhat.com> wrote: >> > On Mon, 22 Apr 2013 11:43:11 +0100 >> > Sachin Prabhu <sprabhu@redhat.com> wrote: >> > >> >> From: Jeff Layton <jlayton@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
diff --git a/fs/cifs/readdir.c b/fs/cifs/readdir.c index df40cc5..49d44f3 100644 --- a/fs/cifs/readdir.c +++ b/fs/cifs/readdir.c @@ -126,6 +126,22 @@ out: dput(dentry); } +/* + * Is it possible that this directory might turn out to be a DFS referral + * once we go to try and use it? + */ +static bool +cifs_dfs_is_possible(struct cifs_sb_info *cifs_sb) +{ +#ifdef CONFIG_CIFS_DFS_UPCALL + struct cifs_tcon *tcon = cifs_sb_master_tcon(cifs_sb); + + if (tcon->Flags & SMB_SHARE_IS_IN_DFS) + return true; +#endif + return false; +} + static void cifs_fill_common_info(struct cifs_fattr *fattr, struct cifs_sb_info *cifs_sb) { @@ -135,6 +151,14 @@ cifs_fill_common_info(struct cifs_fattr *fattr, struct cifs_sb_info *cifs_sb) if (fattr->cf_cifsattrs & ATTR_DIRECTORY) { fattr->cf_mode = S_IFDIR | cifs_sb->mnt_dir_mode; fattr->cf_dtype = DT_DIR; + /* + * CIFS servers generally make DFS referrals look like + * directories in FIND_* responses. Since there's no way to + * tell the difference, we must revalidate directory inodes + * before trying to use them. + */ + if (cifs_dfs_is_possible(cifs_sb)) + fattr->cf_flags |= CIFS_FATTR_NEED_REVAL; } else { fattr->cf_mode = S_IFREG | cifs_sb->mnt_file_mode; fattr->cf_dtype = DT_REG;