Re: [BUG] Regression in mfsymlink handling introduced by "cifs: only set ops for inodes in I_NEW state"

["Stefan (metze) Metzmacher" <metze-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>]

[-- Attachment #1: Type: text/plain, Size: 1941 bytes --]

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 261 bytes --]