On Tue, 6 Aug 2013 10:56:23 -0400 Jeff Layton <jlayton@xxxxxxxxxx> wrote: > On Tue, 06 Aug 2013 16:30:52 +0200 > "Stefan (metze) Metzmacher" <metze@xxxxxxxxx> wrote: > > > Hi David, > > > > > The following commit included in 3.10, and copied to some table-series > > > kernels: > > > > > > commit: c2b93e0699723700f886ce17bb65ffd771195a6d > > > cifs: only set ops for inodes in I_NEW state > > > > > > ... causes a regression in mfsymlink handling under some circumstances. > > > > > > Specifically, this patch modified cifs_fattr_to_inode() to only invoke > > > cifs_set_ops() if that inode still had the I_NEW flag set --- preventing > > > an inode's ops-set from from being changed after it had been > > > initialized, preventing an oops if another thread of execution was > > > already chasing that pointer. > > > > > > However, mfsymlinks are also affected by this commit. In the cold-cache > > > case, a user invoking stat() on an mfsymlink will still see the correct > > > results. But, if the dentry cache is instead populated via > > > cifs_readdir/filldir, then inode property population operates in a two > > > stage mode: > > > > > > - First, the inode is initialized as a regular file, with the > > > CIFS_FATTR_NEED_REVAL flag set. > > > > > > - Later, when the file is stat()'d, then correct operation depends on > > > the (re)validation steps from being able to update the inode's > > > properties --- including the set of file operations permitted. > > > > > > (A comment explains: "trying to get the type and mode can be slow, so > > > just call those regular files for now, and mark for reval") > > > > > > With the above commit, this second revalidation step never updates the > > > operations field, resulting in symlinks not having the readlink(), etc. > > > functions correctly hooked up. > > > > > A naïve fix-up is to modify the conditional to also permit (re)setting > > > ops on S_IFLNK-mode files; however, I suspect a correct fix lies elsewhere? > > > > I guess 'if (inode->i_state & I_NEW)' should be changed to > > 'if ((inode->i_state & I_NEW) || (fattr->cf_flags & CIFS_FATTR_NEED_REVAL))' > > > > Would that solve the problem? > > > > Jeff/Steve, any comment on this? > > > > metze > > > > I don't think so... > > Once an inode's operations are set, you really don't want to go > changing them. That opens the door for a malicious server to change the > file type out from under you and cause oopses. The bug report that > Sachin worked on originally had such an oops. In that case it was > caused by something else, but a malicious server could wreak similar > havoc... > > The right solution is probably to ensure that the dentry fails > d_revalidate if there's a chance it could turn out to be a mfsymlink. > I think that the easiest way to ensure that would probably be to set the > dentry->d_time to 0 in the readdir code. > Specifically, I think the logic in cifs_d_revalidate is just plain wrong. Currently, the dentry->d_time check and the check for lookupCacheEnabled is only done for negative dentries. Shouldn't that be affecting positive dentries as well? If so, then you could simply set the dentries that are possible mfsymlinks with a d_time of 0. They'd fail revalidation at that step and get recreated when you stat them. Sachin, Steve, thoughts? -- Jeff Layton <jlayton@xxxxxxxxxx> -- To unsubscribe from this list: send the line "unsubscribe stable" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html