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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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: signature.asc
Description: OpenPGP digital signature


[Index of Archives]     [Linux Kernel]     [Kernel Development Newbies]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite Hiking]     [Linux Kernel]     [Linux SCSI]